Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Top level error handling #121206

Merged
merged 9 commits into from
Feb 22, 2024
13 changes: 6 additions & 7 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)| {
Expand Down Expand Up @@ -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)),
}
}

Expand Down Expand Up @@ -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();
}
}

Expand Down
5 changes: 1 addition & 4 deletions compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 18 additions & 21 deletions compiler/rustc_driver_impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {});
}
}

Expand Down
14 changes: 10 additions & 4 deletions compiler/rustc_driver_impl/src/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -18,7 +19,6 @@ use std::fmt::Write;

pub use self::PpMode::*;
pub use self::PpSourceMode::*;
use crate::abort_on_err;

struct AstNoAnn;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand Down
83 changes: 51 additions & 32 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -750,17 +753,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
Expand All @@ -769,27 +779,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();
nnethercote marked this conversation as resolved.
Show resolved Hide resolved
// 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;
Expand Down Expand Up @@ -864,10 +876,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();
}
}
Expand Down Expand Up @@ -1268,10 +1282,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;
Expand All @@ -1284,8 +1298,9 @@ impl DiagCtxtInner {
continue;
}
}
self.emit_diagnostic(diag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eagerly calls emit_diagnostic, looks suspicious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand this. How is it suspicious? What is the alternative to calling it "eagerly"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, linked wrong line.

guar = guar.or(self.emit_diagnostic(diag));

If i see or with Option, expecting that or's argument will be 1. cheap (for perf reasons, not applied here) 2. without side effects, otherwise using or is misleading: you eval arg, but throwing out its result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. How would you write it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the idea here is that every ErrorGuaranteed is equivalent. So it doesn't matter which one we use. And it also relies on or not short-circuiting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other problem is that in future someone will see that function call in .or(..) and will refactor into .or_else(..) which will break that not short-circuiting expectation here.

guar = guar.or(self.emit_diagnostic(diag));
}
guar
}

// Return value is only `Some` if the level is `Error` or `DelayedBug`.
Expand Down Expand Up @@ -1329,7 +1344,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();
Expand Down Expand Up @@ -1445,17 +1460,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`.
Expand Down Expand Up @@ -1488,6 +1502,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;
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_incremental/src/persist/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_incremental/src/persist/save.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub fn save_dep_graph(tcx: TyCtxt<'_>) {
return;
}
// This is going to be deleted in finalize_session_directory, so let's not create it.
if sess.dcx().has_errors_or_lint_errors_or_delayed_bugs().is_some() {
if sess.dcx().has_errors_or_delayed_bugs().is_some() {
return;
}

Expand Down Expand Up @@ -87,7 +87,7 @@ pub fn save_work_product_index(
return;
}
// This is going to be deleted in finalize_session_directory, so let's not create it
if sess.dcx().has_errors_or_lint_errors().is_some() {
if sess.dcx().has_errors().is_some() {
return;
}

Expand Down
Loading
Loading