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

impl<T: DerefMut<Target=U>, U: ?Sized + Future> Future for T? #479

Closed
khuey opened this issue May 17, 2017 · 11 comments
Closed

impl<T: DerefMut<Target=U>, U: ?Sized + Future> Future for T? #479

khuey opened this issue May 17, 2017 · 11 comments

Comments

@khuey
Copy link
Contributor

khuey commented May 17, 2017

There's already an impl<'a, F: ?Sized + Future> Future for &'a mut F. This seems like a logical extension.

@khuey
Copy link
Contributor Author

khuey commented May 17, 2017

My concrete use case here is an OwningHandle<H: DerefMut<Target=Future>>. The future performs a computation on data in the OwningHandle's owned object, but I want to be able to send it to the cpupool so it needs a static lifetime. OwningHandle solves the lifetime problem but isn't directly usable as a Future. A wrapper struct with the obvious Future implementation works around this but is rather ugly.

@alexcrichton
Copy link
Member

Thanks for the report! I haven't played around with this much, but historically having blanket impls like this tends to cause problems related to coherence in many other external crates. I'm not 100% sure though if this would cause problems?

I agree it'd be great to have a "smart pointers of futures are they themselves futures" be automatic though!

@leoyvens
Copy link
Contributor

I didn't really understand your problem, can't you impl Future for OwningHandle<H>?

What you propose is to replace the impl for &mut F with a more a more general impl for DerefMut. Issues with that:

  1. There can only be one impl that ends with for T of a trait, no matter the bounds. To add this it needs not only to be useful by itself but also needs to be more useful than any other impl that ends in for T.
  2. It's a breaking change.

Point 1 means we may regret adding this instead of something else, point 2 means it's harder to add it later. The only contenders I can think of are AsMut and BorrowMut which are innapropriate. In conclusion I don't think we would regret adding this.

@khuey
Copy link
Contributor Author

khuey commented May 22, 2017

I didn't really understand your problem, can't you impl Future for OwningHandle?

I can't, though the OwningHandle crate author could. In general it seems like since it implements DerefMut that should be good enough if at all possible.

What you propose is to replace the impl for &mut F with a more a more general impl for DerefMut.

Yes.

There can only be one impl that ends with for T of a trait, no matter the bounds. To add this it needs not only to be useful by itself but also needs to be more useful than any other impl that ends in for T.

There isn't one yet, afaict, and this seems like a pretty good candidate!

@khuey
Copy link
Contributor Author

khuey commented May 22, 2017

I haven't played around with this much, but historically having blanket impls like this tends to cause problems related to coherence in many other external crates. I'm not 100% sure though if this would cause problems?

I haven't done any significant amount of Rust API design but this seems pretty sane. The Deref traits are already somewhat magical since the compiler will use them for coercion, so implementers should already be cautious here. I have seen people abusing Deref for inheritance but that's already pretty crazy.

@leoyvens
Copy link
Contributor

leoyvens commented May 22, 2017

Ah, I thought you had defined OwningHandle. Another issue is that we can't impl Future for any std types after this. We implement Future for Option<F : Future> so that's a conflict right there.

@khuey
Copy link
Contributor Author

khuey commented May 22, 2017

Indeed, that does fail. AIUI that rule exists in general because if the ancestor crate's type were to grow trait implementations that satisfied the bounds in the future (no pun intended) then we would get a coherence violation. In this particular case though, it's hard to see how Option would ever usefully implement DerefMut. Sad that we can't have nice things.

Perhaps the fundamental (pun actually intended this time) problem here is that Box is #[fundamental] and OwningHandle is not, even though it almost certainly wants to be treated identically to Box here. (See also rust-lang/rust#29635)

@leoyvens
Copy link
Contributor

Interesting point about fundamental. We will only be able to do this if/when Future is moved to std, by then this issue will have completely lost context, I guess we may close this?

@khuey
Copy link
Contributor Author

khuey commented May 22, 2017

Well if fundamental is the real problem it's OwningHandle that needs to be modified, once it's possible for non-std types to be fundamental of course.

@alexcrichton
Copy link
Member

Unfortunately I believe this can't be done today:

@alexcrichton
Copy link
Member

er ...

   Compiling futures v0.1.14 (file:///home/alex/code/futures)
error[E0119]: conflicting implementations of trait `future::Future` for type `core::option::Option<_>`:
   --> src/future/mod.rs:910:1
    |
910 | / impl<T: ::std::ops::DerefMut<Target=U>, U: ?Sized + Future> Future for T {
911 | |     type Item = U::Item;
912 | |     type Error = U::Error;
913 | |
...   |
916 | |     }
917 | | }
    | |_^ conflicting implementation for `core::option::Option<_>`
    | 
   ::: src/future/option.rs
    |
5   | / impl<F, T, E> Future for Option<F> where F: Future<Item=T, Error=E> {
6   | |     type Item = Option<T>;
7   | |     type Error = E;
8   | |
...   |
14  | |     }
15  | | }
    | |_- first implementation here

error[E0119]: conflicting implementations of trait `future::IntoFuture` for type `(_, _, _, _, _)`:
   --> src/future/mod.rs:944:1
    |
944 | / impl<F: Future> IntoFuture for F {
945 | |     type Future = F;
946 | |     type Item = F::Item;
947 | |     type Error = F::Error;
...   |
951 | |     }
952 | | }
    | |_^ conflicting implementation for `(_, _, _, _, _)`
    | 
   ::: src/future/join.rs
    |
92  | /         impl<A, $($B),*> IntoFuture for (A, $($B),*)
93  | |             where A: IntoFuture,
94  | |         $(
95  | |             $B: IntoFuture<Error=A::Error>
...   |
111 | |             }
112 | |         }
    | |_________- first implementation here

error: aborting due to 2 previous errors

error: Could not compile `futures`.

To learn more, run the command again with --verbose.

As a result, I'm going to close this.

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

No branches or pull requests

3 participants