Auto merge of #78864 - Mark-Simulacrum:warn-on-forbids, r=pnkfelix

Use true previous lint level when detecting overriden forbids Previously, cap-lints was ignored when checking the previous forbid level, which meant that it was a hard error to do so. This is different from the normal behavior of lints, which are silenced by cap-lints; if the forbid would not take effect regardless, there is not much point in complaining about the fact that we are reducing its level. It might be considered a bug that even `--cap-lints deny` would suffice to silence the error on overriding forbid, depending on if one cares about failing the build or precisely forbid being set. But setting cap-lints to deny is quite odd and not really done in practice, so we don't try to handle it specially. This also unifies the code paths for nested and same-level scopes. However, the special case for CLI lint flags is left in place (introduced by #70918) to fix the regression noted in #70819. That means that CLI flags do not lint on forbid being overridden by a non-forbid level. It is unclear whether this is a bug or a desirable feature, but it is certainly inconsistent. CLI flags are a sufficiently different "type" of place though that this is deemed out of scope for this commit. r? `@pnkfelix` perhaps? cc #77713 -- not marking as "Fixes" because of the lack of proper unused attribute handling in this PR
rust-lang · Dec 2, 2020 · eb4860c · eb4860c
2 parents 18aa5ee + 64efcbe
commit eb4860c
Show file tree

Hide file tree

Showing 18 changed files with 126 additions and 234 deletions.
diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs
@@ -108,18 +108,32 @@ impl<'s> LintLevelsBuilder<'s> {
         id: LintId,
         (level, src): LevelSource,
     ) {
-        if let Some((old_level, old_src)) = specs.get(&id) {
-            if old_level == &Level::Forbid && level != Level::Forbid {
+        // Setting to a non-forbid level is an error if the lint previously had
+        // a forbid level. Note that this is not necessarily true even with a
+        // `#[forbid(..)]` attribute present, as that is overriden by `--cap-lints`.
+        //
+        // This means that this only errors if we're truly lowering the lint
+        // level from forbid.
+        if level != Level::Forbid {
+            if let (Level::Forbid, old_src) =
+                self.sets.get_lint_level(id.lint, self.cur, Some(&specs), &self.sess)
+            {
                 let mut diag_builder = struct_span_err!(
                     self.sess,
                     src.span(),
                     E0453,
-                    "{}({}) incompatible with previous forbid in same scope",
+                    "{}({}) incompatible with previous forbid",
                     level.as_str(),
                     src.name(),
                 );
-                match *old_src {
-                    LintSource::Default => {}
+                diag_builder.span_label(src.span(), "overruled by previous forbid");
+                match old_src {
+                    LintSource::Default => {
+                        diag_builder.note(&format!(
+                            "`forbid` lint level is the default for {}",
+                            id.to_string()
+                        ));
+                    }
                     LintSource::Node(_, forbid_source_span, reason) => {
                         diag_builder.span_label(forbid_source_span, "`forbid` level set here");
                         if let Some(rationale) = reason {
@@ -131,6 +145,8 @@ impl<'s> LintLevelsBuilder<'s> {
                     }
                 }
                 diag_builder.emit();
+
+                // Retain the forbid lint level
                 return;
             }
         }
@@ -414,50 +430,6 @@ impl<'s> LintLevelsBuilder<'s> {
             }
         }
 
-        for (id, &(level, ref src)) in specs.iter() {
-            if level == Level::Forbid {
-                continue;
-            }
-            let forbid_src = match self.sets.get_lint_id_level(*id, self.cur, None) {
-                (Some(Level::Forbid), src) => src,
-                _ => continue,
-            };
-            let forbidden_lint_name = match forbid_src {
-                LintSource::Default => id.to_string(),
-                LintSource::Node(name, _, _) => name.to_string(),
-                LintSource::CommandLine(name, _) => name.to_string(),
-            };
-            let (lint_attr_name, lint_attr_span) = match *src {
-                LintSource::Node(name, span, _) => (name, span),
-                _ => continue,
-            };
-            let mut diag_builder = struct_span_err!(
-                self.sess,
-                lint_attr_span,
-                E0453,
-                "{}({}) overruled by outer forbid({})",
-                level.as_str(),
-                lint_attr_name,
-                forbidden_lint_name
-            );
-            diag_builder.span_label(lint_attr_span, "overruled by previous forbid");
-            match forbid_src {
-                LintSource::Default => {}
-                LintSource::Node(_, forbid_source_span, reason) => {
-                    diag_builder.span_label(forbid_source_span, "`forbid` level set here");
-                    if let Some(rationale) = reason {
-                        diag_builder.note(&rationale.as_str());
-                    }
-                }
-                LintSource::CommandLine(_, _) => {
-                    diag_builder.note("`forbid` lint level was set on command line");
-                }
-            }
-            diag_builder.emit();
-            // don't set a separate error for every lint in the group
-            break;
-        }
-
         let prev = self.cur;
         if !specs.is_empty() {
             self.cur = self.sets.list.len() as u32;

diff --git a/src/test/ui-fulldeps/lint-plugin-forbid-attrs.rs b/src/test/ui-fulldeps/lint-plugin-forbid-attrs.rs
@@ -6,12 +6,12 @@
 //~^ WARN use of deprecated attribute `plugin`
 #![forbid(test_lint)]
 
-fn lintme() { } //~ ERROR item is named 'lintme'
+fn lintme() {} //~ ERROR item is named 'lintme'
 
 #[allow(test_lint)]
-//~^ ERROR allow(test_lint) overruled by outer forbid(test_lint)
-//~| ERROR allow(test_lint) overruled by outer forbid(test_lint)
-//~| ERROR allow(test_lint) overruled by outer forbid(test_lint)
+//~^ ERROR allow(test_lint) incompatible
+//~| ERROR allow(test_lint) incompatible
+//~| ERROR allow(test_lint) incompatible
 pub fn main() {
     lintme();
 }
diff --git a/src/test/ui-fulldeps/lint-plugin-forbid-attrs.stderr b/src/test/ui-fulldeps/lint-plugin-forbid-attrs.stderr
@@ -1,4 +1,4 @@
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-attrs.rs:11:9
    |
 LL | #![forbid(test_lint)]
@@ -7,7 +7,7 @@ LL | #![forbid(test_lint)]
 LL | #[allow(test_lint)]
    |         ^^^^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-attrs.rs:11:9
    |
 LL | #![forbid(test_lint)]
@@ -27,16 +27,16 @@ LL | #![plugin(lint_plugin_test)]
 error: item is named 'lintme'
   --> $DIR/lint-plugin-forbid-attrs.rs:9:1
    |
-LL | fn lintme() { }
-   | ^^^^^^^^^^^^^^^
+LL | fn lintme() {}
+   | ^^^^^^^^^^^^^^
    |
 note: the lint level is defined here
   --> $DIR/lint-plugin-forbid-attrs.rs:7:11
    |
 LL | #![forbid(test_lint)]
    |           ^^^^^^^^^
 
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-attrs.rs:11:9
    |
 LL | #![forbid(test_lint)]

diff --git a/src/test/ui-fulldeps/lint-plugin-forbid-cmdline.rs b/src/test/ui-fulldeps/lint-plugin-forbid-cmdline.rs
@@ -7,9 +7,9 @@
 //~^ WARN use of deprecated attribute `plugin`
 fn lintme() { } //~ ERROR item is named 'lintme'
 
-#[allow(test_lint)] //~ ERROR allow(test_lint) overruled by outer forbid(test_lint)
-                    //~| ERROR allow(test_lint) overruled by outer forbid(test_lint)
-                    //~| ERROR allow(test_lint) overruled by outer forbid(test_lint)
+#[allow(test_lint)] //~ ERROR allow(test_lint) incompatible
+                    //~| ERROR allow(test_lint) incompatible
+                    //~| ERROR allow(test_lint)
 pub fn main() {
     lintme();
 }
diff --git a/src/test/ui-fulldeps/lint-plugin-forbid-cmdline.stderr b/src/test/ui-fulldeps/lint-plugin-forbid-cmdline.stderr
@@ -1,12 +1,12 @@
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-cmdline.rs:10:9
    |
 LL | #[allow(test_lint)]
    |         ^^^^^^^^^ overruled by previous forbid
    |
    = note: `forbid` lint level was set on command line
 
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-cmdline.rs:10:9
    |
 LL | #[allow(test_lint)]
@@ -30,7 +30,7 @@ LL | fn lintme() { }
    |
    = note: requested on the command line with `-F test-lint`
 
-error[E0453]: allow(test_lint) overruled by outer forbid(test_lint)
+error[E0453]: allow(test_lint) incompatible with previous forbid
   --> $DIR/lint-plugin-forbid-cmdline.rs:10:9
    |
 LL | #[allow(test_lint)]

diff --git a/src/test/ui/error-codes/E0453.rs b/src/test/ui/error-codes/E0453.rs
@@ -1,8 +1,8 @@
 #![forbid(non_snake_case)]
 
 #[allow(non_snake_case)]
-//~^ ERROR allow(non_snake_case) overruled by outer forbid(non_snake_case)
-//~| ERROR allow(non_snake_case) overruled by outer forbid(non_snake_case)
-//~| ERROR allow(non_snake_case) overruled by outer forbid(non_snake_case)
+//~^ ERROR allow(non_snake_case) incompatible
+//~| ERROR allow(non_snake_case) incompatible
+//~| ERROR allow(non_snake_case) incompatible
 fn main() {
 }
diff --git a/src/test/ui/error-codes/E0453.stderr b/src/test/ui/error-codes/E0453.stderr
@@ -1,4 +1,4 @@
-error[E0453]: allow(non_snake_case) overruled by outer forbid(non_snake_case)
+error[E0453]: allow(non_snake_case) incompatible with previous forbid
   --> $DIR/E0453.rs:3:9
    |
 LL | #![forbid(non_snake_case)]
@@ -7,7 +7,7 @@ LL |
 LL | #[allow(non_snake_case)]
    |         ^^^^^^^^^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(non_snake_case) overruled by outer forbid(non_snake_case)
+error[E0453]: allow(non_snake_case) incompatible with previous forbid
   --> $DIR/E0453.rs:3:9
    |
 LL | #![forbid(non_snake_case)]
@@ -16,7 +16,7 @@ LL |
 LL | #[allow(non_snake_case)]
    |         ^^^^^^^^^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(non_snake_case) overruled by outer forbid(non_snake_case)
+error[E0453]: allow(non_snake_case) incompatible with previous forbid
   --> $DIR/E0453.rs:3:9
    |
 LL | #![forbid(non_snake_case)]

diff --git a/src/test/ui/lint/forbid-error-capped.rs b/src/test/ui/lint/forbid-error-capped.rs
@@ -0,0 +1,15 @@
+// check-pass
+// compile-args: --cap-lints=warn -Fwarnings
+
+// This checks that the forbid attribute checking is ignored when the forbidden
+// lint is capped.
+
+#![forbid(warnings)]
+#![allow(unused)]
+
+#[allow(unused)]
+mod bar {
+    fn bar() {}
+}
+
+fn main() {}
diff --git a/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs
@@ -17,11 +17,11 @@
 fn forbid_first(num: i32) -> i32 {
     #![forbid(unused)]
     #![deny(unused)]
-    //~^ ERROR: deny(unused) incompatible with previous forbid in same scope [E0453]
+    //~^ ERROR: deny(unused) incompatible with previous forbid
     #![warn(unused)]
-    //~^ ERROR: warn(unused) incompatible with previous forbid in same scope [E0453]
+    //~^ ERROR: warn(unused) incompatible with previous forbid
     #![allow(unused)]
-    //~^ ERROR: allow(unused) incompatible with previous forbid in same scope [E0453]
+    //~^ ERROR: allow(unused) incompatible with previous forbid
 
     num * num
 }

diff --git a/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr
@@ -1,28 +1,28 @@
-error[E0453]: deny(unused) incompatible with previous forbid in same scope
+error[E0453]: deny(unused) incompatible with previous forbid
   --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:19:13
    |
 LL |     #![forbid(unused)]
    |               ------ `forbid` level set here
 LL |     #![deny(unused)]
-   |             ^^^^^^
+   |             ^^^^^^ overruled by previous forbid
 
-error[E0453]: warn(unused) incompatible with previous forbid in same scope
+error[E0453]: warn(unused) incompatible with previous forbid
   --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:21:13
    |
 LL |     #![forbid(unused)]
    |               ------ `forbid` level set here
 ...
 LL |     #![warn(unused)]
-   |             ^^^^^^
+   |             ^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(unused) incompatible with previous forbid in same scope
+error[E0453]: allow(unused) incompatible with previous forbid
   --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:23:14
    |
 LL |     #![forbid(unused)]
    |               ------ `forbid` level set here
 ...
 LL |     #![allow(unused)]
-   |              ^^^^^^
+   |              ^^^^^^ overruled by previous forbid
 
 error: aborting due to 3 previous errors
 

diff --git a/src/test/ui/lint/lint-forbid-attr.rs b/src/test/ui/lint/lint-forbid-attr.rs
@@ -1,8 +1,8 @@
 #![forbid(deprecated)]
 
 #[allow(deprecated)]
-//~^ ERROR allow(deprecated) overruled by outer forbid(deprecated)
-//~| ERROR allow(deprecated) overruled by outer forbid(deprecated)
-//~| ERROR allow(deprecated) overruled by outer forbid(deprecated)
+//~^ ERROR allow(deprecated) incompatible
+//~| ERROR allow(deprecated) incompatible
+//~| ERROR allow(deprecated) incompatible
 fn main() {
 }
diff --git a/src/test/ui/lint/lint-forbid-attr.stderr b/src/test/ui/lint/lint-forbid-attr.stderr
@@ -1,4 +1,4 @@
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-attr.rs:3:9
    |
 LL | #![forbid(deprecated)]
@@ -7,7 +7,7 @@ LL |
 LL | #[allow(deprecated)]
    |         ^^^^^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-attr.rs:3:9
    |
 LL | #![forbid(deprecated)]
@@ -16,7 +16,7 @@ LL |
 LL | #[allow(deprecated)]
    |         ^^^^^^^^^^ overruled by previous forbid
 
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-attr.rs:3:9
    |
 LL | #![forbid(deprecated)]

diff --git a/src/test/ui/lint/lint-forbid-cmdline.rs b/src/test/ui/lint/lint-forbid-cmdline.rs
@@ -1,7 +1,7 @@
 // compile-flags: -F deprecated
 
-#[allow(deprecated)] //~ ERROR allow(deprecated) overruled by outer forbid(deprecated)
-                     //~| ERROR allow(deprecated) overruled by outer forbid(deprecated)
-                     //~| ERROR allow(deprecated) overruled by outer forbid(deprecated)
+#[allow(deprecated)] //~ ERROR allow(deprecated) incompatible
+                     //~| ERROR allow(deprecated) incompatible
+                     //~| ERROR allow(deprecated) incompatible
 fn main() {
 }
diff --git a/src/test/ui/lint/lint-forbid-cmdline.stderr b/src/test/ui/lint/lint-forbid-cmdline.stderr
@@ -1,20 +1,20 @@
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-cmdline.rs:3:9
    |
 LL | #[allow(deprecated)]
    |         ^^^^^^^^^^ overruled by previous forbid
    |
    = note: `forbid` lint level was set on command line
 
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-cmdline.rs:3:9
    |
 LL | #[allow(deprecated)]
    |         ^^^^^^^^^^ overruled by previous forbid
    |
    = note: `forbid` lint level was set on command line
 
-error[E0453]: allow(deprecated) overruled by outer forbid(deprecated)
+error[E0453]: allow(deprecated) incompatible with previous forbid
   --> $DIR/lint-forbid-cmdline.rs:3:9
    |
 LL | #[allow(deprecated)]

diff --git a/src/test/ui/lint/outer-forbid.rs b/src/test/ui/lint/outer-forbid.rs
@@ -4,21 +4,25 @@
 // subsequent allowance of a lint group containing it (here, `nonstandard_style`). See
 // Issue #42873.
 
+// 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.
+//
+// The test is much cleaner if we deduplicate, though.
+
+// compile-flags: -Z deduplicate-diagnostics=yes
+
 #![forbid(unused, non_snake_case)]
 
-#[allow(unused_variables)] //~ ERROR overruled
-                           //~| ERROR overruled
-                           //~| ERROR overruled
+#[allow(unused_variables)] //~ ERROR incompatible with previous
 fn foo() {}
 
-#[allow(unused)] //~ ERROR overruled
-                 //~| ERROR overruled
-                 //~| ERROR overruled
+#[allow(unused)] //~ ERROR incompatible with previous
 fn bar() {}
 
-#[allow(nonstandard_style)] //~ ERROR overruled
-                            //~| ERROR overruled
-                            //~| ERROR overruled
+#[allow(nonstandard_style)] //~ ERROR incompatible with previous
 fn main() {
     println!("hello forbidden world")
 }