Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-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``

Author: m-ou-se

Diff for: compiler/rustc_lint/src/context.rs

@@ -39,6 +39,7 @@ use rustc_session::SessionLintStore;
 use rustc_span::lev_distance::find_best_match_for_name;
 use rustc_span::{symbol::Symbol, MultiSpan, Span, DUMMY_SP};
 use rustc_target::abi::LayoutOf;
+use tracing::debug;
 
 use std::cell::Cell;
 use std::slice;
@@ -335,6 +336,20 @@ impl LintStore {
         }
     }
 
+    /// True if this symbol represents a lint group name.
+    pub fn is_lint_group(&self, lint_name: Symbol) -> bool {
+        debug!(
+            "is_lint_group(lint_name={:?}, lint_groups={:?})",
+            lint_name,
+            self.lint_groups.keys().collect::<Vec<_>>()
+        );
+        let lint_name_str = &*lint_name.as_str();
+        self.lint_groups.contains_key(&lint_name_str) || {
+            let warnings_name_str = crate::WARNINGS.name_lower();
+            lint_name_str == &*warnings_name_str
+        }
+    }
+
     /// Checks the name of a lint for its existence, and whether it was
     /// renamed or removed. Generates a DiagnosticBuilder containing a
     /// warning for renamed and removed lints. This is over both lint

Diff for: compiler/rustc_lint/src/levels.rs

@@ -5,7 +5,7 @@ use rustc_ast::attr;
 use rustc_ast::unwrap_or;
 use rustc_ast_pretty::pprust;
 use rustc_data_structures::fx::FxHashMap;
-use rustc_errors::{struct_span_err, Applicability};
+use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
 use rustc_hir::{intravisit, HirId};
@@ -17,11 +17,15 @@ use rustc_middle::lint::{
 };
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::TyCtxt;
-use rustc_session::lint::{builtin, Level, Lint, LintId};
+use rustc_session::lint::{
+    builtin::{self, FORBIDDEN_LINT_GROUPS},
+    Level, Lint, LintId,
+};
 use rustc_session::parse::feature_err;
 use rustc_session::Session;
 use rustc_span::symbol::{sym, Symbol};
 use rustc_span::{source_map::MultiSpan, Span, DUMMY_SP};
+use tracing::debug;
 
 use std::cmp;
 
@@ -51,6 +55,7 @@ pub struct LintLevelsBuilder<'s> {
     id_to_set: FxHashMap<HirId, u32>,
     cur: u32,
     warn_about_weird_lints: bool,
+    store: &'s LintStore,
 }
 
 pub struct BuilderPush {
@@ -59,13 +64,14 @@ pub struct BuilderPush {
 }
 
 impl<'s> LintLevelsBuilder<'s> {
-    pub fn new(sess: &'s Session, warn_about_weird_lints: bool, store: &LintStore) -> Self {
+    pub fn new(sess: &'s Session, warn_about_weird_lints: bool, store: &'s LintStore) -> Self {
         let mut builder = LintLevelsBuilder {
             sess,
             sets: LintLevelSets::new(),
             cur: 0,
             id_to_set: Default::default(),
             warn_about_weird_lints,
+            store,
         };
         builder.process_command_line(sess, store);
         assert_eq!(builder.sets.list.len(), 1);
@@ -120,36 +126,75 @@ impl<'s> LintLevelsBuilder<'s> {
             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",
-                    level.as_str(),
-                    src.name(),
+                // Backwards compatibility check:
+                //
+                // We used to not consider `forbid(lint_group)`
+                // as preventing `allow(lint)` for some lint `lint` in
+                // `lint_group`. For now, issue a future-compatibility
+                // warning for this case.
+                let id_name = id.lint.name_lower();
+                let fcw_warning = match old_src {
+                    LintLevelSource::Default => false,
+                    LintLevelSource::Node(symbol, _, _) => self.store.is_lint_group(symbol),
+                    LintLevelSource::CommandLine(symbol, _) => self.store.is_lint_group(symbol),
+                };
+                debug!(
+                    "fcw_warning={:?}, specs.get(&id) = {:?}, old_src={:?}, id_name={:?}",
+                    fcw_warning, specs, old_src, id_name
                 );
-                diag_builder.span_label(src.span(), "overruled by previous forbid");
-                match old_src {
-                    LintLevelSource::Default => {
-                        diag_builder.note(&format!(
-                            "`forbid` lint level is the default for {}",
-                            id.to_string()
-                        ));
-                    }
-                    LintLevelSource::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());
+
+                let decorate_diag_builder = |mut diag_builder: DiagnosticBuilder<'_>| {
+                    diag_builder.span_label(src.span(), "overruled by previous forbid");
+                    match old_src {
+                        LintLevelSource::Default => {
+                            diag_builder.note(&format!(
+                                "`forbid` lint level is the default for {}",
+                                id.to_string()
+                            ));
+                        }
+                        LintLevelSource::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());
+                            }
+                        }
+                        LintLevelSource::CommandLine(_, _) => {
+                            diag_builder.note("`forbid` lint level was set on command line");
                         }
                     }
-                    LintLevelSource::CommandLine(_, _) => {
-                        diag_builder.note("`forbid` lint level was set on command line");
-                    }
+                    diag_builder.emit();
+                };
+                if !fcw_warning {
+                    let diag_builder = struct_span_err!(
+                        self.sess,
+                        src.span(),
+                        E0453,
+                        "{}({}) incompatible with previous forbid",
+                        level.as_str(),
+                        src.name(),
+                    );
+                    decorate_diag_builder(diag_builder);
+                } else {
+                    self.struct_lint(
+                        FORBIDDEN_LINT_GROUPS,
+                        Some(src.span().into()),
+                        |diag_builder| {
+                            let diag_builder = diag_builder.build(&format!(
+                                "{}({}) incompatible with previous forbid",
+                                level.as_str(),
+                                src.name(),
+                            ));
+                            decorate_diag_builder(diag_builder);
+                        },
+                    );
                 }
-                diag_builder.emit();
 
-                // Retain the forbid lint level
-                return;
+                // Retain the forbid lint level, unless we are
+                // issuing a FCW. In the FCW case, we want to
+                // respect the new setting.
+                if !fcw_warning {
+                    return;
+                }
             }
         }
         specs.insert(id, (level, src));

Diff for: compiler/rustc_lint_defs/src/builtin.rs

@@ -8,6 +8,42 @@ use crate::{declare_lint, declare_lint_pass, declare_tool_lint};
 use rustc_span::edition::Edition;
 use rustc_span::symbol::sym;
 
+declare_lint! {
+    /// The `forbidden_lint_groups` lint detects violations of
+    /// `forbid` applied to a lint group. Due to a bug in the compiler,
+    /// these used to be overlooked entirely. They now generate a warning.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// #![forbid(warnings)]
+    /// #![deny(bad_style)]
+    ///
+    /// fn main() {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Recommended fix
+    ///
+    /// If your crate is using `#![forbid(warnings)]`,
+    /// we recommend that you change to `#![deny(warnings)]`.
+    ///
+    /// ### Explanation
+    ///
+    /// Due to a compiler bug, applying `forbid` to lint groups
+    /// previously had no effect. The bug is now fixed but instead of
+    /// enforcing `forbid` we issue this future-compatibility warning
+    /// to avoid breaking existing crates.
+    pub FORBIDDEN_LINT_GROUPS,
+    Warn,
+    "applying forbid to lint-groups",
+    @future_incompatible = FutureIncompatibleInfo {
+        reference: "issue #81670 <https://github.com/rust-lang/rust/issues/81670>",
+        edition: None,
+    };
+}
+
 declare_lint! {
     /// The `ill_formed_attribute_input` lint detects ill-formed attribute
     /// inputs that were previously accepted and used in practice.
@@ -2839,6 +2875,7 @@ declare_lint_pass! {
     /// Does nothing as a lint pass, but registers some `Lint`s
     /// that are used by other parts of the compiler.
     HardwiredLints => [
+        FORBIDDEN_LINT_GROUPS,
         ILLEGAL_FLOATING_POINT_LITERAL_PATTERN,
         ARITHMETIC_OVERFLOW,
         UNCONDITIONAL_PANIC,

Diff for: compiler/rustc_middle/src/lint.rs

@@ -5,14 +5,17 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_errors::{DiagnosticBuilder, DiagnosticId};
 use rustc_hir::HirId;
-use rustc_session::lint::{builtin, Level, Lint, LintId};
+use rustc_session::lint::{
+    builtin::{self, FORBIDDEN_LINT_GROUPS},
+    Level, Lint, LintId,
+};
 use rustc_session::{DiagnosticMessageId, Session};
 use rustc_span::hygiene::MacroKind;
 use rustc_span::source_map::{DesugaringKind, ExpnKind, MultiSpan};
 use rustc_span::{symbol, Span, Symbol, DUMMY_SP};
 
 /// How a lint level was set.
-#[derive(Clone, Copy, PartialEq, Eq, HashStable)]
+#[derive(Clone, Copy, PartialEq, Eq, HashStable, Debug)]
 pub enum LintLevelSource {
     /// Lint is at the default level as declared
     /// in rustc or a plugin.
@@ -87,7 +90,12 @@ impl LintLevelSets {
         // If we're about to issue a warning, check at the last minute for any
         // directives against the warnings "lint". If, for example, there's an
         // `allow(warnings)` in scope then we want to respect that instead.
-        if level == Level::Warn {
+        //
+        // We exempt `FORBIDDEN_LINT_GROUPS` from this because it specifically
+        // triggers in cases (like #80988) where you have `forbid(warnings)`,
+        // and so if we turned that into an error, it'd defeat the purpose of the
+        // future compatibility warning.
+        if level == Level::Warn && LintId::of(lint) != LintId::of(FORBIDDEN_LINT_GROUPS) {
             let (warnings_level, warnings_src) =
                 self.get_lint_id_level(LintId::of(builtin::WARNINGS), idx, aux);
             if let Some(configured_warning_level) = warnings_level {

Diff for: src/test/ui/lint/forbid-group-group-1.rs

@@ -0,0 +1,13 @@
+// Check what happens when we forbid a smaller group but
+// then allow a superset of that group.
+
+#![forbid(nonstandard_style)]
+
+// FIXME: Arguably this should be an error, but the WARNINGS group is
+// treated in a very special (and rather ad-hoc) way and
+// it fails to trigger.
+#[allow(warnings)]
+fn main() {
+    let A: ();
+    //~^ ERROR should have a snake case name
+}

Diff for: src/test/ui/lint/forbid-group-group-1.stderr

@@ -0,0 +1,15 @@
+error: variable `A` should have a snake case name
+  --> $DIR/forbid-group-group-1.rs:11:9
+   |
+LL |     let A: ();
+   |         ^ help: convert the identifier to snake case: `a`
+   |
+note: the lint level is defined here
+  --> $DIR/forbid-group-group-1.rs:4:11
+   |
+LL | #![forbid(nonstandard_style)]
+   |           ^^^^^^^^^^^^^^^^^
+   = note: `#[forbid(non_snake_case)]` implied by `#[forbid(nonstandard_style)]`
+
+error: aborting due to previous error
+

Diff for: src/test/ui/lint/forbid-group-group-2.rs

@@ -0,0 +1,26 @@
+// Check what happens when we forbid a bigger group but
+// then deny a subset of that group.
+
+#![forbid(warnings)]
+#![deny(forbidden_lint_groups)]
+
+#[allow(nonstandard_style)]
+//~^ ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+//~| ERROR incompatible with previous
+//~| WARNING previously accepted by the compiler
+fn main() {}