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 IntoFuture for IntoIterator<IntoFuture>? #662

Closed
nyarly opened this issue Nov 26, 2017 · 12 comments
Closed

impl IntoFuture for IntoIterator<IntoFuture>? #662

nyarly opened this issue Nov 26, 2017 · 12 comments

Comments

@nyarly
Copy link

nyarly commented Nov 26, 2017

By analogy to the JoinN implementations, does it make sense for IntoIterator to be IntoFuture via join_all?

@alexcrichton
Copy link
Member

An interesting idea! I don't think this can be done, however, due to coherence :(

@nyarly
Copy link
Author

nyarly commented Nov 27, 2017

I'm not sure what "coherence" means in this context. Can you direct me to a reference?

@nyarly
Copy link
Author

nyarly commented Nov 27, 2017

@nyarly
Copy link
Author

nyarly commented Nov 27, 2017

Would an implementation for Vec or &[IntoFuture] work, then?

@stuhood
Copy link
Contributor

stuhood commented Nov 27, 2017

As an alternative, might it be possible to allow for iter().collect::<JoinAll<_>> by adding impl FromIterator for JoinAll?

@stuhood
Copy link
Contributor

stuhood commented Nov 28, 2017

I looked into this, and was initially optimistic, because the following passes all of the tests:

+impl<I> FromIterator<I::Item> for JoinAll<I>
+    where I: IntoIterator,
+          I::Item: IntoFuture,
+{
+    fn from_iter<X: IntoIterator<Item=I::Item>>(iter: X) -> Self {
+        let elems = iter.into_iter().map(|f| {
+            ElemState::Pending(f.into_future())
+        }).collect();
+        JoinAll { elems: elems }
+    }
+}
+
 pub fn join_all<I>(i: I) -> JoinAll<I>
     where I: IntoIterator,
           I::Item: IntoFuture,
 {
-    let elems = i.into_iter().map(|f| {
-        ElemState::Pending(f.into_future())
-    }).collect();
-    JoinAll { elems: elems }
+    i.into_iter().collect()
 }

...ie, join_all can easily be implemented in terms of FromIterator for JoinAll.

But in practice, actually using into_iter().collect::<JoinAll<_>>() is challenging, because inference fails in seemingly simple cases, such as:

// Fails with "error[E0283]: type annotations required: cannot resolve `_: std::cmp::Eq`": 
assert_done(|| vec![f_ok(1), f_ok(2)].into_iter().collect::<JoinAll<_>>(), Ok(vec![1, 2]));
// Succeeds:
assert_done(|| join_all(vec![f_ok(1), f_ok(2)]), Ok(vec![1, 2]));

Part of the complexity of the inference would seem to stem from the fact that JoinAll places unnecessary bounds on its type parameters. The body of the definition of JoinAll does not actually refer to IntoIterator at all:

pub struct JoinAll<I>
    where I: IntoIterator,
          I::Item: IntoFuture,
{
    elems: Vec<ElemState<<I::Item as IntoFuture>::Future>>,
}

...so it would seem that it could be simplified to:

pub struct JoinAll<I>
    where I: IntoFuture,
{
    elems: Vec<ElemState<<I as IntoFuture>::Future>>,
}

In that case, the inference for into_iter().collect::<JoinAll<_>> would be much simpler, because the underscore would represent the iterated type, rather than the IntoIterator type.

Certainly a "major version bump" type change, but maybe interesting to explore?

@stuhood
Copy link
Contributor

stuhood commented Nov 28, 2017

Mm... I guess my previous comment re-discovers #285. After re-implementing/applying #286, I was able to confirm that:

assert_done(|| vec![f_ok(1), f_ok(2)].into_iter().collect::<JoinAll<_>>(), Ok(vec![1, 2]));

is inferred correctly.

So perhaps once #286 lands this can be re-opened? I've found that using combinators is more legible than using static methods, as static methods require indentation rather than chaining.

EDIT: Posted here for posterity: master...stuhood:stuhood/join-all-simplification

@alexcrichton
Copy link
Member

@nyarly ah yes it is indeed possible to do this for concrete types like Vec and slices, but I actually think @stuhood's idea is a great one and could fit very nicely here!

It's a bit of a bummer though that this may be blocked on #286, but thanks for the investigation @stuhood!

@stuhood
Copy link
Contributor

stuhood commented Nov 28, 2017

@alexcrichton : Thanks! Worth re-titling and re-opening, or shall we open a new issue with regard to impl FromIterator for JoinAll?

@alexcrichton
Copy link
Member

Makes sense to me to retitle, @nyarly do you agree that such an impl would solve your original use case?

@nyarly
Copy link
Author

nyarly commented Nov 29, 2017

Honestly, I'm not sure. I'm looking at:

// I'm eliding a bunch of "and_then" into a pretend url2future! macro
future::join_all(urls.iter().map(|url| url2future!(url)).collect::<Vec<_>>())

I'm not sure if

urls.iter().map(|url| url2future!(url)).collect::<JoinAll<_>>()

is preferable?

If Vec<IntoFuture> were InfoFuture I could

urls.iter().map(|url| url2future!(url))

Is there another benefit to FromIterator for JoinAll?

@stuhood
Copy link
Contributor

stuhood commented Nov 29, 2017

@nyarly : If you're executing in a position that is expecting an (Into)Future, it's possible (unconfirmed) that inference would make the explicit parameter to collect unnecessary: so it would be (imaginary left hand side):

let f: impl Future<_> = urls.into_iter().map(|url| url2future!(url)).collect();
// vs
let f: impl Future<_> = urls.into_iter().map(|url| url2future!(url));

Is there another benefit to FromIterator for JoinAll?

It's (more) explicit? Either an advantage or a disadvantage, depending.

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