Skip to content
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

Establish a new error handling project group #2965

Merged
merged 7 commits into from
Sep 18, 2020
Merged

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jul 23, 2020

- Provide a derive macro for `Error` in std.
- Stabilize the `Backtrace` type but possibly not `fn backtrace` on the Error trait.
- Provide necessary API on `Backtrace` to support crates like `color-backtrace`.
- Move error to core .
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@burdges
Copy link

burdges commented Jul 23, 2020

As noted in https://internals.rust-lang.org/t/idea-fixed-sized-traits/12179/8 we could define a SmallBox<T> type in core that (a) works with trait objects SmallBox<dyn Error> and (b) performs no heap allocation if T has size and alignment less than usize. We can do this without compiler magic, although it exploits knowledge of dyn Trait pointer layout. If alloc exists, then SmallBox<T> would allocates like Box<T> for larger T, but if alloc is absent then it panics if T is too big. I suppose that's a discussion for the group.. :)

@Lokathor
Copy link
Contributor

You should put a "rendered" link in the first post

Here is a tenative starting point, subject to change:

- Use `Result` and `Error` types for recoverable errors.
- Use `panic` for unrecoverable errors.
Copy link

@matu3ba matu3ba Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to touch unwind? I am asking, because that is for no_std and async poorly documented
and let to some confusion here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly well informed on the implementation details of unwinding but I can see it being within the purview of this project group.

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Jul 27, 2020
@nikomatsakis
Copy link
Contributor

Tagging with T-lang and T-libs, though I have to review the plans to tell how much the lang applies. =)

text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
yaahc and others added 2 commits July 27, 2020 18:06
@nikomatsakis nikomatsakis removed the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 28, 2020
@nikomatsakis
Copy link
Contributor

OK, I've read in more detail, and I guess that the scope of this is pretty limited to library API development, so I've removed T-lang. =)

@@ -43,6 +43,13 @@ Here is a tenative starting point, subject to change:
- What is the consensus on handling `dyn Error`s? Should it be encouraged or discouraged? Should we look into making `Box<dyn Error...>` implement `Error`?


### Identify pain points in error handling today

- Backtrace capture is expensive, but without one it can be difficult to pinpoint the origin of errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Backtrace capture is expensive, but without one it can be difficult to pinpoint the origin of errors
- Backtrace capture is expensive, but without it can be difficult to pinpoint the origin of errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally disagree with the universal applicability of this point as well; this is a key point of SNAFU — producing a semantic backtrace via the error types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming I understand what you mean, I think that the semantic backtraces via SNAFU can definitely help here but I don't think they're a universal solution. It still fairly easy to misuse. For example, if a developer introduces a higher level snafu error to handle an io::Error, but then uses this type liberally throughout the code. Unless you carefully create one variant per source location the origin of errors can still be lost.

Here's an example of this kind of thing going wrong via thiserror. ZcashFoundation/zebra#758

text/0000-project-error-handling.md Outdated Show resolved Hide resolved

- Backtrace capture is expensive, but without one it can be difficult to pinpoint the origin of errors
- unwrap on errors without first converting to a reporting type will often discard relevant information
- errors printing from main have to assume a prefixed `Error: `, sub par control of output format when printing during termination.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- errors printing from main have to assume a prefixed `Error: `, sub par control of output format when printing during termination.
- errors printed from `main` have to assume a prefixed `Error: `, sub-par control of output format when printing during termination.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what "have to assume a prefixed Error: " means.

Copy link
Member Author

@yaahc yaahc Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to this default in std's impl of Termination for Result https://doc.rust-lang.org/stable/src/std/process.rs.html#1690-1696, right now there's no way to signal to Debug types that they're being formatted as part of Termination or not. @dtolnay has suggested adding an is_termination flag to Debug. I'm also interested in the possibility of a 3rd std::fmt trait called std::fmt::Report to handle this and reporting in panic hooks, leveraging specialization for backwards compatibility.

- Backtrace capture is expensive, but without one it can be difficult to pinpoint the origin of errors
- unwrap on errors without first converting to a reporting type will often discard relevant information
- errors printing from main have to assume a prefixed `Error: `, sub par control of output format when printing during termination.
- Error trait only exposes 3 forms of context, can only represent singly linked lists for chains of errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Error trait only exposes 3 forms of context, can only represent singly linked lists for chains of errors
- The `Error` trait only exposes three forms of context, can only represent singly-linked lists for chains of errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what the pain points are here.

  • Is three too many or not enough?
  • Why is a singly-linked list painful?

Copy link
Member Author

@yaahc yaahc Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 is not enough, I'd like to be able to return SpanTraces, std::panic::Location, and other such types via the error trait, this is a reference to the generic member access RFC.

Singly linked lists can be painful for parsers for example, where you encounter multiple errors at once, and they're all the source for a single higher level error, so your chain of errors looks a little more like a tree. This is also addressed by the generic member access proposal where you could access a dyn Iter<Item=&(dyn Error + 'static)>.

@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jul 29, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Jul 30, 2020

Since it comes up whenever we talk about Error did you want to touch on the Error in core issue (even listed under non-goals if you think it's not part of the group's scope)?

In general I don't think it would be fair to drop a works in core constraint on the group without another one that was explicitly considering a unified std since there's a much wider design topic with no clear path forward yet, but it looks like you've already been keeping core support in mind in other designs like in #2895

EDIT: My bad! You've already got a section for it: https://github.com/yaahc/rfcs/blob/ehpg/text/0000-project-error-handling.md#consolidate-ecosystem-by-merging-best-practice-crates-into-std

@KodrAus
Copy link
Contributor

KodrAus commented Jul 30, 2020

Thanks for taking the time to put this together @yaahc! This is the first project group that's been proposed for libs, and I think having a group focused on Error is worthwhile. Let's open up to the rest of the team and see what we think of its scope.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 30, 2020

Team member @KodrAus 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jul 30, 2020
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
text/0000-project-error-handling.md Outdated Show resolved Hide resolved
@BurntSushi
Copy link
Member

I'm on the library team, but not on the checkbox list because I can't keep up with FCPs on std, but would like to be involved in RFCs. So I guess consider my imaginary checkbox ticked.

ping @SimonSapin @sfackler @withoutboats For checkboxes. People are eager to get started here. :-)

@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

Based on the current state of the Libs FCP list, I'll go ahead and check SimonSapin's box.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Sep 4, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 4, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 4, 2020

Coming a bit after the party but I think it's worth stating my concern:

In my opinion no error should be unrecoverable for the user. Let me explain a bit further: there are mechanisms that allow you to do something in case of a panic, however it's quite limited. Let's say you use a crate and for some reason, you call a function from it which panics (not because of a parameter but because of a state). Instead of being able to get the full state of your program or just ignore this panic, you're stuck having to go around it. So what I had in mind was to add another type which would indicate that the library is in an unrecoverable state (and would return a Unrecoverable type or something along the line to indicate it), which would allow to differentiate with a "simple" error.

I think the only case where a panic could be appropriate is when a parameter is invalid because it is a bug from your code.

@yaahc
Copy link
Member Author

yaahc commented Sep 4, 2020

Coming a bit after the party but I think it's worth stating my concern:

In my opinion no error should be unrecoverable for the user. Let me explain a bit further: there are mechanisms that allow you to do something in case of a panic, however it's quite limited. Let's say you use a crate and for some reason, you call a function from it which panics (not because of a parameter but because of a state). Instead of being able to get the full state of your program or just ignore this panic, you're stuck having to go around it. So what I had in mind was to add another type which would indicate that the library is in an unrecoverable state (and would return a Unrecoverable type or something along the line to indicate it), which would allow to differentiate with a "simple" error.

I think the only case where a panic could be appropriate is when a parameter is invalid because it is a bug from your code.

This seems like something that's best discussed as part of the project group, rather than the project group proposal. Once we have a repo setup though I'm happy to put this on the agenda of things to discuss.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 10, 2020

We're just about at the end of the FCP here so will be able to get started soon 🎉

What will the next steps be once we're ready to merge the RFC? I was thinking:

  • Set up a tracking issue in https://github.com/rust-lang/libs-team for status updates
  • Set up a rust-lang/project-error-handling repository (rust-lang/project-safe-transmute is an example)
  • Set up a t-libs/project-error-handling stream on Zulip
  • Set up a GitHub team for project-error-handling that can be pinged in the rust-lang org
  • Write up an announcement post for the Inside Rust blog

Does that sound right? As for how to do these things:

  • I think we might need @Mark-Simulacrum's help with the Zulip stream and rust-lang repository.
  • I think the GitHub team would probably just be a matter of PRing rust-lang/team?
  • Would anybody be up for the announcement post? If not I'd be happy to write something up and run it past you all!

I'm super excited to get started on this!

@yaahc
Copy link
Member Author

yaahc commented Sep 10, 2020

I'm super excited to get started on this!

Same ^_^

@seanchen1991
Copy link
Member

Here’s a draft of an announcement post. Let me know what you all think! If I left out anyone’s name that
should be in the post, I apologize 😬

# Announcing the Error Handling Project Group

Today we are announcing the formation of a new project group under the libs team, 
focused on error handling!

Some of the goals this project group will be working on include:

1. Defining and codifying common error handling terminology.
2. Generating consensus on current error handling best practices.
3. Identifying pain points that exist in Rust’s error handling story.
4. Communicating current error handling best practices.
5. Consolidating the Rust error handling ecosystem. 

This new project group is being shepherded by Jane Lusby ([@yaahc](https://github.com/yaahc)), 
Andrew Gallant ([@BurntSushi](https://github.com/burntsushi)), and 
Sean Chen ([@seanchen1991](https://github.com/seanchen1991)).

Anyone interested in helping out with the above goals is invited to come say hi in the group’s 
Zulip stream `#t-libs/project-error-handling`. Feel free to also check out the group’s GitHub 
repository at 
[https://github.com/rust-lang/project-error-handling](https://github.com/rust-lang/project-error-handling).

@KodrAus
Copy link
Contributor

KodrAus commented Sep 11, 2020

That looks great to me @seanchen1991!

@yaahc
Copy link
Member Author

yaahc commented Sep 11, 2020

looks perfect @seanchen1991

@seanchen1991
Copy link
Member

Awesome! Not sure how things get posted to the Inside Rust blog. @KodrAus do you handle that part?

@BatmanAoD
Copy link
Member

@seanchen1991 You can submit a pull request here: https://github.com/rust-lang/blog.rust-lang.org/

I think your draft looks good enough to simply copy-and-paste into a new post under the "Inside Rust" folder.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Sep 14, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 14, 2020

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.

The RFC will be merged soon.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 18, 2020

Ah looks like I haven't got permission to merge this RFC manually, so for whoever comes along that is able to, the tracking issue is this one: rust-lang/libs-team#3 ❤️

@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2020

Huzzah! The @rust-lang/libs team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/libs-team#3

Amanieu added a commit to Amanieu/rfcs that referenced this pull request Sep 18, 2020
Mark-Simulacrum added a commit that referenced this pull request Sep 18, 2020
@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2020

I messed up the merge, but it's fixed in #2986!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.