-
Notifications
You must be signed in to change notification settings - Fork 13k
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
introduce future-compatibility warning for forbidden lint groups #81556
Conversation
cc @pnkfelix |
); | ||
let lint_name_str = &*lint_name.as_str(); | ||
self.lint_groups.contains_key(&lint_name_str) || { | ||
let warnings_name_str = crate::WARNINGS.name_lower(); |
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.
For some reason, WARNINGS
is not in this list.
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.
Hmm. Are there any other "obvious" groups that might also be not on that list for some reason?
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.
No. The reason for WARNINGS is that it is mega hardcoded in the way that it's implemented; it's not a real lint group.
); | ||
decorate_diag_builder(diag_builder); | ||
} else { | ||
self.struct_lint( |
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 logic for creating lints and errors is setup in annoyingly non-analogous ways, but I resisted deeper refactorings.
lint_name, | ||
self.lint_groups.keys().collect::<Vec<_>>() | ||
); | ||
let lint_name_str = &*lint_name.as_str(); |
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 don't know why everything with lints uses as keys, but I couldn't find other ways to do comparisons.
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -0,0 +1,13 @@ | |||
// Check what happens when we forbid a smaller group but | |||
// then deny a superset of that group. |
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.
nit on comment: you're allowing the superset warnings
below, not denying it.
|
||
#![forbid(nonstandard_style)] | ||
|
||
// FIXME: Arguably this should be an error, but the WARNINGS group is |
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.
does this mean you should have a different test that tries this subset/superset case using something other than the special cased WARNINGS group?
(or is the WARNINGS group the only instance of a superset in the first place...?)
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 couldn't find any other group to use.
//~| WARNING being phased out | ||
//~| WARNING incompatible with previous forbid | ||
//~| WARNING being phased out | ||
//~| WARNING incompatible with previous forbid |
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.
sigh. I'm sure the 4x repetition is an artifact of the existing code, and we don't want to deal with fixing it on something that we are beta-backporting. (And I hope that the compiler's de-duplication of diagnostics turns it into a single message.)
But still: Sigh.
@bors r+ |
📌 Commit 59c43a7a1efb653573e3c9623c35189208362cc9 has been approved by |
@bors r- This has errors that need to be resolved first. |
59c43a7
to
66ce370
Compare
We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect.
66ce370
to
b6b897b
Compare
@bors r=pnkfelix |
📌 Commit b6b897b has been approved by |
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? `@Mark-Simulacrum`
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ``@Mark-Simulacrum``
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ```@Mark-Simulacrum```
beta backport approved as per team compiler meeting discussion |
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? `@Mark-Simulacrum`
Rollup of 9 pull requests Successful merges: - rust-lang#74304 (Stabilize the Wake trait) - rust-lang#79805 (Rename Iterator::fold_first to reduce and stabilize it) - rust-lang#81556 (introduce future-compatibility warning for forbidden lint groups) - rust-lang#81645 (Add lint for `panic!(123)` which is not accepted in Rust 2021.) - rust-lang#81710 (OsStr eq_ignore_ascii_case takes arg by value) - rust-lang#81711 (add #[inline] to all the public IpAddr functions) - rust-lang#81725 (Move test to be with the others) - rust-lang#81727 (Revert stabilizing integer::BITS.) - rust-lang#81745 (Stabilize poison API of Once, rename poisoned()) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…lint, r=pnkfelix introduce future-compatibility warning for forbidden lint groups We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218). This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect. r? ``@Mark-Simulacrum``
[beta] backports This backports: * CI: only copy python.exe to python3.exe if the latter does not exist rust-lang#81762 * Make hitting the recursion limit in projection non-fatal rust-lang#81055 * Remove incorrect `delay_span_bug` rust-lang#81532 * introduce future-compatibility warning for forbidden lint groups rust-lang#81556 * Update cargo rust-lang#81755 * rustdoc: Fix visibility of trait and impl items rust-lang#81288 * Work around missing -dev packages in solaris docker image. rust-lang#81229 * Update LayoutError/LayoutErr stability attributes rust-lang#81767 * Revert 78373 ("dont leak return value after panic in drop") rust-lang#81257 * Rename `panic_fmt` lint to `non_fmt_panic` rust-lang#81729
We used to ignore
forbid(group)
scenarios completely. This changed in #78864, but that led to a number of regressions (#80988, #81218).This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect.
r? @Mark-Simulacrum