Overhaul how stashed diagnostics work, again.

Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. #120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes #121451.
Fixes #121477.
Fixes #121504.
Fixes #121508.

Author: nnethercote

compiler/rustc_errors/src/diagnostic.rs

@@ -1289,11 +1289,9 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> {
         drop(self);
     }
 
-    /// Stashes diagnostic for possible later improvement in a different,
-    /// later stage of the compiler. The diagnostic can be accessed with
-    /// the provided `span` and `key` through [`DiagCtxt::steal_diagnostic()`].
-    pub fn stash(mut self, span: Span, key: StashKey) {
-        self.dcx.stash_diagnostic(span, key, self.take_diag());
+    /// See `DiagCtxt::stash_diagnostic` for details.
+    pub fn stash(mut self, span: Span, key: StashKey) -> Option<ErrorGuaranteed> {
+        self.dcx.stash_diagnostic(span, key, self.take_diag())
     }
 
     /// Delay emission of this diagnostic as a bug.

compiler/rustc_errors/src/lib.rs

@@ -434,10 +434,6 @@ struct DiagCtxtInner {
     /// The delayed bugs and their error guarantees.
     delayed_bugs: Vec<(DelayedDiagInner, ErrorGuaranteed)>,
 
-    /// The number of stashed errors. Unlike the other counts, this can go up
-    /// and down, so it doesn't guarantee anything.
-    stashed_err_count: usize,
-
     /// The error count shown to the user at the end.
     deduplicated_err_count: usize,
     /// The warning count shown to the user at the end.
@@ -475,7 +471,7 @@ struct DiagCtxtInner {
     /// add more information). All stashed diagnostics must be emitted with
     /// `emit_stashed_diagnostics` by the time the `DiagCtxtInner` is dropped,
     /// otherwise an assertion failure will occur.
-    stashed_diagnostics: FxIndexMap<(Span, StashKey), DiagInner>,
+    stashed_diagnostics: FxIndexMap<(Span, StashKey), (DiagInner, Option<ErrorGuaranteed>)>,
 
     future_breakage_diagnostics: Vec<DiagInner>,
 
@@ -561,8 +557,14 @@ impl Drop for DiagCtxtInner {
     fn drop(&mut self) {
         // Any stashed diagnostics should have been handled by
         // `emit_stashed_diagnostics` by now.
+        //
+        // Important: it is sound to produce an `ErrorGuaranteed` when stashing
+        // errors because they are guaranteed to have been emitted by here.
         assert!(self.stashed_diagnostics.is_empty());
 
+        // Important: it is sound to produce an `ErrorGuaranteed` when emitting
+        // delayed bugs because they are guaranteed to be emitted here if
+        // necessary.
         if self.err_guars.is_empty() {
             self.flush_delayed()
         }
@@ -615,7 +617,6 @@ impl DiagCtxt {
                 err_guars: Vec::new(),
                 lint_err_guars: Vec::new(),
                 delayed_bugs: Vec::new(),
-                stashed_err_count: 0,
                 deduplicated_err_count: 0,
                 deduplicated_warn_count: 0,
                 emitter,
@@ -676,7 +677,6 @@ impl DiagCtxt {
             err_guars,
             lint_err_guars,
             delayed_bugs,
-            stashed_err_count,
             deduplicated_err_count,
             deduplicated_warn_count,
             emitter: _,
@@ -699,7 +699,6 @@ impl DiagCtxt {
         *err_guars = Default::default();
         *lint_err_guars = Default::default();
         *delayed_bugs = Default::default();
-        *stashed_err_count = 0;
         *deduplicated_err_count = 0;
         *deduplicated_warn_count = 0;
         *must_produce_diag = false;
@@ -715,39 +714,111 @@ impl DiagCtxt {
         *fulfilled_expectations = Default::default();
     }
 
-    /// Stash a given diagnostic with the given `Span` and [`StashKey`] as the key.
-    /// Retrieve a stashed diagnostic with `steal_diagnostic`.
-    pub fn stash_diagnostic(&self, span: Span, key: StashKey, diag: DiagInner) {
-        let mut inner = self.inner.borrow_mut();
-
-        let key = (span.with_parent(None), key);
-
-        if diag.is_error() {
-            if diag.is_lint.is_none() {
-                inner.stashed_err_count += 1;
-            }
-        }
+    /// Stashes a diagnostic for possible later improvement in a different,
+    /// later stage of the compiler. Possible actions depend on the diagnostic
+    /// level:
+    /// - Level::Error: immediately counted as an error that has occurred, because it
+    ///   is guaranteed to be emitted eventually. Can be later accessed with the
+    ///   provided `span` and `key` through
+    ///   [`DiagCtxt::try_steal_modify_and_emit_err`] or
+    ///   [`DiagCtxt::try_steal_replace_and_emit_err`]. These do not allow
+    ///   cancellation or downgrading of the error. Returns
+    ///   `Some(ErrorGuaranteed)`.
+    /// - Level::Warning and lower (i.e. !is_error()): can be accessed with the
+    ///   provided `span` and `key` through [`DiagCtxt::steal_non_err()`]. This
+    ///   allows cancelling and downgrading of the diagnostic. Returns `None`.
+    /// - Others: not allowed, will trigger a panic.
+    pub fn stash_diagnostic(
+        &self,
+        span: Span,
+        key: StashKey,
+        diag: DiagInner,
+    ) -> Option<ErrorGuaranteed> {
+        let guar = if diag.level() == Level::Error {
+            // This `unchecked_error_guaranteed` is valid. It is where the
+            // `ErrorGuaranteed` for stashed errors originates. See
+            // `DiagCtxtInner::drop`.
+            #[allow(deprecated)]
+            Some(ErrorGuaranteed::unchecked_error_guaranteed())
+        } else if !diag.is_error() {
+            None
+        } else {
+            self.span_bug(span, format!("invalid level in `stash_diagnostic`: {}", diag.level));
+        };
 
         // FIXME(Centril, #69537): Consider reintroducing panic on overwriting a stashed diagnostic
         // if/when we have a more robust macro-friendly replacement for `(span, key)` as a key.
         // See the PR for a discussion.
-        inner.stashed_diagnostics.insert(key, diag);
+        let key = (span.with_parent(None), key);
+        self.inner.borrow_mut().stashed_diagnostics.insert(key, (diag, guar));
+
+        guar
     }
 
-    /// Steal a previously stashed diagnostic with the given `Span` and [`StashKey`] as the key.
-    pub fn steal_diagnostic(&self, span: Span, key: StashKey) -> Option<Diag<'_, ()>> {
-        let mut inner = self.inner.borrow_mut();
+    /// Steal a previously stashed non-error diagnostic with the given `Span`
+    /// and [`StashKey`] as the key. Panics if the found diagnostic is an
+    /// error.
+    pub fn steal_non_err(&self, span: Span, key: StashKey) -> Option<Diag<'_, ()>> {
         let key = (span.with_parent(None), key);
         // FIXME(#120456) - is `swap_remove` correct?
-        let diag = inner.stashed_diagnostics.swap_remove(&key)?;
-        if diag.is_error() {
-            if diag.is_lint.is_none() {
-                inner.stashed_err_count -= 1;
-            }
-        }
+        let (diag, guar) = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key)?;
+        assert!(!diag.is_error());
+        assert!(guar.is_none());
         Some(Diag::new_diagnostic(self, diag))
     }
 
+    /// Steals a previously stashed error with the given `Span` and
+    /// [`StashKey`] as the key, modifies it, and emits it. Returns `None` if
+    /// no matching diagnostic is found. Panics if the found diagnostic's level
+    /// isn't `Level::Error`.
+    pub fn try_steal_modify_and_emit_err<F>(
+        &self,
+        span: Span,
+        key: StashKey,
+        mut modify_err: F,
+    ) -> Option<ErrorGuaranteed>
+    where
+        F: FnMut(&mut Diag<'_>),
+    {
+        let key = (span.with_parent(None), key);
+        // FIXME(#120456) - is `swap_remove` correct?
+        let err = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key);
+        err.map(|(err, guar)| {
+            // The use of `::<ErrorGuaranteed>` is safe because level is `Level::Error`.
+            assert_eq!(err.level, Level::Error);
+            assert!(guar.is_some());
+            let mut err = Diag::<ErrorGuaranteed>::new_diagnostic(self, err);
+            modify_err(&mut err);
+            assert_eq!(err.level, Level::Error);
+            err.emit()
+        })
+    }
+
+    /// Steals a previously stashed error with the given `Span` and
+    /// [`StashKey`] as the key, cancels it if found, and emits `new_err`.
+    /// Panics if the found diagnostic's level isn't `Level::Error`.
+    pub fn try_steal_replace_and_emit_err(
+        &self,
+        span: Span,
+        key: StashKey,
+        new_err: Diag<'_>,
+    ) -> ErrorGuaranteed {
+        let key = (span.with_parent(None), key);
+        // FIXME(#120456) - is `swap_remove` correct?
+        let old_err = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key);
+        match old_err {
+            Some((old_err, guar)) => {
+                assert_eq!(old_err.level, Level::Error);
+                assert!(guar.is_some());
+                // Because `old_err` has already been counted, it can only be
+                // safely cancelled because the `new_err` supplants it.
+                Diag::<ErrorGuaranteed>::new_diagnostic(self, old_err).cancel();
+            }
+            None => {}
+        };
+        new_err.emit()
+    }
+
     pub fn has_stashed_diagnostic(&self, span: Span, key: StashKey) -> bool {
         self.inner.borrow().stashed_diagnostics.get(&(span.with_parent(None), key)).is_some()
     }
@@ -757,41 +828,40 @@ impl DiagCtxt {
         self.inner.borrow_mut().emit_stashed_diagnostics()
     }
 
-    /// This excludes lint errors, delayed bugs and stashed errors.
+    /// This excludes lint errors, and delayed bugs.
     #[inline]
     pub fn err_count_excluding_lint_errs(&self) -> usize {
-        self.inner.borrow().err_guars.len()
+        let inner = self.inner.borrow();
+        inner.err_guars.len()
+            + inner
+                .stashed_diagnostics
+                .values()
+                .filter(|(diag, guar)| guar.is_some() && diag.is_lint.is_none())
+                .count()
     }
 
-    /// This excludes delayed bugs and stashed errors.
+    /// This excludes delayed bugs.
     #[inline]
     pub fn err_count(&self) -> usize {
         let inner = self.inner.borrow();
-        inner.err_guars.len() + inner.lint_err_guars.len()
+        inner.err_guars.len()
+            + inner.lint_err_guars.len()
+            + inner.stashed_diagnostics.values().filter(|(_diag, guar)| guar.is_some()).count()
     }
 
-    /// This excludes normal errors, lint errors, and delayed bugs. Unless
-    /// absolutely necessary, avoid using this. It's dubious because stashed
-    /// errors can later be cancelled, so the presence of a stashed error at
-    /// some point of time doesn't guarantee anything -- there are no
-    /// `ErrorGuaranteed`s here.
-    pub fn stashed_err_count(&self) -> usize {
-        self.inner.borrow().stashed_err_count
-    }
-
-    /// This excludes lint errors, delayed bugs, and stashed errors. Unless
-    /// absolutely necessary, prefer `has_errors` to this method.
+    /// This excludes lint errors and delayed bugs. Unless absolutely
+    /// necessary, prefer `has_errors` to this method.
     pub fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
         self.inner.borrow().has_errors_excluding_lint_errors()
     }
 
-    /// This excludes delayed bugs and stashed errors.
+    /// This excludes delayed bugs.
     pub fn has_errors(&self) -> Option<ErrorGuaranteed> {
         self.inner.borrow().has_errors()
     }
 
-    /// This excludes stashed errors. Unless absolutely necessary, prefer
-    /// `has_errors` to this method.
+    /// This excludes nothing. Unless absolutely necessary, prefer `has_errors`
+    /// to this method.
     pub fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
         self.inner.borrow().has_errors_or_delayed_bugs()
     }
@@ -876,10 +946,10 @@ impl DiagCtxt {
         }
     }
 
-    /// This excludes delayed bugs and stashed errors. Used for early aborts
-    /// after errors occurred -- e.g. because continuing in the face of errors is
-    /// likely to lead to bad results, such as spurious/uninteresting
-    /// additional errors -- when returning an error `Result` is difficult.
+    /// This excludes delayed bugs. Used for early aborts after errors occurred
+    /// -- e.g. because continuing in the face of errors is likely to lead to
+    /// bad results, such as spurious/uninteresting additional errors -- when
+    /// returning an error `Result` is difficult.
     pub fn abort_if_errors(&self) {
         if self.has_errors().is_some() {
             FatalError.raise();
@@ -963,7 +1033,7 @@ impl DiagCtxt {
         inner
             .stashed_diagnostics
             .values_mut()
-            .for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
+            .for_each(|(diag, _guar)| diag.update_unstable_expectation_id(unstable_to_stable));
         inner
             .future_breakage_diagnostics
             .iter_mut()
@@ -1270,12 +1340,8 @@ impl DiagCtxtInner {
     fn emit_stashed_diagnostics(&mut self) -> Option<ErrorGuaranteed> {
         let mut guar = None;
         let has_errors = !self.err_guars.is_empty();
-        for (_, diag) in std::mem::take(&mut self.stashed_diagnostics).into_iter() {
-            if diag.is_error() {
-                if diag.is_lint.is_none() {
-                    self.stashed_err_count -= 1;
-                }
-            } else {
+        for (_, (diag, _guar)) in std::mem::take(&mut self.stashed_diagnostics).into_iter() {
+            if !diag.is_error() {
                 // Unless they're forced, don't flush stashed warnings when
                 // there are errors, to avoid causing warning overload. The
                 // stash would've been stolen already if it were important.
@@ -1334,7 +1400,8 @@ impl DiagCtxtInner {
                 } else {
                     let backtrace = std::backtrace::Backtrace::capture();
                     // This `unchecked_error_guaranteed` is valid. It is where the
-                    // `ErrorGuaranteed` for delayed bugs originates.
+                    // `ErrorGuaranteed` for delayed bugs originates. See
+                    // `DiagCtxtInner::drop`.
                     #[allow(deprecated)]
                     let guar = ErrorGuaranteed::unchecked_error_guaranteed();
                     self.delayed_bugs
@@ -1446,11 +1513,31 @@ impl DiagCtxtInner {
     }
 
     fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
-        self.err_guars.get(0).copied()
+        self.err_guars.get(0).copied().or_else(|| {
+            if let Some((_diag, guar)) = self
+                .stashed_diagnostics
+                .values()
+                .find(|(diag, guar)| guar.is_some() && diag.is_lint.is_none())
+            {
+                *guar
+            } else {
+                None
+            }
+        })
     }
 
     fn has_errors(&self) -> Option<ErrorGuaranteed> {
-        self.has_errors_excluding_lint_errors().or_else(|| self.lint_err_guars.get(0).copied())
+        self.err_guars.get(0).copied().or_else(|| self.lint_err_guars.get(0).copied()).or_else(
+            || {
+                if let Some((_diag, guar)) =
+                    self.stashed_diagnostics.values().find(|(_diag, guar)| guar.is_some())
+                {
+                    *guar
+                } else {
+                    None
+                }
+            },
+        )
     }
 
     fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {

compiler/rustc_hir_analysis/src/astconv/lint.rs

@@ -1,5 +1,5 @@
 use rustc_ast::TraitObjectSyntax;
-use rustc_errors::{codes::*, Diag, EmissionGuarantee, StashKey};
+use rustc_errors::{codes::*, Diag, EmissionGuarantee, Level, StashKey};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_lint_defs::{builtin::BARE_TRAIT_OBJECTS, Applicability};
@@ -237,7 +237,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                 }
                 // check if the impl trait that we are considering is a impl of a local trait
                 self.maybe_lint_blanket_trait_impl(self_ty, &mut diag);
-                diag.stash(self_ty.span, StashKey::TraitMissingMethod);
+                match diag.level() {
+                    Level::Error => {
+                        diag.stash(self_ty.span, StashKey::TraitMissingMethod);
+                    }
+                    Level::DelayedBug => {
+                        diag.emit();
+                    }
+                    _ => unreachable!(),
+                }
             } else {
                 let msg = "trait objects without an explicit `dyn` are deprecated";
                 tcx.node_span_lint(BARE_TRAIT_OBJECTS, self_ty.hir_id, self_ty.span, msg, |lint| {

compiler/rustc_hir_analysis/src/check/check.rs

@@ -1350,8 +1350,7 @@ fn check_type_alias_type_params_are_used<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalD
     let ty = tcx.type_of(def_id).instantiate_identity();
     if ty.references_error() {
         // If there is already another error, do not emit an error for not using a type parameter.
-        // Without the `stashed_err_count` part this can fail (#120856).
-        assert!(tcx.dcx().has_errors().is_some() || tcx.dcx().stashed_err_count() > 0);
+        assert!(tcx.dcx().has_errors().is_some());
         return;
     }