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

Error propagation through futures isn't very ergonomic #402

Closed
koute opened this Issue Mar 5, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@koute

koute commented Mar 5, 2017

Currently the error propagation is not very ergonomic, and when you make a mistake the compiler produces somewhat confusing error messages. For example if we have this:

fn initial_future() -> impl Future<Item = (), Error = Box<error::Error>> {
    // ...
}

fn intermediate_future(value: ()) -> impl Future<Item = (), Error = io::Error> {
    // ..
}

let future = initial_future().and_then(|value| {
    intermediate_future(value)
});

we'll get:

error[E0271]: type mismatch resolving `<impl futures::Future as futures::IntoFuture>::Error == std::boxed::Box<std::error::Error + 'static>`
   --> src/lib.rs:299:35
    |
299 |     let future = initial_future().and_then( |value| {
    |                                   ^^^^^^^^ expected struct `std::io::Error`, found struct `std::boxed::Box`
    |
    = note: expected type `std::io::Error`
               found type `std::boxed::Box<std::error::Error + 'static>`

This error seems totally backwards to me; you'd think it should tell you that it expected the Box<...> type and found the io::Error, but it does it the other way, and that it should point at intermediate_future but it doesn't. (Yes, I know the technical reason why it does so; I'm just pointing it out from purely an user's perspective.)

In this (quite simple) case it might be easy to figure it out, but if you have a significantly longer (and more nested) future chains it gets worse.

The way to fix this code would be to do this:

let future = initial_future().and_then(|value| {
    intermediate_future(value).map_err(|e| e.into())
});

but, again, this is not entirely obvious and is generally a pain.

Leaving the issue of error messages aside, it might be nice if this could be done automatically; if we look at one of the methods in the Future trait we can see that it requires the next future in the chain to have the same error type:

trait Future {
// ...
    fn and_then<F, B>(self, f: F) -> AndThen<Self, B, F> 
        where F: FnOnce(Self::Item) -> B,
              B: IntoFuture<Error=Self::Error>,
              Self: Sized
// ...
}

Ideally we'd maybe like something like this:

trait Future {
// ...
    fn and_then<F, B>(self, f: F) -> AndThen<Self, B, F> 
        where F: FnOnce(Self::Item) -> B,
              B: IntoFuture,
              B::Error: Into<Self::Error>,
              Self: Sized
// ...
}

This however has its own issues where if we do this:

future.and_then(|_| { Ok(()) });

the compiler won't be able to figure out the type inside Result::Err (even though it doesn't matter) so it'll emit an error:

error[E0283]: type annotations required: cannot resolve `_: std::convert::Into<()>

What would help here is having an impl like this:

impl IntoFuture for T {
    type Future = FutureResult<T, !>;
    type Item = T;
    type Error = !;

    #[inline]
    fn into_future(self) -> Self::Future {
        result(Ok(T))
    }
}

while simultaneously still being specialized for other cases for which IntoFuture is currently defined and while having impl<T> From<!> for T defined in core. This is, of course, not (yet?) possible in current Rust.

The other alternative (which should be feasible right now) would be to just change the definition of futures::future::ok from this:

pub fn ok<T, E>(t: T) -> FutureResult<T, E> {
    result(Ok(t))
}

into this:

pub fn ok<T, E>(t: T) -> FutureResult<T, impl Into<E>> {
    let out: Result<T, E> = Ok(t);
    result(out)
}

and then require the use of futures::future::ok instead of Result::Ok when someone wants to infallibly return a value.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 6, 2017

Member

Yeah I definitely agree that this is one of the pain points for futures today, and I think you're spot on in that there's not an easy fix today as the "easy fixes" have their own pitfalls. That being said though we can definitely try to leverage future language features to implement this!

In the meantime crates like error-chain are considering futures support which can help with this problem.

Member

alexcrichton commented Mar 6, 2017

Yeah I definitely agree that this is one of the pain points for futures today, and I think you're spot on in that there's not an easy fix today as the "easy fixes" have their own pitfalls. That being said though we can definitely try to leverage future language features to implement this!

In the meantime crates like error-chain are considering futures support which can help with this problem.

@mmacedoeu

This comment has been minimized.

Show comment
Hide comment
@mmacedoeu

mmacedoeu Apr 13, 2017

I have tried to Call .chain_err("my message here")? inside a Future and got a Compiler error: the trait std::ops::Carrier is not implemented for futures::FutureResult<...> ! I think this is related to Futures crate as Carrier is not implemented.

mmacedoeu commented Apr 13, 2017

I have tried to Call .chain_err("my message here")? inside a Future and got a Compiler error: the trait std::ops::Carrier is not implemented for futures::FutureResult<...> ! I think this is related to Futures crate as Carrier is not implemented.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 19, 2017

Member

@mmacedoeu ah yeah unfortunately in a function that returns a future I don't think we can get ? to workright now. The good news though is that with async/await syntax (or at least hypothetically so) we should be able to get ? to work just fine!

Member

alexcrichton commented Apr 19, 2017

@mmacedoeu ah yeah unfortunately in a function that returns a future I don't think we can get ? to workright now. The good news though is that with async/await syntax (or at least hypothetically so) we should be able to get ? to work just fine!

@Kixunil

This comment has been minimized.

Show comment
Hide comment
@Kixunil

Kixunil Jun 12, 2017

impl IntoFuture for T { with specialisation is something I'm dreaming about.

Kixunil commented Jun 12, 2017

impl IntoFuture for T { with specialisation is something I'm dreaming about.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 15, 2017

Member

I'm going to close this in favor of #291 which I believe is a highly related issue.

It's also worth pointing out that async/await are likely to make much of this problem go away, as the ? operator can then be used natively.

Member

alexcrichton commented Aug 15, 2017

I'm going to close this in favor of #291 which I believe is a highly related issue.

It's also worth pointing out that async/await are likely to make much of this problem go away, as the ? operator can then be used natively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment