Fix `EmissionGuarantee` #119097

nnethercote · 2023-12-18T22:59:19Z

There are some problems with the DiagCtxt API related to EmissionGuarantee. This PR fixes them.

This commit replaces this pattern: ``` err.into_diagnostic(dcx) ``` with this pattern: ``` dcx.create_err(err) ``` in a lot of places. It's a little shorter, makes the error level explicit, avoids some `IntoDiagnostic` imports, and is a necessary prerequisite for the next commit which will add a `level` arg to `into_diagnostic`. This requires adding `track_caller` on `create_err` to avoid mucking up the output of `tests/ui/track-diagnostics/track4.rs`. It probably should have been there already.

Presumably these are a hangover from an earlier time when they were necessary.

First, it is parameterized by the name of the diagnostic and the DiagCtxt. These are given to `session_diagnostic_derive` and `lint_diagnostic_derive`. But the names are hard-wired as "diag" and "handler" (should be "dcx"), and there's no clear reason for the parameterization. So this commit removes the parameterization and hard-wires the names internally. Once that is done `DiagnosticDeriveBuilder` is reduced to a trivial wrapper around `DiagnosticDeriveKind`, and can be removed. Also, `DiagnosticDerive` and `LintDiagnosticDerive` don't need the `builder` field, because it has been reduced to a kind, and they know their own kind. This avoids the need for some `let`/`else`/`unreachable!` kind checks And `DiagnosticDeriveVariantBuilder` no longer needs a lifetime, because the `parent` field is changed to `kind`, which is now a trivial copy type.

And make all hand-written `IntoDiagnostic` impls generic, by using `DiagnosticBuilder::new(dcx, level, ...)` instead of e.g. `dcx.struct_err(...)`. This means the `create_*` functions are the source of the error level. This change will let us remove `struct_diagnostic`. Note: `#[rustc_lint_diagnostics]` is added to `DiagnosticBuilder::new`, it's necessary to pass diagnostics tests now that it's used in `into_diagnostic` functions.

`EmissionGuarantee` no longer determines the error level, the `create_*` functions do.

This lets different error levels share the same return type from `emit_*`. - A lot of inconsistencies in the `DiagCtxt` API are removed. - `Noted` is removed. - `FatalAbort` is introduced for fatal errors (abort via `raise`), replacing the `EmissionGuarantee` impl for `!`. - `Bug` is renamed `BugAbort` (to avoid clashing with `Level::Bug` and to mirror `FatalAbort`), and modified to work in the new way with bug errors (abort via panic). - Various diagnostic creators and emitters updated to the new, better signatures. Note that `DiagCtxt::bug` no longer needs to call `panic_any`, because `emit` handles that. Also shorten the obnoxiously long `diagnostic_builder_emit_producing_guarantee` name.

This makes `DiagCtxt::bug` look like the other similar functions.

rustbot · 2023-12-18T22:59:28Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in need_type_info.rs

cc @lcnr

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

nnethercote · 2023-12-18T23:00:29Z

Here's an explanation of what this PR is doing. Apologies for the length, but
it's necessary to get the full picture.

Let's look at the EmissionGuarantee impls. Four of them are very consistent:

Handler::struct_err -> DB<EG>
Handler::create_err -> DB<EG>
Handler::err        -> EG
Handler::emit_err   -> EG

Handler::struct_warn    -> DB<()>
Handler::create_warning -> DB<()>
Handler::warn           -> ()
Handler::emit_warning   -> ()

Handler::struct_fatal   -> DB<!>
Handler::create_fatal   -> DB<!>
Handler::fatal          -> !
Handler::emit_fatal     -> !

Handler::struct_almost_fatal -> DB<FatalError>
Handler::create_almost_fatal -> DB<FatalError>
Handler::almost_fatal        -> FatalError        // missing
Handler::emit_almost_fatal   -> FatalError

But two are different:

Handler::struct_note -> DB<()>
Handler::create_note -> DB<Noted> // different
Handler::note        -> ()
Handler::emit_note   -> Noted     // different; return value never used

Handler::struct_bug -> DB<!>      // missing
Handler::create_bug -> DB<Bug>    // differing; unused
Handler::bug        -> !          // has a different structure to other similar functions
Handler::emit_bug   -> Bug        // different; misleading return type, it actually panics!

The problem is that warnings and notes both want to return (), and fatal errors and bugs both want to return !. But only one error level can be associated with a return type via EmissionGuarantee. Warnings get (), fatal errors gets !, which means that bugs and notes are second-class citizens and get these marker types.

This isn't so bad for notes; you can just ignore Noted as if it is (). But it is worse for bugs: the return type of emit_bug is actively misleading. (It passes type checking because it panics, which has type !, which fits with any type, including Bug.)

(This is a genuine problem. While working on #118933 I accidentally broke ICE handling completely by making a change that caused bug level errors to behave like fatal errors!)

So it would be nice to get rid of Noted and Bug, and give create_{note,bug} and emit_{note,bug} the correct return types. The difficulty is that derived diagnostics call struct_diagnostic to create the diagnostic. struct_diagnostic calls G::make_diagnostic_builder, a method of EmissionGuarantee. This ties each G to a particular level, and prevents two different diagnostic levels from sharing an EmissionGuarantee type. And this will only get worse if we add create_help, create_once_note, etc., all of which want to return ().

So, can we break this one-EmissionGuarantee-per-level constraint? Can we eliminate struct_diagnostic and G::make_diagnostic_builder?

This leads to an interesting question: for diagnostics defined via IntoDiagnostic, what really dictates the error level and EmissionGuarantee?

Consider a trivial error diagnostic:

pub struct AbcError;

Here's how it would likely be implemented by hand:

impl<'a> IntoDiagnostic<'a, ErrorGuaranteed> for AbcErrorHand {
    fn into_diagnostic(self, handler: &'a Handler) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
        #[allow(rustc::untranslatable_diagnostic)]
        let diag = handler.struct_err("abc");
        diag
    }   
}

This must be an error.

IntoDiagnostic and DiagnosticBuilder are not generic, but use ErrorGuaranteed, which is the EmissionGuarantee that corresponds to errors.
Also, struct_err is used in the body.

So it can only be used via create_err/emit_err, e.g.:

sess.diagnostic().emit_err(AbcErrorHand);       // ok
sess.diagnostic().emit_warning(AbcErrorHand);   // compile error
sess.diagnostic().emit_note(AbcErrorHand);      // compile error

Here's how it is generated for derive(Diagnostic) (with a lot of cleaning up to make it similar to the hand-written case):

impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G> for AbcErrorDerive {
    fn into_diagnostic(self, handler: &'a Handler) -> DiagnosticBuilder<'a, G> {
        #[allow(rustc::untranslatable_diagnostic)]
        let diag = handler.struct_diagnostic("abc");
        diag
    }   
}

The only difference is that the error level is unspecified.

IntoDiagnostic and DiagnosticBuilder are generic over G.
struct_diagnostic is used in the body, which calls G::make_diagnostic_builder, which results in a level that depends on G.

So it can be used at any level, via any create_*/emit_*, e.g.:

sess.diagnostic().emit_err(AbcErrorHand);       // ok
sess.diagnostic().emit_warning(AbcErrorHand);   // ok
sess.diagnostic().emit_note(AbcErrorHand);      // ok

A final possibility:

impl<'a> IntoDiagnostic<'a, ErrorGuaranteed> for AbcErrorMixed {
    fn into_diagnostic(self, handler: &'a Handler) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
        #[allow(rustc::untranslatable_diagnostic)]
        let diag = handler.struct_diagnostic("abc");
        diag    
    }           
}

This can also only be used as an error, despite using struct_diagnostic. It's a bit confused, using a non-generic signature but a wannabe-generic body. Prior to #118933, some of the hand-written impls used this style, but I changed them all to use a non-generic body function such as struct_err, to avoid the confusion.

Back to the original question: for diagnostics defined via IntoDiagnostic, what dictates the error level and EmissionGuarantee?

For hand-written impls, it's currently the IntoDiagnostic impl, which means the error level and EmissionGuarantee is fixed, and only one create_*/emit_* function can be used.
For derived impls, it's currently the creation point (create_*/emit_*), because the diagnostics are generic.

This inconsistency is bad. We should make them all generic or non-generic. And the whole G::make_diagnostic_builder part is also hard to understand.

Generic
- They would all use G.
- struct_diagnostic/G::make_diagnostic_builder can be replaced with DiagnosticBuilder::new.
- This is the simpler change.
- This is closest to the current approach, because there are many more derived impls than hand-written impls.
- Each diagnostic could be used at multiple levels, though this might never be useful in practice; you could even argue it's bad.
Non-generic
- Derived ones would need an additional annotation to indicate the level. (Though they could default to error in the case of no annotation.)
- The create_*/emit_* functions could just be reduced to create/emit, which would be a simpler API, though would obscure the error level at use sites -- you'd have to consult the IntoDiagnostic definition to know what error level to use.

I prefer the generic approach. All diagnostics (both hand-written and derived) would look like this:

impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G> for AbcErrorDerive {
    fn into_diagnostic(self, handler: &'a Handler, level: Level) -> DiagnosticBuilder<'a, G> {
        #[allow(rustc::untranslatable_diagnostic)]
        let diag = DiagnosticBuilder::new(handler, level, abc");
        diag
    }   
}

Why does this matter? Well, consistency and simplicity. But also, we now have a
way to solve the original one-level-per-EmissionGuarantee problem.

If we make all diagnostics generic, then we can pass the error level to into_diagnostic from the create_*/emit_* function.
If we make all diagnostics non-generic, then every diagnostic inherently "knows" its level.

Either way, into_diagnostic has access to the level, and doesn't need to get it from G::make_diagnostic_builder, which can be removed, breaking the one-level-per-EmissionGuarantee constraint.

This fixes the notes case: we can remove Noted and use () as the return
type for notes.

Bugs are a little trickier, because we need to distinguish between a ! that aborts (fatal errors) and one that panics (bugs). We can give EmissionGuarantee an associated type called EmitResult. For most error levels EmitResult would just be Self. But we'd have two marker types FatalAbort and BugAbort, both of which would have EmitResult of !.

Here are the APIs that would be changed (showing before and after):

Handler::struct_fatal   -> DB<!>      ==> DB<FatalAbort>  // changed
Handler::create_fatal   -> DB<!>      ==> DB<FatalAbort>  // changed
Handler::fatal          -> !          ==> !
Handler::emit_fatal     -> !          ==> !

Handler::struct_note -> DB<()>        ==> DB<()> 
Handler::create_note -> DB<Noted>     ==> DB<()>          // changed
Handler::note        -> ()            ==> ()
Handler::emit_note   -> Noted         ==> ()              // changed

Handler::struct_bug -> DB<!>          ==> DB<BugAbort>    // changed
Handler::create_bug -> DB<Bug>        ==> DB<BugAbort> 
Handler::bug        -> !              ==> !
Handler::emit_bug   -> Bug            ==> !               // changed

This fixes the original problems, yay!

compiler/rustc_hir_typeck/src/lib.rs

GuillaumeGomez

Changes in cg_gcc look good to me, thanks!

Also big 👍 for the detailed explanations, it was really pleasant to read and made it easy to understand the changes!

The easter egg ICE on `break rust` is weird: it's the one ICE in the entire compiler that doesn't immediately abort, which makes it annoyingly inconsistent. This commit changes it to abort. As part of this, the extra notes are now appended onto the bug dignostic, rather than being printed as individual note diagnostics, which changes the output format a bit. These changes don't interferes with the joke, but they do help with my ongoing cleanups to error handling.

nnethercote · 2023-12-20T21:41:52Z

A TL;DR for the wall of text above:

Each diagnostic emitted by the compiler has a level (and a corresponding EmissionGuarantee). What determines the level? There are two possible answers:

The diagnostic itself, i.e. the into_diagnostic method in the IntoDiagnostic impl.
The emission point, i.e. the call to emit_err, create_warn, etc.

For derive(Diagnostic) diagnostics, they are generic and can be used at any level, so the answer is 2.

For diagnostics with hand-written IntoDiagnostic impls, the current answer is both 1 and 2, and the two places must match otherwise there will be a compile error.

This PR changes hand-written diagnostics to be generic, which changes the answer to 2 for all diagnostics.

This has the knock-on benefit of removing the need for DiagCtxt::struct_diagnostic and EmissionGuarantee::make_diagnostic_builder, which then means the signatures of various DiagCtxt functions can be improved. In particular, it lets us fix the current confusion between fatal errors and bugs, both of which have return type ! but abort in different ways.

compiler-errors · 2023-12-20T22:29:31Z

compiler/rustc_errors/src/lib.rs

@@ -1101,8 +1105,7 @@ impl DiagCtxt {
    }

    pub fn bug(&self, msg: impl Into<DiagnosticMessage>) -> ! {
-        DiagnosticBuilder::<diagnostic_builder::Bug>::new(self, Bug, msg).emit();
-        panic::panic_any(ExplicitBug);
+        DiagnosticBuilder::<BugAbort>::new(self, Bug, msg).emit()


compiler-errors · 2023-12-20T22:32:27Z

@bors r+

bors · 2023-12-20T22:32:29Z

📌 Commit 006446e has been approved by compiler-errors

It is now in the queue for this repository.

nnethercote · 2023-12-21T10:51:38Z

@bors rollup=never

bors · 2023-12-21T12:12:21Z

⌛ Testing commit 006446e with merge 99f4bfc...

…=compiler-errors Fix `EmissionGuarantee` There are some problems with the `DiagCtxt` API related to `EmissionGuarantee`. This PR fixes them. r? `@compiler-errors`

pietroalbini · 2023-12-21T12:32:34Z

@bors retry

Yield priority to the stable release.

rust-log-analyzer · 2023-12-21T12:36:05Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-12-21T14:31:08Z

⌛ Testing commit 006446e with merge e4f405f...

…=compiler-errors Fix `EmissionGuarantee` There are some problems with the `DiagCtxt` API related to `EmissionGuarantee`. This PR fixes them. r? `@compiler-errors`

pietroalbini · 2023-12-21T14:44:22Z

@bors retry

Yield priority to the release.

rust-log-analyzer · 2023-12-21T14:46:29Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-12-22T00:04:00Z

⌛ Testing commit 006446e with merge cee794e...

bors · 2023-12-22T01:59:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing cee794e to master...

rust-timer · 2023-12-22T03:38:28Z

Finished benchmarking commit (cee794e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[1.0%, 1.0%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.232s -> 672.574s (-0.25%)
Artifact size: 312.80 MiB -> 312.78 MiB (-0.01%)

PR rust-lang#119097 made the decision to make all `IntoDiagnostic` impls generic, because this allowed a bunch of nice cleanups. But four hand-written impls were unintentionally overlooked. This commit makes them generic.

nnethercote added 8 commits December 18, 2023 20:46

Remove unnecessary use items in derived IntoDiagnostic impls.

18251c4

Presumably these are a hangover from an earlier time when they were necessary.

Remove struct_diagnostic and G::make_diagnostic_builder.

3a5f28f

`EmissionGuarantee` no longer determines the error level, the `create_*` functions do.

Factor out common emit_producing_guarantee code.

9ed8733

Introduce DiagCtxt::struct_bug.

072c157

This makes `DiagCtxt::bug` look like the other similar functions.

rustbot assigned compiler-errors Dec 18, 2023

compiler-errors reviewed Dec 18, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/lib.rs Outdated Show resolved Hide resolved

GuillaumeGomez approved these changes Dec 18, 2023

View reviewed changes

nnethercote added 2 commits December 19, 2023 20:58

Remove unused DiagCtxt::span_bug_no_panic.

006446e

nnethercote force-pushed the fix-EmissionGuarantee branch from d93bad8 to 006446e Compare December 19, 2023 09:59

nnethercote mentioned this pull request Dec 20, 2023

Cleanup error handlers: round 4 #119171

Merged

compiler-errors reviewed Dec 20, 2023

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2023

bors merged commit cee794e into rust-lang:master Dec 22, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 22, 2023

nnethercote deleted the fix-EmissionGuarantee branch December 22, 2023 02:28

This was referenced Dec 22, 2023

Improve handling of expressions in patterns #118625

Merged

Delay literal unescaping #118699

Closed

nnethercote mentioned this pull request Feb 22, 2024

Fix panic when compiling Rocket. #121427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `EmissionGuarantee` #119097

Fix `EmissionGuarantee` #119097

nnethercote commented Dec 18, 2023

rustbot commented Dec 18, 2023

nnethercote commented Dec 18, 2023

GuillaumeGomez left a comment

nnethercote commented Dec 20, 2023

compiler-errors Dec 20, 2023

compiler-errors commented Dec 20, 2023

bors commented Dec 20, 2023

nnethercote commented Dec 21, 2023

bors commented Dec 21, 2023

pietroalbini commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

bors commented Dec 21, 2023

pietroalbini commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

Fix EmissionGuarantee #119097

Fix EmissionGuarantee #119097

Conversation

nnethercote commented Dec 18, 2023

rustbot commented Dec 18, 2023

nnethercote commented Dec 18, 2023

GuillaumeGomez left a comment

Choose a reason for hiding this comment

nnethercote commented Dec 20, 2023

compiler-errors Dec 20, 2023

Choose a reason for hiding this comment

compiler-errors commented Dec 20, 2023

bors commented Dec 20, 2023

nnethercote commented Dec 21, 2023

bors commented Dec 21, 2023

pietroalbini commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

bors commented Dec 21, 2023

pietroalbini commented Dec 21, 2023

rust-log-analyzer commented Dec 21, 2023

bors commented Dec 22, 2023

bors commented Dec 22, 2023

rust-timer commented Dec 22, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Fix `EmissionGuarantee` #119097

Fix `EmissionGuarantee` #119097