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 to 2024 edition #4976

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

migrate to 2024 edition #4976

wants to merge 5 commits into from

Conversation

jonaspleyer
Copy link
Contributor

This is an initial advancement to increase the edition version to 2024. In particular I have done the following non-structural changes:

  • Add the unsafe keyword to function calls of unsafe functions
  • Mark extern "C" blocks as unsafe extern "C"

For now, the error messages which are being produced are different to the previous ones. I have commented out the test_compile_errors test case. The remaining tests are all passing.
I have not yet added an entry into the newsfragments folder but in the process of merging this PR, I can do this as well.

Let me know what you think.

@alex
Copy link
Contributor

alex commented Mar 13, 2025

Thanks for working on this.

Since this requires raising our MSRV significantly, this isn't something we'll be able to consider merging in the new future.

It looks like overwhelmingly these changes relate to how unsafe scoping works. I believe there are some warnings we can enable on older versions of Rust to get the same behavior? If so, it probably makes sense to see if those can be landed now, independent of raising the edition.

Copy link

codspeed-hq bot commented Mar 13, 2025

CodSpeed Performance Report

Merging #4976 will not alter performance

Comparing jonaspleyer:main (7f601f0) with main (f08cc60)

Summary

✅ 87 untouched benchmarks

@jonaspleyer
Copy link
Contributor Author

There are two lints which the new Rust edition mentions (see https://doc.rust-lang.org/edition-guide/rust-2024/index.html).

#![warn(missing_unsafe_on_extern, unsafe_op_in_unsafe_fn)]

https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#missing-unsafe-on-extern
https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unsafe-op-in-unsafe-fn

However, it would mean that we would need to apply these warnings for each crate in the workspace individually.

@alex
Copy link
Contributor

alex commented Mar 13, 2025 via email

@jonaspleyer
Copy link
Contributor Author

Benchmarks breakdown

Benchmark BASE HEAD Change
extract_hashbrown_set 16.1 ms 18.4 ms -12.62%

Since my changes have only been semantic, and not changed any functionality or versions of dependecies, this should not have introduced a regression this big. Either the test was an outlier or something else is happening when switching to the new edition and a new compiler version.

@jonaspleyer
Copy link
Contributor Author

Hmm, is there some way to use workspace settings to enable those warnings universally?

I am not sure about this. I have opened a thread on the users.rust-lang.org forum to see if anyone has a idea on how to do this.
https://users.rust-lang.org/t/warnings-on-workspace-level/126967
I will keep you updated here.

@davidhewitt
Copy link
Member

We should be able to configure the lints via the [workspace.lints.rust] setting already present in our Cargo.toml.

@jonaspleyer jonaspleyer force-pushed the main branch 2 times, most recently from d587fe7 to adf3685 Compare March 13, 2025 22:10
@jonaspleyer
Copy link
Contributor Author

I have rebased the branch to only include the following commits addressing the previously mentioned comments:

  • a4c52df mark ffi extern blocks and functions as unsafe
  • 866a6c5 mark unsafe function calls with keyword
  • e574347 mark unsafe function calls in tests as well

Hmm, is there some way to use workspace settings to enable those warnings
universally?

We should be able to configure the lints via the [workspace.lints.rust] setting already present in our Cargo.toml.

  • adf3685 add workspace lints to enhance migration to 2024 edition

@alex alex added the CI-skip-changelog Skip checking changelog entry label Mar 13, 2025
@davidhewitt
Copy link
Member

Thanks for moving forward with this. cargo fmt needs to be run else the CI won't try further.

I had a quick test and on our MSRV (1.63) unsafe extern is not valid syntax:

error: extern block cannot be declared unsafe
  --> pyo3-ffi/src/abstract_.rs:25:1
   |
25 | unsafe extern "C" {
   | ^^^^^^

... so we may have to drop a4c52df from this PR.

@jonaspleyer
Copy link
Contributor Author

... so we may have to drop a4c52df from this PR.

I have updated the PR and added another small commit for formatting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants