Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

Derive From? #10

Closed
m4b opened this issue Nov 3, 2017 · 14 comments
Closed

Derive From? #10

m4b opened this issue Nov 3, 2017 · 14 comments

Comments

@m4b
Copy link

m4b commented Nov 3, 2017

Sorry if this is addressed but I didn’t immediately see how From<SomeError> works for an error that wraps another error, e.g. io::Error

Are we currently expected to implement from conversion for error types wrapping other errors ourselves?

@withoutboats
Copy link
Contributor

withoutboats commented Nov 3, 2017

@aturon and I agree that the pattern that's emerged so far of having something like Io variant with an io::Error is a mistake. Instead, if you're going to have a variant, it should introduce some kind of additional context as to what that error means for you. There's a pattern to enable this which looks roughly like this:

#[derive(Fail)]
struct Error {
    inner: Context<ErrorKind>,
}

#[derive(Fail)]
enum ErrorKind {
    // the different kinds of "semantic" errors that could occur
}

This enables a many-to-many relationship between the ErrorKinds and the underlying errors, so an io Error might mean different things in different contexts. What this means though is that you don't get From and you have to insert that context instead of just using ?.

If you don't want to plumb that extra context in around your error types, we would recommend not having this kind of wrapper at all and just using the Error type instead.

If you disagree and really want something like an Io variant, you do have to provide the from impl yourself, but it should be fairly rote, e.g.

impl From<io::Error> for MyError {
     fn from(err: io::Error) -> MyError { MyError::Io(err) }
}

Since From is in the prelude and the types are already in scope, its only these 3 lines of code that you have to write.

@U007D
Copy link
Contributor

U007D commented Nov 4, 2017

@withoutboats can you explain this a bit more? I have been looking for exactly this guidance as I worked on my own error library (which I think I shall abandon now, in favor of Failure).

Specifically I'd like to better understand the many-to-many relationship that enum ErrorKind {...} enables. I think that might help me better understand what the indirection buys over basic enum Error {...} style variants.

Also, can you show an example of what you mean by 'you have to insert that context instead of just using ?'. Edit: I see now you just mean a call to some_func()?; becomes some_func().context("more context)?;.

@cramertj
Copy link

cramertj commented Nov 4, 2017

Just for reference, here are a few (fairly randomly selected) codebases that all use the Io(IoError) pattern: hyper, serde-json, sccache

It'd be interesting to try removing the From<io::Error> impls in each of these cases and see what the differences are in the ergonomics/clarity of the code.

@m4b
Copy link
Author

m4b commented Nov 7, 2017

So having read your response @withoutboats a couple times and being a little puzzled by it I now I realized from impls shouldn’t be required due to how crate is designed (and the example has IO errors present in the body of the function but not explicitly wrapped by the error type/enum).

This is a net win for me (and what I thought the crate was doing) as I dislike adding from lines and additional enum wrapping variants in either quicker error, error chain or custom errors.

I have to investigate the cause (I probably just messed something up) but I was experimentally porting an old error from error chain to failure and it gave “no from impls” errors and would not work until I explicitly added them in the enum; but iiuc, this is should not be required at all using this crate (unless as you said, you really want to).

So I think this issue can be closed since, iiuc, you should never have to wrap an error in order to use ? in the body of your error returning function as long the “foreign error” impls Error (or failure) or you want to for some reason, yes?

@withoutboats
Copy link
Contributor

@m4b The example in the README uses the Error type. The Error type can be cast from any type that implements Fail or std::error::Error.

However, if you want to not use Error, and instead cast your custom failure from another failure, you have to add the from impl yourself manually. However, in my opinion, there are many use cases (especially applications) where just using the Error type is the right choice.

Does that clarify things?

@m4b
Copy link
Author

m4b commented Nov 7, 2017

Yea it’s clear. Unfortunately this whole issue was result of a red herring (which i suspect other users will probably stumble into)

The error I was converting (like many errors) was named Error. When I ported to use this crate it, I did not change/forgot to change the error type name, and hence the failure error gets shadowed.

Consequently one will see a bunch of errors about From not impld for each of the error variants present in the body (which was the impetus of this issue). (And will perhaps, like me, miss the warning that the Error from failure is an unused import)

That’s why I was confused about your response because it sounded like you were saying no impl it yourself and I was like damn I dunno what value this crate adds if I have to do it all by hand again 😆

So yea this can be closed since what I was asking for is not necessary for the use I intended it for (error chain, custom hand rolled errors which wrap others in enum, etc, and auto derive from impl so it works with ?)

I hope it’s clear what i was confused about :P

Maybe add a little gotcha or warning to not forget to not shadow the error type if your error is named Error?

@est31
Copy link

est31 commented Nov 7, 2017

Putting my suggestion from IRC into a more permanent form: I think it would be great to have the ability to at least opt-in to From impl generation: #[from] Io(IoError) should autogenerate a From impl for the inner type IoError and the Io variant. This way it would be off by default but if in your library you really only have one cause for IoError (e.g. you are having a decoder library which usually operate on one file) then this would be a great help.

@U007D
Copy link
Contributor

U007D commented Nov 7, 2017

@m4b I too was still in a mindset of thinking I manually needed to declare my Froms, but now that I understand Failure's design a little better, I definitely like the ability to have foreign errors automatically coerced to failure::Error, or to have them coerced to my custom error type by using From. Having a #[from] derive as per @est31's suggestion would be icing on the cake, further reducing boilerplate for the latter scenario.

@mitsuhiko
Copy link
Contributor

@withoutboats i played with this quite a bit now and I think that failure::Error is great for the application side of things. However is the expectation that libraries will also return errors now? Because at least for libraries interior IO errors seem to be hard to get rid of.

@cramertj
Copy link

@mitsuhiko I think the idea is to separate io::Errors out based on the context in which they occurred (like this) rather than just having one blanket "this IO error occurred somewhere" variant like you get with From. I could have misunderstood, though, so feel free to correct me if I'm wrong.

@Emm
Copy link

Emm commented Feb 4, 2018

I disagree. This means using map_err left and right, which, in failure, is needlessly verbose for custom error enums as soon as you have a backtrace in your variant. I do think the library should offer to derive From on a per-variant basis.

@withoutboats
Copy link
Contributor

@Emm Not saying this feature won't ever be added, but the alternative is not map_err - its implementing From manually.

@Emm
Copy link

Emm commented Feb 4, 2018

@withoutboats, you are right, of course. I conflated two issues: writing From boilerplate implementation, and the verbosity of writing map_err for variants with backtraces, which is a different problem.

@mitsuhiko
Copy link
Contributor

Closing this. From turns out to not be a ideal approach with failure. While there are some limited uses where it's useful, for those its easy enough to add manually.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants