Rollup merge of #94670 - xFrednet:rfc-2383-expect-impl-after-party, r=flip1995,wesleywiser

Improve `expect` impl and handle `#[expect(unfulfilled_lint_expectations)]` (RFC 2383)

This PR updates unstable `ExpectationIds` in stashed diagnostics and adds some asserts to ensure that the stored expectations are really empty in the end. Additionally, it handles the `#[expect(unfulfilled_lint_expectations)]` case.

According to the [Errors and lints docs](https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-levels) the `error` level should only be used _"when the compiler detects a problem that makes it unable to compile the program"_. As this isn't the case with `#[expect(unfulfilled_lint_expectations)]` I decided to only create a warning. To avoid adding a new lint only for this case, I simply emit a `unfulfilled_lint_expectations` diagnostic with an additional note.

---

r? `@wesleywiser` I'm requesting a review from you since you reviewed the previous PR #87835. You are welcome to reassign it if you're busy 🙃

rfc: [RFC-2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html)

tracking issue: #85549

cc: `@flip1995` In case you're also interested in this :)

Author: matthiaskrgr

compiler/rustc_errors/src/diagnostic.rs

@@ -5,7 +5,8 @@ use crate::Substitution;
 use crate::SubstitutionPart;
 use crate::SuggestionStyle;
 use crate::ToolMetadata;
-use rustc_lint_defs::Applicability;
+use rustc_data_structures::stable_map::FxHashMap;
+use rustc_lint_defs::{Applicability, LintExpectationId};
 use rustc_serialize::json::Json;
 use rustc_span::edition::LATEST_STABLE_EDITION;
 use rustc_span::{MultiSpan, Span, DUMMY_SP};
@@ -138,6 +139,28 @@ impl Diagnostic {
         }
     }
 
+    pub fn update_unstable_expectation_id(
+        &mut self,
+        unstable_to_stable: &FxHashMap<LintExpectationId, LintExpectationId>,
+    ) {
+        if let Level::Expect(expectation_id) = &mut self.level {
+            if expectation_id.is_stable() {
+                return;
+            }
+
+            // 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 lint_index = expectation_id.get_lint_index();
+            expectation_id.set_lint_index(None);
+            let mut stable_id = *unstable_to_stable
+                .get(&expectation_id)
+                .expect("each unstable `LintExpectationId` must have a matching stable id");
+
+            stable_id.set_lint_index(lint_index);
+            *expectation_id = stable_id;
+        }
+    }
+
     pub fn has_future_breakage(&self) -> bool {
         match self.code {
             Some(DiagnosticId::Lint { has_future_breakage, .. }) => has_future_breakage,

compiler/rustc_errors/src/lib.rs

@@ -522,6 +522,11 @@ impl Drop for HandlerInner {
                 "no warnings or errors encountered even though `delayed_good_path_bugs` issued",
             );
         }
+
+        assert!(
+            self.unstable_expect_diagnostics.is_empty(),
+            "all diagnostics with unstable expectations should have been converted",
+        );
     }
 }
 
@@ -942,29 +947,30 @@ impl Handler {
 
         let mut inner = self.inner.borrow_mut();
         for mut diag in diags.into_iter() {
-            let mut unstable_id = diag
+            diag.update_unstable_expectation_id(unstable_to_stable);
+
+            let stable_id = diag
                 .level
                 .get_expectation_id()
                 .expect("all diagnostics inside `unstable_expect_diagnostics` must have a `LintExpectationId`");
-
-            // 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 lint_index = unstable_id.get_lint_index();
-            unstable_id.set_lint_index(None);
-            let mut stable_id = *unstable_to_stable
-                .get(&unstable_id)
-                .expect("each unstable `LintExpectationId` must have a matching stable id");
-
-            stable_id.set_lint_index(lint_index);
-            diag.level = Level::Expect(stable_id);
             inner.fulfilled_expectations.insert(stable_id);
 
             (*TRACK_DIAGNOSTICS)(&diag);
         }
+
+        inner
+            .stashed_diagnostics
+            .values_mut()
+            .for_each(|diag| 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
     /// [`HandlerInner`] 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(),

compiler/rustc_lint/src/expect.rs

@@ -30,10 +30,6 @@ fn emit_unfulfilled_expectation_lint(
     hir_id: HirId,
     expectation: &LintExpectation,
 ) {
-    // FIXME: The current implementation doesn't cover cases where the
-    // `unfulfilled_lint_expectations` is actually expected by another lint
-    // expectation. This can be added here by checking the lint level and
-    // retrieving the `LintExpectationId` if it was expected.
     tcx.struct_span_lint_hir(
         builtin::UNFULFILLED_LINT_EXPECTATIONS,
         hir_id,
@@ -43,6 +39,11 @@ fn emit_unfulfilled_expectation_lint(
             if let Some(rationale) = expectation.reason {
                 diag.note(&rationale.as_str());
             }
+
+            if expectation.is_unfulfilled_lint_expectations {
+                diag.note("the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message");
+            }
+
             diag.emit();
         },
     );

compiler/rustc_lint/src/levels.rs

@@ -14,7 +14,7 @@ use rustc_middle::lint::{
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::{RegisteredTools, TyCtxt};
 use rustc_session::lint::{
-    builtin::{self, FORBIDDEN_LINT_GROUPS},
+    builtin::{self, FORBIDDEN_LINT_GROUPS, UNFULFILLED_LINT_EXPECTATIONS},
     Level, Lint, LintExpectationId, LintId,
 };
 use rustc_session::parse::{add_feature_diagnostics, feature_err};
@@ -218,6 +218,14 @@ impl<'s> LintLevelsBuilder<'s> {
                 }
             }
         }
+
+        // The lint `unfulfilled_lint_expectations` can't be expected, as it would suppress itself.
+        // Handling expectations of this lint would add additional complexity with little to no
+        // benefit. The expect level for this lint will therefore be ignored.
+        if let Level::Expect(_) = level && id == LintId::of(UNFULFILLED_LINT_EXPECTATIONS) {
+            return;
+        }
+
         if let Level::ForceWarn = old_level {
             self.current_specs_mut().insert(id, (old_level, old_src));
         } else {
@@ -350,6 +358,22 @@ impl<'s> LintLevelsBuilder<'s> {
                     self.store.check_lint_name(&name, tool_name, self.registered_tools);
                 match &lint_result {
                     CheckLintNameResult::Ok(ids) => {
+                        // This checks for instances where the user writes `#[expect(unfulfilled_lint_expectations)]`
+                        // in that case we want to avoid overriding the lint level but instead add an expectation that
+                        // can't be fulfilled. The lint message will include an explanation, that the
+                        // `unfulfilled_lint_expectations` lint can't be expected.
+                        if let Level::Expect(expect_id) = level {
+                            // The `unfulfilled_lint_expectations` lint is not part of any lint groups. Therefore. we
+                            // only need to check the slice if it contains a single lint.
+                            let is_unfulfilled_lint_expectations = match ids {
+                                [lint] => *lint == LintId::of(UNFULFILLED_LINT_EXPECTATIONS),
+                                _ => false,
+                            };
+                            self.lint_expectations.push((
+                                expect_id,
+                                LintExpectation::new(reason, sp, is_unfulfilled_lint_expectations),
+                            ));
+                        }
                         let src = LintLevelSource::Node(
                             meta_item.path.segments.last().expect("empty lint name").ident.name,
                             sp,
@@ -360,10 +384,6 @@ impl<'s> LintLevelsBuilder<'s> {
                                 self.insert_spec(id, (level, src));
                             }
                         }
-                        if let Level::Expect(expect_id) = level {
-                            self.lint_expectations
-                                .push((expect_id, LintExpectation::new(reason, sp)));
-                        }
                     }
 
                     CheckLintNameResult::Tool(result) => {
@@ -381,7 +401,7 @@ impl<'s> LintLevelsBuilder<'s> {
                                 }
                                 if let Level::Expect(expect_id) = level {
                                     self.lint_expectations
-                                        .push((expect_id, LintExpectation::new(reason, sp)));
+                                        .push((expect_id, LintExpectation::new(reason, sp, false)));
                                 }
                             }
                             Err((Some(ids), ref new_lint_name)) => {
@@ -425,7 +445,7 @@ impl<'s> LintLevelsBuilder<'s> {
                                 }
                                 if let Level::Expect(expect_id) = level {
                                     self.lint_expectations
-                                        .push((expect_id, LintExpectation::new(reason, sp)));
+                                        .push((expect_id, LintExpectation::new(reason, sp, false)));
                                 }
                             }
                             Err((None, _)) => {
@@ -531,7 +551,7 @@ impl<'s> LintLevelsBuilder<'s> {
                         }
                         if let Level::Expect(expect_id) = level {
                             self.lint_expectations
-                                .push((expect_id, LintExpectation::new(reason, sp)));
+                                .push((expect_id, LintExpectation::new(reason, sp, false)));
                         }
                     } else {
                         panic!("renamed lint does not exist: {}", new_name);

compiler/rustc_lint_defs/src/lib.rs

@@ -54,7 +54,7 @@ pub enum Applicability {
 /// Expected `Diagnostic`s get the lint level `Expect` which stores the `LintExpectationId`
 /// to match it with the actual expectation later on.
 ///
-/// The `LintExpectationId` has to be has stable between compilations, as diagnostic
+/// The `LintExpectationId` has to be stable between compilations, as diagnostic
 /// instances might be loaded from cache. Lint messages can be emitted during an
 /// `EarlyLintPass` operating on the AST and during a `LateLintPass` traversing the
 /// HIR tree. The AST doesn't have enough information to create a stable id. The
@@ -71,7 +71,7 @@ pub enum Applicability {
 #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash, Encodable, Decodable)]
 pub enum LintExpectationId {
     /// Used for lints emitted during the `EarlyLintPass`. This id is not
-    /// has stable and should not be cached.
+    /// hash stable and should not be cached.
     Unstable { attr_id: AttrId, lint_index: Option<u16> },
     /// The [`HirId`] that the lint expectation is attached to. This id is
     /// stable and can be cached. The additional index ensures that nodes with
@@ -113,7 +113,9 @@ impl<HCX: rustc_hir::HashStableContext> HashStable<HCX> for LintExpectationId {
                 lint_index.hash_stable(hcx, hasher);
             }
             _ => {
-                unreachable!("HashStable should only be called for a filled `LintExpectationId`")
+                unreachable!(
+                    "HashStable should only be called for filled and stable `LintExpectationId`"
+                )
             }
         }
     }

compiler/rustc_middle/src/lint.rs

@@ -204,11 +204,19 @@ pub struct LintExpectation {
     pub reason: Option<Symbol>,
     /// The [`Span`] of the attribute that this expectation originated from.
     pub emission_span: Span,
+    /// Lint messages for the `unfulfilled_lint_expectations` lint will be
+    /// adjusted to include an additional note. Therefore, we have to track if
+    /// the expectation is for the lint.
+    pub is_unfulfilled_lint_expectations: bool,
 }
 
 impl LintExpectation {
-    pub fn new(reason: Option<Symbol>, attr_span: Span) -> Self {
-        Self { reason, emission_span: attr_span }
+    pub fn new(
+        reason: Option<Symbol>,
+        emission_span: Span,
+        is_unfulfilled_lint_expectations: bool,
+    ) -> Self {
+        Self { reason, emission_span, is_unfulfilled_lint_expectations }
     }
 }
 

src/test/ui/lint/rfc-2383-lint-reason/expect_unfulfilled_expectation.rs

@@ -0,0 +1,39 @@
+// check-pass
+// ignore-tidy-linelength
+
+#![feature(lint_reasons)]
+#![warn(unused_mut)]
+
+#![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
+//~^ WARNING this lint expectation is unfulfilled
+//~| NOTE `#[warn(unfulfilled_lint_expectations)]` on by default
+//~| NOTE idk why you would expect this
+//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+
+#[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
+//~^ WARNING this lint expectation is unfulfilled
+//~| NOTE a local: idk why you would expect this
+//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+pub fn normal_test_fn() {
+    #[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
+    //~^ WARNING this lint expectation is unfulfilled
+    //~| NOTE this expectation will create a diagnostic with the default lint level
+    let mut v = vec![1, 1, 2, 3, 5];
+    v.sort();
+
+    // Check that lint lists including `unfulfilled_lint_expectations` are also handled correctly
+    #[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
+    //~^ WARNING this lint expectation is unfulfilled
+    //~| NOTE the expectation for `unused` should be fulfilled
+    //~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+    let value = "I'm unused";
+}
+
+#[expect(warnings, reason = "this suppresses all warnings and also suppresses itself. No warning will be issued")]
+pub fn expect_warnings() {
+    // This lint trigger will be suppressed
+    #[warn(unused_mut)]
+    let mut v = vec![1, 1, 2, 3, 5];
+}
+
+fn main() {}

src/test/ui/lint/rfc-2383-lint-reason/expect_unfulfilled_expectation.stderr

@@ -0,0 +1,38 @@
+warning: this lint expectation is unfulfilled
+  --> $DIR/expect_unfulfilled_expectation.rs:7:11
+   |
+LL | #![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
+   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(unfulfilled_lint_expectations)]` on by default
+   = note: idk why you would expect this
+   = note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+
+warning: this lint expectation is unfulfilled
+  --> $DIR/expect_unfulfilled_expectation.rs:13:10
+   |
+LL | #[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
+   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: a local: idk why you would expect this
+   = note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+
+warning: this lint expectation is unfulfilled
+  --> $DIR/expect_unfulfilled_expectation.rs:18:14
+   |
+LL |     #[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
+   |              ^^^^^^^^^^
+   |
+   = note: this expectation will create a diagnostic with the default lint level
+
+warning: this lint expectation is unfulfilled
+  --> $DIR/expect_unfulfilled_expectation.rs:25:22
+   |
+LL |     #[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: the expectation for `unused` should be fulfilled
+   = note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
+
+warning: 4 warnings emitted
+