Error in panic #103169

DebugSteven · 2022-10-17T21:41:40Z

This PR integrates the trait Error so that you can propagate error source with panics.

rustbot · 2022-10-17T21:41:43Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-17T21:41:44Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

yaahc · 2022-10-17T22:21:07Z

src/test/ui/error-trait/error-in-panic.run.stderr

+thread 'main' panicked at 'here's my panic error message', $DIR/error-in-panic.rs:49:5
+Source: my source error message
+
+Caused by:
+      my source's source error message
+note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


The format here is intentionally not intended to be the final format, right now it's fairly inconsistent. I'm not really sure exactly how we'd want to format this. My default suggestion would be something like

thread 'main' panicked at 'here's my panic error message', $DIR/error-in-panic.rs:49:5 source: my source error message caused by: my source's source error message note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

m-ou-se · 2022-10-18T12:26:10Z

library/core/src/panic/panic_info.rs

@@ -62,6 +65,12 @@ impl<'a> PanicInfo<'a> {
        self.payload = info;
    }

+    #[unstable(feature = "error_in_panic", issue = "none")]


Can you open a tracking issue for this feature? Thanks!

Tracking issue is here: #103820

library/core/src/panic/panic_info.rs

library/core/src/panicking.rs

library/std/src/panicking.rs

m-ou-se · 2022-10-18T12:47:29Z

What's the plan for using this new core::panicking::panic_source() function? I assume this is to be used by a method on Result?

Since panic_source takes both a fmt::Arguments and an Error, would the error message be duplicated in the output? What should the fmt::Arguments represent in case of an Error?

src/test/ui/error-trait/error-in-panic.rs

yaahc · 2022-10-18T17:51:59Z

What's the plan for using this new core::panicking::panic_source() function? I assume this is to be used by a method on Result?

details are in the ACP, but yea pretty much, except there are Issues.

Since panic_source takes both a fmt::Arguments and an Error, would the error message be duplicated in the output? What should the fmt::Arguments represent in case of an Error?

The fmt::Arguments is the error message for the panic itself, so the message would be something like "called Result::unwrap() on an Err value" and the Error would be the E value that was unwrapped.

apiraino · 2022-12-22T11:05:01Z

I'll remove T-compiler from the review queue as this PR seems to be in T-libs territory (but please correct me if I'm wrong).

@rustbot label -T-compiler

DebugSteven · 2022-12-22T17:48:39Z

@apiraino Thank you, removing the compiler tag seems right for the time being. I believe it was there because panic_source would ideally be const before it is stabilized. If/when panic_source is made const I think that code is what would need to be reviewed by the compiler team.

bors · 2022-12-26T00:19:25Z

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

DebugSteven · 2023-04-17T17:18:57Z

I rebased & I believe this is ready again.
@rustbot review

bors · 2023-04-22T14:56:46Z

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

m-ou-se · 2023-04-24T11:47:06Z

(Bumping this to the top of my review to do list.)

m-ou-se · 2023-04-25T14:14:10Z

Since panic_source takes both a fmt::Arguments and an Error, would the error message be duplicated in the output? What should the fmt::Arguments represent in case of an Error?

The fmt::Arguments is the error message for the panic itself, so the message would be something like "called Result::unwrap() on an Err value" and the Error would be the E value that was unwrapped.

In that case I'd not call this source but just error, since it represents the error itself, not the source of the outermost error or something like that. And in the context of PanicInfo, it's not clear that source should be an Error, as there can be other sources of panics too.

I'm wondering though in how many cases the message will really contain something useful. I imagine most of the time one would just want to do basically panic_error(error); instead of having to add a mostly meaningless panic message like panic_error("an error occured", error);.

Separately from that, the PanicInfo type is starting to get a bit messy, where it can optionally have a payload (which is usually either &str or String), optionally have a message, and now optionally have an error too. Maybe we can simplify it. Perhaps the error should also be used as the payload(), such that the payload is either a &dyn Any or a (more specific) &dyn Error. And in that case it might make sense to not have a separate .message() either for error-panics, but just start with the error message itself.

But we can leave that for later. ^^

What's the plan for using this new core::panicking::panic_source() function? I assume this is to be used by a method on Result?

details are in the ACP, but yea pretty much, except there are Issues.

Alright, then I'm curious to see the follow-up PRs to turn this feature into something usable that can be experimented with.

m-ou-se · 2023-04-25T13:37:26Z

library/core/src/panic/panic_info.rs

@@ -62,6 +65,12 @@ impl<'a> PanicInfo<'a> {
        self.payload = info;
    }

+    #[unstable(feature = "error_in_panic", issue = "none")]


Suggested change

#[unstable(feature = "error_in_panic", issue = "none")]

#[unstable(feature = "error_in_panic", issue = "103820")]

m-ou-se · 2023-04-25T13:42:38Z

library/core/src/panicking.rs

+#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
+#[cfg_attr(feature = "panic_immediate_abort", inline)]
+#[track_caller]
+#[unstable(feature = "error_in_panic", issue = "none")]


This is not part of the public error_in_panic API that might be stabilized in the future, right? (But just used internally, like the other functions in this module.) In that case:

Suggested change

#[unstable(feature = "error_in_panic", issue = "none")]

#[unstable(feature = "core_panic", issue = "none")]

m-ou-se · 2023-04-25T14:14:43Z

library/core/src/panic/panic_info.rs

+    pub fn source(&self) -> Option<&(dyn Error + 'static)> {
+        self.source


Suggested change

pub fn source(&self) -> Option<&(dyn Error + 'static)> {

self.source

pub fn error(&self) -> Option<&(dyn Error + 'static)> {

self.error

(etc.)

See comment above.

m-ou-se · 2023-04-25T14:16:41Z

We should make sure this doesn't add unnecessary overhead, especially for no_std (e.g. embedded) programs.

This isn't the best benchmark for that, but let's check it anyway:

@bors try @rust-timer queue

bors · 2023-04-25T14:16:51Z

⌛ Trying commit 125171c with merge 8627cf0077fee2bcf01e47c82159e1af1695e583...

bors · 2023-04-25T15:58:27Z

☀️ Try build successful - checks-actions
Build commit: 8627cf0077fee2bcf01e47c82159e1af1695e583 (8627cf0077fee2bcf01e47c82159e1af1695e583)

rust-timer · 2023-04-25T18:07:49Z

Finished benchmarking commit (8627cf0077fee2bcf01e47c82159e1af1695e583): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.0%, 1.6%]	2
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	0.8%	[0.0%, 1.6%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

m-ou-se · 2023-04-28T08:26:59Z

Separately from that, the PanicInfo type is starting to get a bit messy, where it can optionally have a payload (which is usually either &str or String), optionally have a message, and now optionally have an error too. Maybe we can simplify it. Perhaps the error should also be used as the payload(), such that the payload is either a &dyn Any or a (more specific) &dyn Error. And in that case it might make sense to not have a separate .message() either for error-panics, but just start with the error message itself.

#110799 should make it possible to just return the error from .payload(), simplifying things a bit by not having a separate/independent payload and error. (I'm not saying that that's what we should do. But it's a possibility we should consider.)

Dylan-DPC · 2023-07-27T12:31:30Z

@DebugSteven any updates on this? thanks

DebugSteven · 2023-08-04T18:27:55Z

I don't think I'm able to address Mara's technical feedback in a way that satisfies all stakeholders. Right now, my primary focus is on family-related health matters, so I apologize for the delay. If anyone else is interested in picking up the work and completing it, they're more than welcome to do so.

Dylan-DPC · 2023-10-07T13:16:44Z

Closing this as the author has stated that they do not have time for it at the moment. Thanks for contributing

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 17, 2022

rust-highfive assigned m-ou-se Oct 17, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2022

DebugSteven force-pushed the error-in-panic branch from 364f9fe to e12d483 Compare October 17, 2022 21:43

This comment has been minimized.

Sign in to view

yaahc mentioned this pull request Oct 17, 2022

Integrate Error trait with panic interfaces rust-lang/libs-team#124

Open

yaahc reviewed Oct 17, 2022

View reviewed changes

m-ou-se requested changes Oct 18, 2022

View reviewed changes

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 Oct 18, 2022

m-ou-se reviewed Oct 18, 2022

View reviewed changes

src/test/ui/error-trait/error-in-panic.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

DebugSteven force-pushed the error-in-panic branch from 4a8060f to 3ee9353 Compare October 31, 2022 22:09

This comment has been minimized.

Sign in to view

DebugSteven mentioned this pull request Oct 31, 2022

Tracking Issue for integrating Error trait with panic interfaces #103820

Open

6 tasks

DebugSteven force-pushed the error-in-panic branch from 3ee9353 to 4fcc3ba Compare November 2, 2022 00:55

This comment has been minimized.

Sign in to view

DebugSteven force-pushed the error-in-panic branch from 4fcc3ba to 2d49e02 Compare November 7, 2022 22:03

yaahc requested a review from m-ou-se November 7, 2022 23:14

Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2022

rustbot removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 22, 2022

move tests

78cff68

DebugSteven force-pushed the error-in-panic branch from c07fa9c to 78cff68 Compare April 13, 2023 23:42

This comment has been minimized.

Sign in to view

DebugSteven added 2 commits April 14, 2023 13:30

fix errors introduced in rebase

03df365

fix test

125171c

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

m-ou-se reviewed Apr 25, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 25, 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 Apr 25, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 25, 2023

Dylan-DPC closed this Oct 7, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error in panic #103169

Error in panic #103169

DebugSteven commented Oct 17, 2022

rustbot commented Oct 17, 2022

rust-highfive commented Oct 17, 2022

This comment has been minimized.

yaahc Oct 17, 2022

m-ou-se Oct 18, 2022

DebugSteven Nov 7, 2022

m-ou-se commented Oct 18, 2022 •

edited

Loading

yaahc commented Oct 18, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

apiraino commented Dec 22, 2022 •

edited

Loading

DebugSteven commented Dec 22, 2022

bors commented Dec 26, 2022

This comment has been minimized.

DebugSteven commented Apr 17, 2023

bors commented Apr 22, 2023

m-ou-se commented Apr 24, 2023

m-ou-se commented Apr 25, 2023

m-ou-se Apr 25, 2023

m-ou-se Apr 25, 2023

m-ou-se Apr 25, 2023

m-ou-se commented Apr 25, 2023

This comment has been minimized.

bors commented Apr 25, 2023

bors commented Apr 25, 2023

This comment has been minimized.

rust-timer commented Apr 25, 2023

m-ou-se commented Apr 28, 2023

Dylan-DPC commented Jul 27, 2023

DebugSteven commented Aug 4, 2023

Dylan-DPC commented Oct 7, 2023

	#[unstable(feature = "error_in_panic", issue = "none")]
	#[unstable(feature = "error_in_panic", issue = "103820")]

	#[unstable(feature = "error_in_panic", issue = "none")]
	#[unstable(feature = "core_panic", issue = "none")]

		pub fn source(&self) -> Option<&(dyn Error + 'static)> {
		self.source

Error in panic #103169

Error in panic #103169

Conversation

DebugSteven commented Oct 17, 2022

rustbot commented Oct 17, 2022

rust-highfive commented Oct 17, 2022

This comment has been minimized.

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 Oct 18, 2022 • edited Loading

yaahc commented Oct 18, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

apiraino commented Dec 22, 2022 • edited Loading

DebugSteven commented Dec 22, 2022

bors commented Dec 26, 2022

This comment has been minimized.

DebugSteven commented Apr 17, 2023

bors commented Apr 22, 2023

m-ou-se commented Apr 24, 2023

m-ou-se commented Apr 25, 2023

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 Apr 25, 2023

This comment has been minimized.

bors commented Apr 25, 2023

bors commented Apr 25, 2023

This comment has been minimized.

rust-timer commented Apr 25, 2023

Overall result: ❌ regressions - no action needed

m-ou-se commented Apr 28, 2023

Dylan-DPC commented Jul 27, 2023

DebugSteven commented Aug 4, 2023

Dylan-DPC commented Oct 7, 2023

m-ou-se commented Oct 18, 2022 •

edited

Loading

yaahc commented Oct 18, 2022 •

edited

Loading

apiraino commented Dec 22, 2022 •

edited

Loading