Skip to content

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Sep 20, 2023

Objective

Solution

  • Added unsafe_op_in_unsafe_fn to the lint options for CI clippy task.
  • Added #![forbid(unsafe_op_in_unsafe_fn)] to all crates that did not already forbid unsafe
  • Placed unsafe { ... } statements around specific uses of unsafe where lint raised warnings.

Notes

  • My safety comments are fairly generic due to the shear volume required. I would encourage people to submit change suggestions to better comments where they may have a better understanding of the specific unsafe function contract.
  • I've neglected the generated.rs file in bevy_mikktspace, as something like Refactor bevy_mikktspace to entirely safe rust #9050 will resolve that issue on its own.

This is a work-in-progress, there are thousands of violations of this lint that must be addressed before CI will complete successfully.
@james7132 james7132 self-requested a review September 20, 2023 02:52
@joseph-gio
Copy link
Member

IMO it would be better to add this lint to each crate individually, rather than putting it in CI. That would provide a better development experience since contributors won't be surprised by CI failing when clippy passed locally.

It would also make it easier to fix all of the occurrences since we could do it for each crate/groups of crates separately.

@bushrat011899
Copy link
Contributor Author

IMO it would be better to add this lint to each crate individually

Yeah I think you're right, I'll add that but keep it in the CI as a catch for future crates.

@bushrat011899 bushrat011899 changed the title Add unsafe_op_in_unsafe_fn to Clippy CI Add unsafe_op_in_unsafe_fn to Clippy CI & Crates Sep 20, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior C-Code-Quality A section of code that is hard to understand or change labels Sep 20, 2023
Still WIP, currently 600+ compiler errors.

Tentative Completion

WIP

Reverted Changes to `bevy_mikktspace`
@bushrat011899 bushrat011899 marked this pull request as ready for review September 21, 2023 02:43
@Kanabenki
Copy link
Contributor

Once #10011 is merged, this could be rebased to integrate the unsafe_op_in_unsafe_fn in the Cargo.toml lint table 🙂

@alice-i-cecile
Copy link
Member

Yeah, IMO we should move this to the universal lint config.

@mockersf
Copy link
Member

Could you keep the lint at the warn level rather than forbid? It gives a much better local dev experience, and will be denied by CI

@bushrat011899 bushrat011899 marked this pull request as draft November 19, 2023 06:06
@bushrat011899
Copy link
Contributor Author

I'm going to have another crack at this now that #10011 is merged, with the goal being to warn on unsafe_op_in_unsafe_fn, and fix any violations. I'm switching this to draft just to indicate it's not ready yet.

@BD103
Copy link
Member

BD103 commented Feb 1, 2024

There's been some recent work in this area through #11590. Can any of this PR help with that? (Especially bevy_ecs. It would be a shame for all this work to be repeated.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change P-Unsound A bug that results in undefined compiler behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lint against (and fix) missing unsafe blocks in unsafe functions

6 participants