Rollup merge of rust-lang#121206 - nnethercote:top-level-error-handling, r=oli-obk

Top level error handling

The interactions between the following things are surprisingly complicated:
- `emit_stashed_diagnostics`,
- `flush_delayed`,
- normal return vs `abort_if_errors`/`FatalError.raise()` unwinding in the call to the closure in `interface::run_compiler`.

This PR disentangles it all.

r? `@oli-obk`

Author: Noratrieb

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -881,7 +881,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                         &item.vis,
                         errors::VisibilityNotPermittedNote::TraitImpl,
                     );
-                    // njn: use Dummy here
                     if let TyKind::Err(_) = self_ty.kind {
                         this.dcx().emit_err(errors::ObsoleteAuto { span: item.span });
                     }

compiler/rustc_codegen_ssa/src/back/link.rs

@@ -3,7 +3,7 @@ use rustc_ast::CRATE_NODE_ID;
 use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
 use rustc_data_structures::memmap::Mmap;
 use rustc_data_structures::temp_dir::MaybeTempDir;
-use rustc_errors::{DiagCtxt, ErrorGuaranteed};
+use rustc_errors::{DiagCtxt, ErrorGuaranteed, FatalError};
 use rustc_fs_util::{fix_windows_verbatim_for_gcc, try_canonicalize};
 use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
 use rustc_metadata::find_native_static_library;
@@ -487,7 +487,9 @@ fn collate_raw_dylibs<'a, 'b>(
             }
         }
     }
-    sess.compile_status()?;
+    if let Some(guar) = sess.dcx().has_errors() {
+        return Err(guar);
+    }
     Ok(dylib_table
         .into_iter()
         .map(|(name, imports)| {
@@ -720,10 +722,7 @@ fn link_dwarf_object<'a>(
         Ok(())
     }) {
         Ok(()) => {}
-        Err(e) => {
-            sess.dcx().emit_err(errors::ThorinErrorWrapper(e));
-            sess.dcx().abort_if_errors();
-        }
+        Err(e) => sess.dcx().emit_fatal(errors::ThorinErrorWrapper(e)),
     }
 }
 
@@ -999,7 +998,7 @@ fn link_natively<'a>(
                 sess.dcx().emit_note(errors::CheckInstalledVisualStudio);
                 sess.dcx().emit_note(errors::InsufficientVSCodeProduct);
             }
-            sess.dcx().abort_if_errors();
+            FatalError.raise();
         }
     }
 

compiler/rustc_codegen_ssa/src/base.rs

@@ -449,10 +449,7 @@ pub fn maybe_create_entry_wrapper<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         let Some(llfn) = cx.declare_c_main(llfty) else {
             // FIXME: We should be smart and show a better diagnostic here.
             let span = cx.tcx().def_span(rust_main_def_id);
-            let dcx = cx.tcx().dcx();
-            dcx.emit_err(errors::MultipleMainFunctions { span });
-            dcx.abort_if_errors();
-            bug!();
+            cx.tcx().dcx().emit_fatal(errors::MultipleMainFunctions { span });
         };
 
         // `main` should respect same config for frame pointer elimination as rest of code

compiler/rustc_driver_impl/src/lib.rs

@@ -144,16 +144,6 @@ pub const EXIT_FAILURE: i32 = 1;
 pub const DEFAULT_BUG_REPORT_URL: &str = "https://github.com/rust-lang/rust/issues/new\
     ?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md";
 
-pub fn abort_on_err<T>(result: Result<T, ErrorGuaranteed>, sess: &Session) -> T {
-    match result {
-        Err(..) => {
-            sess.dcx().abort_if_errors();
-            panic!("error reported but abort_if_errors didn't abort???");
-        }
-        Ok(x) => x,
-    }
-}
-
 pub trait Callbacks {
     /// Called before creating the compiler instance
     fn config(&mut self, _config: &mut interface::Config) {}
@@ -349,27 +339,33 @@ fn run_compiler(
         },
     };
 
-    callbacks.config(&mut config);
-
-    default_early_dcx.abort_if_errors();
     drop(default_early_dcx);
 
+    callbacks.config(&mut config);
+
     interface::run_compiler(config, |compiler| {
         let sess = &compiler.sess;
         let codegen_backend = &*compiler.codegen_backend;
 
+        // This is used for early exits unrelated to errors. E.g. when just
+        // printing some information without compiling, or exiting immediately
+        // after parsing, etc.
+        let early_exit = || {
+            if let Some(guar) = sess.dcx().has_errors() { Err(guar) } else { Ok(()) }
+        };
+
         // This implements `-Whelp`. It should be handled very early, like
         // `--help`/`-Zhelp`/`-Chelp`. This is the earliest it can run, because
         // it must happen after lints are registered, during session creation.
         if sess.opts.describe_lints {
             describe_lints(sess);
-            return sess.compile_status();
+            return early_exit();
         }
 
         let early_dcx = EarlyDiagCtxt::new(sess.opts.error_format);
 
         if print_crate_info(&early_dcx, codegen_backend, sess, has_input) == Compilation::Stop {
-            return sess.compile_status();
+            return early_exit();
         }
 
         if !has_input {
@@ -378,16 +374,16 @@ fn run_compiler(
 
         if !sess.opts.unstable_opts.ls.is_empty() {
             list_metadata(&early_dcx, sess, &*codegen_backend.metadata_loader());
-            return sess.compile_status();
+            return early_exit();
         }
 
         if sess.opts.unstable_opts.link_only {
             process_rlink(sess, compiler);
-            return sess.compile_status();
+            return early_exit();
         }
 
         let linker = compiler.enter(|queries| {
-            let early_exit = || sess.compile_status().map(|_| None);
+            let early_exit = || early_exit().map(|_| None);
             queries.parse()?;
 
             if let Some(ppm) = &sess.opts.pretty {
@@ -659,10 +655,11 @@ fn process_rlink(sess: &Session, compiler: &interface::Compiler) {
                 };
             }
         };
-        let result = compiler.codegen_backend.link(sess, codegen_results, &outputs);
-        abort_on_err(result, sess);
+        if compiler.codegen_backend.link(sess, codegen_results, &outputs).is_err() {
+            FatalError.raise();
+        }
     } else {
-        dcx.emit_fatal(RlinkNotAFile {})
+        dcx.emit_fatal(RlinkNotAFile {});
     }
 }
 

compiler/rustc_driver_impl/src/pretty.rs

@@ -2,6 +2,7 @@
 
 use rustc_ast as ast;
 use rustc_ast_pretty::pprust as pprust_ast;
+use rustc_errors::FatalError;
 use rustc_hir as hir;
 use rustc_hir_pretty as pprust_hir;
 use rustc_middle::bug;
@@ -18,7 +19,6 @@ use std::fmt::Write;
 
 pub use self::PpMode::*;
 pub use self::PpSourceMode::*;
-use crate::abort_on_err;
 
 struct AstNoAnn;
 
@@ -243,7 +243,9 @@ impl<'tcx> PrintExtra<'tcx> {
 
 pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) {
     if ppm.needs_analysis() {
-        abort_on_err(ex.tcx().analysis(()), sess);
+        if ex.tcx().analysis(()).is_err() {
+            FatalError.raise();
+        }
     }
 
     let (src, src_name) = get_source(sess);
@@ -334,7 +336,9 @@ pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) {
         ThirTree => {
             let tcx = ex.tcx();
             let mut out = String::new();
-            abort_on_err(rustc_hir_analysis::check_crate(tcx), tcx.sess);
+            if rustc_hir_analysis::check_crate(tcx).is_err() {
+                FatalError.raise();
+            }
             debug!("pretty printing THIR tree");
             for did in tcx.hir().body_owners() {
                 let _ = writeln!(out, "{:?}:\n{}\n", did, tcx.thir_tree(did));
@@ -344,7 +348,9 @@ pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) {
         ThirFlat => {
             let tcx = ex.tcx();
             let mut out = String::new();
-            abort_on_err(rustc_hir_analysis::check_crate(tcx), tcx.sess);
+            if rustc_hir_analysis::check_crate(tcx).is_err() {
+                FatalError.raise();
+            }
             debug!("pretty printing THIR flat");
             for did in tcx.hir().body_owners() {
                 let _ = writeln!(out, "{:?}:\n{}\n", did, tcx.thir_flat(did));

compiler/rustc_errors/src/lib.rs

@@ -476,9 +476,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>,
@@ -563,7 +564,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()
@@ -755,17 +758,24 @@ 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()
     }
 
-    /// This excludes lint errors, delayed bugs, and stashed errors.
+    /// This excludes lint errors, delayed bugs and stashed errors.
     #[inline]
-    pub fn err_count(&self) -> usize {
+    pub fn err_count_excluding_lint_errs(&self) -> usize {
         self.inner.borrow().err_guars.len()
     }
 
-    /// This excludes normal errors, lint errors and delayed bugs. Unless
+    /// This excludes delayed bugs and stashed errors.
+    #[inline]
+    pub fn err_count(&self) -> usize {
+        let inner = self.inner.borrow();
+        inner.err_guars.len() + inner.lint_err_guars.len()
+    }
+
+    /// 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
@@ -774,27 +784,29 @@ impl DiagCtxt {
         self.inner.borrow().stashed_err_count
     }
 
-    /// This excludes lint errors, delayed bugs, and stashed errors.
-    pub fn has_errors(&self) -> Option<ErrorGuaranteed> {
-        self.inner.borrow().has_errors()
+    /// This excludes lint errors, delayed bugs, and stashed errors. 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. Unless absolutely
-    /// necessary, prefer `has_errors` to this method.
-    pub fn has_errors_or_lint_errors(&self) -> Option<ErrorGuaranteed> {
-        self.inner.borrow().has_errors_or_lint_errors()
+    /// This excludes delayed bugs and stashed errors.
+    pub fn has_errors(&self) -> Option<ErrorGuaranteed> {
+        self.inner.borrow().has_errors()
     }
 
     /// This excludes stashed errors. Unless absolutely necessary, prefer
-    /// `has_errors` or `has_errors_or_lint_errors` to this method.
-    pub fn has_errors_or_lint_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
-        self.inner.borrow().has_errors_or_lint_errors_or_delayed_bugs()
+    /// `has_errors` to this method.
+    pub fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
+        self.inner.borrow().has_errors_or_delayed_bugs()
     }
 
     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;
@@ -869,10 +881,12 @@ 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.
     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();
         }
     }
@@ -1273,10 +1287,10 @@ 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() {
-            // Decrement the count tracking the stash; emitting will increment it.
             if diag.is_error() {
                 if diag.is_lint.is_none() {
                     self.stashed_err_count -= 1;
@@ -1289,8 +1303,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`.
@@ -1334,7 +1349,7 @@ impl DiagCtxtInner {
             DelayedBug => {
                 // If we have already emitted at least one error, we don't need
                 // to record the delayed bug, because it'll never be used.
-                return if let Some(guar) = self.has_errors_or_lint_errors() {
+                return if let Some(guar) = self.has_errors() {
                     Some(guar)
                 } else {
                     let backtrace = std::backtrace::Backtrace::capture();
@@ -1451,17 +1466,16 @@ impl DiagCtxtInner {
             .is_some_and(|c| self.err_guars.len() + self.lint_err_guars.len() + 1 >= c.get())
     }
 
-    fn has_errors(&self) -> Option<ErrorGuaranteed> {
+    fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
         self.err_guars.get(0).copied()
     }
 
-    fn has_errors_or_lint_errors(&self) -> Option<ErrorGuaranteed> {
-        self.has_errors().or_else(|| self.lint_err_guars.get(0).copied())
+    fn has_errors(&self) -> Option<ErrorGuaranteed> {
+        self.has_errors_excluding_lint_errors().or_else(|| self.lint_err_guars.get(0).copied())
     }
 
-    fn has_errors_or_lint_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
-        self.has_errors_or_lint_errors()
-            .or_else(|| self.delayed_bugs.get(0).map(|(_, guar)| guar).copied())
+    fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
+        self.has_errors().or_else(|| self.delayed_bugs.get(0).map(|(_, guar)| guar).copied())
     }
 
     /// Translate `message` eagerly with `args` to `SubdiagnosticMessage::Eager`.
@@ -1484,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;
         }

compiler/rustc_incremental/src/persist/fs.rs

@@ -312,7 +312,7 @@ pub fn finalize_session_directory(sess: &Session, svh: Option<Svh>) {
 
     let incr_comp_session_dir: PathBuf = sess.incr_comp_session_dir().clone();
 
-    if sess.dcx().has_errors_or_lint_errors_or_delayed_bugs().is_some() {
+    if sess.dcx().has_errors_or_delayed_bugs().is_some() {
         // If there have been any errors during compilation, we don't want to
         // publish this session directory. Rather, we'll just delete it.