Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unsafe_op_in_unsafe_fn to Clippy CI & Crates #9860

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
@JoJoJet
Copy link
Member

JoJoJet commented Sep 20, 2023

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