Cleanup errors handlers even more #118933

nnethercote · 2023-12-14T05:30:06Z

A sequel to #118587.

It has a single call site.

It's necessary for `derive(Diagnostic)`, but is best avoided elsewhere because there are clearer alternatives. This required adding `Handler::struct_almost_fatal`.

It's unclear why this is used here. All entries in the third column of `UNICODE_ARRAY` are covered by `ASCII_ARRAY`, so if the lookup fails it's a genuine compiler bug. It was added way back in rust-lang#29837, for no clear reason. This commit changes it to `span_bug`, which is more typical.

rustbot · 2023-12-14T05:30:15Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

compiler/rustc_errors/src/lib.rs

compiler-errors · 2023-12-14T14:05:35Z

compiler/rustc_hir_typeck/src/lib.rs

        "It looks like you're trying to break rust; would you like some ICE?",
-    );
+    ));
    handler.note("the compiler expectedly panicked. this is a feature.");
    handler.note(


Why is this not making a DiagnosticBuilder<'_, Bug> and appending the notes onto that? 🤔

Good question. A couple of reasons, AIUI.

First, the API around Bug is currently a bit of a mess:

Handler::struct_bug -> DB<!>

Handler::span_bug -> !

Handler::bug -> !

Handler::create_bug -> DB<Bug> (unused)

Handler::emit_bug -> Bug (unused)

Bug::diagnostic_builder_emit_producing_guarantee -> ! (not Bug)

I.e. it's a mix of return types, unlike all the other diagnostic levels. I want to straighten this out, but I haven't gotten to it yet. I think part of the problem is that the EmissionGuarantee implemented for ! is tied to Level::Fatal rather than Level::Bug, even though Level::Bug should also cause immediate aborts.

Except, this is the one ICE that doesn't actually abort. So if we used struct_bug + emit it would abort. I guess we could use create_bug + emit_bug, but that would require a new type. Maybe good for a follow-up. Or maybe it should just abort? That would make life easier in the long run.

Also, generating distinct notes gives output like this:

error: internal compiler error: It looks like you're trying to break rust; would you like some ICE? note: the compiler expectedly panicked. this is a feature. note: we would appreciate a joke overview: https://github.com/rust-lang/rust/issues/43162#issuecomment-320764675 note: rustc 1.76.0-dev running on x86_64-unknown-linux-gnu

Generating a single (non-fatal) abort with appended notes looks like this:

error: internal compiler error: It looks like you're trying to break rust; would you like some ICE? | = note: the compiler expectedly panicked. this is a feature. = note: we would appreciate a joke overview: https://github.com/rust-lang/rust/issues/43162#issuecomment-320764675 = note: rustc 1.76.0-dev running on x86_64-unknown-linux-gnu

It's possible that the latter output is better, but I didn't want to change it. (I have been wondering what is the use of standalone notes, as opposed to notes attached to errors/warnings/etc...)

As per our discussion on Zulip: I changed this to a normal aborting ICE, which meant I could just use struct_bug and emit. It did require changing the notes from standalone diagnostics to notes attached to the bug diagnostic, but that seems fine.

Turns out my introduction of struct_bug broke some things. So in the end I removed that, and also the removal of span_bug_no_panic, and also the changes to this ICE. They can be done in a follow-up once I figure out how to fix struct_bug properly.

compiler-errors · 2023-12-14T14:06:34Z

r=me

To `msg: impl Into<DiagnosticMessage>`, like all the other diagnostics. For consistency.

The `Handler` functions that directly emit diagnostics can be more easily implemented using `struct_foo(msg).emit()`. This mirrors `Handler::emit_err` which just does `create_err(err).emit()`. `Handler::bug` is not converted because of weirdness involving conflation bugs and fatal errors with `EmissionGuarantee`. I'll fix that later.

Compare `Handler::warn` and `Handler::span_warn`. Conceptually they are almost identical. But their implementations are weirdly different. `warn`: - calls `DiagnosticBuilder::<()>::new(self, Warning(None), msg)`, then `emit()` - which calls `G::diagnostic_builder_emit_producing_guarantee(self)` - which calls `handler.emit_diagnostic(&mut db.inner.diagnostic)` `span_warn`: - calls `self.emit_diag_at_span(Diagnostic::new(Warning(None), msg), span)` - which calls `self.emit_diagnostic(diag.set_span(sp))` I.e. they both end up at `emit_diagnostic`, but take very different routes to get there. This commit changes `span_*` and similar ones to not use `emit_diag_at_span`. Instead they just call `struct_span_*` + `emit`. Some nice side-effects of this: - `span_fatal` and `span_fatal_with_code` don't need `FatalError.raise()`, because `emit` does that. - `span_err` and `span_err_with_code` doesn't need `unwrap`. - `struct_span_note`'s `span` arg type is changed from `Span` to `impl Into<MultiSpan>` like all the other functions.

Currently, `emit_diagnostic` takes `&mut self`. This commit changes it so `emit_diagnostic` takes `self` and the new `emit_diagnostic_without_consuming` function takes `&mut self`. I find the distinction useful. The former case is much more common, and avoids a bunch of `mut` and `&mut` occurrences. We can also restrict the latter with `pub(crate)` which is nice.

nnethercote · 2023-12-14T23:17:09Z

This has some stuff removed from the version given r=me, as described above, so I think it should be fine to approve.

@bors r=compiler-errors

bors · 2023-12-14T23:17:12Z

📌 Commit 9a78412 has been approved by compiler-errors

It is now in the queue for this repository.

…kingjubilee Rollup of 4 pull requests Successful merges: - rust-lang#118908 (Add all known `target_feature` configs to check-cfg) - rust-lang#118933 (Cleanup errors handlers even more) - rust-lang#118943 (update `measureme` to 10.1.2 to deduplicate `parking_lot`) - rust-lang#118948 (Use the `Waker::noop` API in tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#118933 - nnethercote:cleanup-errors-even-more, r=compiler-errors Cleanup errors handlers even more A sequel to rust-lang#118587. r? `@compiler-errors`

…mpiler-errors Cleanup error handlers: round 4 More `rustc_errors` cleanups. A sequel to rust-lang#118933. r? `@compiler-errors`

Rollup merge of rust-lang#119171 - nnethercote:cleanup-errors-4, r=compiler-errors Cleanup error handlers: round 4 More `rustc_errors` cleanups. A sequel to rust-lang#118933. r? `@compiler-errors`

…mpiler-errors Cleanup error handlers: round 4 More `rustc_errors` cleanups. A sequel to rust-lang#118933. r? `@compiler-errors`

nnethercote added 4 commits December 14, 2023 15:53

Remove unused Handler::treat_err_as_bug.

43a0f55

Inline and remove HandlerInner::emit_diag_at_span.

dc05a30

It has a single call site.

Avoid struct_diagnostic where possible.

7bdb227

It's necessary for `derive(Diagnostic)`, but is best avoided elsewhere because there are clearer alternatives. This required adding `Handler::struct_almost_fatal`.

rustbot assigned compiler-errors Dec 14, 2023

This comment has been minimized.

Sign in to view

nnethercote force-pushed the cleanup-errors-even-more branch from 4d84a6e to 792a2e3 Compare December 14, 2023 05:52

compiler-errors approved these changes Dec 14, 2023

View reviewed changes

compiler-errors 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 Dec 14, 2023

nnethercote added 3 commits December 15, 2023 09:42

Change msg: impl Into<String> for bug diagnostics.

19d28a4

To `msg: impl Into<DiagnosticMessage>`, like all the other diagnostics. For consistency.

nnethercote force-pushed the cleanup-errors-even-more branch from 7e122e6 to 3425a85 Compare December 14, 2023 23:08

This comment has been minimized.

Sign in to view

nnethercote force-pushed the cleanup-errors-even-more branch from 3425a85 to 9a78412 Compare December 14, 2023 23:16

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 Dec 14, 2023

workingjubilee mentioned this pull request Dec 15, 2023

Rollup of 4 pull requests #118957

Merged

bors merged commit 9e872b7 into rust-lang:master Dec 15, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Dec 15, 2023

nnethercote deleted the cleanup-errors-even-more branch December 15, 2023 02:47

This was referenced Dec 18, 2023

Fix EmissionGuarantee #119097

Merged

Cleanup error handlers: round 4 #119171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup errors handlers even more #118933

Cleanup errors handlers even more #118933

nnethercote commented Dec 14, 2023

rustbot commented Dec 14, 2023

This comment has been minimized.

compiler-errors Dec 14, 2023

nnethercote Dec 14, 2023

nnethercote Dec 14, 2023

nnethercote Dec 14, 2023

compiler-errors commented Dec 14, 2023

This comment has been minimized.

nnethercote commented Dec 14, 2023

bors commented Dec 14, 2023

Cleanup errors handlers even more #118933

Cleanup errors handlers even more #118933

Conversation

nnethercote commented Dec 14, 2023

rustbot commented Dec 14, 2023

This comment has been minimized.

compiler-errors Dec 14, 2023

Choose a reason for hiding this comment

nnethercote Dec 14, 2023

Choose a reason for hiding this comment

nnethercote Dec 14, 2023

Choose a reason for hiding this comment

nnethercote Dec 14, 2023

Choose a reason for hiding this comment

compiler-errors commented Dec 14, 2023

This comment has been minimized.

nnethercote commented Dec 14, 2023

bors commented Dec 14, 2023