Handle panicking like rustc CTFE does #16935
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -309,43 +334,6 @@ impl Evaluator<'_> { | |||
| let mut args = args.iter(); | |||
| match it { | |||
| BeginPanic => Err(MirEvalError::Panic("<unknown-panic-payload>".to_owned())), | |||
| PanicFmt => { | |||
There was a problem hiding this comment.
Don't we need to have something like this? How const_panic_fmt can return MirEvalError::Panic on its own?
There was a problem hiding this comment.
There was a problem hiding this comment.
So we need to hook panic_display in r-a as well? What is the output of rust-analyzer(debug-command) interpret function on a function that panics? It should be MirEvalError::Panic("panic message") but I guess it is now execution limit exceeded or something like that as it will try to execute panic_fmt and panic_display over and over.
There was a problem hiding this comment.
Hooking that function is what my rustc_const_panic_str change does.
There was a problem hiding this comment.
Ah I see. So we now lose the panic message? but let's track it in #16938 and merge this one.
crates/hir-ty/src/mir/eval/tests.rs
Outdated
| // -> Err(ConstEvalError::Panic) | ||
| check_pass( | ||
| r#" | ||
| //- minicore: fmt, panic |
There was a problem hiding this comment.
somehow these tests don't work correctly yet, they currently fail MIR building (also, they shouldn't be check_pass but check_panic or whatever). I am currently unable to debug the build failure, so help welcome
There was a problem hiding this comment.
I dropped this test, will do it in a follow-up PR. I added a simpler test for the hover behavior, which should be enough end-to-end.
|
Someone else who had this problem in their project (making it unusable) confirmed that this also fixed their problem. I can drop the tests from this PR and do them in a follow up PR (I promise I won't forget about it :3) to merge this ASAP. |
Nilstrieb
force-pushed
the
dont-panic
branch
2 times, most recently
from
ada455d
to
d8ef3f3
Compare
Instead of using `core::fmt::format` to format panic messages, which may in turn panic too and cause recursive panics and other messy things, redirect `panic_fmt` to `const_panic_fmt` like CTFE, which in turn goes to `panic_display` and does the things normally. See the tests for the full call stack.
|
I created #16938 to figure out the tests, so this PR should now work (and have a small hover test to ensure that end-to-end behavior doesn't regress). So this should be ready for merging now. |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Implement `BeginPanic` handling in const eval for #16935, needs some figuring out of how to write these tests correctly
Implement `BeginPanic` handling in const eval for #16935, needs some figuring out of how to write these tests correctly
Implement `BeginPanic` handling in const eval for #16935, needs some figuring out of how to write these tests correctly
Instead of using
core::fmt::formatto format panic messages, which may in turn panic too and cause recursive panics and other messy things, redirectpanic_fmttoconst_panic_fmtlike CTFE, which in turn goes topanic_displayand does the things normally. See the tests for the full call stack.The tests don't work yet, I probably missed something in minicore.
fixes #16907 in my local testing, I also need to add a test for it