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

Migrate lints to use 2024 edition practice of unsafe_op_in_unsafe_fn #4994

Merged
merged 14 commits into from
Mar 21, 2025

Conversation

davidhewitt
Copy link
Member

Credit to @jonaspleyer for the work here in #4976, I accidentally closed that PR and now can't push to that branch. (Calling it main made it awkward for me to push to, sorry 😬)

I added an additional commit which should hopefully fix the MSRV build...

@davidhewitt davidhewitt added CI-skip-changelog Skip checking changelog entry CI-build-full labels Mar 20, 2025
@davidhewitt davidhewitt added the CI-no-fail-fast If one job fails, allow the rest to keep testing label Mar 20, 2025
@davidhewitt
Copy link
Member Author

Having started down this road, I am now considering resetting this for pyo3-ffi and explicitly marking #[allow(unsafe_op_in_unsafe_fn)] for that whole crate.

Justification: the contents of pyo3-ffi are deliberately written as a faithful translation of the CPython source code, and having the unsafe { } all over the place just adds to the translation burden without really improving the situation (i.e. we need to match the CPython implementations anyway).

@alex (as you commented on the original PR) - what do you think about partially backing this out?

For the main pyo3 crate I think warn(unsafe_op_in_unsafe_fn) is 100% justified.

@alex
Copy link
Contributor

alex commented Mar 20, 2025

I don't feel strongly. Certainly it seems fine to me if we want to start with just doing the pyo3 crate, and then maybe we can consider pyo3-ffi later.

@davidhewitt davidhewitt enabled auto-merge March 20, 2025 19:23
@davidhewitt davidhewitt added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2025
@jonaspleyer
Copy link
Contributor

Some type of network error seems to have killed the windows cross-compilation workflow. 😐

@davidhewitt
Copy link
Member Author

Yes, it downloads some build tools. We cache them but they keep getting evicted, will try #4995...

@davidhewitt davidhewitt added this pull request to the merge queue Mar 21, 2025
Merged via the queue into main with commit ed37c40 Mar 21, 2025
89 checks passed
@davidhewitt davidhewitt deleted the edition-2024 branch March 21, 2025 10:44
@mejrs
Copy link
Member

mejrs commented Mar 22, 2025

Having started down this road, I am now considering resetting this for pyo3-ffi

A bit late but I agree, it's a bit silly for a crate that's all just unsafe.

@jonaspleyer
Copy link
Contributor

Having started down this road, I am now considering resetting this for pyo3-ffi

A bit late but I agree, it's a bit silly for a crate that's all just unsafe.

I think it has been included in ed37c40. See https://github.com/PyO3/pyo3/blob/ea5adbf90413d524fe89ffe00c4ed6f219dafe09/pyo3-ffi/src/lib.rs#L337C6-L337C34.
In particular this does provides forward Compatibility for users already on edition 2024. They will not get swarmed with warnings relating to this lint which is nice. I think that it is a good choice to have it like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-build-full CI-no-fail-fast If one job fails, allow the rest to keep testing CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants