Various improvements to the incompatible_msrv lint#14433
Various improvements to the incompatible_msrv lint#14433Jarcho merged 6 commits intorust-lang:masterfrom
incompatible_msrv lint#14433Conversation
rustbot
added
the
S-waiting-on-review
samueltardieu
force-pushed
the
push-lorsokqwwpny
branch
3 times, most recently
from
2c30fe4 to
1296d5e
Compare
|
Rebased after rustup |
|
|
||
| /// Heuristic checking if the node `hir_id` is under a `#[cfg()]` or `#[cfg_attr()]` | ||
| /// attribute. | ||
| fn is_under_cfg_attribute(cx: &LateContext<'_>, hir_id: HirId) -> bool { |
There was a problem hiding this comment.
Should this only add the note under cfg(feature)? I think this is only useful otherwise for cfg(test), which feels like an edge case to me IMO. I feel like it's silly to even lint in tests, but what are your thoughts here?
There was a problem hiding this comment.
My rationale was that the lint only triggers when a MSRV is set on the current crate. However, when features are used, the author might want to express the fact that, in some configurations, they can go beyond the default MSRV. If the used entity is not behind a crate feature or a cfg item (possibly set by the build script), then it is likely that this is a real issue with the default MSRV, and it is probably a bad idea to suggest silencing Clippy with a #[clippy::msrv] attribute there.
For example, anyhow has a MSRV of 1.39, but when compiled with more recent compilers, it takes advantage of std::backtrace::Backtrace. Those uses are cfg gated. I feel this is the best place to make the suggestion without risking making a counterproductive one.
|
@rustbot author |
rustbot
added
S-waiting-on-author
samueltardieu
force-pushed
the
push-lorsokqwwpny
branch
from
1296d5e to
38bcc26
Compare
|
@rustbot review |
rustbot
added
S-waiting-on-review
samueltardieu
force-pushed
the
push-lorsokqwwpny
branch
from
38bcc26 to
026f308
Compare
|
Rebased, and adapted to the newer Rust sources: symbols for |
|
r? clippy |
This comment has been minimized.
This comment has been minimized.
The first non-MSRV-compatible item located under a `#[cfg(…)]` or `#[cfg_attr(…)]` attribute will get an additional note suggesting changing `#[clippy::msrv]` locally.
A later commit will introduce the non-linting of feature-enabled items. Switch to public items from `core` for the tests as to not interfere.
If an item has been enabled through a feature, it will not be linted even though the MSRV is not compatible. This use case may happen when stable compilers are allowed to enable unstable features, e.g. in the Rust for Linux toolchain.
New Rust releases also stabilize enum variants, constants, and so on. Lint them as well.
Also, update error marker for consistency.
samueltardieu
force-pushed
the
push-lorsokqwwpny
branch
from
026f308 to
68ea461
Compare
|
Rebased |
|
r? clippy |
|
Ping @Jarcho |
rustbot
removed
the
S-waiting-on-review
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
Thank you @samueltardieu!
|
|
||
| impl<'tcx> LateLintPass<'tcx> for IncompatibleMsrv { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { | ||
| // TODO: check for const stability when in const context |
There was a problem hiding this comment.
Is this limitation documented? Might be good to file an issue.
There was a problem hiding this comment.
I've opened #15297 about it, thanks for reminding me of it.
`clippy::incompatible_msrv` is not buying us much, and we discussed allowing it several times in the past. For instance, there was recently another patch sent to `allow` it where needed [1]. While that particular case would not be needed after the minimum version bump to 1.85.0, it is simpler to just allow it to prevent future instances. [ In addition, the lint fired without taking into account the features that have been enabled in a crate [2]. While this was improved in Rust 1.90.0 [3], it would still fire in a case like this patch. ] Thus do so, and remove the last instance of locally allowing it we have in the tree (except the one in the vendored `proc_macro2` crate). Note that we still keep the `msrv` config option in `clippy.toml` since that affects other lints as well. Link: https://lore.kernel.org/rust-for-linux/20260404212831.78971-4-jhubbard@nvidia.com/ [1] Link: rust-lang/rust-clippy#14425 [2] Link: rust-lang/rust-clippy#14433 [3] Link: https://patch.msgid.link/20260405235309.418950-8-ojeda@kernel.org Reviewed-by: Gary Guo <gary@garyguo.net> Reviewed-by: Tamir Duberstein <tamird@kernel.org> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
6 participants
This PR supersedes #14328 following the 2025-03-18 Clippy meeting discussion. It uses a simpler approach than what was proposed initially in #14328 and does not add new options.
First, it documents how
cfg_attrcan be used to change the MSRV considered by Clippy to trigger the lint. This allows the MSRV to be feature gated, or to be raised in tests.Also, the lint stops warning about items which have been explicitly allowed through a rustc feature. This works even if the feature has been stabilized since. It allows using an older compiler with some features turned on, as is done in Rust for Linux. This fixes #14425.
Then, if the lint triggers, and it looks like the code is located below a
cfgorcfg_attrattribute, an additional note is issued, once, to indicate that theclippy::msrvattribute can be controlled by an attribute.Finally, the lint is extended to cover any path, not just method and function calls. For example, enumeration variants, or constants, were not MSRV checked. This required replacing two
u32::MAXbyu32::max_value()in MSRV-limited tests.An extra commit adds a TODO for checking the const stability also, as this is not done right now.
@Centri3 I'll assign this to you because you were assigned #14328 and you were the one who nominated the issue for discussion (thanks!), but of course feel free to reroll!
r? @Centri3
changelog: [
incompatible_msrv]: better documentation, honor thefeaturesattribute, and lint non-function entities as well