rust-lang / project-error-handling

Error handling project group
Apache License 2.0
268 stars 18 forks source link

Make &dyn Error and Box<dyn Error> implement Error #16

Open KodrAus opened 3 years ago

KodrAus commented 3 years ago

This is a tracking issue to pursue the following trait impls:

impl<E: ?Sized + Error> Error for &'_ E;
impl<E: ?Sized + Error> Error for Box<E>;

Each of these impls comes with caveats that make them non-trivial to just introduce. We should try explore these caveats to see what it would take to make them possible.

KodrAus commented 3 years ago

The first impl can be added, but does cause a small amount of breakage: https://github.com/rust-lang/rust/pull/75180

KodrAus commented 3 years ago

Here's what I've been thinking so far. I thought we could discuss it at our next meeting:

We somewhat hitched our wagon to dyn Error over impl Error when we stabilized the Error trait. It's not really useful to use generically, because the two standard methods for passing dynamic values, &dyn Error and Box<dyn Error> don't implement the Error trait, so can't be passed to methods accepting impl Error. We can make &dyn Error satisfy impl Error, but not Box<dyn Error>. For completeness, I think we should still pursue impl<'a, E: Error + ?Sized + 'a> Error for E independently of impl<'a, E: Error + ?Sized + 'a> Error for Box<E>. It breaks some cases where you want to call methods on a &&dyn Error, but those are currently very niche (only an example from eyre broke in a crater run).

Even without Box<dyn Error> implementing the Error trait, you can still manually deref it to get a value that satisfies impl Error through &dyn Error.

So now the question is should we recommend impl Error or dyn Error? Ideally it would be up to consumers to pick between impl and dyn based on their own cases, but I think errors are a bit special, since they imply you're off the happy path (it also doesn't help that for reasons we looked at above impl Error isn't very useful). I think that makes us lean more towards recommending dyn Error over impl Error when you want to abstract over error types.

What bound should we recommend users accept when they want to accept dynamic errors? Is it dyn Error or dyn Error + 'static?

I think the recommendation should be:

If you do really want impl Error then don't add a 'static bound unless you're taking ownership of the error. That's because impl Error + 'static prevents anybody with a Box<dyn Error + 'static> from being able to call your method. All they could get is a &(dyn Error + 'static), which will be impl Error, but not impl Error + 'static. If you do want to take ownership, then prefer impl Error + Send + Sync + 'static.

KodrAus commented 3 years ago

For a recommendation on how to accept errors abstractly, we discussed this at the recent project group meeting and settled on: