-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Make PanicInfo::message
infallible
#115561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -615,7 +615,7 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! { | |||||
} | ||||||
|
||||||
let loc = info.location().unwrap(); // The current implementation always returns Some | ||||||
let msg = info.message().unwrap(); // The current implementation always returns Some | ||||||
let msg = info.message(); | ||||||
crate::sys_common::backtrace::__rust_end_short_backtrace(move || { | ||||||
// FIXME: can we just pass `info` along rather than taking it apart here, only to have | ||||||
// `rust_panic_with_hook` construct a new `PanicInfo`? | ||||||
|
@@ -629,7 +629,7 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! { | |||||
); | ||||||
} else { | ||||||
rust_panic_with_hook( | ||||||
&mut PanicPayload::new(msg), | ||||||
&mut PanicPayload::new(&msg), | ||||||
info.message(), | ||||||
loc, | ||||||
info.can_unwind(), | ||||||
|
@@ -658,9 +658,11 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! { | |||||
|
||||||
let loc = Location::caller(); | ||||||
return crate::sys_common::backtrace::__rust_end_short_backtrace(move || { | ||||||
let message = | ||||||
*(&msg as &dyn Any).downcast_ref::<&'static str>().unwrap_or(&"<non-str payload>"); | ||||||
Comment on lines
+661
to
+662
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be good if this also attempts to downcast to rust/library/std/src/panicking.rs Line 262 in 8769c26
Then (For a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can't be done, as we'd be borrowing from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. It is possible, since the payload isn't actually consumed until after the PanicInfo is gone (at the end of rust_panic_with_hook), but it'd require refactoring some of the code in this file quite a bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is the It's all a bit roundabout, but if we're willing to break some use cases that assume there's a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The specific problematic impl is https://github.com/rust-lang/rust/blob/1b9567fcaa842b1028f9913838c4b70952470767/library/std/src/panicking.rs#L585 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure, but .get() is always called when constructing the PanicInfo that's passed to the panic hook, so at that point the String will have been allocated and can be borrowed by format_args!(). So, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, a lot of the annoyances here come from PanicInfo (the argument to #[panic_handler]) and PanicInfo (the argument to the panic hook) being the same type. I'm hoping we might be able to do something about that: #115974 That'd make this all a lot easier. :)
Comment on lines
+661
to
+662
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The default panic hook prints rust/library/std/src/panicking.rs Line 264 in 8769c26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, even better: the default panic hook should not do any downcasting and instead use .message(), now that it works in all cases. |
||||||
rust_panic_with_hook( | ||||||
&mut PanicPayload::new(msg), | ||||||
None, | ||||||
core::fmt::Arguments::new_v1(&[message], &[]), | ||||||
loc, | ||||||
/* can_unwind */ true, | ||||||
/* force_no_backtrace */ false, | ||||||
|
@@ -707,7 +709,7 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! { | |||||
/// abort or unwind. | ||||||
fn rust_panic_with_hook( | ||||||
payload: &mut dyn BoxMeUp, | ||||||
message: Option<&fmt::Arguments<'_>>, | ||||||
message: fmt::Arguments<'_>, | ||||||
location: &Location<'_>, | ||||||
can_unwind: bool, | ||||||
force_no_backtrace: bool, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// revisions: e2015 e2018 e2021 | ||
//[e2018] compile-flags: --edition=2018 | ||
//[e2021] compile-flags: --edition=2021 | ||
// run-pass | ||
|
||
#![feature(panic_info_message)] | ||
|
||
fn main() { | ||
std::panic::set_hook(Box::new(|info| assert_eq!(info.message().as_str().unwrap(), "cake"))); | ||
let _ = std::panic::catch_unwind(|| panic!("cake")); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.