Skip to content

Commit

Permalink
Unrolled build for rust-lang#121669
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#121669 - nnethercote:count-stashed-errs-again, r=estebank

Count stashed errors again

Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.

rust-lang#120828 and rust-lang#121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by rust-lang#121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.

r? `@oli-obk`
  • Loading branch information
rust-timer authored Feb 29, 2024
2 parents 384d26f + 82961c0 commit 0cb66bd
Show file tree
Hide file tree
Showing 40 changed files with 475 additions and 342 deletions.
8 changes: 3 additions & 5 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1308,11 +1308,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.
Expand Down
219 changes: 154 additions & 65 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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>,

Expand Down Expand Up @@ -559,10 +555,18 @@ pub struct DiagCtxtFlags {

impl Drop for DiagCtxtInner {
fn drop(&mut self) {
// Any stashed diagnostics should have been handled by
// `emit_stashed_diagnostics` by now.
assert!(self.stashed_diagnostics.is_empty());
// For tools using `interface::run_compiler` (e.g. rustc, rustdoc)
// stashed diagnostics will have already been emitted. But for others
// that don't use `interface::run_compiler` (e.g. rustfmt, some clippy
// lints) this fallback is necessary.
//
// Important: it is sound to produce an `ErrorGuaranteed` when stashing
// errors because they are guaranteed to be emitted here or earlier.
self.emit_stashed_diagnostics();

// 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()
}
Expand Down Expand Up @@ -615,7 +619,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,
Expand Down Expand Up @@ -676,7 +679,6 @@ impl DiagCtxt {
err_guars,
lint_err_guars,
delayed_bugs,
stashed_err_count,
deduplicated_err_count,
deduplicated_warn_count,
emitter: _,
Expand All @@ -699,7 +701,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;
Expand All @@ -715,39 +716,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()
}
Expand All @@ -757,41 +830,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()
}

/// 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
inner.err_guars.len()
+ inner.lint_err_guars.len()
+ inner.stashed_diagnostics.values().filter(|(_diag, guar)| guar.is_some()).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()
}
Expand Down Expand Up @@ -876,10 +948,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();
Expand Down Expand Up @@ -963,7 +1035,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()
Expand Down Expand Up @@ -1270,12 +1342,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.
Expand Down Expand Up @@ -1334,7 +1402,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
Expand Down Expand Up @@ -1446,11 +1515,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> {
Expand Down
12 changes: 10 additions & 2 deletions compiler/rustc_hir_analysis/src/astconv/lint.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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| {
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
Loading

0 comments on commit 0cb66bd

Please sign in to comment.