Auto merge of #130050 - cjgillot:expect-attr-id, r=fee1-dead

Enumerate lint expectations using AttrId

This PR implements the idea I outlined in #127884 (comment)

We can uniquely identify a lint expectation `#[expect(lint0, lint1...)]` using the `AttrId` and the index of the lint inside the attribute. This PR uses this property in `check_expectations`.

In addition, this PR stops stashing expected diagnostics to wait for the unstable -> stable `LintExpectationId` mapping: if the lint is emitted with an unstable attribute, it must have been emitted by an `eval_always` query (like inside the resolver), so won't be loaded from cache. Decoding an `AttrId` from the on-disk cache ICEs, so we have no risk of accidentally checking an expectation.

Fixes #127884

cc `@xFrednet`

Author: bors

compiler/rustc_errors/src/diagnostic.rs

@@ -8,11 +8,11 @@ use std::thread::panicking;
 
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_error_messages::{fluent_value_from_str_list_sep_by_and, FluentValue};
-use rustc_lint_defs::{Applicability, LintExpectationId};
+use rustc_lint_defs::Applicability;
 use rustc_macros::{Decodable, Encodable};
 use rustc_span::source_map::Spanned;
 use rustc_span::symbol::Symbol;
-use rustc_span::{AttrId, Span, DUMMY_SP};
+use rustc_span::{Span, DUMMY_SP};
 use tracing::debug;
 
 use crate::snippet::Style;
@@ -354,26 +354,6 @@ impl DiagInner {
         }
     }
 
-    pub(crate) fn update_unstable_expectation_id(
-        &mut self,
-        unstable_to_stable: &FxIndexMap<AttrId, LintExpectationId>,
-    ) {
-        if let Level::Expect(expectation_id) | Level::ForceWarning(Some(expectation_id)) =
-            &mut self.level
-            && let LintExpectationId::Unstable { attr_id, lint_index } = *expectation_id
-        {
-            // The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
-            // The lint index inside the attribute is manually transferred here.
-            let Some(stable_id) = unstable_to_stable.get(&attr_id) else {
-                panic!("{expectation_id:?} must have a matching stable id")
-            };
-
-            let mut stable_id = *stable_id;
-            stable_id.set_lint_index(lint_index);
-            *expectation_id = stable_id;
-        }
-    }
-
     /// Indicates whether this diagnostic should show up in cargo's future breakage report.
     pub(crate) fn has_future_breakage(&self) -> bool {
         matches!(self.is_lint, Some(IsLint { has_future_breakage: true, .. }))

compiler/rustc_errors/src/lib.rs

@@ -69,7 +69,7 @@ use rustc_macros::{Decodable, Encodable};
 pub use rustc_span::fatal_error::{FatalError, FatalErrorMarker};
 use rustc_span::source_map::SourceMap;
 pub use rustc_span::ErrorGuaranteed;
-use rustc_span::{AttrId, Loc, Span, DUMMY_SP};
+use rustc_span::{Loc, Span, DUMMY_SP};
 pub use snippet::Style;
 // Used by external projects such as `rust-gpu`.
 // See https://github.com/rust-lang/rust/pull/115393.
@@ -497,28 +497,18 @@ struct DiagCtxtInner {
 
     future_breakage_diagnostics: Vec<DiagInner>,
 
-    /// The [`Self::unstable_expect_diagnostics`] should be empty when this struct is
-    /// dropped. However, it can have values if the compilation is stopped early
-    /// or is only partially executed. To avoid ICEs, like in rust#94953 we only
-    /// check if [`Self::unstable_expect_diagnostics`] is empty, if the expectation ids
-    /// have been converted.
-    check_unstable_expect_diagnostics: bool,
-
-    /// Expected [`DiagInner`][struct@diagnostic::DiagInner]s store a [`LintExpectationId`] as part
-    /// of the lint level. [`LintExpectationId`]s created early during the compilation
-    /// (before `HirId`s have been defined) are not stable and can therefore not be
-    /// stored on disk. This buffer stores these diagnostics until the ID has been
-    /// replaced by a stable [`LintExpectationId`]. The [`DiagInner`][struct@diagnostic::DiagInner]s
-    /// are submitted for storage and added to the list of fulfilled expectations.
-    unstable_expect_diagnostics: Vec<DiagInner>,
-
     /// expected diagnostic will have the level `Expect` which additionally
     /// carries the [`LintExpectationId`] of the expectation that can be
     /// marked as fulfilled. This is a collection of all [`LintExpectationId`]s
     /// that have been marked as fulfilled this way.
     ///
+    /// Emitting expectations after having stolen this field can happen. In particular, an
+    /// `#[expect(warnings)]` can easily make the `UNFULFILLED_LINT_EXPECTATIONS` lint expect
+    /// itself. To avoid needless complexity in this corner case, we tolerate failing to track
+    /// those expectations.
+    ///
     /// [RFC-2383]: https://rust-lang.github.io/rfcs/2383-lint-reasons.html
-    fulfilled_expectations: FxHashSet<LintExpectationId>,
+    fulfilled_expectations: FxIndexSet<LintExpectationId>,
 
     /// The file where the ICE information is stored. This allows delayed_span_bug backtraces to be
     /// stored along side the main panic backtrace.
@@ -605,13 +595,6 @@ impl Drop for DiagCtxtInner {
                 );
             }
         }
-
-        if self.check_unstable_expect_diagnostics {
-            assert!(
-                self.unstable_expect_diagnostics.is_empty(),
-                "all diagnostics with unstable expectations should have been converted",
-            );
-        }
     }
 }
 
@@ -740,8 +723,6 @@ impl DiagCtxt {
             emitted_diagnostics,
             stashed_diagnostics,
             future_breakage_diagnostics,
-            check_unstable_expect_diagnostics,
-            unstable_expect_diagnostics,
             fulfilled_expectations,
             ice_file: _,
         } = inner.deref_mut();
@@ -761,8 +742,6 @@ impl DiagCtxt {
         *emitted_diagnostics = Default::default();
         *stashed_diagnostics = Default::default();
         *future_breakage_diagnostics = Default::default();
-        *check_unstable_expect_diagnostics = false;
-        *unstable_expect_diagnostics = Default::default();
         *fulfilled_expectations = Default::default();
     }
 
@@ -1094,44 +1073,10 @@ impl<'a> DiagCtxtHandle<'a> {
         inner.emitter.emit_unused_externs(lint_level, unused_externs)
     }
 
-    pub fn update_unstable_expectation_id(
-        &self,
-        unstable_to_stable: FxIndexMap<AttrId, LintExpectationId>,
-    ) {
-        let mut inner = self.inner.borrow_mut();
-        let diags = std::mem::take(&mut inner.unstable_expect_diagnostics);
-        inner.check_unstable_expect_diagnostics = true;
-
-        if !diags.is_empty() {
-            inner.suppressed_expected_diag = true;
-            for mut diag in diags.into_iter() {
-                diag.update_unstable_expectation_id(&unstable_to_stable);
-
-                // Here the diagnostic is given back to `emit_diagnostic` where it was first
-                // intercepted. Now it should be processed as usual, since the unstable expectation
-                // id is now stable.
-                inner.emit_diagnostic(diag, self.tainted_with_errors);
-            }
-        }
-
-        inner
-            .stashed_diagnostics
-            .values_mut()
-            .for_each(|(diag, _guar)| diag.update_unstable_expectation_id(&unstable_to_stable));
-        inner
-            .future_breakage_diagnostics
-            .iter_mut()
-            .for_each(|diag| diag.update_unstable_expectation_id(&unstable_to_stable));
-    }
-
     /// This methods steals all [`LintExpectationId`]s that are stored inside
     /// [`DiagCtxtInner`] and indicate that the linked expectation has been fulfilled.
     #[must_use]
-    pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet<LintExpectationId> {
-        assert!(
-            self.inner.borrow().unstable_expect_diagnostics.is_empty(),
-            "`DiagCtxtInner::unstable_expect_diagnostics` should be empty at this point",
-        );
+    pub fn steal_fulfilled_expectation_ids(&self) -> FxIndexSet<LintExpectationId> {
         std::mem::take(&mut self.inner.borrow_mut().fulfilled_expectations)
     }
 
@@ -1440,8 +1385,6 @@ impl DiagCtxtInner {
             emitted_diagnostics: Default::default(),
             stashed_diagnostics: Default::default(),
             future_breakage_diagnostics: Vec::new(),
-            check_unstable_expect_diagnostics: false,
-            unstable_expect_diagnostics: Vec::new(),
             fulfilled_expectations: Default::default(),
             ice_file: None,
         }
@@ -1471,24 +1414,6 @@ impl DiagCtxtInner {
         mut diagnostic: DiagInner,
         taint: Option<&Cell<Option<ErrorGuaranteed>>>,
     ) -> Option<ErrorGuaranteed> {
-        match diagnostic.level {
-            Expect(expect_id) | ForceWarning(Some(expect_id)) => {
-                // The `LintExpectationId` can be stable or unstable depending on when it was
-                // created. Diagnostics created before the definition of `HirId`s are unstable and
-                // can not yet be stored. Instead, they are buffered until the `LintExpectationId`
-                // is replaced by a stable one by the `LintLevelsBuilder`.
-                if let LintExpectationId::Unstable { .. } = expect_id {
-                    // We don't call TRACK_DIAGNOSTIC because we wait for the
-                    // unstable ID to be updated, whereupon the diagnostic will be
-                    // passed into this method again.
-                    self.unstable_expect_diagnostics.push(diagnostic);
-                    return None;
-                }
-                // Continue through to the `Expect`/`ForceWarning` case below.
-            }
-            _ => {}
-        }
-
         if diagnostic.has_future_breakage() {
             // Future breakages aren't emitted if they're `Level::Allow` or
             // `Level::Expect`, but they still need to be constructed and
@@ -1564,9 +1489,6 @@ impl DiagCtxtInner {
                 return None;
             }
             Expect(expect_id) | ForceWarning(Some(expect_id)) => {
-                if let LintExpectationId::Unstable { .. } = expect_id {
-                    unreachable!(); // this case was handled at the top of this function
-                }
                 self.fulfilled_expectations.insert(expect_id);
                 if let Expect(_) = diagnostic.level {
                     // Nothing emitted here for expected lints.

compiler/rustc_lint/src/expect.rs

@@ -1,10 +1,10 @@
-use rustc_data_structures::fx::FxIndexMap;
-use rustc_hir::{HirId, CRATE_OWNER_ID};
+use rustc_data_structures::fx::FxHashSet;
+use rustc_hir::CRATE_OWNER_ID;
 use rustc_middle::lint::LintExpectation;
 use rustc_middle::query::Providers;
 use rustc_middle::ty::TyCtxt;
 use rustc_session::lint::builtin::UNFULFILLED_LINT_EXPECTATIONS;
-use rustc_session::lint::{Level, LintExpectationId};
+use rustc_session::lint::LintExpectationId;
 use rustc_span::Symbol;
 
 use crate::lints::{Expectation, ExpectationNote};
@@ -17,68 +17,56 @@ fn lint_expectations(tcx: TyCtxt<'_>, (): ()) -> Vec<(LintExpectationId, LintExp
     let krate = tcx.hir_crate_items(());
 
     let mut expectations = Vec::new();
-    let mut unstable_to_stable_ids = FxIndexMap::default();
 
-    let mut record_stable = |attr_id, hir_id, attr_index| {
-        let expect_id = LintExpectationId::Stable { hir_id, attr_index, lint_index: None };
-        unstable_to_stable_ids.entry(attr_id).or_insert(expect_id);
-    };
-    let mut push_expectations = |owner| {
+    for owner in std::iter::once(CRATE_OWNER_ID).chain(krate.owners()) {
         let lints = tcx.shallow_lint_levels_on(owner);
-        if lints.expectations.is_empty() {
-            return;
-        }
-
         expectations.extend_from_slice(&lints.expectations);
-
-        let attrs = tcx.hir_attrs(owner);
-        for &(local_id, attrs) in attrs.map.iter() {
-            // Some attributes appear multiple times in HIR, to ensure they are correctly taken
-            // into account where they matter. This means we cannot just associate the AttrId to
-            // the first HirId where we see it, but need to check it actually appears in a lint
-            // level.
-            // FIXME(cjgillot): Can this cause an attribute to appear in multiple expectation ids?
-            if !lints.specs.contains_key(&local_id) {
-                continue;
-            }
-            for (attr_index, attr) in attrs.iter().enumerate() {
-                let Some(Level::Expect(_)) = Level::from_attr(attr) else { continue };
-                record_stable(attr.id, HirId { owner, local_id }, attr_index.try_into().unwrap());
-            }
-        }
-    };
-
-    push_expectations(CRATE_OWNER_ID);
-    for owner in krate.owners() {
-        push_expectations(owner);
     }
 
-    tcx.dcx().update_unstable_expectation_id(unstable_to_stable_ids);
     expectations
 }
 
 fn check_expectations(tcx: TyCtxt<'_>, tool_filter: Option<Symbol>) {
     let lint_expectations = tcx.lint_expectations(());
     let fulfilled_expectations = tcx.dcx().steal_fulfilled_expectation_ids();
 
-    for (id, expectation) in lint_expectations {
-        // This check will always be true, since `lint_expectations` only
-        // holds stable ids
-        if let LintExpectationId::Stable { hir_id, .. } = id {
-            if !fulfilled_expectations.contains(id)
-                && tool_filter.map_or(true, |filter| expectation.lint_tool == Some(filter))
-            {
-                let rationale = expectation.reason.map(|rationale| ExpectationNote { rationale });
-                let note = expectation.is_unfulfilled_lint_expectations;
-                tcx.emit_node_span_lint(
-                    UNFULFILLED_LINT_EXPECTATIONS,
-                    *hir_id,
-                    expectation.emission_span,
-                    Expectation { rationale, note },
-                );
+    // Turn a `LintExpectationId` into a `(AttrId, lint_index)` pair.
+    let canonicalize_id = |expect_id: &LintExpectationId| {
+        match *expect_id {
+            LintExpectationId::Unstable { attr_id, lint_index: Some(lint_index) } => {
+                (attr_id, lint_index)
+            }
+            LintExpectationId::Stable { hir_id, attr_index, lint_index: Some(lint_index) } => {
+                // We are an `eval_always` query, so looking at the attribute's `AttrId` is ok.
+                let attr_id = tcx.hir().attrs(hir_id)[attr_index as usize].id;
+                (attr_id, lint_index)
             }
-        } else {
+            _ => panic!("fulfilled expectations must have a lint index"),
+        }
+    };
+
+    let fulfilled_expectations: FxHashSet<_> =
+        fulfilled_expectations.iter().map(canonicalize_id).collect();
+
+    for (expect_id, expectation) in lint_expectations {
+        // This check will always be true, since `lint_expectations` only holds stable ids
+        let LintExpectationId::Stable { hir_id, .. } = expect_id else {
             unreachable!("at this stage all `LintExpectationId`s are stable");
+        };
+
+        let expect_id = canonicalize_id(expect_id);
+
+        if !fulfilled_expectations.contains(&expect_id)
+            && tool_filter.map_or(true, |filter| expectation.lint_tool == Some(filter))
+        {
+            let rationale = expectation.reason.map(|rationale| ExpectationNote { rationale });
+            let note = expectation.is_unfulfilled_lint_expectations;
+            tcx.emit_node_span_lint(
+                UNFULFILLED_LINT_EXPECTATIONS,
+                *hir_id,
+                expectation.emission_span,
+                Expectation { rationale, note },
+            );
         }
     }
 }

tests/ui/lint/expect-unused-imports.rs

@@ -0,0 +1,9 @@
+//@ check-pass
+
+#![deny(unused_imports)]
+#![deny(unfulfilled_lint_expectations)]
+
+#[expect(unused_imports)]
+use std::{io, fs};
+
+fn main() {}

tests/ui/lint/rfc-2383-lint-reason/expect_warnings.rs

@@ -0,0 +1,6 @@
+//@ check-pass
+
+#![expect(warnings)]
+
+#[expect(unused)]
+fn main() {}

tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.stderr

@@ -1,3 +1,11 @@
+warning: denote infinite loops with `loop { ... }`
+  --> $DIR/force_warn_expected_lints_fulfilled.rs:8:5
+   |
+LL |     while true {
+   |     ^^^^^^^^^^ help: use `loop`
+   |
+   = note: requested on the command line with `--force-warn while-true`
+
 warning: unused variable: `x`
   --> $DIR/force_warn_expected_lints_fulfilled.rs:18:9
    |
@@ -28,13 +36,5 @@ warning: unused variable: `this_should_fulfill_the_expectation`
 LL |     let this_should_fulfill_the_expectation = "The `#[allow]` has no power here";
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_this_should_fulfill_the_expectation`
 
-warning: denote infinite loops with `loop { ... }`
-  --> $DIR/force_warn_expected_lints_fulfilled.rs:8:5
-   |
-LL |     while true {
-   |     ^^^^^^^^^^ help: use `loop`
-   |
-   = note: requested on the command line with `--force-warn while-true`
-
 warning: 5 warnings emitted