use futures::Poll (or clone of it) instead of nb::Error

[vitiral]

Prefix and Apology

I went down the wrong track in #2 and rust-embedded/embedded-hal#13. My error was because I was not familiar enough with the inner workings of futures and I used the incorrect terminology.

I do NOT think we should return a Future, but rather that our asyncronous functions should return a type identical to futures::Poll (or possibly futures::Poll itself)

Issue at hand

I suggest that although this is not futures, we should try and keep the API as similar as possible to futures. Future::poll returns the type Poll which is just Result<Async<T>, E>. See: Async.

These types are mathematically identical:
futures:

type Poll = Result<Async<T>, E>;

pub enum Async<T> {
    Ready(T),
    NotReady,
}

nb:

type MyResult = Result<T, nb::Error<E>>;
enum Error<E> {
    Other(E),
    WouldBlock,
}

We have simply moved the WouldBlock/NotReady around from Ok to Err.

However, Result<V, nb::Error<E>> is much less ergonomic for a function that is supposed to be asyncronous:

• When someone creates a function that is non blocking, they should be returning a type that specifies the function as non-blocking. That type is the Poll type (although it doesn't have to come from futures).
• Having the function return a Result that could have an Error::WouldBlock is much more difficult to follow -- you have to dig into what the error type is to understand that this is an async function!

Summary

An async function should return Poll type -- not have an "async error." In general, it should mimick the API of Future::poll. Fundamentally this is a usability/readability question, but it will also make it easier to reason about what futures::Poll::from::<nb::Poll>() does, as well as allow us to more easily leverage futures.

I posit that it would be better if we used the futures::Poll type directly, as that type really is zero cost by definition (futures::Poll<T, E> is mathenatically identical to Result<T, nb::Error<E>>) and would make converting async functions to futures much more readable and streamlined (they would automatically implement Future::poll!)

[vitiral]

Also, according to the trait documentation, Future::poll is the only required method to implement a Future! If we simply return future::Poll for our functions, we will have basically already implemented them as futures (we will just have to have a struct which just calls it for the poll method).

[Kixunil]

I actually don't feel like returning Result<Async<T>, E> is the best idea. When playing with futures and trying to implement my own version of them, I realised Async<Result<T, E>> would be better approach.

When writing combinators, most of them don't care whether it's Ok or Err. They just return when Async::NotReady is encountered. So instead of doing this:

let foo = match async_op() {
    Ok(Async::Ready(val)) => val,
    Ok(Async::NotReady) => return Ok(Async::NotReady),
    Err(e) => return Err(e),
};

One would write:

let foo = match async_op() {
    Async::Ready(val) => val,
    Async::NotReady => return Ok(Async::NotReady),
};

It's usually done in macros, but those macros generate the code. Also it avoids type conversion.

Secondly, this allows one to cleanly express operations that can't fail. (Although, I have nothing against Result<T, !>, I think it shouldn't be over-used.)

Thirdly, I guess that would play nice with ? operator.

Finally, I suppose returning Async::NotReady is the common case. Err is probably least common case.

Disclaimer: I haven't explored all uses of Futures, so maybe I'm missing something.

[vitiral]

I also like Async as a type much better, but I REALLY don't think it's worth having an API different than Future::poll here. Futures are the standard in simple async handling, we should maintain as similar an API as we can to make conversion simple and learning straightforward.

[Kixunil]

Maybe open an issue there? While Futures is becoming standard, it's not developed yet and may change.

[vitiral]

The reason Poll is the type it is is so that futures can use and_then, etc (i.e. They behave like iterators). So it's a pretty decent reason.

It's also a reason why putting WouldBlock/NotReady in the Error is not a good idea, Result already has pretty much this exact same api, but it only works if you stop on the first error.

[Kixunil]

I don't think implementing and_then etc on Future would be a problem. Quite the opposite.

[vitiral]

Sorry, I confused a few different topics there... In order for Future to have and_then, Poll has to have an Err type (since Poll is how Future knows whether there was an error). That is why just using Async doesn't work (Async has no concept of errors).

Having Poll just be a Result where Err is a fatal error means it has roughly the same API, which is a benefit as well.

Edit: I tried to explain a bit better and added formatting

[Kixunil]

That is solvable:

trait Future {
    type Item; // Just the item, no Result here.

    // ...

    fn and_then(self) -> AndThen where Self: Future<Item=Result> {
        // ...
    }
}

I was actually trying to do something similar with where Self::Item: SomeTrait, impl<T, E> SomeTrait for Result<T, E>, impl<T> SomeTriat for Option<T>. Unfortunately it collided in some way (Rust is currently unable to understand that Foo<Bar=Result<T, E>> and Foo<Bar=Option<T>> are different traits.)

[vitiral]

but Future::poll has type:

fn poll(&mut self) -> Poll<Self::Item, Self::Error>;

That is why Item doesn't have to be a Result... because Error is defined spearately!

AndThen is then implemented like this:

impl<A, B, F> Future for AndThen<A, B, F>
    where A: Future,
          B: IntoFuture<Error=A::Error>,
          F: FnOnce(A::Item) -> B,
{
    type Item = B::Item;
    type Error = B::Error;

    fn poll(&mut self) -> Poll<B::Item, B::Error> {
        self.state.poll(|result, f| {
            result.map(|e| {
                Err(f(e).into_future())
            })
        })
    }
}

So again, poll has to return a Result in order for AndThen to know how to do result.map in it's poll() method. I'm pretty sure there is no way around that.

[Kixunil]

That is why Item doesn't have to be a Result... because Error is defined spearately!

I know. I suggest removing Error associated type and instead use Result as item for cases when the Future can fail. Don't use Result if future is infallible. Option is another option. ;)

[vitiral]

but if you use a Result as the Item (and don't have an assigned Error) then and_then and related functions don't work.

You can just set Error = ! if future is infallible.

[japaric]

I have already mentioned this but when writing RTFM tasks / callback style
code WouldBlock as an error is the right type. The API in this crate is meant
to be usable in blocking mode, with futures, with the future #[async] /
await! (generators) and with callback code and the current API fits the bill
for all of them.

Neither Result<Async<T, E>> or Async<Result<T, E>> has an advantage over
nb::Result<T, E> in terms of pattern matching, but both are misleading in
callback style code. The former because serial.write(0).unwrap() would not
check if the operation completed or not, and the latter because
serial.write(0) does not even raise a warning about the operation not even
having occurred.

When someone creates a function that is non blocking, they should be returning
a type that specifies the function as non-blocking.

The standard library Read / Write traits when used in non-blocking mode
return a Result and use the ErrorKind::WouldBlock variant to signal that the
operation needs to block to complete. So in a sense we are already using the
standard return type for this operation.

Having the function return a Result that could have an Error::WouldBlock is
much more difficult to follow

This doesn't matter at all when writing blocking / futures-based code as you are
either using block!, try_nb!, hal-blocking or hal-futures and neither of
those returns nb::Result; they return a plain Result (or a future that will
converge to a Result).

For callback style code this is not as ergonomic as it should be but the
alternatives Result<Async<T, E>> and Async<Result<T, E>> are worse (see the
second paragraph).

(nb would be nicer if we had "untagged enums" as in Result<T, MyError | WouldBlock>; that would reduce one layer from the pattern matching:

match serial.write(byte) {
    Ok(()) => ..
    Err(MyError::Noise) => ..
    Err(MyError::Overrun) => ..
    Err(WouldBlock) => ..
    // ...
}

but that doesn't exist today)

An async function should return Poll type -- not have an "async error."

The Poll type was created to be the return type of polling a future not to
be the return type of an async function. Futures based async functions return
impl Future (a type that implements the Future trait). Proper #[async]
functions will return an impl Generator (a type that implements the
Generator trait)

The only nonblocking functions I have seen in practice are the ones in the
standard library Read / Write traits and those return a Result and use the
WouldBlock variant to indicate "this operation needs to block to complete".
nb is modeled after those functions.

but it will also make it easier to reason about what
futures::Poll::from::nb::Poll() does, as well as allow us to more easily
leverage futures.

So is the whole point to simplify writing futures based APIs on top of nb? You
want to go from:

fn write<S>(byte: u8) -> impl Future<Item = (), Error = Error>
where
    S: hal::Serial,
{
    future::poll_fn(|| {
        Ok(Async::Ready(try_nb!(serial.write(byte))))
    })
}

to:

fn write<S>(byte: u8) -> impl Future<Item = (), Error = Error>
where
    S: hal::Serial,
{
    future::poll_fn(|| serial.write(byte).into())
}

? Well, the required From implementation to make that possible requires (a)
that the nb crate depends on the futures crate, or (b) that the futures
crate depends on the nb crate. (a) is against the goal of the nb crate and
(b) seems unlikely so I don't think either is going to happen.

[Kixunil]

@vitiral why and_then couldn't be implemented for F: Future<Item=Result<T, E>>?

I know one can use !. I just found it cleaner to not differentiate between Ok and Err in then() implementation (and others as well).

@japaric

and the latter because
serial.write(0) does not even raise a warning about the operation not even
having occurred.

The solution is obviously very simple: #[must_use = "Operation might not completed"]

The standard library Read / Write traits when used in non-blocking mode
return a Result and use the ErrorKind::WouldBlock variant to signal that the
operation needs to block to complete. So in a sense we are already using the
standard return type for this operation.

This breaks when read_exact() and write_all() are used. I find it quite unfortunate.