Fix `ErrorGuaranteed` unsoundness with stash/steal. #120828

nnethercote · 2024-02-09T05:56:31Z

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

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.

rustbot · 2024-02-09T05:56:39Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

nnethercote · 2024-02-09T07:29:15Z

I botched the review request syntax:

r? @oli-obk

oli-obk · 2024-02-09T07:33:49Z

Thanks! This is great, now I can tackle these just like the has_errors checks

@bors r+ rollup

bors · 2024-02-09T07:33:52Z

📌 Commit 7619792 has been approved by oli-obk

It is now in the queue for this repository.

…-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

nnethercote · 2024-02-09T09:21:01Z

Thanks! This is great, now I can tackle these just like the has_errors checks

What does "these" refer to here? I have an in-progress change that removes the unchecked_error_guaranteed() calls in DiagCtxt::has_errors and its related methods...

nnethercote · 2024-02-09T09:23:54Z

#120833 has the in-progress work. The second commit contains the unchecked_error_guaranteed removals. I have a couple of follow-up changes I want to work on next week. But I posted it because I don't want us to duplicate any work.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) r? `@ghost` `@rustbot` modify labels: rollup

oli-obk · 2024-02-09T09:49:28Z

compiler/rustc_infer/src/infer/mod.rs

+            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();


I am planning on getting rid of this one by correctly bubbling up errors instead.

oli-obk · 2024-02-09T09:50:34Z

compiler/rustc_infer/src/infer/mod.rs

+            // 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");


This one is harder due to the stashing. I'm considering differentiating between "avoid type_annotations_needed" usages and other usages.

IIRC, if the we get rid of the MaybeForgetReturn StashKey by just emitting those errors immediately, we can then remove this stashed_err_count call and most or all of the extra "type annotations needed" errors go away, and the reduction in quality of other error messages is very small. In other words, stashing MaybeForgetReturn errors seems like it's more trouble than it's worth.

…-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

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#113026 (Introduce `run-make` V2 infrastructure, a `run_make_support` library and port over 2 tests as example) - rust-lang#113671 (Make privacy visitor use types more (instead of HIR)) - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) - rust-lang#120831 (Startup objects disappearing from sysroot) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113671 (Make privacy visitor use types more (instead of HIR)) - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) - rust-lang#120831 (Startup objects disappearing from sysroot) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120828 - nnethercote:fix-stash-steal, r=oli-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

The meaning of this assertion changed in rust-lang#120828 when the meaning of `has_errors` changed to exclude stashed errors. Evidently the new meaning is too restrictive. Fixes rust-lang#120856.

Loosen an assertion to account for stashed errors. The meaning of this assertion changed in rust-lang#120828 when the meaning of `has_errors` changed to exclude stashed errors. Evidently the new meaning is too restrictive. Fixes rust-lang#120856. r? `@oli-obk`

Loosen an assertion to account for stashed errors. The meaning of this assertion changed in rust-lang#120828 when the meaning of `has_errors` changed to exclude stashed errors. Evidently the new meaning is too restrictive. Fixes rust-lang#120856. r? ``@oli-obk``

Loosen an assertion to account for stashed errors. The meaning of this assertion changed in rust-lang#120828 when the meaning of `has_errors` changed to exclude stashed errors. Evidently the new meaning is too restrictive. Fixes rust-lang#120856. r? ```@oli-obk```

Rollup merge of rust-lang#120859 - nnethercote:fix-120856, r=oli-obk Loosen an assertion to account for stashed errors. The meaning of this assertion changed in rust-lang#120828 when the meaning of `has_errors` changed to exclude stashed errors. Evidently the new meaning is too restrictive. Fixes rust-lang#120856. r? ```@oli-obk```

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113671 (Make privacy visitor use types more (instead of HIR)) - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) - rust-lang#120831 (Startup objects disappearing from sysroot) r? `@ghost` `@rustbot` modify labels: rollup

Stashed errors used to be counted as errors, but could then be cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed that, closing the soundness hole. But it introduced other difficulties because you sometimes have to account for pending stashed errors when making decisions about whether errors have occured/will occur and it's easy to overlook these. This commit aims for a middle ground. - Stashed errors (not warnings) are counted immediately as emitted errors, avoiding the possibility of forgetting to consider them. - The ability to cancel (or downgrade) stashed errors is eliminated, by disallowing the use of `steal_diagnostic` with errors, and introducing the more restrictive methods `try_steal_{modify,replace}_and_emit_err` that can be used instead. Other things: - `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both return `Option<ErrorGuaranteed>`, which enables the removal of two `delayed_bug` calls and one `Ty::new_error_with_message` call. This is possible because we store error guarantees in `DiagCtxt::stashed_diagnostics`. - Storing the guarantees also saves us having to maintain a counter. - Calls to the `stashed_err_count` method are no longer necessary alongside calls to `has_errors`, which is a nice simplification, and eliminates two more `span_delayed_bug` calls and one FIXME comment. - Tests are added for three of the four fixed PRs mentioned below. - `issue-121108.rs`'s output improved slightly, omitting a non-useful error message. Fixes rust-lang#121451. Fixes rust-lang#121477. Fixes rust-lang#121504. Fixes rust-lang#121508.

…in, 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`

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`

Stashed errors used to be counted as errors, but could then be cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed that, closing the soundness hole. But it introduced other difficulties because you sometimes have to account for pending stashed errors when making decisions about whether errors have occured/will occur and it's easy to overlook these. This commit aims for a middle ground. - Stashed errors (not warnings) are counted immediately as emitted errors, avoiding the possibility of forgetting to consider them. - The ability to cancel (or downgrade) stashed errors is eliminated, by disallowing the use of `steal_diagnostic` with errors, and introducing the more restrictive methods `try_steal_{modify,replace}_and_emit_err` that can be used instead. Other things: - `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both return `Option<ErrorGuaranteed>`, which enables the removal of two `delayed_bug` calls and one `Ty::new_error_with_message` call. This is possible because we store error guarantees in `DiagCtxt::stashed_diagnostics`. - Storing the guarantees also saves us having to maintain a counter. - Calls to the `stashed_err_count` method are no longer necessary alongside calls to `has_errors`, which is a nice simplification, and eliminates two more `span_delayed_bug` calls and one FIXME comment. - Tests are added for three of the four fixed PRs mentioned below. - `issue-121108.rs`'s output improved slightly, omitting a non-useful error message. Fixes rust-lang#121451. Fixes rust-lang#121477. Fixes rust-lang#121504. Fixes rust-lang#121508.

…in, 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`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113671 (Make privacy visitor use types more (instead of HIR)) - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) - rust-lang#120831 (Startup objects disappearing from sysroot) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned petrochenkov Feb 9, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 9, 2024

rustbot assigned oli-obk and unassigned petrochenkov Feb 9, 2024

rust-lang deleted a comment from rustbot Feb 9, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2024

matthiaskrgr mentioned this pull request Feb 9, 2024

Rollup of 7 pull requests #120829

Closed

oli-obk reviewed Feb 9, 2024

View reviewed changes

matthiaskrgr mentioned this pull request Feb 9, 2024

Rollup of 9 pull requests #120837

Closed

matthiaskrgr mentioned this pull request Feb 9, 2024

Rollup of 8 pull requests #120843

Merged

bors merged commit 2f1ac41 into rust-lang:master Feb 9, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Feb 9, 2024

matthiaskrgr mentioned this pull request Feb 9, 2024

ICE: tcx.dcx().has_errors().is_some() #120856

Closed

nnethercote deleted the fix-stash-steal branch February 9, 2024 21:32

nnethercote mentioned this pull request Feb 9, 2024

ICE: error[E0782]: trait objects must include the dyn keyword #120482

Open

nnethercote mentioned this pull request Feb 9, 2024

Loosen an assertion to account for stashed errors. #120859

Merged

nnethercote mentioned this pull request Feb 26, 2024

Miri executing code with errors #121508

Closed

nnethercote mentioned this pull request Feb 27, 2024

Count stashed errors again #121669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ErrorGuaranteed` unsoundness with stash/steal. #120828

Fix `ErrorGuaranteed` unsoundness with stash/steal. #120828

nnethercote commented Feb 9, 2024 •

edited

Loading

rustbot commented Feb 9, 2024

nnethercote commented Feb 9, 2024

oli-obk commented Feb 9, 2024

bors commented Feb 9, 2024

nnethercote commented Feb 9, 2024

nnethercote commented Feb 9, 2024

oli-obk Feb 9, 2024

oli-obk Feb 9, 2024

nnethercote Feb 9, 2024

Fix ErrorGuaranteed unsoundness with stash/steal. #120828

Fix ErrorGuaranteed unsoundness with stash/steal. #120828

Conversation

nnethercote commented Feb 9, 2024 • edited Loading

rustbot commented Feb 9, 2024

nnethercote commented Feb 9, 2024

oli-obk commented Feb 9, 2024

bors commented Feb 9, 2024

nnethercote commented Feb 9, 2024

nnethercote commented Feb 9, 2024

oli-obk Feb 9, 2024

Choose a reason for hiding this comment

oli-obk Feb 9, 2024

Choose a reason for hiding this comment

nnethercote Feb 9, 2024

Choose a reason for hiding this comment

Fix `ErrorGuaranteed` unsoundness with stash/steal. #120828

Fix `ErrorGuaranteed` unsoundness with stash/steal. #120828

nnethercote commented Feb 9, 2024 •

edited

Loading