-
Notifications
You must be signed in to change notification settings - Fork 48
Remove if_chain crate now that we're on MSRV 1.88
#1863
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
base: master
Are you sure you want to change the base?
Remove if_chain crate now that we're on MSRV 1.88
#1863
Conversation
Update the root Cargo.lock file to remove if_chain dependency references after the crate was removed from the workspace.
Don't enable the let_chains nightly feature
This reverts commit a86f6b8.
|
@smoelius I'm curious if you have a recommended fix for the failing build as of f924fdc. Originally I tried to fix it with a86f6b8, but that was pretty naive. The problem as I understand it is that some of the nightly toolchains in use in this project are old enough that they require |
|
Hey, @brannondorsey. I am currrently traveling, but I will try to look at this in the next couple of days. |
|
No problem @smoelius. Safe travels! |
smoelius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me longer than expected to get to this. Thanks again for working on this.
internal/src/lib.rs
Outdated
| #![cfg_attr(dylint_lib = "general", allow(crate_wide_allow))] | ||
| #![cfg_attr(dylint_lib = "supplementary", allow(nonexistent_path_in_comment))] | ||
| #![cfg_attr(nightly, feature(rustc_private))] | ||
| #![cfg_attr(nightly, feature(let_chains))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #![cfg_attr(nightly, feature(let_chains))] |
Please remove the above.
And please change the following line:
| let rust_version = Version::parse(msrv::MSRV).unwrap(); |
to:
let rust_version = Version::parse(msrv::MSRV_PLUS_1).unwrap();Let chains were stabilized in Rust 1.88.0. So I was confused as to why MSRV (=1.88.0) was not working. I think the reason is that MSRV_CHANNEL = nightly-2025-04-22 is when Clippy switched to version 0.1.88, not when Rust switched to 1.88.0. The latter occurred around 2025-06-23.
Anyway, when I test locally with MSRV_PLUS_1, it seems to work. Please tell me if your experience is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied these suggested changes, however CI tests fail here with:
error[E0658]: `let` expressions in this position are unstable
--> /home/runner/work/dylint/dylint/internal/src/clippy_utils/revs_no_preinstall.rs:120:20
|
120 | if let Some(prev_rev) = prev_rev
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
= help: add `#![feature(let_chains)]` to the crate attributes to enable
= note: this compiler was built on 2025-04-21; consider upgrading it if it is out of date
I think the reason is that the list::one_name_multiple_chains integration test relies on MSRV_CHANNEL (see here), which requires the let_chains feature.
You can reproduce this failing test in isolation with:
cargo test --package cargo-dylint --test integration one_name_multiple_toolchains
I previously tried to resolve this via a86f6b8 but that fails because of breaking clippy_utils changes changes on nightly.
I got a little scared off by that change. But revising it today, it looks like I can get that test to pass by bumping the MSRV nightly day by exactly one day, which resolves the error without introducing breaking API changes (see 307626d). 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info log from rustup gives a hint as to why 307626d works...
Building dylint-template with channel `nightly-2025-04-22`
info: syncing channel updates for 'nightly-2025-04-22-aarch64-apple-darwin'
info: latest update on 2025-04-22, rust version 1.88.0-nightly (d6c1e454a 2025-04-21)
nightly-2025-04-22 actually uses a release built on 2025-04-21 (although confusingly, it does include 1.88- in the name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that resolves most of the errors and we're on to just one more failure.
https://github.com/trailofbits/dylint/actions/runs/20079593588/job/57602989209
Maybe we just need to bump MSRV_CLIPPY_UTILS_REV now?
This was produced by running: ``` cargo nested fetch ```
|
Let me please look at this some more. |
This fixes the last::one_name_multiple_chains test
This one better matches the commit we're currently using in the trunk branch
Completes the task originally discussed in #1806.