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

RFC: error interoperation #201

Merged
merged 3 commits into from
Oct 3, 2014
Merged

Conversation

aturon
Copy link
Member

@aturon aturon commented Aug 15, 2014

This RFC improves interoperation between APIs with different error
types. It proposes to:

  • Increase the flexibility of the try! macro for clients of multiple
    libraries with disparate error types.
  • Standardize on basic functionality that any error type should have
    by introducing an Error trait.
  • Support easy error chaining when crossing abstraction boundaries.

The proposed changes are all library changes; no language changes are
needed -- except that this proposal depends on
multidispatch happening.

Rendered

@reem
Copy link

reem commented Aug 15, 2014

After some discussion on IRC @aturon suggested I link to my efforts on this here. Link: https://github.com/reem/rust-error

It is at least partially inspired by some ideas from this RFC, but provides an interface that allows third-party errors the same interface as first-party errors, which a plain enum can't do. I came up with this abstraction while trying to create an abstract error type for Iron, which has rather demanding constraints.

@lilyball
Copy link
Contributor

I didn't have time to do more than a brief skimming of this, and while I'm generally in favor of the motivation of this RFC, I'm worried that this makes try!() unusable for the potentially large category of Result<T,E> instances where E does not conform to Error. Right now, I can use try!() just fine for, say, Result<String,int>, and I'd rather not lose that.

@alexcrichton
Copy link
Member

As a data point, cargo has been using a strategy similar to this for quite some time now, and it's been working out marvelously.

@mitsuhiko
Copy link
Contributor

I'm a huge fan of something like this proposal. Composability of APIs is very important and currently the different error types make for a very hard API. I do not mind losing the ability to have non error things in Err() to be honest.

@aturon
Copy link
Member Author

aturon commented Aug 18, 2014

@kballard That's a good point.

I actually think the two part of this proposal -- the Error and FromError traits -- could be separated. There's not a strong reason that FromError has to inherit from Error, and not doing so will give try! maximal flexibility.

@reem
Copy link

reem commented Aug 18, 2014

My main issue with this RFC is that this error type does not provide a good way to handle errors. The Error trait makes reporting and propagating errors very nice, but when I receive a E: Error I have no way to actually recover from the error unless I match against the &'static str field, which is an error-prone process at best.

I think this is definitely a large step in the right direction, but much more thought needs to be put in to providing a good way to not only safely propagate and mix errors across library boundaries, but to also handle errors across library boundaries. This is a hard problem, but it's one Rust absolutely must solve if we don't want returning Err to just be sugar for "fail! but do it later".

@nielsle
Copy link

nielsle commented Aug 19, 2014

This looks nice. I like the idea of separating Error from FromError. Perhaps Error could just be an empty trait, that provided FromErrror.

pub trait Error{}
impl<E: Error> FromError<E> for E { ... }

#[deriving(Error)]
struct MyError { ... }

BTW: When running the code example in the RFC i seem to get an error http://is.gd/lHcbR2

<anon>:8:1: 12:2 error: conflicting implementations for trait `FromError` [E0119]
<anon>:8 impl<E: Error> FromError<E> for E {
<anon>:9     fn from_err(err: E) -> E {
<anon>:10         err
<anon>:11     }
<anon>:12 }
<anon>:14:1: 18:2 note: note conflicting implementation here
<anon>:14 impl<E: Error> FromError<E> for Box<Error> {
<anon>:15     fn from_err(err: E) -> Box<Error> {
<anon>:16         box err as Box<Error>
<anon>:17     }
<anon>:18 }

@reem
Copy link

reem commented Aug 19, 2014

@nielsle This proposal relies on multi-dispatch, which is added in a different RFC.

@lilyball
Copy link
Contributor

@aturon Splitting Error and FromError is a good idea, as there is no need for FromError to inherit from Error, but it will still require adding a FromError impl for every error type we want try!() to be able to return. And that's not doable if the error type is a type we did not create, e.g. uint, or ().

I feel like the only real solution to this issue is to have two macros, one that is the current try!(), and a separate one that uses FromError, e.g. tryerr!(). It could of course be one macro try!() with two separate invocation patterns, but that just seems more confusing.

@aturon
Copy link
Member Author

aturon commented Aug 22, 2014

@kballard Sorry, I think I was unclear. If we decouple the two mechanisms, we can write:

impl<E> FromError<E> for E {
    fn from_err(err: E) -> E {
        err
    }
}

which, with multidispatch, will still allow you to define other impls when actually converting between error types. This would allow the revised try macro to be used exactly as it is today.

@aturon
Copy link
Member Author

aturon commented Aug 27, 2014

Note: I've revised the RFC to decouple FromError and Error.

@reem
Copy link

reem commented Aug 27, 2014

@aturon Have you considered adding Any-like methods to Box<Error> and &'a Error for handling generic errors? A macro like this one: https://github.com/reem/rust-error/blob/master/src/lib.rs#L70-L87 makes using this much nicer, see: https://github.com/reem/rust-error/blob/master/src/lib.rs#L107-L112

This kind of functionality is important because there are situations, like in Iron, where you want to express "this should be a generic error handler" but you want that handler to actually be able to handle and introspect errors to a higher degree than just checking their name.

@aturon
Copy link
Member Author

aturon commented Aug 27, 2014

@reem Yes, in fact I just pushed another update to bound Error by Send+Any. The general feeling is that we'd like to land this basic infrastructure initially, which should allow for experiments in error matching like your macro. We can then move those into libstd once they've matured.

@reem
Copy link

reem commented Aug 27, 2014

Unfortunately, because of the way Any works right now, a bit more effort is needed (at minimum, AnyRefExt needs to be implemented for both Box<Error> and &'a Error in the same crate which defines Error), the alternative is that everyone has to use Box<Error + Any + Send> everywhere, and I'm not actually sure that you will be able to call downcast_ref on Box<Error + Any + Send>.

@aturon
Copy link
Member Author

aturon commented Aug 28, 2014

@reem You're right, my proposal relies on being able to upcast trait objects in general, which is something we intend to be possible but are still working on.

Until that lands, we'll probably need a workaround along the lines you're suggesting. I'll revise the RFC accordingly.

@reem
Copy link

reem commented Aug 28, 2014

@aturon An additional workaround I've found necessary is to make all the uses of Box<Error> actually be Box<Error + Send> because if not then it becomes very hard to store a Box<Error> in a struct that implements Error due to the Send bound.

Basically cause should get you a Box<Error + Send>.

@alexcrichton
Copy link
Member

I believe that it is planned such that trait Error: Send implies that all trait objects are Send (instead of having to write Box<Error + Send>, but I also don't believe that it's quite implemented just yet.

@reem
Copy link

reem commented Sep 16, 2014

@alexcrichton Is there an issue I can track for that?

@aturon
Copy link
Member Author

aturon commented Oct 6, 2014

Using Any is going to be a major backwards compatibility problem if we ever decide to make reflection require a type bound like Reflect. Type bounds on generics really don't mean anything when the code inside can just use type_id / Any.

To me, this seems like a more general argument to remove Any (or at least treat it as experimental), rather than related to the specifics of this RFC.

Is there a specific proposal about this Reflect bound on the table? This seems like something that needs to be resolved (in terms of at least having a clear plan) before 1.0.

@glaebhoerl
Copy link
Contributor

@aturon I think this might be related/referring to the fact that Any is currently implemented such that it is always implied by 'static, which is pretty bad (see related discussion) -- for instance it effectively makes Send + Any redundant because Send implies 'static, which implies Any. But I don't see why that would make an explicit Any here in particular a bad idea, nor why we would add a separate Reflect bound rather than fix Any, so it's possible that I'm misunderstanding @thestinger's point.

Re: previous comment, I had been under the impression that using Box<Error> at API boundaries was going to be the recommended practice, so I'm relieved to learn that that's not the case. I do agree that FromError is independently useful as a way to make the (a) approach work smoothly, and didn't mean to imply otherwise.

@ghost
Copy link

ghost commented Oct 6, 2014

I feel the stack of error messages shouldn't be part of the function signatures if we don't have enough type system to do it right. We could maintain a task-local error queue for example.

@aturon
Copy link
Member Author

aturon commented Oct 6, 2014

@glaebhoerl

it effectively makes Send + Any redundant because Send implies 'static, which implies Any

But not for vtable purposes! Which is, of course, the only time you want Any anyway :-)

@aturon
Copy link
Member Author

aturon commented Oct 6, 2014

@glaebhoerl Also, that discussion you linked is fascinating. You've basically shown that, as things stand, we can trivially violate parametricity for generics. I will definitely make sure the rest of the team is aware of this.

@reem
Copy link

reem commented Oct 7, 2014

@aturon I currently support Iron's error functionality in a library and wouldn't be strongly opposed to continuing to do so with a DynamicError: Error + Any or so, as long as there is a relatively easy and ergonomic way to convert from an Error to a DynamicError (right now I have to write tons and tons of boilerplate to wrap various error types).

This could be accomplished downstream with something like:

pub struct Lifted<E> where E: Error + Any {
   underlying: E
}

pub trait DynamicError: Error + Any {
    fn lift<E: Error + Any>(e: E) -> Box<DynamicError> {
            box Lifted { underlying: e }     
    } 
}

impl<E> DynamicError for Lifted<E> where E: Error + Any {}

impl Error for Box<DynamicError> { ... }

with only one major problem I see: without either Box<DynamicError + Any + Error> or the ability to call Error or Any methods through Box<DynamicError> this is effectively unusable as you can't downcast or call error methods through a dynamic error unless you force users to write many boilerplate methods. (all the methods on Error and Any, effectively.)

If the above was solved, then everything would be fine.

@aturon
Copy link
Member Author

aturon commented Oct 13, 2014

@reem I'm not sure I understand the worry at the end of your comment. If you impl Error on Box<DynamicError> as you're suggesting here, and also implement the Any functionality (as we'd have to do for Error anyway), it seems like you're good to go. Am I missing something?

The main downside to this over baking in Any is that if you're handed a Box<Error> value, you'd have no way to downcast. In other words, it doesn't make downcasting a built-in aspect of being an Error (an obvious downside). OTOH, as @glaebhoerl points out, advertising downcasting is likely to lead people astray in terms of the preferred way of modeling and handling errors.

@mitsuhiko
Copy link
Contributor

Just some food for thought: It would be awesome if this could be the start for making wrapping for results on return implicit. Imagine there was a ToResult trait that converts non errors into Ok(x) and errors into Err(x):

Old and boring:

fn foo(x: bool) -> Result<int, MyError> {
    if (x) {
        let _ : () = try!(do_something())
    }
    Ok(42)
}

New and fancy:

fn foo(x: bool) -> Result<int, MyError> {
    if (x) {
        try!(do_something())
    }
    42
}

@glaebhoerl
Copy link
Contributor

@mitsuhiko That is essentially part of #243 in this form:

fn foo(x: bool) -> int throws MyError {
    if (x) {
        do_something()?
    }
    42
}

(I believe this is necessary, as opposed to -> Result<int, MyError>, for the types to work out. But see the RFC and discussion for details.)

@reem
Copy link

reem commented Oct 15, 2014

@aturon Ya, with sufficient code duplication I think it's possible to create a general E: Error -> Box<DyanmicError> that has downcasting etc., it's a slight ergonomic problem, but since the code only needs to be written once, I think it's ok.

aturon added a commit to aturon/rust that referenced this pull request Apr 24, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

This commit furthermore marks `Reflect` as stable, since we are already
essentially committed to it via `Any`. Note that in the future, if we
determine that the parametricity aspects of `Reflect` are not needed, we
can deprecate the trait and provide a blanket implementation for it
for *all* types (rather than by using OIBIT), which would allow all
mentions of `Reflect` to be dropped over time. So there is not a strong
commitment here.

[breaking-change]
aturon added a commit to aturon/rust that referenced this pull request Apr 25, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

This commit furthermore marks `Reflect` as stable, since we are already
essentially committed to it via `Any`. Note that in the future, if we
determine that the parametricity aspects of `Reflect` are not needed, we
can deprecate the trait and provide a blanket implementation for it
for *all* types (rather than by using OIBIT), which would allow all
mentions of `Reflect` to be dropped over time. So there is not a strong
commitment here.

[breaking-change]
aturon added a commit to aturon/rust that referenced this pull request Apr 30, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to something like

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

except that `Reflect` is currently unstable (and should remain so for
the time being). For now, code can instead bound by `Any`:

```rust
impl<T: Any> Error for MyErrorType<T> { ... }
```

which *is* stable and has `Reflect` as a super trait. The downside is
that this imposes a `'static` constraint, but that only
constrains *when* `Error` is implemented -- it does not actually
constrain the types that can implement `Error`.

[breaking-change]
aturon added a commit to aturon/rust that referenced this pull request Apr 30, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to something like

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

except that `Reflect` is currently unstable (and should remain so for
the time being). For now, code can instead bound by `Any`:

```rust
impl<T: Any> Error for MyErrorType<T> { ... }
```

which *is* stable and has `Reflect` as a super trait. The downside is
that this imposes a `'static` constraint, but that only
constrains *when* `Error` is implemented -- it does not actually
constrain the types that can implement `Error`.

[breaking-change]
aturon added a commit to aturon/rust that referenced this pull request May 1, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to something like

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

except that `Reflect` is currently unstable (and should remain so for
the time being). For now, code can instead bound by `Any`:

```rust
impl<T: Any> Error for MyErrorType<T> { ... }
```

which *is* stable and has `Reflect` as a super trait. The downside is
that this imposes a `'static` constraint, but that only
constrains *when* `Error` is implemented -- it does not actually
constrain the types that can implement `Error`.

[breaking-change]
aturon added a commit to aturon/rust that referenced this pull request May 1, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to something like

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

except that `Reflect` is currently unstable (and should remain so for
the time being). For now, code can instead bound by `Any`:

```rust
impl<T: Any> Error for MyErrorType<T> { ... }
```

which *is* stable and has `Reflect` as a super trait. The downside is
that this imposes a `'static` constraint, but that only
constrains *when* `Error` is implemented -- it does not actually
constrain the types that can implement `Error`.

[breaking-change]
bors added a commit to rust-lang/rust that referenced this pull request May 1, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

This commit furthermore marks `Reflect` as stable, since we are already
essentially committed to it via `Any`. Note that in the future, if we
determine that the parametricity aspects of `Reflect` are not needed, we
can deprecate the trait and provide a blanket implementation for it
for *all* types (rather than by using OIBIT), which would allow all
mentions of `Reflect` to be dropped over time. So there is not a strong
commitment here.

[breaking-change]

r? @alexcrichton
alexcrichton pushed a commit to alexcrichton/rust that referenced this pull request May 5, 2015
This commit brings the `Error` trait in line with the [Error interoperation
RFC](rust-lang/rfcs#201) by adding downcasting,
which has long been intended. This change means that for any `Error`
trait objects that are `'static`, you can downcast to concrete error
types.

To make this work, it is necessary for `Error` to inherit from
`Reflect` (which is currently used to mark concrete types as "permitted
for reflection, aka downcasting"). This is a breaking change: it means
that impls like

```rust
impl<T> Error for MyErrorType<T> { ... }
```

must change to something like

```rust
impl<T: Reflect> Error for MyErrorType<T> { ... }
```

except that `Reflect` is currently unstable (and should remain so for
the time being). For now, code can instead bound by `Any`:

```rust
impl<T: Any> Error for MyErrorType<T> { ... }
```

which *is* stable and has `Reflect` as a super trait. The downside is
that this imposes a `'static` constraint, but that only
constrains *when* `Error` is implemented -- it does not actually
constrain the types that can implement `Error`.

[breaking-change]

Conflicts:
	src/libcore/marker.rs
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
fix some copy/paste errors in docs
@Centril Centril added A-traits-libstd Standard library trait related proposals & ideas A-error-handling Proposals relating to error handling. A-macros-libstd Proposals that introduce new standard library macros labels Nov 23, 2018
wycats pushed a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Proposals relating to error handling. A-macros-libstd Proposals that introduce new standard library macros A-traits-libstd Standard library trait related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants