Skip to content

Commit

Permalink
Rollup merge of rust-lang#120828 - nnethercote:fix-stash-steal, r=oli…
Browse files Browse the repository at this point in the history
…-obk

Fix `ErrorGuaranteed` unsoundness with stash/steal.

When you stash an error, the error count is incremented. You can then use the non-zero error count to get an `ErrorGuaranteed`. You can then steal the error, which decrements the error count. You can then cancel the error.

Example code:
```
fn unsound(dcx: &DiagCtxt) -> ErrorGuaranteed {
    let sp = rustc_span::DUMMY_SP;
    let k = rustc_errors::StashKey::Cycle;
    dcx.struct_err("bogus").stash(sp, k);           // increment error count on stash
    let guar = dcx.has_errors().unwrap();           // ErrorGuaranteed from error count > 0
    let err = dcx.steal_diagnostic(sp, k).unwrap(); // decrement error count on steal
    err.cancel();                                   // cancel error
    guar                                            // ErrorGuaranteed with no error emitted!
}
```

This commit fixes the problem in the simplest way: by not counting stashed errors in `DiagCtxt::{err_count,has_errors}`.

However, just doing this without any other changes leads to over 40 ui test failures. Mostly because of uninteresting extra errors (many saying "type annotations needed" when type inference fails), and in a few cases, due to delayed bugs causing ICEs when no normal errors are printed.

To fix these, this commit adds `DiagCtxt::stashed_err_count`, and uses it in three places alongside `DiagCtxt::{has_errors,err_count}`. It's dodgy to rely on it, because unlike `DiagCtxt::err_count` it can go up and down. But it's needed to preserve existing behaviour, and at least the three places that need it are now obvious.

r? oli-obk
  • Loading branch information
matthiaskrgr authored Feb 9, 2024
2 parents 374d2b8 + 7619792 commit e0b21b8
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 42 deletions.
45 changes: 27 additions & 18 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ struct DiagCtxtInner {
/// The number of non-lint errors that have been emitted, including duplicates.
err_count: usize,

/// 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 @@ -602,6 +606,7 @@ impl DiagCtxt {
flags: DiagCtxtFlags { can_emit_warnings: true, ..Default::default() },
lint_err_count: 0,
err_count: 0,
stashed_err_count: 0,
deduplicated_err_count: 0,
deduplicated_warn_count: 0,
has_printed: false,
Expand Down Expand Up @@ -658,6 +663,7 @@ impl DiagCtxt {
let mut inner = self.inner.borrow_mut();
inner.lint_err_count = 0;
inner.err_count = 0;
inner.stashed_err_count = 0;
inner.deduplicated_err_count = 0;
inner.deduplicated_warn_count = 0;
inner.has_printed = false;
Expand All @@ -679,10 +685,8 @@ impl DiagCtxt {
let key = (span.with_parent(None), key);

if diag.is_error() {
if diag.is_lint.is_some() {
inner.lint_err_count += 1;
} else {
inner.err_count += 1;
if diag.is_lint.is_none() {
inner.stashed_err_count += 1;
}
}

Expand All @@ -698,10 +702,8 @@ impl DiagCtxt {
let key = (span.with_parent(None), key);
let diag = inner.stashed_diagnostics.remove(&key)?;
if diag.is_error() {
if diag.is_lint.is_some() {
inner.lint_err_count -= 1;
} else {
inner.err_count -= 1;
if diag.is_lint.is_none() {
inner.stashed_err_count -= 1;
}
}
Some(DiagnosticBuilder::new_diagnostic(self, diag))
Expand Down Expand Up @@ -927,13 +929,22 @@ impl DiagCtxt {
self.struct_bug(msg).emit()
}

/// This excludes lint errors and delayed bugs.
/// This excludes lint errors, delayed bugs, and stashed errors.
#[inline]
pub fn err_count(&self) -> usize {
self.inner.borrow().err_count
}

/// This excludes lint errors and delayed bugs.
/// 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.
pub fn has_errors(&self) -> Option<ErrorGuaranteed> {
self.inner.borrow().has_errors().then(|| {
// FIXME(nnethercote) find a way to store an `ErrorGuaranteed`.
Expand All @@ -942,8 +953,8 @@ impl DiagCtxt {
})
}

/// This excludes delayed bugs. Unless absolutely necessary, prefer
/// `has_errors` to this method.
/// 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> {
let inner = self.inner.borrow();
let result = inner.has_errors() || inner.lint_err_count > 0;
Expand All @@ -954,8 +965,8 @@ impl DiagCtxt {
})
}

/// Unless absolutely necessary, prefer `has_errors` or
/// `has_errors_or_lint_errors` to this method.
/// 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> {
let inner = self.inner.borrow();
let result =
Expand Down Expand Up @@ -1229,10 +1240,8 @@ impl DiagCtxtInner {
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_some() {
self.lint_err_count -= 1;
} else {
self.err_count -= 1;
if diag.is_lint.is_none() {
self.stashed_err_count -= 1;
}
} else {
// Unless they're forced, don't flush stashed warnings when
Expand Down
14 changes: 9 additions & 5 deletions compiler/rustc_hir_typeck/src/writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -753,10 +753,14 @@ impl<'cx, 'tcx> Resolver<'cx, 'tcx> {
}

fn report_error(&self, p: impl Into<ty::GenericArg<'tcx>>) -> ErrorGuaranteed {
match self.fcx.dcx().has_errors() {
Some(e) => e,
None => self
.fcx
if let Some(guar) = self.fcx.dcx().has_errors() {
guar
} else if self.fcx.dcx().stashed_err_count() > 0 {
// Without this case we sometimes get uninteresting and extraneous
// "type annotations needed" errors.
self.fcx.dcx().delayed_bug("error in Resolver")
} else {
self.fcx
.err_ctxt()
.emit_inference_failure_err(
self.fcx.tcx.hir().body_owner_def_id(self.body.id()),
Expand All @@ -765,7 +769,7 @@ impl<'cx, 'tcx> Resolver<'cx, 'tcx> {
E0282,
false,
)
.emit(),
.emit()
}
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_infer/src/infer/at.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ impl<'tcx> InferCtxt<'tcx> {
reported_signature_mismatch: self.reported_signature_mismatch.clone(),
tainted_by_errors: self.tainted_by_errors.clone(),
err_count_on_creation: self.err_count_on_creation,
stashed_err_count_on_creation: self.stashed_err_count_on_creation,
universe: self.universe.clone(),
intercrate,
next_trait_solver: self.next_trait_solver,
Expand Down
43 changes: 24 additions & 19 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,12 @@ pub struct InferCtxt<'tcx> {
// FIXME(matthewjasper) Merge into `tainted_by_errors`
err_count_on_creation: usize,

/// Track how many errors were stashed when this infcx is created.
/// Used for the same purpose as `err_count_on_creation`, even
/// though it's weaker because the count can go up and down.
// FIXME(matthewjasper) Merge into `tainted_by_errors`
stashed_err_count_on_creation: usize,

/// What is the innermost universe we have created? Starts out as
/// `UniverseIndex::root()` but grows from there as we enter
/// universal quantifiers.
Expand Down Expand Up @@ -711,6 +717,7 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
reported_signature_mismatch: Default::default(),
tainted_by_errors: Cell::new(None),
err_count_on_creation: tcx.dcx().err_count(),
stashed_err_count_on_creation: tcx.dcx().stashed_err_count(),
universe: Cell::new(ty::UniverseIndex::ROOT),
intercrate,
next_trait_solver,
Expand Down Expand Up @@ -1262,26 +1269,24 @@ impl<'tcx> InferCtxt<'tcx> {
/// inference variables, regionck errors).
#[must_use = "this method does not have any side effects"]
pub fn tainted_by_errors(&self) -> Option<ErrorGuaranteed> {
debug!(
"is_tainted_by_errors(err_count={}, err_count_on_creation={}, \
tainted_by_errors={})",
self.dcx().err_count(),
self.err_count_on_creation,
self.tainted_by_errors.get().is_some()
);

if let Some(e) = self.tainted_by_errors.get() {
return Some(e);
}

if self.dcx().err_count() > self.err_count_on_creation {
// errors reported since this infcx was made
let e = self.dcx().has_errors().unwrap();
self.set_tainted_by_errors(e);
return Some(e);
if let Some(guar) = self.tainted_by_errors.get() {
Some(guar)
} else if self.dcx().err_count() > self.err_count_on_creation {
// Errors reported since this infcx was made.
let guar = self.dcx().has_errors().unwrap();
self.set_tainted_by_errors(guar);
Some(guar)
} else if self.dcx().stashed_err_count() > self.stashed_err_count_on_creation {
// Errors stashed since this infcx was made. Not entirely reliable
// because the count of stashed errors can go down. But without
// this case we get a moderate number of uninteresting and
// extraneous "type annotations needed" errors.
let guar = self.dcx().delayed_bug("tainted_by_errors: stashed bug awaiting emission");
self.set_tainted_by_errors(guar);
Some(guar)
} else {
None
}

None
}

/// Set the "tainted by errors" flag to true. We call this when we
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,10 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
// kindck is gone now). -nmatsakis
if let Some(reported) = sess.dcx().has_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"));
}

sess.time("misc_checking_3", || {
Expand Down

0 comments on commit e0b21b8

Please sign in to comment.