Move to Rust 2024 #1422
Open
Move to Rust 2024 #1422
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Minor cleanup. This feature is not used by scylla-cql.
mismatched_lifetime_syntaxes is introduced in Rust 1.89. It warns when an elided lifetime (like &self) is bound to a hidden lifetime (like RawValue that is really RawValue<'_>).
In Rust 1.89, those structs started to trigger dead_code warning with information that they are never constructed. In row_tests / value_tests I don't want to allow/expect dead_code, because part of the reason for existence of those structs is to verify generated impls for those structs. The solution I chose is to write a never-called function that calles the relevant deserialize impl. This seems to work. Curiously, TestRowByName in hygiene tests doesn't trigger the warning. I am unable to tell why. I tried to replace derives with their generated impls, but I didn't notice any difference that could have caused this.
Such lifetimes are often written intentionally, to make code easier to read. Lint will start to trigger when we bump MSRV to 1.85, so lets allow it.
There is a lint for that that starts to trigger after MSRV is bumped to 1.85.
There is a lint for that that starts to trigger when MSRV is bumped to 1.85.
This time I bumped all the crates. Why? Because I want us to move to edition 2024.
"gen" became a reserved keyword in edition 2024. We want to move to that edition, so I renamed all of those to "generator". Fortunately, none of them appear in the public API.
Those became errors in 2024.
How this commit was created: I executed `cargo fix --edition`. After that I looked through the changes, and reverted the ones I thought were unnecessary. What changes I reverted? - Rust 2024 added new expression types matched by `expr` fragment specifier. For backwards compatibility `expr_2021` exists as well. cargo moves the code to use it, but for our macros `expr` is still fine - they are only internal after all. - In Rust 2024, lifetime of temporary expression in `if let Pattern = Expression` changed. Now it is dropped before entering `else` branch. Cargo switches the code to use `match` instead of `if let` for compatibility. I reviewed affected places in our code and concluded that we can keep using `if let` everywhere. That means that the only commited changes are related to RPIT lifetime capture rules. Cargo introduced `use` bounds in several places. Most of them are not important since they are not pub so we can change them later. Two places were pub, and cargo correctly preserved current semantics (which are also reasonable imo). The pub places: - `ColumnSpecs::iter`: `use<'slice, 'spec>` was introduced. We don't need or want to bind to lifetime of a self reference, only to lifetimes of held slice, so it makes sense. - `Sharder::iter_source_ports_for_shard`. `use<>` was introduced by Cargo. This is correct - the returned iterator does not depend on self. self is only needed to get the bounds of the port range. There is one thing that automatic migration doesn't handle the "outlives trick": https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html#migrating-away-from-the-outlives-trick I don't understand it completely unfortunately. We should understand it, and if needed migrate from it, before migrating to 2024 edition. We have a method like this for example: ``` pub(crate) fn iter_working_connections_per_node( &self, ) -> Result< impl Iterator<Item = (Uuid, impl Iterator<Item = Arc<Connection>> + use<>)> + '_, ConnectionPoolError, > ``` Is it an example of this trick? Should we change this `+ '_` to `use<'_>`?
Running `cargo fix --edition` in previous commit produced a lot of output. Vast majority of it is about drop order of temporaries changing in 2024. I briefly looked through some of those warnings, but I did not see anything interesting. I doubt we rely on this drop order anywhere. The other type of warning is about the RPIT lifetime capture rules. There is a case that the tool does not handle automatically and only warns about: https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html#migrating-cases-involving-apit I went through those warnings, and performed the changes manually.
Scope changes of temporaries in Rust 2024 would cause compilation error here, because the printer would be dropped too quickly. See more at https://doc.rust-lang.org/edition-guide/rust-2024/temporary-tail-expr-scope.html
Returning a just-bound let variable triggers a lint in Rust 2024.
I kept rustfmt edition at 2021 for now, because I want to introduce formatting changes in a separate commit.
Most of the changes are just reordered imports and function/macro calls with long arguments getting split into multiple lines.
This resolver is MSRV-aware which should remove the need for our Cargo.lock.msrv file.
It should not be needed anymore.
This simplifies the code, removing the need for locking.
It is imo more straightforward, easier to understand.
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the entire codebase to Rust 2024 edition while implementing necessary fixes for Clippy changes and bumping the MSRV to 1.85. The primary purpose is to modernize the codebase to take advantage of the latest Rust edition features and improvements.
- Updates all crates to edition 2024 and changes resolver to v3
- Fixes import ordering issues introduced by new Clippy lints in Rust 1.89
- Updates async closure usage in CCM code to utilize new async fn syntax
- Removes redundant Cargo.lock.msrv file since resolver v3 handles MSRV requirements
Reviewed Changes
Copilot reviewed 123 out of 124 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Cargo.toml files | Updated edition to "2024" and resolver to "3" across all crates |
| Import statements | Reordered imports to satisfy new Clippy lint requirements |
| CCM library code | Modernized async closure syntax and error handling patterns |
| Various source files | Applied automatic formatting changes for edition 2024 compatibility |
Comments suppressed due to low confidence (4)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
TODO:
See the commit Preparing for 2024 edition: automatic changes
There is a pattern that is not handled by automatic migration tool: https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html#migrating-away-from-the-outlives-trick
We have code like this in many places:
Is that an example of this trick? Should we change this code from
+ '_to+ use<'_>?Fixes: #1421
Pre-review checklist
I added relevant tests for new features and bug fixes.I have provided docstrings for the public items that I want to introduce.I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.