Convert basic futures combinators to futures-core 0.3 #980

aturon · 2018-04-20T20:45:12Z

This commit includes the introduction of a FutureResult trait along the lines of the RFC (with some slight revisions).

It tackles all the "basic" combinators, but leaves off shared, select, and join for the moment.

cramertj · 2018-04-20T20:58:11Z

futures-util/src/future/flatten.rs

@@ -11,14 +12,13 @@ use super::chain::Chain;
 ///
 /// This is created by the `Future::flatten` method.
 #[must_use = "futures do nothing unless polled"]
-pub struct Flatten<A> where A: Future, A::Item: IntoFuture {
-    state: Chain<A, <A::Item as IntoFuture>::Future, ()>,
+pub struct Flatten<A> where A: Future, A::Output: Future {


Should we also have Result-y versions of this and FlattenStream? Or maybe we just make impl<T> Future for Result<T, E> where T: Future (and the same for Stream)?

This commit includes the introduction of a `FutureResult` trait along the lines of the RFC (with some slight revisions). It tackles all the "basic" combinators, but leaves off `shared`, `select`, and `join` for the moment.

cramertj · 2018-04-20T21:06:32Z

futures-util/src/future/lazy.rs

-    Second(R),
-    Moved,
-}
+// safe because we never generate `Pin<F>`


cramertj · 2018-04-20T21:53:13Z

futures-util/src/future_result/mod.rs

+
+/// A convenience for futures that return `Result` values that includes
+/// a variety of adapters tailored to such futures.
+pub trait FutureResult {


I don't see the into_future method that was brought up in the RFC thread-- are you still planning to add it? Also, I think that this should probably live in futures-core and have a FutureResultExt here, since folks will use FutureResult as a bound.

cramertj · 2018-04-20T21:54:47Z

LGTM aside from FutureResult and the Result-y versions of flatten.

seanmonstar · 2018-04-21T06:23:06Z

futures-util/src/future_result/mod.rs

+    /// This method is a stopgap for a compiler limitation that prevents us from
+    /// directly inheriting from the `Future` trait; in the future it won't be
+    /// needed.
+    fn poll_result(self: Pin<Self>, cx: &mut task::Context) -> Poll<Result<Self::Item, Self::Error>>;


Why is this method needed? Is it an error to leave this method off and just call poll?

This type doesn't have the Future trait as a bound, so you can't call Future methods directly without into_future.

(unless you know that your type is both a Future and a FutureResult)

Ah right of course, poll() is returning Self::Output, and there's no way to define that Output is a Result in the trait definition. 😢

I must admit, that situation is rather unfortunate... (while this doesn't fix it, separately: what about try_poll?)

Nemo157 · 2018-04-21T17:09:31Z

futures-util/src/future/poll_fn.rs

-        (self.inner)(cx)
+    fn poll(mut self: Pin<Self>, cx: &mut task::Context) -> Poll<T> {
+        // safe because we never expose a Pin<F>
+        (unsafe { Pin::get_mut(&mut self) }.inner)(cx)


There's a few places where you get mutable references to Unpin fields (well, in this case a field that could be Unpin without affecting it since FnMut already implies it must be Unpin). I have been using an additional macro when doing this to reduce the number of unsafe patterns needed to check:

macro_rules! unpinned_field { ($pin:expr, $field:ident) => { &mut *pinned_field!($pin, $field) }; }

(although, I'm still also using a "safe" pinned_field macro that relies on me not doing anything stupid in Drop, so I'm not sure how much benefit this gives when your pinned_field macro is unsafe to call).

aturon · 2018-04-25T01:45:37Z

I'm going to go ahead and merge this, then open a follow-up PR that tracks the new RFC proposal.

sdroege · 2019-04-22T14:00:32Z

This PR was removing future::loop_fn, and it seems it was not added back in one way or another since then. Are there any fundamental reasons why it can't possibly implemented (seems similar enough to stream::unfold) or was it just forgotten?

If it was just forgotten I could send a PR for adding it back in the next days.

cramertj · 2019-04-22T21:10:13Z

@sdroege It certainly can be implemented, but it's pretty redundant with loop + async/await.

sdroege · 2019-04-23T06:57:32Z

@sdroege It certainly can be implemented, but it's pretty redundant with loop + async/await.

Sure, but the same could be said about many of the existing combinators too :) Would you consider adding it back if I created a PR?

nrc · 2019-05-02T21:04:36Z

This PR was removing future::loop_fn, and it seems it was not added back in one way or another since then

I just hit this while converting a medium size crate from 0.1 to 0.3. I'm doing so since the futures library parts are being stabilised, but I want to avoid async/await until that is stable (I'm assuming this migration will make the later one easier).

Anyway, I think it would be good for to make some equivalent of loop_fn/Loop (and any other removed things) available in some form to make the transition from 0.1 to 1.0 as easy as possible. For large crates that is already a huge task. There is no documentation about what to do here, so even though it is easy if you know what you are doing, it is fairly difficult to find that out.

cramertj · 2019-05-02T21:20:50Z

I'd be happy to accept things like this under a separate feature (e.g. migration or something like that). There are a number of combinators whose new versions will be similarly impossible to use if you're trying to do a conversion to 0.3 without using async/await, such as most of the methods on AsyncReadExt, AsyncWriteExt, and Sink, since they take a borrowed self rather than an owned one like the previous versions. I attempted to make a combinator that would allow using borrowed futures types using combinator-only code, but that fell afoul of about a million HTRB-related compiler bugs (see #1023).

I'd prefer not to include these types in the default feature set or in the normal locations as those namespaces are already quite crowded, and I'd like to avoid suggesting combinators to users which are essentially useless post-async/await.

nrc · 2019-05-02T21:23:14Z

That sounds good to me. I agree we don't want to recommend such combinators given async/await but purely for migrating code they seem pretty useful

cramertj · 2019-05-02T21:26:27Z

Yup! Sounds good.

sdroege · 2019-05-03T06:43:00Z

I just hit this while converting a medium size crate from 0.1 to 0.3.

My workaround so far was to build something around stream::unfold and for_each btw

aturon requested a review from cramertj April 20, 2018 20:45

cramertj reviewed Apr 20, 2018

View reviewed changes

Convert basic futures combinators to futures-core 0.3

f9c468e

This commit includes the introduction of a `FutureResult` trait along the lines of the RFC (with some slight revisions). It tackles all the "basic" combinators, but leaves off `shared`, `select`, and `join` for the moment.

aturon force-pushed the 0.3-util branch from c5bbcad to f9c468e Compare April 20, 2018 20:59

cramertj reviewed Apr 20, 2018

View reviewed changes

futures-util/src/future/lazy.rs

Second(R),

Moved,

}

// safe because we never generate `Pin<F>`

Copy link

Member

cramertj Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

cramertj reviewed Apr 20, 2018

View reviewed changes

aturon mentioned this pull request Apr 21, 2018

RFC: add futures to libcore rust-lang/rfcs#2395

Closed

seanmonstar reviewed Apr 21, 2018

View reviewed changes

Nemo157 reviewed Apr 21, 2018

View reviewed changes

aturon merged commit ae9979e into rust-lang:0.3 Apr 25, 2018

c410-f3r mentioned this pull request Nov 12, 2019

Bring back loop_fn #1973

Closed

taiki-e mentioned this pull request May 15, 2021

adding loop_fn_back in to the future-rs #2444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert basic futures combinators to futures-core 0.3 #980

Convert basic futures combinators to futures-core 0.3 #980

aturon commented Apr 20, 2018

cramertj Apr 20, 2018

cramertj Apr 20, 2018

cramertj Apr 20, 2018

cramertj commented Apr 20, 2018

seanmonstar Apr 21, 2018

cramertj Apr 21, 2018

cramertj Apr 21, 2018

seanmonstar Apr 23, 2018

Nemo157 Apr 21, 2018

aturon commented Apr 25, 2018

sdroege commented Apr 22, 2019

cramertj commented Apr 22, 2019

sdroege commented Apr 23, 2019

nrc commented May 2, 2019

cramertj commented May 2, 2019

nrc commented May 2, 2019

cramertj commented May 2, 2019

sdroege commented May 3, 2019

Convert basic futures combinators to futures-core 0.3 #980

Convert basic futures combinators to futures-core 0.3 #980

Conversation

aturon commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Apr 25, 2018

sdroege commented Apr 22, 2019

cramertj commented Apr 22, 2019

sdroege commented Apr 23, 2019

nrc commented May 2, 2019

cramertj commented May 2, 2019

nrc commented May 2, 2019

cramertj commented May 2, 2019

sdroege commented May 3, 2019