Overhaul the handling of errors at the top-level.

nnethercote · nnethercote · commit 72b172bdf631 · 2024-02-22T08:03:47.000+11:00
Currently `emit_stashed_diagnostic` is called from four(!) different
places: `print_error_count`, `DiagCtxtInner::drop`, `abort_if_errors`,
and `compile_status`.

And `flush_delayed` is called from two different places:
`DiagCtxtInner::drop` and `Queries`.

This is pretty gross! Each one should really be called from a single
place, but there's a bunch of entanglements. This commit cleans up this
mess.

Specifically, it:
- Removes all the existing calls to `emit_stashed_diagnostic`, and adds
  a single new call in `finish_diagnostics`.
- Removes the early `flush_delayed` call in `codegen_and_build_linker`,
  replacing it with a simple early return if delayed bugs are present.
- Changes `DiagCtxtInner::drop` and `DiagCtxtInner::flush_delayed` so
  they both assert that the stashed diagnostics are empty (i.e.
  processed beforehand).
- Changes `interface::run_compiler` so that any errors emitted during
  `finish_diagnostics` (i.e. late-emitted stashed diagnostics) are
  counted and cannot be overlooked. This requires adding
  `ErrorGuaranteed` return values to several functions.
- Removes the `stashed_err_count` call in `analysis`. This is possible
  now that we don't have to worry about calling `flush_delayed` early
  from `codegen_and_build_linker` when stashed diagnostics are pending.
- Changes the `span_bug` case in `handle_tuple_field_pattern_match` to a
  `delayed_span_bug`, because it now can be reached due to the removal
  of the `stashed_err_count` call in `analysis`.
- Slightly changes the expected output of three tests. If no errors are
  emitted but there are delayed bugs, the error count is no longer
  printed. This is because delayed bugs are now always printed after the
  error count is printed (or not printed, if the error count is zero).

There is a lot going on in this commit. It's hard to break into smaller
pieces because the existing code is very tangled. It took me a long time
and a lot of effort to understand how the different pieces interact, and
I think the new code is a lot simpler and easier to understand.
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
@@ -471,9 +471,10 @@ struct DiagCtxtInner {
     emitted_diagnostics: FxHashSet<Hash128>,
 
     /// Stashed diagnostics emitted in one stage of the compiler that may be
-    /// stolen by other stages (e.g. to improve them and add more information).
-    /// The stashed diagnostics count towards the total error count.
-    /// When `.abort_if_errors()` is called, these are also emitted.
+    /// stolen and emitted/cancelled by other stages (e.g. to improve them and
+    /// 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), Diagnostic>,
 
     future_breakage_diagnostics: Vec<Diagnostic>,
@@ -558,7 +559,9 @@ pub struct DiagCtxtFlags {
 
 impl Drop for DiagCtxtInner {
     fn drop(&mut self) {
-        self.emit_stashed_diagnostics();
+        // Any stashed diagnostics should have been handled by
+        // `emit_stashed_diagnostics` by now.
+        assert!(self.stashed_diagnostics.is_empty());
 
         if self.err_guars.is_empty() {
             self.flush_delayed()
@@ -750,7 +753,7 @@ impl DiagCtxt {
     }
 
     /// Emit all stashed diagnostics.
-    pub fn emit_stashed_diagnostics(&self) {
+    pub fn emit_stashed_diagnostics(&self) -> Option<ErrorGuaranteed> {
         self.inner.borrow_mut().emit_stashed_diagnostics()
     }
 
@@ -796,7 +799,9 @@ impl DiagCtxt {
     pub fn print_error_count(&self, registry: &Registry) {
         let mut inner = self.inner.borrow_mut();
 
-        inner.emit_stashed_diagnostics();
+        // Any stashed diagnostics should have been handled by
+        // `emit_stashed_diagnostics` by now.
+        assert!(inner.stashed_diagnostics.is_empty());
 
         if inner.treat_err_as_bug() {
             return;
@@ -872,9 +877,7 @@ impl DiagCtxt {
     }
 
     pub fn abort_if_errors(&self) {
-        let mut inner = self.inner.borrow_mut();
-        inner.emit_stashed_diagnostics();
-        if !inner.err_guars.is_empty() {
+        if self.has_errors().is_some() {
             FatalError.raise();
         }
     }
@@ -1275,7 +1278,8 @@ impl DiagCtxt {
 // `DiagCtxtInner::foo`.
 impl DiagCtxtInner {
     /// Emit all stashed diagnostics.
-    fn emit_stashed_diagnostics(&mut self) {
+    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() {
@@ -1290,8 +1294,9 @@ impl DiagCtxtInner {
                     continue;
                 }
             }
-            self.emit_diagnostic(diag);
+            guar = guar.or(self.emit_diagnostic(diag));
         }
+        guar
     }
 
     // Return value is only `Some` if the level is `Error` or `DelayedBug`.
@@ -1493,6 +1498,11 @@ impl DiagCtxtInner {
     }
 
     fn flush_delayed(&mut self) {
+        // Stashed diagnostics must be emitted before delayed bugs are flushed.
+        // Otherwise, we might ICE prematurely when errors would have
+        // eventually happened.
+        assert!(self.stashed_diagnostics.is_empty());
+
         if self.delayed_bugs.is_empty() {
             return;
         }
diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs
@@ -423,18 +423,43 @@ pub fn run_compiler<R: Send>(config: Config, f: impl FnOnce(&Compiler) -> R + Se
                 Compiler { sess, codegen_backend, override_queries: config.override_queries };
 
             rustc_span::set_source_map(compiler.sess.parse_sess.clone_source_map(), move || {
-                let r = {
-                    let _sess_abort_error = defer(|| {
-                        compiler.sess.finish_diagnostics(&config.registry);
+                // There are two paths out of `f`.
+                // - Normal exit.
+                // - Panic, e.g. triggered by `abort_if_errors`.
+                //
+                // We must run `finish_diagnostics` in both cases.
+                let res = {
+                    // If `f` panics, `finish_diagnostics` will run during
+                    // unwinding because of the `defer`.
+                    let mut guar = None;
+                    let sess_abort_guard = defer(|| {
+                        guar = compiler.sess.finish_diagnostics(&config.registry);
                     });
 
-                    f(&compiler)
+                    let res = f(&compiler);
+
+                    // If `f` doesn't panic, `finish_diagnostics` will run
+                    // normally when `sess_abort_guard` is dropped.
+                    drop(sess_abort_guard);
+
+                    // If `finish_diagnostics` emits errors (e.g. stashed
+                    // errors) we can't return an error directly, because the
+                    // return type of this function is `R`, not `Result<R, E>`.
+                    // But we need to communicate the errors' existence to the
+                    // caller, otherwise the caller might mistakenly think that
+                    // no errors occurred and return a zero exit code. So we
+                    // abort (panic) instead, similar to if `f` had panicked.
+                    if guar.is_some() {
+                        compiler.sess.dcx().abort_if_errors();
+                    }
+
+                    res
                 };
 
                 let prof = compiler.sess.prof.clone();
-
                 prof.generic_activity("drop_compiler").run(move || drop(compiler));
-                r
+
+                res
             })
         },
     )
diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs
@@ -772,12 +772,11 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
     // lot of annoying errors in the ui tests (basically,
     // lint warnings and so on -- kindck used to do this abort, but
     // kindck is gone now). -nmatsakis
-    if let Some(reported) = sess.dcx().has_errors_excluding_lint_errors() {
-        return Err(reported);
-    } else if sess.dcx().stashed_err_count() > 0 {
-        // Without this case we sometimes get delayed bug ICEs and I don't
-        // understand why. -nnethercote
-        return Err(sess.dcx().delayed_bug("some stashed error is waiting for use"));
+    //
+    // But we exclude lint errors from this, because lint errors are typically
+    // less serious and we're more likely to want to continue (#87337).
+    if let Some(guar) = sess.dcx().has_errors_excluding_lint_errors() {
+        return Err(guar);
     }
 
     sess.time("misc_checking_3", || {
diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs
@@ -222,12 +222,12 @@ impl<'tcx> Queries<'tcx> {
 
     pub fn codegen_and_build_linker(&'tcx self) -> Result<Linker> {
         self.global_ctxt()?.enter(|tcx| {
-            // Don't do code generation if there were any errors
-            self.compiler.sess.compile_status()?;
-
-            // If we have any delayed bugs, for example because we created TyKind::Error earlier,
-            // it's likely that codegen will only cause more ICEs, obscuring the original problem
-            self.compiler.sess.dcx().flush_delayed();
+            // Don't do code generation if there were any errors. Likewise if
+            // there were any delayed bugs, because codegen will likely cause
+            // more ICEs, obscuring the original problem.
+            if let Some(guar) = self.compiler.sess.dcx().has_errors_or_delayed_bugs() {
+                return Err(guar);
+            }
 
             // Hook for UI tests.
             Self::check_for_rustc_errors_attr(tcx);
diff --git a/compiler/rustc_passes/src/dead.rs b/compiler/rustc_passes/src/dead.rs
@@ -237,7 +237,10 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
     ) {
         let variant = match self.typeck_results().node_type(lhs.hir_id).kind() {
             ty::Adt(adt, _) => adt.variant_of_res(res),
-            _ => span_bug!(lhs.span, "non-ADT in tuple struct pattern"),
+            _ => {
+                self.tcx.dcx().span_delayed_bug(lhs.span, "non-ADT in tuple struct pattern");
+                return;
+            }
         };
         let dotdot = dotdot.as_opt_usize().unwrap_or(pats.len());
         let first_n = pats.iter().enumerate().take(dotdot);
diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs
@@ -258,7 +258,8 @@ impl Session {
         }
     }
 
-    fn check_miri_unleashed_features(&self) {
+    fn check_miri_unleashed_features(&self) -> Option<ErrorGuaranteed> {
+        let mut guar = None;
         let unleashed_features = self.miri_unleashed_features.lock();
         if !unleashed_features.is_empty() {
             let mut must_err = false;
@@ -279,18 +280,22 @@ impl Session {
             // If we should err, make sure we did.
             if must_err && self.dcx().has_errors().is_none() {
                 // We have skipped a feature gate, and not run into other errors... reject.
-                self.dcx().emit_err(errors::NotCircumventFeature);
+                guar = Some(self.dcx().emit_err(errors::NotCircumventFeature));
             }
         }
+        guar
     }
 
     /// Invoked all the way at the end to finish off diagnostics printing.
-    pub fn finish_diagnostics(&self, registry: &Registry) {
-        self.check_miri_unleashed_features();
+    pub fn finish_diagnostics(&self, registry: &Registry) -> Option<ErrorGuaranteed> {
+        let mut guar = None;
+        guar = guar.or(self.check_miri_unleashed_features());
+        guar = guar.or(self.dcx().emit_stashed_diagnostics());
         self.dcx().print_error_count(registry);
         if self.opts.json_future_incompat {
             self.dcx().emit_future_breakage_report();
         }
+        guar
     }
 
     /// Returns true if the crate is a testing one.
@@ -314,7 +319,6 @@ impl Session {
 
     pub fn compile_status(&self) -> Result<(), ErrorGuaranteed> {
         if let Some(reported) = self.dcx().has_errors() {
-            self.dcx().emit_stashed_diagnostics();
             Err(reported)
         } else {
             Ok(())
diff --git a/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr b/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr
@@ -21,5 +21,3 @@ LL |     same_output(foo, rpit);
 
 query stack during panic:
 end of query stack
-error: aborting due to 2 previous errors
-
diff --git a/tests/ui/inference/issue-80409.no-compat.stderr b/tests/ui/inference/issue-80409.no-compat.stderr
@@ -12,5 +12,3 @@ LL |     builder.state().on_entry(|_| {});
 
 query stack during panic:
 end of query stack
-error: aborting due to 1 previous error
-
diff --git a/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr b/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr
@@ -21,5 +21,3 @@ LL |     query(get_rpit);
 
 query stack during panic:
 end of query stack
-error: aborting due to 2 previous errors
-

Original file line number	Diff line number	Diff line change
`@@ -258,7 +258,8 @@ impl Session {`
`258`	`258`	`}`
`259`	`259`	`}`
`260`	`260`
`261`		`- fn check_miri_unleashed_features(&self) {`
	`261`	`+ fn check_miri_unleashed_features(&self) -> Option<ErrorGuaranteed> {`
	`262`	`+ let mut guar = None;`
`262`	`263`	`let unleashed_features = self.miri_unleashed_features.lock();`
`263`	`264`	`if !unleashed_features.is_empty() {`
`264`	`265`	`let mut must_err = false;`
`@@ -279,18 +280,22 @@ impl Session {`
`279`	`280`	`// If we should err, make sure we did.`
`280`	`281`	`if must_err && self.dcx().has_errors().is_none() {`
`281`	`282`	`// We have skipped a feature gate, and not run into other errors... reject.`
`282`		`- self.dcx().emit_err(errors::NotCircumventFeature);`
	`283`	`+ guar = Some(self.dcx().emit_err(errors::NotCircumventFeature));`
`283`	`284`	`}`
`284`	`285`	`}`
	`286`	`+ guar`
`285`	`287`	`}`
`286`	`288`
`287`	`289`	`/// Invoked all the way at the end to finish off diagnostics printing.`
`288`		`- pub fn finish_diagnostics(&self, registry: &Registry) {`
`289`		`- self.check_miri_unleashed_features();`
	`290`	`+ pub fn finish_diagnostics(&self, registry: &Registry) -> Option<ErrorGuaranteed> {`
	`291`	`+ let mut guar = None;`
	`292`	`+ guar = guar.or(self.check_miri_unleashed_features());`
	`293`	`+ guar = guar.or(self.dcx().emit_stashed_diagnostics());`
`290`	`294`	`self.dcx().print_error_count(registry);`
`291`	`295`	`if self.opts.json_future_incompat {`
`292`	`296`	`self.dcx().emit_future_breakage_report();`
`293`	`297`	`}`
	`298`	`+ guar`
`294`	`299`	`}`
`295`	`300`
`296`	`301`	`/// Returns true if the crate is a testing one.`
`@@ -314,7 +319,6 @@ impl Session {`
`314`	`319`
`315`	`320`	`pub fn compile_status(&self) -> Result<(), ErrorGuaranteed> {`
`316`	`321`	`if let Some(reported) = self.dcx().has_errors() {`
`317`		`- self.dcx().emit_stashed_diagnostics();`
`318`	`322`	`Err(reported)`
`319`	`323`	`} else {`
`320`	`324`	`Ok(())`