Make `PanicInfo::message` infallible #115561

oli-obk · 2023-09-05T09:30:07Z

addresses the remaining T-libs-api concern in #66745

I picked <non-str payload> as the fallback message

r? libs-api

library/std/src/panicking.rs

Zoxc · 2023-09-05T17:03:37Z

I'm very much against this change as it doesn't make sense that panic!() has an associated message.

The use of Arguments should probably be replaced with a new type core::panic::Message so it can use a more code-size optimized implementation.

library/core/src/panic/panic_info.rs

m-ou-se · 2023-09-19T13:37:21Z

library/std/src/panicking.rs

+        let message =
+            *(&msg as &dyn Any).downcast_ref::<&'static str>().unwrap_or(&"<non-str payload>");


It might be good if this also attempts to downcast to String, just like the default panic hook does:

rust/library/std/src/panicking.rs

Line 262 in 8769c26

None => match info.payload().downcast_ref::<String>() {

Then panic_any(some_string), and panic!(format!(..)) in Rust ≤2018, will also work.

(For a String you'll have to use format_args!("{}", s) though, rather than new_v1.

This can't be done, as we'd be borrowing from msg while also using it as the payload (and thus moving out of it).

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.

The problem is the payload.get() method call, not the move. BoxMeUp::get must be mutable in order for the #[panic_handler] to be able to lazily allocate a String by formatting the Message from the PanicInfo that gets passed to it.

It's all a bit roundabout, but if we're willing to break some use cases that assume there's a String in the payload for panic!("{}", 42), then that does seem fixable.

The specific problematic impl is https://github.com/rust-lang/rust/blob/1b9567fcaa842b1028f9913838c4b70952470767/library/std/src/panicking.rs#L585

The problem is the payload.get() method call, not the move. BoxMeUp::get must be mutable in order for the #[panic_handler] to be able to lazily allocate a String by formatting the Message from the PanicInfo that gets passed to it.

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, rust_panic_with_hook could just .get() the payload first and then try downcasting it to &'static str and String to create a format_args!("{s}") to make the PanicInfo with. (That also avoids formatting message twice when someone uses .message() in a panic hook. (Since at that point the message has already been formatted into a String.))

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. :)

m-ou-se · 2023-09-19T13:45:17Z

library/std/src/panicking.rs

+        let message =
+            *(&msg as &dyn Any).downcast_ref::<&'static str>().unwrap_or(&"<non-str payload>");


"<non-str payload>"

The default panic hook prints "Box<dyn Any>" in that case. I'm not particularly attached to that specific message, but we should probably keep those the same.

rust/library/std/src/panicking.rs

Line 264 in 8769c26

None => "Box<dyn Any>",

Or, even better: the default panic hook should not do any downcasting and instead use .message(), now that it works in all cases.

m-ou-se · 2023-09-19T13:59:04Z

Dumping some of my notes here from digging through the panic code:

rust_panic_with_hook is called from three places:

From std's #[panic_handler] when Arguments::as_str() is Some (e.g. from panic!("hi"))
From std's #[panic_handler] when Arguments::as_str() is None (e.g. from panic!("{x}"))
From std's begin_panic() (e.g. from panic_any(x), or panic_2015!(x).)

Case 1 results in a &'static str payload.

Case 2 results in a String payload.

Case 3 results in a Box<dyn Any> payload. (Which could still be a &'static str or String.)

A fmt::Arguments (from the panic!() invocation) is available in case 1 and 2 (containing just the str/String), but not 3.

Case 3 only exists in std; it cannot happen as a result from a panic from no_std code.

std's default panic hook tries downcasting the payload to &'static str and String, and prints "Box<dyn Any>" otherwise. This covers cases 1, 2, and some instances of 3.

This PR changes case 3, begin_panic() to create a new fmt::Arguments in the &'static str case.

My conclusion from that all: if this PR also makes begin_panic() handle the String case in case 3, then the default panic hook can just use .message() instead of downcasting. It will then be impossible to invoke the panic hook with a &'static str or String payload without an identical .message().

m-ou-se · 2023-09-19T15:04:07Z

Somewhat related:

I think core::panic::PanicInfo and std::panic::PanicInfo should have been different types. The core::panic::PanicInfo passed to the #[panic_handler] never has a payload (but always has a fmt::Arguments), while the std::panic::PanicInfo passed to the (std) panic hook always has a payload.

m-ou-se · 2023-09-19T15:16:02Z

Thinking more about that:

If core::panic::PanicInfo (the argument to the #[panic_handler]) and std::panic::PanicInfo (the argument to the std panic hook) were different types, would we have .message() on both, or only on the first? The second one always has a &str or String available whenever there is a message.

Perhaps the first type should just be roughtly { message: fmt::Arguments, Location }, while the second type should be roughly { payload: &dyn Any, Location }.

Then the first type will have an unconditional .message(), while the second type can have a .payload_as_str() method that gives a &str into either the &'static str or String payload when available.

m-ou-se · 2023-09-19T16:18:06Z

#yolo, let's see what happens when we try to run this through crater: #115974 :)

…try> Split core's PanicInfo and std's PanicInfo This experimental PR is based on some thoughts I was having here: rust-lang#115561 (comment) --- `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 an experiment to see what happens if we make 1 and 2 separate types. To try to avoid breaking existing code, they are still named `core::panic::PanicInfo` and `std::panic::PanicInfo`, in the hope that people write the former when used in `#[panic_handler]` (since then they'd be using `no_std`) and the latter when used in a panic hook (since then they're definitely using `std`). Let's see how much explodes. :)

m-ou-se · 2023-09-20T17:35:24Z

Marking this as temporariliy blocked on #115974.

If splitting core and std's PanicInfo turns out to be feasible, I think .message() -> fmt::Arguments should only exist on core's PanicInfo. (Perhaps std's PanicInfo can have a method that returns a &str or Option<&str> that we can name payload_as_str() or message() or something like that.)

If splitting turns out to be infeasible, I think .message() should instead return an opaque PanicMessage type that implements Display. Then it can be implemented as an enum that holds either a fmt::Arguments (when used on the PanicInfo from a #[panic_handler]), or a &dyn Any to be downcasted to &str and String (when used on the PanicInfo from a std panic hook).

It might be worth considering using the opaque PanicMessage return type idea in the first situation as well, to allow us to extend core's panic machinery in the future (e.g. if we want to add something like panic_error(&dyn Error) in core).

bors · 2023-09-21T03:43:16Z

☔ The latest upstream changes (presumably #116013) made this pull request unmergeable. Please resolve the merge conflicts.

m-ou-se · 2024-05-16T14:54:25Z

#115974 looks feasible. I addressed the last few review comments on that PR, so hopefully it can move forward soon. It makes core::PanicInfo::message() infallible and adds std::Panic[Hook]Info::payload_as_str().

…manieu 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()`.

m-ou-se · 2024-06-12T11:31:43Z

#115974 has been merged! We now have an infallible core::panic::PanicInfo::message!

Closing this.

oli-obk added 2 commits September 5, 2023 09:24

Make PanicInfo::message infallible

9ad005b

Remove an unnecessary indirection

df19b29

rustbot assigned dtolnay Sep 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 5, 2023

fmease reviewed Sep 5, 2023

View reviewed changes

library/std/src/panicking.rs Outdated Show resolved Hide resolved

Remove now-outdated comments

6042f52

m-ou-se reviewed Sep 19, 2023

View reviewed changes

m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 19, 2023

m-ou-se assigned m-ou-se and unassigned dtolnay Sep 19, 2023

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023

m-ou-se mentioned this pull request Sep 19, 2023

Split core's PanicInfo and std's PanicInfo #115974

Merged

m-ou-se mentioned this pull request Sep 20, 2023

Tracking issue for cleaning up core's and std's panic machinery #116005

Open

15 tasks

m-ou-se added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2023

m-ou-se closed this Jun 12, 2024

m-ou-se mentioned this pull request Jun 12, 2024

Tracking issue for PanicInfo::message #66745

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `PanicInfo::message` infallible #115561

Make `PanicInfo::message` infallible #115561

oli-obk commented Sep 5, 2023

Zoxc commented Sep 5, 2023

m-ou-se Sep 19, 2023

oli-obk Sep 19, 2023

m-ou-se Sep 20, 2023

oli-obk Sep 20, 2023

oli-obk Sep 20, 2023

m-ou-se Sep 20, 2023

m-ou-se Sep 20, 2023

m-ou-se Sep 19, 2023

m-ou-se Sep 19, 2023

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 19, 2023 •

edited

Loading

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 20, 2023

bors commented Sep 21, 2023

m-ou-se commented May 16, 2024

m-ou-se commented Jun 12, 2024 •

edited

Loading

		let message =
		*(&msg as &dyn Any).downcast_ref::<&'static str>().unwrap_or(&"<non-str payload>");

Make PanicInfo::message infallible #115561

Make PanicInfo::message infallible #115561

Conversation

oli-obk commented Sep 5, 2023

Zoxc commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 19, 2023 • edited Loading

m-ou-se commented Sep 19, 2023

m-ou-se commented Sep 20, 2023

bors commented Sep 21, 2023

m-ou-se commented May 16, 2024

m-ou-se commented Jun 12, 2024 • edited Loading

Make `PanicInfo::message` infallible #115561

Make `PanicInfo::message` infallible #115561

m-ou-se commented Sep 19, 2023 •

edited

Loading

m-ou-se commented Jun 12, 2024 •

edited

Loading