Rollup merge of rust-lang#73379 - pnkfelix:issue-70819-disallow-override-forbid-in-same-scope, r=petrochenkov

Disallow later override of forbid lint in same scope

Fix rust-lang#70819

When building lint specs map for a given scope, check if forbid present on each insert.

Drive-by changes:

  1. make `LintLevelsBuilder::push` private to the crate.

  2. Add methods to `LintSource` for extracting its name symbol or its span.

  3. Rewrote old test so that its use of lint attributes are consistent with what we want in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...)

Author: tmandry

src/librustc_lint/levels.rs

@@ -10,6 +10,7 @@ use rustc_hir as hir;
 use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
 use rustc_hir::{intravisit, HirId};
 use rustc_middle::hir::map::Map;
+use rustc_middle::lint::LevelSource;
 use rustc_middle::lint::LintDiagnosticBuilder;
 use rustc_middle::lint::{struct_lint_level, LintLevelMap, LintLevelSets, LintSet, LintSource};
 use rustc_middle::ty::query::Providers;
@@ -95,6 +96,44 @@ impl<'s> LintLevelsBuilder<'s> {
         self.sets.list.push(LintSet::CommandLine { specs });
     }
 
+    /// Attempts to insert the `id` to `level_src` map entry. If unsuccessful
+    /// (e.g. if a forbid was already inserted on the same scope), then emits a
+    /// diagnostic with no change to `specs`.
+    fn insert_spec(
+        &mut self,
+        specs: &mut FxHashMap<LintId, LevelSource>,
+        id: LintId,
+        (level, src): LevelSource,
+    ) {
+        if let Some((old_level, old_src)) = specs.get(&id) {
+            if old_level == &Level::Forbid && level != Level::Forbid {
+                let mut diag_builder = struct_span_err!(
+                    self.sess,
+                    src.span(),
+                    E0453,
+                    "{}({}) incompatible with previous forbid in same scope",
+                    level.as_str(),
+                    src.name(),
+                );
+                match *old_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();
+                return;
+            }
+        }
+        specs.insert(id, (level, src));
+    }
+
     /// Pushes a list of AST lint attributes onto this context.
     ///
     /// This function will return a `BuilderPush` object which should be passed
@@ -109,7 +148,7 @@ impl<'s> LintLevelsBuilder<'s> {
     ///   `#[allow]`
     ///
     /// Don't forget to call `pop`!
-    pub fn push(
+    pub(crate) fn push(
         &mut self,
         attrs: &[ast::Attribute],
         store: &LintStore,
@@ -221,7 +260,7 @@ impl<'s> LintLevelsBuilder<'s> {
                         let src = LintSource::Node(name, li.span(), reason);
                         for &id in ids {
                             self.check_gated_lint(id, attr.span);
-                            specs.insert(id, (level, src));
+                            self.insert_spec(&mut specs, id, (level, src));
                         }
                     }
 
@@ -235,7 +274,7 @@ impl<'s> LintLevelsBuilder<'s> {
                                     reason,
                                 );
                                 for id in ids {
-                                    specs.insert(*id, (level, src));
+                                    self.insert_spec(&mut specs, *id, (level, src));
                                 }
                             }
                             Err((Some(ids), new_lint_name)) => {
@@ -272,7 +311,7 @@ impl<'s> LintLevelsBuilder<'s> {
                                     reason,
                                 );
                                 for id in ids {
-                                    specs.insert(*id, (level, src));
+                                    self.insert_spec(&mut specs, *id, (level, src));
                                 }
                             }
                             Err((None, _)) => {

src/librustc_middle/lint.rs

@@ -9,7 +9,7 @@ use rustc_session::lint::{builtin, Level, Lint, LintId};
 use rustc_session::{DiagnosticMessageId, Session};
 use rustc_span::hygiene::MacroKind;
 use rustc_span::source_map::{DesugaringKind, ExpnKind, MultiSpan};
-use rustc_span::{Span, Symbol};
+use rustc_span::{symbol, Span, Symbol, DUMMY_SP};
 
 /// How a lint level was set.
 #[derive(Clone, Copy, PartialEq, Eq, HashStable)]
@@ -25,6 +25,24 @@ pub enum LintSource {
     CommandLine(Symbol),
 }
 
+impl LintSource {
+    pub fn name(&self) -> Symbol {
+        match *self {
+            LintSource::Default => symbol::kw::Default,
+            LintSource::Node(name, _, _) => name,
+            LintSource::CommandLine(name) => name,
+        }
+    }
+
+    pub fn span(&self) -> Span {
+        match *self {
+            LintSource::Default => DUMMY_SP,
+            LintSource::Node(_, span, _) => span,
+            LintSource::CommandLine(_) => DUMMY_SP,
+        }
+    }
+}
+
 pub type LevelSource = (Level, LintSource);
 
 pub struct LintLevelSets {

src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs

@@ -0,0 +1,49 @@
+// This test is checking that you cannot override a `forbid` by adding in other
+// attributes later in the same scope. (We already ensure that you cannot
+// override it in nested scopes).
+
+// 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 curretly 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
+
+fn forbid_first(num: i32) -> i32 {
+    #![forbid(unused)]
+    #![deny(unused)]
+    //~^ ERROR: deny(unused) incompatible with previous forbid in same scope [E0453]
+    #![warn(unused)]
+    //~^ ERROR: warn(unused) incompatible with previous forbid in same scope [E0453]
+    #![allow(unused)]
+    //~^ ERROR: allow(unused) incompatible with previous forbid in same scope [E0453]
+
+    num * num
+}
+
+fn forbid_last(num: i32) -> i32 {
+    #![deny(unused)]
+    #![warn(unused)]
+    #![allow(unused)]
+    #![forbid(unused)]
+
+    num * num
+}
+
+fn forbid_multiple(num: i32) -> i32 {
+    #![forbid(unused)]
+    #![forbid(unused)]
+
+    num * num
+}
+
+fn main() {
+    forbid_first(10);
+    forbid_last(10);
+    forbid_multiple(10);
+}

src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr

@@ -0,0 +1,29 @@
+error[E0453]: deny(unused) incompatible with previous forbid in same scope
+  --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:19:13
+   |
+LL |     #![forbid(unused)]
+   |               ------ `forbid` level set here
+LL |     #![deny(unused)]
+   |             ^^^^^^
+
+error[E0453]: warn(unused) incompatible with previous forbid in same scope
+  --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:21:13
+   |
+LL |     #![forbid(unused)]
+   |               ------ `forbid` level set here
+...
+LL |     #![warn(unused)]
+   |             ^^^^^^
+
+error[E0453]: allow(unused) incompatible with previous forbid in same scope
+  --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:23:14
+   |
+LL |     #![forbid(unused)]
+   |               ------ `forbid` level set here
+...
+LL |     #![allow(unused)]
+   |              ^^^^^^
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0453`.

src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs

@@ -8,25 +8,25 @@ extern "C" {
         #[allow(unused_mut)] a: i32,
         #[cfg(something)] b: i32,
         #[cfg_attr(something, cfg(nothing))] c: i32,
-        #[deny(unused_mut)] d: i32,
-        #[forbid(unused_mut)] #[warn(unused_mut)] ...
+        #[forbid(unused_mut)] d: i32,
+        #[deny(unused_mut)] #[warn(unused_mut)] ...
     );
 }
 
 type FnType = fn(
     #[allow(unused_mut)] a: i32,
     #[cfg(something)] b: i32,
     #[cfg_attr(something, cfg(nothing))] c: i32,
-    #[deny(unused_mut)] d: i32,
-    #[forbid(unused_mut)] #[warn(unused_mut)] e: i32
+    #[forbid(unused_mut)] d: i32,
+    #[deny(unused_mut)] #[warn(unused_mut)] e: i32
 );
 
 pub fn foo(
     #[allow(unused_mut)] a: i32,
     #[cfg(something)] b: i32,
     #[cfg_attr(something, cfg(nothing))] c: i32,
-    #[deny(unused_mut)] d: i32,
-    #[forbid(unused_mut)] #[warn(unused_mut)] _e: i32
+    #[forbid(unused_mut)] d: i32,
+    #[deny(unused_mut)] #[warn(unused_mut)] _e: i32
 ) {}
 
 // self