Stream: project-error-handling

Topic: impl Error or dyn Error?


view this post on Zulip Ashley Mannix (Oct 13 2020 at 22:50):

Do we have any guidance on whether code that accepts generic errors (my case is a serialization framework) should accept:

Maybe this is covered a bit by the ##project-error-handling>dyn error handling topic. For a trait like Debug, my suggestion would normally be impl Debug + 'a, but that doesn't quite work for Error, because then you can't downcast, and &dyn Error isn't covered because of missing trait implementations. Then you have questions about Send + Sync.

Do we want to make impl Error more useful? Or do we want to recommend code that abstracts over error types prefer one of:

with dyn Error + 'a being the loosest, and dyn Error + Send + Sync + 'static being the tightest?

view this post on Zulip Joshua Nelson (Oct 13 2020 at 22:51):

I think we should recommend Send + Sync, the way the API guidelines do: https://rust-lang.github.io/api-guidelines/interoperability.html#error-types-are-meaningful-and-well-behaved-c-good-err

view this post on Zulip Joshua Nelson (Oct 13 2020 at 22:51):

no opinion yet on impl vs dyn

view this post on Zulip Ashley Mannix (Oct 13 2020 at 22:56):

impl won't really be useful unless we do something like https://github.com/rust-lang/rust/pull/75180

view this post on Zulip Ashley Mannix (Oct 13 2020 at 22:57):

Which seems like it can interact badly with existing inherent methods on dyn Error. I guess we've really geared the Error trait to be used as a trait object

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:20):

I think dyn is probably the better option of those two (not confident)

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:21):

and for the lifetime bounds, iunno, does serialization need downcasting?

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:32):

I wouldn't think so, but you _may_ need to forward through some other API that still expects 'static (I hit this with tracing)

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:33):

So maybe the ultra-conservative dyn Error + Send + Sync + 'static is really the only useful recommendation to make?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:34):

seems like a safe default

view this post on Zulip DPC (Oct 14 2020 at 00:40):

imo impl Error might make things more convenient than using dyn

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:41):

@DPC It _should_... But unfortunately it doesn't :smile:

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:41):

It excludes &dyn Error and Box<dyn Error>

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:41):

dreaming of specialization

view this post on Zulip DPC (Oct 14 2020 at 00:41):

fair

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:42):

Which could be point-in-time issues, but there isn't a clear path to making Box<dyn Error> implement the Error trait yet

view this post on Zulip DPC (Oct 14 2020 at 00:42):

i meant more in a "perfect world" where we are starting from scratch and we didn't have Box<dyn Error> anywhere

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:43):

Ashley Mannix said:

Which could be point-in-time issues, but there isn't a clear path to making Box<dyn Error> implement the Error trait yet

does specialization not give us this in theory?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:43):

either specialization or negative impls

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:43):

wait no

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:43):

In a perfect world I think it would look something like:

pub trait Error {}

impl<E: ?Sized + Error> Error for &E {}
impl<E: ?Sized + Error> Error for Box<E> {}

pub struct TheRealBoxError {}
impl Error for TheRealBoxError {}

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:43):

negative impls doesnt help this one

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:44):

yea

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:44):

want

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:44):

@Jane Lusby I always forget what specialization does and doesn't cover, but I thought because one of the conflicting impls isn't a subset of the other that it's not a specialization candidate :thinking:

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:45):

possibly

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:45):

It wasn't the impl Error for Box<dyn Error> that breaks was it. It's some unrelated From impls?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:45):

yea

view this post on Zulip DPC (Oct 14 2020 at 00:45):

a small "divergence" , is there a reason people use Box<dyn Error> for errors?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:45):

its From<T> for T and From<E: Error> For Box<dyn Error>

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:45):

Ahh that's right

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:46):

did we decide to not add support for explicit resolution of overlap like that?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:46):

if we added a third impl From<Box<dyn Error>> for Box<dyn Error> that would resolve it

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:46):

because that's pretty clearly a subset of both overlapping impls

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:47):

afaik

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:48):

Hmm, maybe we need some t-lang input on whether that's supposed to work :thinking:

view this post on Zulip DPC (Oct 14 2020 at 00:49):

if we added a third impl From<Box<dyn Error>> for Box<dyn Error> that would resolve it

but wouldn' that conflict with From<T> for <T>? (unless you are talking about +specialisation here)

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:50):

Do we have a tracking issue already for making &E: Error and Box<E: Error> implement Error?

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:50):

@DPC It would need a sprinkling of specialization magic

view this post on Zulip DPC (Oct 14 2020 at 00:50):

this is one issue: https://github.com/rust-lang/rust/pull/75180

view this post on Zulip DPC (Oct 14 2020 at 00:50):

err PR

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:51):

The thing I'm not sure of is whether it's supposed to be covered by specialization or not

view this post on Zulip DPC (Oct 14 2020 at 00:51):

doesn't seem to link to anything, so i assume we don't have any tracking issue related to it

view this post on Zulip Jake Goulding (Oct 14 2020 at 00:51):

It may be the "lattice" form of specialization

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:53):

https://github.com/rust-lang/rfcs/blob/master/text/1210-impl-specialization.md#the-lattice-rule

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:54):

in the alternatives section

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:54):

F

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:54):

this is saddening news

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:55):

Moving to the lattice rule is backwards compatible

A ray of hope

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:55):

Box<dyn Error> is an error 2030 edition lets gooo

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:55):

Hmm, definitely looks like that would be a long way off if it was going to be pursued. I might set up a tracking issue then and write some stuff up on where we're at and what we could do

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:56):

good idea

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:56):

I'll add that to the action items

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:57):

I can assign that to myself unless someone else feels confident / interested in filling out that issue

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:58):

I'm happy to do it :smile:

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:59):

k

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:59):

all yours :D

view this post on Zulip Ashley Mannix (Oct 14 2020 at 00:59):

Just opened up https://github.com/rust-lang/project-error-handling/issues/16 as a start

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:59):

the former impl in that issue isn't that hard right?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:59):

just that its breaking right?

view this post on Zulip Jane Lusby (Oct 14 2020 at 00:59):

just

view this post on Zulip Ashley Mannix (Oct 14 2020 at 01:00):

That's right. We could just accept it and add it. It just clobbers auto deref on dyn Error and breaks a few things.

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:01):

It just clobbers auto deref on dyn Error

wait what

view this post on Zulip Ashley Mannix (Oct 14 2020 at 01:02):

That's the breakage wasn't it? Where you have &'short &'long dyn Error so the lifetime you get in methods like source is 'short instead of 'long?

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:02):

ooo

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:02):

because auto deref doesn't care about lifetimes, and now &dyn Error would be an error

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:02):

thats wild

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:03):

I have very poor memory, so I didn't recall

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:04):

does the rust project make breaking changes when they think the impact is small enough?

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:04):

I've always assumed that the rule was a hard one

view this post on Zulip Ashley Mannix (Oct 14 2020 at 01:05):

We can decide on a case-by-case basic whether or not to accept breakage :smile: Pretty much _everything_ breaks somebody

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:05):

ooh dang

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:05):

very cool

view this post on Zulip Ashley Mannix (Oct 14 2020 at 01:06):

In this case, I would personally happily accept that breakage by fixing those crates because making &dyn Error implement Error seems really worthwhile

view this post on Zulip Ashley Mannix (Oct 14 2020 at 01:06):

I think there was about 4 of them

view this post on Zulip DPC (Oct 14 2020 at 01:07):

yeah normally there's a crater run to assess the breakage in the ecosystem, and a decision is made based on that

view this post on Zulip Jake Goulding (Oct 14 2020 at 01:33):

But, as is usually pointed out, there is more code out there than is visible to crater.

view this post on Zulip Jake Goulding (Oct 14 2020 at 01:33):

e.g., we are working on a 75kloc Rust project that is not open source (yet?)

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:34):

I remember hearing once of plans to make crater support private codebases

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:34):

where you could opt into running crater internally and post the results somehow

view this post on Zulip Jane Lusby (Oct 14 2020 at 01:34):

kinda off topic

view this post on Zulip Joshua Nelson (Oct 14 2020 at 01:35):

crater does not feel very useful to me until it becomes more accurate :( especially for rustdoc https://github.com/rust-lang/crater/issues/532

view this post on Zulip Ashley Mannix (Oct 14 2020 at 02:49):

I'll aim to have something concrete to report on by the time we sync next, which is in 2 weeks I think :smile:

view this post on Zulip DPC (Oct 14 2020 at 11:56):

rustdoc is an "exception", but for most of the other changes, crater is good enough


Last updated: Jan 26 2022 at 14:02 UTC