Cleanup errors handlers even more#118933
Conversation
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
added
A-query-system
|
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 |
This comment has been minimized.
This comment has been minimized.
nnethercote
force-pushed
the
cleanup-errors-even-more
branch
from
4d84a6e to
792a2e3
Compare
| ); | ||
| )); | ||
| handler.note("the compiler expectedly panicked. this is a feature."); | ||
| handler.note( |
There was a problem hiding this comment.
Why is this not making a DiagnosticBuilder<'_, Bug> and appending the notes onto that? 🤔
There was a problem hiding this comment.
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->!(notBug)
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
r=me |
compiler-errors
added
S-waiting-on-author
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.
nnethercote
force-pushed
the
cleanup-errors-even-more
branch
from
7e122e6 to
3425a85
Compare
This comment has been minimized.
This comment has been minimized.
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
force-pushed
the
cleanup-errors-even-more
branch
from
3425a85 to
9a78412
Compare
|
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
added
S-waiting-on-bors
…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`
5 participants
A sequel to #118587.
r? @compiler-errors