Top level error handling by nnethercote · Pull Request #121206 · rust-lang/rust

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

                    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.

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.

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop This PR fixes rust-lang#125323 ## Context According to the issue, the ICE happens since rust-lang#121206. In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in rust-lang#121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE. ## Change - Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation. - Add test for the ICE - Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests. ## NOTE The 4th commit aims to revert the fix in rust-lang#123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as rust-lang#125323 . But I can discard this commit since we can fix rust-lang#125323 without it.

Rollup merge of #138679 - Shunpoco:issue-125323, r=oli-obk Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop This PR fixes #125323 ## Context According to the issue, the ICE happens since #121206. In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in #121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE. ## Change - Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation. - Add test for the ICE - Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests. ## NOTE The 4th commit aims to revert the fix in #123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as #125323 . But I can discard this commit since we can fix #125323 without it.

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

Closed

Shunpoco mentioned this pull request Mar 18, 2025

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop #138679

Merged

GrigorenkoPV mentioned this pull request Sep 10, 2025

Asynchronous closures make trait constraint inference recursive #146399

Open

Uh oh!

Conversation

nnethercote commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 16, 2024

Uh oh!

bors commented Feb 17, 2024

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klensy Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

nnethercote Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

klensy Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

nnethercote Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

nnethercote Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

klensy Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Feb 19, 2024

Uh oh!

bors commented Feb 19, 2024

Uh oh!

bors commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nnethercote commented Feb 16, 2024 •

edited

Loading