-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
add unreachable_cfg_select_predicates lint
#149960
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: main
Are you sure you want to change the base?
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing |
|
rustbot has assigned @WaffleLapkin. Use |
|
The proposed name for this lint seems quite unfortunate to me. When I first read it I though it had something to do with the I think it would be better include the word "arm" or "cfg_select" in it, to clearly disambiguate it from the other places where cfgs can appear. Maybe |
|
There is some reasoning for then name at #149783 (comment). cc @traviscross was there any particular reason for you to leave out the select or arm/branch parts? |
|
The reasoning was: For Here that would suggest the name But we have an existing lint, To your point about detecting Sitting with it now, though, I think |
|
☔ The latest upstream changes (presumably #146348) made this pull request unmergeable. Please resolve the merge conflicts. |
This is emitted on branches of a `cfg_select!` that are statically known to be unreachable.
542a992 to
20f27e5
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
unreachable_cfgs lintunreachable_cfg_select_predicates lint
|
I do like |
| // `#[feature(cfg_select)]` is a libs feature (so, not a lang feature), but it lints on unreachable | ||
| // branches, and that lint should only be emitted when the feature is enabled. |
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.
Q: why should it only lint when the feature is enabled?
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.
Because then no separate FCP is required to merge this lint. At least that is my interpretation of #149783 (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.
Probably what I had in mind is to go ahead and make it into (or otherwise add) a lang feature gate and then to feature gate the lint itself in the declare_lint!.
| Future breakage diagnostic: | ||
| error: warn(unused) incompatible with previous forbid | ||
| --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:22:13 | ||
| | | ||
| LL | #![forbid(unused)] | ||
| | ------ `forbid` level set here | ||
| LL | #![deny(unused)] | ||
| LL | #![warn(unused)] | ||
| | ^^^^^^ overruled by previous forbid | ||
| | | ||
| = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! | ||
| = note: for more information, see issue #81670 <https://github.com/rust-lang/rust/issues/81670> | ||
| note: the lint level is defined here | ||
| --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:17:11 | ||
| | | ||
| LL | #![forbid(forbidden_lint_groups)] | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
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.
What happened, how is this change related?
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.
The test has this comment
// If you turn off deduplicate diagnostics (which rustc turns on by default but
// compiletest turns off when it runs ui tests), then the errors are
// (unfortunately) repeated here because the checking is done as we read in the
// errors, and currently that happens two or three different times, depending on
// compiler flags.
//
// I decided avoiding the redundant output was not worth the time in engineering
// effort for bug like this, which 1. end users are unlikely to run into in the
// first place, and 2. they won't see the redundant output anyway.
//@ compile-flags: -Z deduplicate-diagnostics=yes
I believe that the logic here added another time that the error is emitted. That's unfortunate but as the comment mentions it's kind of not worth the engineering effort to fix because it is unlikely that end users ever see this repeated error.
| if let Some((underscore, _, _)) = branches.wildcard | ||
| && features.map_or(false, |f| f.enabled(rustc_span::sym::cfg_select)) | ||
| { | ||
| for (predicate, _, _) in &branches.unreachable { | ||
| let span = predicate.span(); | ||
| p.psess.buffer_lint( | ||
| UNREACHABLE_CFG_SELECT_PREDICATES, | ||
| span, | ||
| lint_node_id, | ||
| BuiltinLintDiag::UnreachableCfg { span, wildcard_span: underscore.span }, | ||
| ); | ||
| } | ||
| } |
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 doesn't seem to warn, unless there is a wildcard, why so? Does it not warn in case of
cfg_select! {
unix => true,
not(unix) => false,
windows => false,
}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.
That is correct, it's hard to do better than that. we'd need to actually have some sort of logic solver to do so in general, and even then I think it might misfire if there is some sort of feature flag implication that the checker does not know about.
So the current imlementation is basic but reliable.
|
@rustbot ready |
tracking issue: #115585
Split out from #149783. This lint is emitted on branches of a
cfg_select!that are statically known to be unreachable. The lint is only emitted when the feature is enabled, so this change specifically does not need an FCP, and the lint will be stabilized alongside the feature (see #149783 (comment)).