Top level error handling #121206

nnethercote · 2024-02-16T22:45:29Z

The interactions between the following things are surprisingly complicated:

emit_stashed_diagnostics,
flush_delayed,
normal return vs abort_if_errors/FatalError.raise() unwinding in the call to the closure in interface::run_compiler.

This PR disentangles it all.

r? @oli-obk

rustbot · 2024-02-16T22:45:33Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

bors · 2024-02-17T21:06:35Z

☔ The latest upstream changes (presumably #121240) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk

r=me with some smaller things fixed

compiler/rustc_errors/src/lib.rs

compiler/rustc_codegen_ssa/src/back/link.rs

compiler/rustc_interface/src/passes.rs

compiler/rustc_codegen_ssa/src/base.rs

klensy · 2024-02-19T12:09:56Z

compiler/rustc_errors/src/lib.rs

@@ -1289,8 +1299,9 @@ impl DiagCtxtInner {
                    continue;
                }
            }
-            self.emit_diagnostic(diag);


This eagerly calls emit_diagnostic, looks suspicious.

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

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.

I see. How would you write it instead?

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.

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.

oli-obk · 2024-02-19T22:05:13Z

@bors r+

bors · 2024-02-19T22:05:15Z

📌 Commit a094903 has been approved by oli-obk

It is now in the queue for this repository.

…ng, r=oli-obk Top level error handling The interactions between the following things are surprisingly complicated: - `emit_stashed_diagnostics`, - `flush_delayed`, - normal return vs `abort_if_errors`/`FatalError.raise()` unwinding in the call to the closure in `interface::run_compiler`. This PR disentangles it all. r? `@oli-obk`

Rollup of 8 pull requests Successful merges: - rust-lang#121167 (resolve: Scale back unloading of speculatively loaded crates) - rust-lang#121196 (Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions)) - rust-lang#121206 (Top level error handling) - rust-lang#121223 (intrinsics::simd: add missing functions) - rust-lang#121241 (Implement `NonZero` traits generically.) - rust-lang#121242 (Generate `getelementptr` instead of `inttoptr` for `ptr::invalid`) - rust-lang#121278 (Remove the "codegen" profile from bootstrap) - rust-lang#121286 (Rename `ConstPropLint` to `KnownPanicsLint`) r? `@ghost` `@rustbot` modify labels: rollup

…ng, r=oli-obk Top level error handling The interactions between the following things are surprisingly complicated: - `emit_stashed_diagnostics`, - `flush_delayed`, - normal return vs `abort_if_errors`/`FatalError.raise()` unwinding in the call to the closure in `interface::run_compiler`. This PR disentangles it all. r? ``@oli-obk``

bors · 2024-02-20T14:52:27Z

☔ The latest upstream changes (presumably #120576) made this pull request unmergeable. Please resolve the merge conflicts.

Rollup merge of rust-lang#121206 - nnethercote:top-level-error-handling, r=oli-obk Top level error handling The interactions between the following things are surprisingly complicated: - `emit_stashed_diagnostics`, - `flush_delayed`, - normal return vs `abort_if_errors`/`FatalError.raise()` unwinding in the call to the closure in `interface::run_compiler`. This PR disentangles it all. r? `@oli-obk`

This was caused by 72b172b in rust-lang#121206. That commit removed an early return from `analysis` when there are stashed errors. As a result, it's possible to reach privacy analysis when there are stashed errors, which means more code paths can be reached. One such code path was handled in that commit, where a `span_bug` was changed to a `span_delayed_bug`. This commit handles another such code path uncovered by fuzzing, in much the same way. Fixes rust-lang#121455.

Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450.

Allow for a missing `adt_def` in `NamePrivacyVisitor`. This was caused by 72b172b in rust-lang#121206. That commit removed an early return from `analysis` when there are stashed errors. As a result, it's possible to reach privacy analysis when there are stashed errors, which means more code paths can be reached. One such code path was handled in that commit, where a `span_bug` was changed to a `span_delayed_bug`. This commit handles another such code path uncovered by fuzzing, in much the same way. Fixes rust-lang#121455. r? `@oli-obk`

Explicitly call `emit_stashed_diagnostics`. Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450. r? `@oli-obk`

Rollup merge of rust-lang#121487 - nnethercote:fix-121450, r=oli-obk Explicitly call `emit_stashed_diagnostics`. Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450. r? `@oli-obk`

Rollup merge of rust-lang#121482 - nnethercote:fix-121455, r=oli-obk Allow for a missing `adt_def` in `NamePrivacyVisitor`. This was caused by 72b172b in rust-lang#121206. That commit removed an early return from `analysis` when there are stashed errors. As a result, it's possible to reach privacy analysis when there are stashed errors, which means more code paths can be reached. One such code path was handled in that commit, where a `span_bug` was changed to a `span_delayed_bug`. This commit handles another such code path uncovered by fuzzing, in much the same way. Fixes rust-lang#121455. r? `@oli-obk`

I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.

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

I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.

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

Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450.

I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.

nnethercote marked this pull request as draft February 16, 2024 22:45

nnethercote force-pushed the top-level-error-handling branch 2 times, most recently from 55fd9f0 to 8d50bc1 Compare February 19, 2024 03:54

rustbot assigned oli-obk Feb 19, 2024

nnethercote marked this pull request as ready for review February 19, 2024 08:51

oli-obk requested changes Feb 19, 2024

View reviewed changes

compiler/rustc_errors/src/lib.rs Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

compiler/rustc_interface/src/passes.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/base.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024

klensy reviewed Feb 19, 2024

View reviewed changes

nnethercote force-pushed the top-level-error-handling branch from 8d50bc1 to a094903 Compare February 19, 2024 21:56

oli-obk approved these changes Feb 19, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2024

Noratrieb mentioned this pull request Feb 20, 2024

Rollup of 8 pull requests #121332

Closed

This was referenced Feb 20, 2024

Rollup of 8 pull requests #121339

Closed

Rollup of 8 pull requests #121345

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2024

nnethercote deleted the top-level-error-handling branch February 22, 2024 02:54

This was referenced Feb 22, 2024

ICE None privacy #121455

Closed

ICE: clippy: entered unreachable code: expr must be an array or slice due to ExprKind::Array #121477

Closed

nnethercote mentioned this pull request Feb 23, 2024

Allow for a missing adt_def in NamePrivacyVisitor. #121482

Merged

nnethercote mentioned this pull request Feb 23, 2024

Explicitly call emit_stashed_diagnostics. #121487

Merged

matthiaskrgr mentioned this pull request Feb 23, 2024

ICE: still mutable #121504

Closed

matthiaskrgr mentioned this pull request Feb 23, 2024

ICE: unexpected type in foreign function: {type error}' #121451

Closed

nnethercote mentioned this pull request Feb 23, 2024

Miri executing code with errors #121508

Closed

matthiaskrgr mentioned this pull request Feb 24, 2024

ICE: None compiler/rustc_lint/src/array_into_iter.rs #121532

Closed

This was referenced Feb 27, 2024

Count stashed errors again #121669

Merged

ICE: assertion failed: self.stashed_diagnostics.is_empty() rust-lang/rust-clippy#12364

Closed

ICE: rustfmt: self.stashed_diagnostics.is_empty() #121450

Closed

matthiaskrgr mentioned this pull request Feb 28, 2024

ICE: sty: range end index 18446744073709551615 out of range for slice of length 0 #121769

Closed

matthiaskrgr mentioned this pull request Apr 3, 2024

ICE: named field access on non-ADT #123428

Closed

matthiaskrgr mentioned this pull request Jun 8, 2024

ICE: dead: non-ADT in struct pattern #125323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top level error handling #121206

Top level error handling #121206

nnethercote commented Feb 16, 2024 •

edited

Loading

rustbot commented Feb 16, 2024

bors commented Feb 17, 2024

oli-obk left a comment

klensy Feb 19, 2024

nnethercote Feb 19, 2024

klensy Feb 20, 2024

nnethercote Feb 20, 2024

nnethercote Feb 20, 2024

klensy Feb 28, 2024

oli-obk commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 20, 2024

Top level error handling #121206

Top level error handling #121206

Conversation

nnethercote commented Feb 16, 2024 • edited Loading

rustbot commented Feb 16, 2024

bors commented Feb 17, 2024

oli-obk left a comment

Choose a reason for hiding this comment

klensy Feb 19, 2024

Choose a reason for hiding this comment

nnethercote Feb 19, 2024

Choose a reason for hiding this comment

klensy Feb 20, 2024

Choose a reason for hiding this comment

nnethercote Feb 20, 2024

Choose a reason for hiding this comment

nnethercote Feb 20, 2024

Choose a reason for hiding this comment

klensy Feb 28, 2024

Choose a reason for hiding this comment

oli-obk commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 20, 2024

nnethercote commented Feb 16, 2024 •

edited

Loading