Adjust bug!/span_bug! emission - #162873
nnethercote wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d3786b6 to
4239b50
Compare
|
cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4239b50 to
882d803
Compare
This comment has been minimized.
This comment has been minimized.
| broken MIR in Item(DefId) (after phase change to runtime-optimized) at bb0[1]: | ||
| place (*(_2.0: *mut i32)) has deref as a later projection (it is only permitted as the first projection) | ||
| thread 'rustc' ($TID) panicked at compiler/rustc_interface/src/callbacks.rs:LL:CC: | ||
| Box<dyn Any> |
There was a problem hiding this comment.
This looks like a regression, the useful error got replaced by Box<dyn Any>?
There was a problem hiding this comment.
Ah, this is a partial revert of #161873.
This commit reverts those changes. This brings back Box output in a few tests because the panic payload is ExplicitBug (which std doesn't know about) rather than String. That might be worth fixing again in the future, but if so, it should be done in a way that works with all aborts, not just those done via the macros.
I don't know, having nice errors in some cases seems better than never having nice errors.
There was a problem hiding this comment.
The thing to note is that isn't obvious from the (default, unexpanded) diff: the nice error always appears a few lines above in the actual ICE diagnostic, with a proper span. So the Box<dyn Any> replacement is just repetition.
There was a problem hiding this comment.
Yeah I know that. Though this can be easy to miss when looking at the output in a terminal or copying it into an issue tracker.
We have lived with that for years so it's not the end of the world. It is just unfortunate that we just managed to fix the problem and should now re-introduce it...
There was a problem hiding this comment.
I'll give some thought to a proper fix.
|
The problem with panicking inside the callback is that you can't use example: #[test]
fn x() {
let _x = Ident::new(sym::empty, DUMMY_SP);
}Current output: Output with this PR:
You could change pub fn span_bug(self, span: Span, msg: impl Into<Cow<'static, str>>) -> ! {
let msg = msg.into();
rustc_span::span_bug!(span, "{}", msg)
} |
|
Another problem with #161873: because the macros are no longer panicking with
Four of them were added to These messages are not supposed to be printed for controlled |
It covers a case that isn't currently covered: `-Ztrack-diagnostics` in combination with a `span_bug!` ICE. Notably, the "created at" line mentions `callbacks.rs`, which is the wrong location. This will be fixed in the next commit.
PR rust-lang#161873 moved these macros from `rustc_middle` to `rustc_span`. In doing so it made some undesirable changes. - The internal `emit_producing_nothing` became public. - It gave the macros different behaviour to function-based ICEs like `dcx.bug(..)` or `dcx.struct_bug(..).emit()`. - `ExplicitBug` is not used, so `report_ice` prints an extraneous, unintended "the compiler unexpectedly panicked" message. - It broke the interaction between `span_bug!` and `-Ztrack-diagnostics`. This commit adjusts things to avoid the undesirable changes. It required moving `ExplicitBug` to `rustc_span`. Setting `diag.emitted_at` fixed the `-Ztrack-diagnostics` problem, seen in the change from `callback.rs` to `validate.rs` in `track7.stderr`. `Box<dyn Any>` output now occurs again in a few tests because the panic payload is `ExplicitBug` (which std doesn't know about) rather than `String`. That might be worth addressing in the future but it would require a different/additional mechanism and is beyond the scope of this PR.
882d803 to
0e617d4
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
|
I've updated the code. It now fixes the source locations, including a new case that was degraded by #161873. The |
PR #161873 moved these macros from
rustc_middletorustc_span. In doing so it made some undesirable changes.The internal
emit_producing_nothingbecame public.The macros now have different behaviour to function-based ICEs like
dcx.bug(..)ordcx.struct_bug(..).emit().This commit reverts those changes. This brings back
Box<dyn Any>output in a few tests because the panic payload isExplicitBug(which std doesn't know about) rather thanString. That might be worth fixing again in the future, but if so, it should be done in a way that works with all aborts, not just those done via the macros.r? @mejrs