Skip to content

Upgrade to Rust 1.86 and bump MSRV to 1.84 #17171

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

Merged
merged 4 commits into from
Apr 3, 2025
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Apr 3, 2025

Summary

I decided to disable the new needless_continue rule because I often found the explicit continue more readable over an empty block or having to invert the condition of an other branch.

Test Plan

cargo test

Copy link
Contributor

github-actions bot commented Apr 3, 2025

mypy_primer results

No ecosystem changes detected ✅

Copy link
Contributor

github-actions bot commented Apr 3, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 3, 2025
@MichaReiser MichaReiser marked this pull request as ready for review April 3, 2025 12:45
op: Operator::Mod { .. },
op: Operator::Mod,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, is this now allowed or was it always allowed and I wasn't aware of it? i.e., matching against an enum struct variant without specifying the fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operator::Mod has no fields. But it seems that Rust still allows you to use { .. } in that case because that also matches "no-fields"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see and I'm guessing that it's the same for other struct variants for which the { .. } was removed in this PR.

@MichaReiser MichaReiser enabled auto-merge (squash) April 3, 2025 15:56
@MichaReiser MichaReiser merged commit 8a4158c into main Apr 3, 2025
21 checks passed
@MichaReiser MichaReiser deleted the micha/rust-1.86 branch April 3, 2025 15:59
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Sorry for the late review. I was in the middle of reviewing this when I scratched my cornea, so I figured I'd finish it haha.

&mut output,
"Derived from the **{}** linter.",
linter.name()
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda wonder if we should propose add something to std to make this nicer. I like the way the x.push_str(&format!("...")) construction reads much nicer, but I guess this approach avoids an intermediate allocation. But this approach has the very ugly let _ = ...; construction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great. I definitely don't love it and it has the downside that later refactoring the code to eg use a Write instead of a string now incorrectly suppresses errors (instead of Rust telling you that this might now fail)

dcreager added a commit that referenced this pull request Apr 3, 2025
* origin/main:
  [red-knot] Add `Type::TypeVar` variant (#17102)
  [red-knot] update to latest Salsa with fixpoint caching fix (#17179)
  Upgrade to Rust 1.86 and bump MSRV to 1.84 (#17171)
  [red-knot] Avoid unresolved-reference in unreachable code (#17169)
  Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path (#17178)
  [DO NOT LAND] bump Salsa version (#17176)
  [syntax-errors] Detect duplicate keys in `match` mapping patterns (#17129)
maxmynter pushed a commit to maxmynter/ruff that referenced this pull request Apr 3, 2025
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

I decided to disable the new
[`needless_continue`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue)
rule because I often found the explicit `continue` more readable over an
empty block or having to invert the condition of an other branch.


## Test Plan

`cargo test`

---------

Co-authored-by: Alex Waygood <[email protected]>
@eskultety
Copy link

Hello, I'd like to know what the motivation to pin the Rust version to the latest release in rust-toolchain.toml was? Were there any interesting features that this project required to adopt that quickly (literally on the same day of Rust release)?
The reason for my question is that IIUC you're essentially making it impossible for any distro out there to keep up with packaging and everyone is required to rely on rustup to get the exact toolchain version needed to build the project. Before I dive into any further discussion I'd first like to know where my lack of understanding the matter resides and therefore what the real motivation here was/is.

@MichaReiser
Copy link
Member Author

Hello, I'd like to know what the motivation to pin the Rust version to the latest release in rust-toolchain.toml was? Were there any interesting features that this project required to adopt that quickly (literally on the same day of Rust release)? The reason for my question is that IIUC you're essentially making it impossible for any distro out there to keep up with packaging and everyone is required to rely on rustup to get the exact toolchain version needed to build the project. Before I dive into any further discussion I'd first like to know where my lack of understanding the matter resides and therefore what the real motivation here was/is.

We're aware that always using the latest version is challenging for downstream packagers. That's why we only use the latest Rust version for local development (and in our release pipelines) but maintain backwards compatibility with latest - 2 as the MSRV with which ruff compiles (see versioning policy). That means Ruff can still be built with Rust 1.84 today.

I think what you're running into is that you clone Ruff's repository and invoke cargo build directly. Cargo then picks up the version specified in rust-toolchain.toml. The version in rust-toolchain.toml is the version we use and using the latest Rust version is desired because it gives us new clippy rules and enables new compiler optimizations in the release artifacts built as part of our pipeline.

I understand is that you want to use a different Rust version. You have a few options:

  • use rustup override set <version>
  • Use a toolchain override shorthand cargo +stable
  • Use the RUSTUP_TOOLCHAIN
  • Delete the rust-toolchain.toml

You can use any rust version that's compatible with our MSRV (1.84 today). It can be newer or older than the version in our rust-toolchain.toml but it can't be older than the MSRV.

I hope this helps

@eskultety
Copy link

That's why we only use the latest Rust version for local development (and in our release pipelines)

Hmm, have you considered adopting the latest & greatest primarily in nightly CI builds to get early feedback on new stuff but move the upstream dependency on a slower pace to essentially not "test in production" and by production means all local dev setups? Kinda what Fedora Rawhide is for and commonly consumed in projects' CIs.

I think what you're running into is that you clone Ruff's repository and invoke cargo build directly. Cargo then picks up the version specified in rust-toolchain.toml

Our use case is actually quite different (ties to cargo vendor, not important), but yes, the result is the same. We work in the secure software supply chain field with a strong compliance with the strictest SLSA levels which, among other things, means that builds are network-isolated, hence no rustup updates (also no rustup package). The other major factor (as if network isolation weren't enough :) ) is strict provenance, to put it simply - consuming packages via source tarballs is preferred over binaries.

Use a toolchain override shorthand cargo +stable
Use the RUSTUP_TOOLCHAIN
Delete the rust-toolchain.toml

We may actually consider some of ^this, thanks for the pointer, I admit I didn't pay thorough attention when reading https://rust-lang.github.io/rustup/overrides.html before dropping a comment on this PR. That said, if you say that rust-toolchain.toml is for development and to consume optimizations early for your binary artifacts, would you consider excluding the file from the Python sdist since based on the premise (and based on your backwards compatibility claim) it's irrelevant to have it there for released packages.

@MichaReiser
Copy link
Member Author

That said, if you say that rust-toolchain.toml is for development and to consume optimizations early for your binary artifacts, would you consider excluding the file from the Python sdist since based on the premise (and based on your backwards compatibility claim) it's irrelevant to have it there for released packages.

I think that makes sense

@MichaReiser
Copy link
Member Author

I created #17909. I'm trying to figure out why we included it in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants