bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Fix ErrorGuaranteed unsoundness with stash/steal.

Browse source 
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.
This commit is contained in:
Nicholas Nethercote 2024-02-09 06:42:50 +11:00
parent 6894f435d3
commit 7619792107
5 changed files with 65 additions and 42 deletions
Download patch file Download diff file Expand all files Collapse all files

45
compiler/rustc_errors/src/lib.rs
View file

@ -429,6 +429,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.
@ -598,6 +602,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,
@ -654,6 +659,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;
@ -675,10 +681,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;
}
}
@ -694,10 +698,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))
@ -922,13 +924,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`.
@ -937,8 +948,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;
@ -949,8 +960,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 =
@ -1224,10 +1235,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