Various improvements to the incompatible_msrv lint
#14433
Conversation
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
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
1296d5e to
38bcc26
Compare
|
@rustbot review |
rustbot
added
S-waiting-on-review
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