-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Tracking issue for PanicInfo::message #66745
Comments
Instead of making a PR go through the queue to fix the tracking issue, should we stabilize this method? @rfcbot fcp merge The doc-comment is slightly wrong or out of date. Its second line shoudl be removed. At first returning |
This comment was marked as outdated.
This comment was marked as outdated.
The |
@rfcbot concern single-arg core::panic Should we change libcore’s Pushing this idea further, it’s tempting to maybe remove |
I thought the point of the message field was that core can't make a payload since it can't allocate? |
The struct is: pub struct PanicInfo<'a> {
payload: &'a (dyn Any + Send),
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
} Both the It’s in the case of unwinding that there’s an owned |
@RalfJung Based on the timing of https://www.ralfj.de/blog/2019/11/25/how-to-panic-in-rust.html it looks like you may have thoughts on this :) |
Ah, https://www.ralfj.de/blog/2019/11/25/how-to-panic-in-rust.html goes into how and why the panic handler in libstd (which is called by We could probably change this but it gets tricky: while most implementations of |
I have zero recollection for why this is the way it is, and I'm not really particularly interested in doing all the archaeology to dig up why it is the way it is. I'm not a huge fan of just stabilizing things because they happen to be there, but I don't really see why we wouldn't want to stabilize this. If there were a reason to not stabilize this though an investigation would presumably turn that up, but I'm not up to do that investigation myself. |
That’s totally fair. I’m confident that this was more likely forgotten than deliberately left unstable. If deliberate we would have kept the tracking issue open or made a new one for the remaining bits. This is what lead me to propose stabilization. Having dug a little more, and thanks to Ralf’s blog post, I think we may want at some point to revisit what data we expect Given those possibly-desirable changes, it may not be the time to add more constraints: @rfcbot fcp cancel |
@SimonSapin proposal cancelled. |
Lol, I had no idea. ;) CleanupSo, indeed, I was wondering if we could do something about the duality of the Concretely, I was thinking of adjusting pub struct PanicInfo<'a> {
payload: Payload<'a>,
location: &'a Location<'a>,
}
enum Payload<'a> {
Any(&'a (dyn Any + Send)),
Message(&'a fmt::Arguments<'a>),
} That would at least remove the need for the I think this would retain all current functionality while clarifying the invariant of Extension: convenient convert-to-stringWith this change, the panic hook would always get a Lines 171 to 177 in 7d761fe
I was imagining something like an extension trait on Extension: payload-carrying panics from libcoreThis is the hard part, as we cannot use I don't see any way to do this entirely safely; the panic handler that wants to take out the payload and put it somewhere needs to dynamically determine the size of the payload and arrange for there to be enough space. In liballoc, we could provide a safe wrapper for this that puts things into a Also, we certainly do not want a panic hook to |
I agree, but it’s can easily be an inherent method of
I considered this yesterday and come to pretty much the same conclusions. To take ownership of a payload of unknown size through a non-generic API we pretty much heap allocation, which for libcore means something like receiving an It’s not pretty, but if only libcore and libstd deal with this internally maybe that’s ok? Is there a use case for a |
I was thinking of a slightly different API actually:
Here, |
…=Amanieu Split core's PanicInfo and std's PanicInfo `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. --- This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named `core::panic::PanicInfo`, and `std::panic::PanicInfo` is a new (deprecated) alias to `PanicHookInfo`. The crater run showed this as a viable option, since people write `core::` when defining a `#[panic_handler]` (because they're in `no_std`) and `std::` when writing a panic hook (since then they're definitely using `std`). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type. (Based on some thoughts I was having here: rust-lang#115561 (comment)) --- For the release notes: > We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. > > `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. > > The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.
…=Amanieu Split core's PanicInfo and std's PanicInfo `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. --- This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named `core::panic::PanicInfo`, and `std::panic::PanicInfo` is a new (deprecated) alias to `PanicHookInfo`. The crater run showed this as a viable option, since people write `core::` when defining a `#[panic_handler]` (because they're in `no_std`) and `std::` when writing a panic hook (since then they're definitely using `std`). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type. (Based on some thoughts I was having here: rust-lang#115561 (comment)) --- For the release notes: > We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. > > `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. > > The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.
…=Amanieu Split core's PanicInfo and std's PanicInfo `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. --- This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named `core::panic::PanicInfo`, and `std::panic::PanicInfo` is a new (deprecated) alias to `PanicHookInfo`. The crater run showed this as a viable option, since people write `core::` when defining a `#[panic_handler]` (because they're in `no_std`) and `std::` when writing a panic hook (since then they're definitely using `std`). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type. (Based on some thoughts I was having here: rust-lang#115561 (comment)) --- For the release notes: > We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. > > `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. > > The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.
Rollup merge of rust-lang#115974 - m-ou-se:panicinfo-and-panicinfo, r=Amanieu Split core's PanicInfo and std's PanicInfo `PanicInfo` is used in two ways: 1. As argument to the `#[panic_handler]` in `no_std` context. 2. As argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in `std` context. In situation 1, the `PanicInfo` always has a *message* (of type `fmt::Arguments`), but never a *payload* (of type `&dyn Any`). In situation 2, the `PanicInfo` always has a *payload* (which is often a `String`), but not always a *message*. Having these as the same type is annoying. It means we can't add `.message()` to the first one without also finding a way to properly support it on the second one. (Which is what rust-lang#66745 is blocked on.) It also means that, because the implementation is in `core`, the implementation cannot make use of the `String` type (which doesn't exist in `core`): https://github.com/rust-lang/rust/blob/0692db1a9082380e027f354912229dfd6af37e78/library/core/src/panic/panic_info.rs#L171-L172 This also means that we cannot easily add a useful method like `PanicInfo::payload_as_str() -> Option<&str>` that works for both `&'static str` and `String` payloads. I don't see any good reasons for these to be the same type, other than historical reasons. --- This PR is makes 1 and 2 separate types. To try to avoid breaking existing code and reduce churn, the first one is still named `core::panic::PanicInfo`, and `std::panic::PanicInfo` is a new (deprecated) alias to `PanicHookInfo`. The crater run showed this as a viable option, since people write `core::` when defining a `#[panic_handler]` (because they're in `no_std`) and `std::` when writing a panic hook (since then they're definitely using `std`). On top of that, many definitions of a panic hook don't specify a type at all: they are written as a closure with an inferred argument type. (Based on some thoughts I was having here: rust-lang#115561 (comment)) --- For the release notes: > We have renamed `std::panic::PanicInfo` to `std::panic::PanicHookInfo`. The old name will continue to work as an alias, but will result in a deprecation warning starting in Rust 1.82.0. > > `core::panic::PanicInfo` will remain unchanged, however, as this is now a *different type*. > > The reason is that these types have different roles: `std::panic::PanicHookInfo` is the argument to the [panic hook](https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html) in std context (where panics can have an arbitrary payload), while `core::panic::PanicInfo` is the argument to the [`#[panic_handler]`](https://doc.rust-lang.org/nomicon/panic-handler.html) in no_std context (where panics always carry a formatted *message*). Separating these types allows us to add more useful methods to these types, such as `std::panic::PanicHookInfo::payload_as_str()` and `core::panic::PanicInfo::message()`.
@StackOverflowExcept1on Yes, soon, hopefully! I've updated the overview at the top of this tracking issue. The main question left before stabilization is the return type of the method. Should this return |
Can it return |
I would vote for an opaque |
Did that in this PR: #126330 |
Return opaque type from PanicInfo::message() This changes the return type of the (unstable) PanicInfo::message() method to an opaque type (that implements Display). This allows for a bit more flexibility in the future. See rust-lang#66745
Return opaque type from PanicInfo::message() This changes the return type of the (unstable) PanicInfo::message() method to an opaque type (that implements Display). This allows for a bit more flexibility in the future. See rust-lang/rust#66745
@rfcbot merge |
Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Rollup merge of rust-lang#126732 - StackOverflowExcept1on:master, r=m-ou-se Stabilize `PanicInfo::message()` and `PanicMessage` Resolves rust-lang#66745 This stabilizes the [`PanicInfo::message()`](https://doc.rust-lang.org/nightly/core/panic/struct.PanicInfo.html#method.message) and [`PanicMessage`](https://doc.rust-lang.org/nightly/core/panic/struct.PanicMessage.html). Demonstration of [custom panic handler](https://github.com/StackOverflowExcept1on/panicker): ```rust #![no_std] #![no_main] extern crate libc; #[no_mangle] extern "C" fn main() -> libc::c_int { panic!("I just panic every time"); } #[panic_handler] fn my_panic(panic_info: &core::panic::PanicInfo) -> ! { use arrayvec::ArrayString; use core::fmt::Write; let message = panic_info.message(); let location = panic_info.location().unwrap(); let mut debug_msg = ArrayString::<1024>::new(); let _ = write!(&mut debug_msg, "panicked with '{message}' at '{location}'"); if debug_msg.try_push_str("\0").is_ok() { unsafe { libc::puts(debug_msg.as_ptr() as *const _); } } unsafe { libc::exit(libc::EXIT_FAILURE) } } ``` ``` $ cargo +stage1 run --release panicked with 'I just panic every time' at 'src/main.rs:8:5' ``` - [x] FCP: rust-lang#66745 (comment) r? libs-api
#44489 was closed when #51366 stabilized the corresponding language feature, but we didn’t realize that it was also the tracking issue for this standard library feature:
History:
PanicInfo::message
infallible #115561PanicInfo::message()
andPanicMessage
#126732Unresolved questions:
fmt::Arguments
or an opaquePanicMessage
? The first is simpler, but the latter allows a bit more flexibility for future changes.The text was updated successfully, but these errors were encountered: