port futures 0.3 to std futures #1034

tinaun · 2018-06-07T20:33:35Z

No description provided.

tinaun · 2018-06-08T20:51:55Z

this won't compile until a nightly with futures in it is built, but i think its ready to be looked at - theres a whole lot of extension traits which im kinda ambivalent about - i could just add more inherent methods in rust-lang/rust#51442 instead

tinaun · 2018-06-09T05:09:55Z

i have no idea whats causing those doc errors. a missing path in a link maybe?

MajorBreakfast · 2018-06-12T06:42:04Z

futures-core/src/future/either.rs

+//             }
+//         }
+//     }
+// }


Why was this removed?

since either is defined in https://github.com/bluss/either and Future in std it can't exist here because of orphan rules - i might write a pr moving this to the either crate as an unstable feature

I'd gently suggest removing the commented code, if it is, in fact, going away

MajorBreakfast · 2018-06-12T06:44:06Z

futures-core/src/poll.rs

+    fn is_ready(&self) -> bool;
+    /// Returns whether this is `Poll::Pending`
+    fn is_pending(&self) -> bool {
+        !self.is_ready()


Your PR to core has been merged rust-lang/rust#51442 These methods are now not required

MajorBreakfast · 2018-06-12T06:45:17Z

futures-core/src/lib.rs

@@ -63,7 +64,7 @@ macro_rules! unsafe_unpinned {
 }

 mod poll;
-pub use poll::Poll;
+pub use poll::{Poll, PollExt, PollResultExt};


Also not required because core PR has been merged

tinaun · 2018-06-12T07:26:40Z

rebased to latest nightly

cramertj · 2018-06-12T18:41:41Z

futures-core/src/future/mod.rs

-    type Output = F::Output;
+impl<T: ?Sized> CoreFutureExt for T where T: Future {}
+
+// should be in std


This can be removed now, right?

cramertj · 2018-06-12T18:43:07Z

futures-core/src/task/mod.rs

-    fn drop(&mut self) {
-        unsafe {
-            (self.drop)(self.ptr)
+    impl TaskObjExt for TaskObj {


This implements the trait for dyn TaskObj-- did you mean to implement TaskObjExt for T: TaskObj?

isn't TaskObj a struct that pretends to be a trait object currently?

cramertj · 2018-06-12T18:45:03Z

futures-core/src/task/mod.rs

-            (self.poll)(self.ptr, cx)
-        }
+    /// Extension trait for `TaskObj`, adding methods that require allocation.
+    pub trait TaskObjExt {


What's the purpose of this trait? Since the method doesn't take self, it's not actually callable using method syntax, so it might as well be a free function. IMO users should either call TaskObj::from_poll_task directly or call the spawn method on the ContextExt trait.

cramertj · 2018-06-12T18:45:53Z

futures-core/src/task/mod.rs

+    impl<'a> ContextExt for Context<'a> {
+        fn spawn<F>(&mut self, f: F) where F: Future<Output = ()> + 'static + Send {
+            self.executor()
+                .spawn_obj(TaskObj::new(f)).unwrap()


As I mentioned above, I'd change this to call from_poll_task directly.

MajorBreakfast · 2018-06-17T09:57:47Z

About reexports: I'm not sure whether we should offer reexports at all. Normally reexports are used for compatibility. However futures 0.3 can't be compatible with 0.2 in any meaningful way because nearly all signatures change a little bit. Therefore should there be reexports at all? I think we shouldn't offer reexports and instead encourage to either import the prelude or import from core directly. What do you think?

Terkwood · 2018-06-17T10:08:59Z

How hard is it to preserve the re-export capability? It seems useful in the case where one isn't facing a compatibility issue.

MajorBreakfast · 2018-06-17T13:18:00Z

It seems useful in the case where one isn't facing a compatibility issue.

@Terkwood Please elaborate

The reason I propose to remove the reexports is for consistency. It seems inconsistent to import core::mem::PinMut directly, but not do the same for core::future::Future. And to offer reexports for core::future::Future, but not for core::mem::PinMut.

Terkwood · 2018-06-17T17:25:33Z

Just to make sure I'm speaking coherently:

About reexports: I'm not sure whether we should offer reexports at all. Normally reexports are used for compatibility.

You're talking about removing reexports from the language entirely, right? If that's right: I think they're a mildly useful feature (in theory). Coming from a background in other languages, I liked the idea of having this facility available for use in lieu of writing boilerplate wrapper code.

I don't want to express too much enthusiasm for the reexport feature in rust. Futures are a big deal.

Your statement in favor of consistency is good. If the only way that reexports can be kept in the language will involve introducing a confusing inconsistency in their use, then... meh? That's not very appealing, when you put it that way, no.

Thanks for your time and effort!

Terkwood

Thank you!

Terkwood · 2018-06-17T17:27:09Z

futures-core/src/future/either.rs

+//             }
+//         }
+//     }
+// }


I'd gently suggest removing the commented code, if it is, in fact, going away

Terkwood · 2018-06-17T17:29:01Z

futures-core/src/future/mod.rs

-    /// the `fuse` adaptor which defines the behavior of `poll`, but comes with
-    /// a little bit of extra cost.
-    fn poll(self: PinMut<Self>, cx: &mut task::Context) -> Poll<Self::Output>;
+pub use core::future::Future;


MajorBreakfast · 2018-06-17T17:34:09Z

You're talking about removing reexports from the language entirely, right?

@Terkwood No no ^^' I merely suggest to remove the reexport statements in futures-core. In other words: Only export the stuff that is actually defined in futures-core (e.g. Stream)

Terkwood · 2018-06-17T17:59:20Z

Oh, ok😀 Then my earlier comment was misinformed. Thanks for bearing with me! Please proceed!

…

On Sun, Jun 17, 2018, 13:34 Josef Brandl ***@***.***> wrote: You're talking about removing reexports from the language entirely, right? @Terkwood <https://github.com/Terkwood> No no ^^' I merely suggest to remove the reexport statements in futures-core. In other words: Only export the stuff that is actually defined in futures-core (e.g. Stream) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1034 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AlDziCTScHqAYFTjRaYqrT6aRAx-sAVyks5t9pMUgaJpZM4UfEoo> .

withoutboats · 2018-06-17T21:24:14Z

The re-export is important for getting futures 0.3 on stable faster. As soon as arbitrary_self_types is stable, the new Future API can be implemented in futures on stable. We can cfg nightly vs stable to re-export or define it here, before the futures_api feature is stable. Then every library written against futures, without depending on std::future::Future directly, will compile on stable.

MajorBreakfast · 2018-06-17T22:13:42Z

@withoutboats That requires:

Not removing the definitions of Future, Executor, etc. from futures-core. Instead, they need to be updated so that they're identical to their counterparts in core.
PinMut needs a polyfill/dummy impl

MajorBreakfast · 2018-06-19T21:16:25Z

It was decided in a quick discussion on the wg-net Discord channel to leave the reexports in, because they give us the possibility to maybe do the "stable shim dance" later

aturon · 2018-06-19T22:13:25Z

Merged, thanks much @tinaun!

There are a couple of minor things I will clean up in a follow-up PR.

remove futures-stable

de9ab1d

tinaun force-pushed the 0.3 branch 2 times, most recently from 04fbfd5 to d87e9e3 Compare June 8, 2018 04:53

tinaun changed the title ~~port 0.3 to std futures [wip]~~ port futures-core 0.3 to std futures Jun 8, 2018

tinaun changed the title ~~port futures-core 0.3 to std futures~~ port futures 0.3 to std futures Jun 9, 2018

aturon requested a review from cramertj June 11, 2018 15:41

MajorBreakfast reviewed Jun 12, 2018

View reviewed changes

tinaun force-pushed the 0.3 branch from 605d1c3 to 3626fc6 Compare June 12, 2018 07:25

This was referenced Jun 12, 2018

Executors and partly reenable futures crate #1037

Merged

0.3 alpha burndown issue #1033

Closed

cramertj reviewed Jun 12, 2018

View reviewed changes

tinaun added 8 commits June 14, 2018 00:29

stdify Poll

33d6ba4

remove PollExt, add methods to std

c533b88

fix imports, add comments

785feed

stdify futures-channel

7f4b87a

stdify futures-io

b832ebb

stdify futures-sink

1cca462

stdify futures-util

88af39e

remove taskobjext

adbe9f9

tinaun force-pushed the 0.3 branch from e41a450 to adbe9f9 Compare June 14, 2018 04:30

Terkwood approved these changes Jun 17, 2018

View reviewed changes

aturon merged commit 6676c2d into rust-lang:0.3 Jun 19, 2018

tinaun deleted the 0.3 branch June 20, 2018 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port futures 0.3 to std futures #1034

port futures 0.3 to std futures #1034

tinaun commented Jun 7, 2018

tinaun commented Jun 8, 2018 •

edited

Loading

tinaun commented Jun 9, 2018

MajorBreakfast Jun 12, 2018

tinaun Jun 12, 2018 •

edited

Loading

Terkwood Jun 17, 2018

MajorBreakfast Jun 12, 2018

MajorBreakfast Jun 12, 2018 •

edited

Loading

tinaun commented Jun 12, 2018

cramertj Jun 12, 2018

cramertj Jun 12, 2018

tinaun Jun 13, 2018

cramertj Jun 12, 2018 •

edited

Loading

cramertj Jun 12, 2018

MajorBreakfast commented Jun 17, 2018 •

edited

Loading

Terkwood commented Jun 17, 2018 •

edited

Loading

MajorBreakfast commented Jun 17, 2018

Terkwood commented Jun 17, 2018

Terkwood left a comment

Terkwood Jun 17, 2018

Terkwood Jun 17, 2018

MajorBreakfast commented Jun 17, 2018

Terkwood commented Jun 17, 2018 via email

withoutboats commented Jun 17, 2018

MajorBreakfast commented Jun 17, 2018

MajorBreakfast commented Jun 19, 2018 •

edited

Loading

aturon commented Jun 19, 2018

port futures 0.3 to std futures #1034

port futures 0.3 to std futures #1034

Conversation

tinaun commented Jun 7, 2018

tinaun commented Jun 8, 2018 • edited Loading

tinaun commented Jun 9, 2018

Choose a reason for hiding this comment

tinaun Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

tinaun commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Jun 17, 2018 • edited Loading

Terkwood commented Jun 17, 2018 • edited Loading

MajorBreakfast commented Jun 17, 2018

Terkwood commented Jun 17, 2018

Terkwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Jun 17, 2018

Terkwood commented Jun 17, 2018 via email

withoutboats commented Jun 17, 2018

MajorBreakfast commented Jun 17, 2018

MajorBreakfast commented Jun 19, 2018 • edited Loading

aturon commented Jun 19, 2018

tinaun commented Jun 8, 2018 •

edited

Loading

tinaun Jun 12, 2018 •

edited

Loading

MajorBreakfast Jun 12, 2018 •

edited

Loading

cramertj Jun 12, 2018 •

edited

Loading

MajorBreakfast commented Jun 17, 2018 •

edited

Loading

Terkwood commented Jun 17, 2018 •

edited

Loading

MajorBreakfast commented Jun 19, 2018 •

edited

Loading