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

Ban manual implementation of TryFuture and TryStream #1776

Closed
taiki-e opened this issue Aug 5, 2019 · 7 comments · Fixed by #1777
Closed

Ban manual implementation of TryFuture and TryStream #1776

taiki-e opened this issue Aug 5, 2019 · 7 comments · Fixed by #1777

Comments

@taiki-e
Copy link
Member

taiki-e commented Aug 5, 2019

Future<Output = Result<T, E>> automatically implements Tryfuture. Also it can call .await and FutureExt's methods without TryFuture::into_future.

Also, we may want to rewrite this trait to inherit Future in the future, so I think it makes sense to ban manual implementation for forward compatibility.
https://github.com/rust-lang-nursery/futures-rs/blob/78c5d852ec24edcf630a4c569c1234d078416f4f/futures-core/src/future/mod.rs#L61-L63

I'm thinking of using the approach that SliceIndex uses, but there may be better ways.

@taiki-e
Copy link
Member Author

taiki-e commented Aug 5, 2019

cc @cramertj @Nemo157

@Nemo157
Copy link
Member

Nemo157 commented Aug 5, 2019

I think adding Future as a super trait combined with the existing impl TryFuture for impl Future would also accomplish this. Is there a reason to not have that supertrait bound now?

@taiki-e
Copy link
Member Author

taiki-e commented Aug 5, 2019

Is there a reason to not have that supertrait bound now?

Nope, there is no particular reason. It's probably a better option.
(I thought that when TryFuture was defined, someone might did this intentionally.)

@cramertj
Copy link
Member

cramertj commented Aug 5, 2019

Yeah, @aturon and I hit a bug w/ cycles in the trait checker when attempting to do that:

trait Future { 
    type Output;
}

trait TryFuture: Future<Output = Result<Self::Ok, Self::Err>> {
    type Ok;
    type Err;
}

gives

error[E0391]: cycle detected when computing the supertraits of `TryFuture`
 --> src/main.rs:5:1
  |
5 | trait TryFuture: Future<Output = Result<Self::Ok, Self::Err>> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: ...which again requires computing the supertraits of `TryFuture`, completing the cycle
note: cycle used when collecting item types in top-level module
 --> src/main.rs:5:1
  |
5 | trait TryFuture: Future<Output = Result<Self::Ok, Self::Err>> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

IIRC @nikomatsakis said this was fixed in chalk, but there's no ETA for the fix as far as I know.

@taiki-e
Copy link
Member Author

taiki-e commented Aug 5, 2019

Seems it can be avoided by using <Self as TryFuture>::Ok. EDIT: see #1776 (comment)

trait Future { 
    type Output;
}

trait TryFuture: Future<Output = Result<<Self as TryFuture>::Ok, <Self as TryFuture>::Err>> {
    type Ok;
    type Err;
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a9d4353ff24b3f2fa6a9c9281889dd86

@taiki-e
Copy link
Member Author

taiki-e commented Aug 5, 2019

Nope, this still doesn't work.

error[E0275]: overflow evaluating the requirement `<Self as std::future::Future>::Output`
  --> src/lib.rs:7:1
   |
7  | / pub trait TryFuture:
8  | |     Future<Output = Result<<Self as TryFuture>::Ok, <Self as TryFuture>::Error>>
9  | | {
10 | |     type Ok;
...  |
14 | |     fn try_poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<Self::Ok, Self::Error>>;
15 | | }
   | |_^
   |
   = note: required because of the requirements on the impl of `TryFuture` for `Self`
   = note: required because of the requirements on the impl of `TryFuture` for `Self`

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=fad9b6f6899e6748118abec9fb199d26

@taiki-e
Copy link
Member Author

taiki-e commented Aug 5, 2019

So I believe #1777 is a good way at this time.

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

Successfully merging a pull request may close this issue.

3 participants