Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

Handling traits that return non-stable Futures #71

Closed
Nemo157 opened this issue Mar 8, 2018 · 11 comments
Closed

Handling traits that return non-stable Futures #71

Nemo157 opened this issue Mar 8, 2018 · 11 comments

Comments

@Nemo157
Copy link
Contributor

Nemo157 commented Mar 8, 2018

Unfortunately there's a lot of extraneous code here, but if you save the following as examples/foo.rs and run cargo build --examples you get the below error message.

The key part is declaring a trait with a Future as associated type, then passing an instance of that trait into a function defined via #[async_move] and attempting to await the returned Future.

#![feature(proc_macro, conservative_impl_trait, generators)]

extern crate futures_await as futures;

use futures::prelude::{async_move, await};
use futures::{Future, Async, Poll, task::Context};

trait Foo {
    type Future: Future<Item = (), Error = ()>;
    fn foo(&self) -> Self::Future;
}

struct Bar;

impl Foo for () {
    type Future = Bar;
    fn foo(&self) -> Self::Future { Bar }
}

impl Future for Bar {
    type Item = ();
    type Error = ();
    fn poll(&mut self, _: &mut Context) -> Poll<(), ()> { Ok(Async::Ready(())) }
}

#[async_move]
fn foobar<F>(foo: &F) -> Result<(), ()> where F: Foo {
    await!(foo.foo())
}

fn main() { }
error[E0277]: the trait bound `<F as Foo>::Future: futures::__rt::<unnamed>::Unpin` is not satisfied
  --> examples/foo.rs:28:5
   |
28 |     await!(foo.foo())
   |     ^^^^^^^^^^^^^^^^^ the trait `futures::__rt::<unnamed>::Unpin` is not implemented for `<F as Foo>::Future`
   |
   = help: consider adding a `where <F as Foo>::Future: futures::__rt::<unnamed>::Unpin` bound
   = note: required because of the requirements on the impl of `futures::__rt::StableFuture` for `<F as Foo>::Future`
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

There's a straightforward fix:

@@ -1,7 +1,9 @@
 #![feature(proc_macro, conservative_impl_trait, generators)]

+extern crate pin_api;
 extern crate futures_await as futures;

+use pin_api::Unpin;
 use futures::prelude::{async_move, await};
 use futures::{Future, Async, Poll, task::Context};

@@ -24,7 +26,7 @@ impl Future for Bar {
 }

 #[async_move]
-fn foobar<F>(foo: &F) -> Result<(), ()> where F: Foo {
+fn foobar<F>(foo: &F) -> Result<(), ()> where F: Foo, F::Future: Unpin {
     await!(foo.foo())

but I'm wondering if there's a nicer way to do this that could be documented somewhere?

@withoutboats
Copy link
Collaborator

Interesting. I think there's a bound in StableFuture that could/should be relaxed, but I have to think about it.

Right now there's this impl

impl<F: Future + Unpin> StableFuture for F

I think that doesn't need to have the Unpin bound. But I'm not 100% confident.

@withoutboats
Copy link
Collaborator

Yea, since its safe to call poll with &mut (or your impl is incorrect), Unpin shouldn't matter here.

I'd merge a PR to remove Unpin from the Future -> StableFuture and Stream -> StableStream blanket impls in futures-stable, if you want to make it. :)

@Nemo157
Copy link
Contributor Author

Nemo157 commented Mar 9, 2018

Yeah, it seems to me that that bound is backwards, it's always safe to call a Future as a StableFuture as StableFuture applies strictly more requirements on the caller, to make a StableFuture callable as a Future you would need the Unpin bound to exactly remove the additional requirement that StableFuture has over Future; i.e. it would be safe to provide

impl<F: StableFuture + Unpin> Future for F

Which matches up with the existing pin_api APIs

impl<T: Unpin> PinMut<T> { fn new(&mut T) -> PinMut<T>; }
impl<T: Unpin> DerefMut for PinMut<T>

I'll open a PR to futures-stable soon.

@withoutboats
Copy link
Collaborator

The impl has been changed to the futures-stable that is now merged into the rust-lang-nursery/futures-rs repository. futures-await needs to be changed to build off of that version of futures-rs, but once it is this will be fixed, so I'm closing this issue. :)

@manuels
Copy link

manuels commented Mar 31, 2018

Hey, any updates on this issue? I still get this error.

@Nemo157
Copy link
Contributor Author

Nemo157 commented Mar 31, 2018

@manuels this should be working with the latest master, you may need to cargo update -p futures-await.

In the mean time this repository has been merged into the futures repo, you should be able to remove your futures-await dependency and instead depend on futures = { git = "https://github.com/rust-lang-nursery/futures-rs", features = ["nightly"] }, the async and await macros should now be available from futures::prelude.

@manuels
Copy link

manuels commented Mar 31, 2018

Thanks for your answer, @Nemo157!
That's weird: I changed to rust-lang-nursery/futures-rs and I still get

error[E0277]: the trait bound `tokio::timer::Delay: futures::Future` is not satisfied
   --> src/lib.rs:142:17
    |
142 |                 await!(tokio::timer::Delay::new(next));
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `futures::Future` is not implemented for `tokio::timer::Delay`
    |
    = note: required because of the requirements on the impl of `futures::stable::StableFuture` for `tokio::timer::Delay`
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

@Nemo157
Copy link
Contributor Author

Nemo157 commented Mar 31, 2018

Ohhh, is that maybe because tokio::timer::Delay implements futures(0.1)::Future rather than futures(0.2)::Future (or some other not-exactly-the-same trait)? I'm not up to date on whether tokio is futures v0.2 compatible.

EDIT: cargo tree -i -p futures is the easiest way I know of to check if you have incompatible versions installed.

@manuels
Copy link

manuels commented Apr 1, 2018

Oh, that's why. Bummer, the futures-await brach 0.1.0 does not work anymore (probably it's incompatible with nightly now).

@Nemo157
Copy link
Contributor Author

Nemo157 commented Apr 1, 2018

You may be able to use the fork linked from #79 (comment), I'm not sure if there'll ever be a 0.1.1.

@manuels
Copy link

manuels commented Apr 1, 2018

Great hint, thanks, @Nemo157!

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

3 participants