`Compat` implementation #1119

tinaun · 2018-07-25T04:22:11Z

based on @MajorBreakfast's approach here rustasync/team#26 (comment)

issues:

executors are blocked on Impl Executor for Box<E: Executor> rust#52674
more tests / examples
~~where should 0.1 -> 0.3 combinators live?~~
renaming blocked on Apply dependency renamings when running rustdoc cargo#5794
name of NeverError type

MajorBreakfast · 2018-07-25T06:41:24Z

futures-util/src/future/never_error.rs

+impl<Fut, T> Future for NeverError<Fut>
+    where Fut: Future<Output = T>,
+{
+    type Output = Result<T, ()>;


I didn't know that Never was introduced only in 0.2 when I proposed the name NeverError. I think we should come up with another name for this type. Maybe UnitError?

That said, I continue to believe that this combinator is useful. .map(|x| Ok::<_, ()>(x)) would be damn annoying to type.

MajorBreakfast · 2018-07-25T06:42:41Z

futures-util/src/future/never_error.rs

+    type Output = Result<T, ()>;
+
+    fn poll(mut self: PinMut<Self>, cx: &mut task::Context) -> Poll<Result<T, ()>> {
+        match self.future().poll(cx) {


self.future().poll(cx).map(|x| Ok(x)) is shorter :)

Or even .map(Ok) 😉

@Nemo157 Yeah ^^'
@tinaun I just noticed something funny: You added the Poll::map method to libcore yourself 😁

MajorBreakfast · 2018-07-25T06:50:00Z

futures-util/src/future/mod.rs

@@ -83,6 +86,8 @@ if_std! {

    mod shared;
    pub use self::shared::Shared;
+
+    use std::boxed::PinBox;


Best placed right underneath if_std! {. We usually have the imports on top

MajorBreakfast · 2018-07-25T07:02:09Z

futures-util/src/compat/executor.rs

+
+pub trait ExecCompat: Executor01<
+        Compat<FutureObj<'static, ()>, BoxedExecutor>
+    > + Clone + Send + 'static


pub trait ExecCompat: Executor01<Fut> + Clone + Send + 'static where Fut: Compat<FutureObj<'static, ()>, BoxedExecutor> {

The multiline <...> reminds me of C++ xD

Edit: Meh we would need to create this type param... Ignore that then (I tested it, but it compiled because this file was never mentioned in a mod statement)

Can you rename the trait to Executor01CompatExt?

MajorBreakfast · 2018-07-25T07:10:40Z

futures-util/src/compat/executor.rs

+}
+
+#[derive(Clone)]
+pub struct ExecutorCompat<E> {


Rename to CompatExecutor

MajorBreakfast · 2018-07-25T07:12:10Z

futures-util/src/compat/executor.rs

+impl<E> Executor03 for ExecutorCompat<E> 
+    where E: Executor01<
+        Compat<FutureObj<'static, ()>, Box<Executor03>>
+    >,


Fits on single line (or if you prefer to split it use a type param)

Nemo157 · 2018-07-25T07:12:51Z

futures-util/Cargo.toml

@@ -29,6 +30,7 @@ futures-io-preview = { path = "../futures-io", version = "0.3.0-alpha.1", defaul
 futures-sink-preview = { path = "../futures-sink", version = "0.3.0-alpha.1", default-features = false}
 either = { version = "1.4", default-features = false }
 slab = { version = "0.4", optional = true }
+futures = { version = "0.1", optional = true }


I’m surprised this plus the dev-dependency below don’t cause issues for the tests (both have a lib name of futures). Again here’s a place where having crate renaming for the edition would be super useful to make this futures01 or something.

Ah yes, it does:

→ cargo test --features compat --manifest-path futures-util/Cargo.toml Downloading futures v0.1.23 Compiling futures v0.1.23 Compiling futures-util-preview v0.3.0-alpha.1 (file:///home/wim/sources/github.com/Nemo157/futures-rs/futures-util) Compiling futures-executor-preview v0.3.0-alpha.1 (file:///home/wim/sources/github.com/Nemo157/futures-rs/futures-executor) Compiling futures-preview v0.3.0-alpha.1 (file:///home/wim/sources/github.com/Nemo157/futures-rs/futures) Finished dev [unoptimized + debuginfo] target(s) in 13.43s Running target/debug/deps/futures_util-0b0ee17c1f6f37a3 running 1 test test sink::fanout::tests::it_works ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out Doc-tests futures_util error[E0465]: multiple rlib candidates for `futures` found --> /home/wim/sources/github.com/Nemo157/futures-rs/futures-util/src/compat/mod.rs:6:5 | 6 | use futures::Future as Future01; | ^^^^^^^ | note: candidate #1: /home/wim/sources/github.com/Nemo157/futures-rs/target/debug/deps/libfutures-e0fcc61a6ffa167e.rlib --> /home/wim/sources/github.com/Nemo157/futures-rs/futures-util/src/compat/mod.rs:6:5 | 6 | use futures::Future as Future01; | ^^^^^^^ note: candidate #2: /home/wim/sources/github.com/Nemo157/futures-rs/target/debug/deps/libfutures-6ebec9b92185abd2.rlib --> /home/wim/sources/github.com/Nemo157/futures-rs/futures-util/src/compat/mod.rs:6:5 | 6 | use futures::Future as Future01; | ^^^^^^^ error[E0463]: can't find crate for `futures` --> /home/wim/sources/github.com/Nemo157/futures-rs/futures-util/src/compat/mod.rs:6:5 | 6 | use futures::Future as Future01; | ^^^^^^^ can't find crate

And unfortunately renaming dependencies has issues that block it being used for this.

We should convene the council of elders

One temporary solution could be to fully rename our libraries to futures-preview. Long term though, we'll need crate renaming because both crates will share a name in the end.

could we get away with moving Compat into its own futures-compat sub crate?

I think that'll work for now, since it doesn't need to import the 0.3 futures facade it won't run into the name conflict. Once renaming is working then it could be merged back into futures-util.

Alright, testing with a locally built cargo including rust-lang/cargo#5794 using futures01 = { version = "0.1", optional = true, package = "futures" } (and adjusting all the uses) appears to run the tests fine. (EDIT: here's the commit I tested with)

So, if you're willing to wait for a new cargo to be released to nightly that's one option.

@Nemo157 How long do you think will this take? If it's just a day or two, then we should wait. @tinaun requires some time to tweak this PR anyway

I'm not sure what the Cargo release cycle is like, my current nightly toolchain has rustc from 2018-07-23 but cargo from 2018-07-17, so I don't think it's always the latest master.

this pr requires a new nightly anyway for the Executor stuff.

@tinaun Wouldn't the newtype approach be more lightweight?

MajorBreakfast · 2018-07-25T07:13:41Z

futures-util/src/compat/executor.rs

+    >,
+    E: Clone + Send + 'static,
+{
+    fn spawn_obj(&mut self, obj: FutureObj<'static, ()>) -> Result<(), task::SpawnObjError> {


Long line

fn spawn_obj( &mut self, future: FutureObj<'static, ()>, ) -> Result<(), task::SpawnObjError> {

Note: Trailing comma, renamed obj to future (This is the thing that I and Aaron want to call future, currently it is called "task")

MajorBreakfast · 2018-07-25T07:24:19Z

futures-util/src/compat/mod.rs

+
+use core::mem::PinMut;
+use core::marker::Unpin;
+use std::sync::Arc;


Use std instead of core (we do that elsewhere as well)

MajorBreakfast · 2018-07-25T07:24:55Z

futures-util/src/compat/mod.rs

+use core::mem::PinMut;
+use core::marker::Unpin;
+use std::sync::Arc;
+


mod executor; pub use self::executor::*;

(But list individual exports instead of *)

MajorBreakfast · 2018-07-25T07:39:08Z

@tinaun Brilliant work!

ATM it doesn't compile because the mod executor statement was missing. I'll comment more later once you've fixed that. (Doesn't make sense to fix it myself if you do it anyway 🙂)

Add to all 01 imports the suffix 01 and no suffix for 03. Or, alternatively (if you prefer) add suffixes to everything. Currently it's a bit mixed.
Try if extern crate futures as futures01 in lib.rs can be used for referring to the futures 0.1 crate as futures01 in use statements

Nemo157 · 2018-07-25T07:48:52Z

@tinaun are you planning on working on adaptors for Stream, Sink, or the io traits as well? If not I can build off this for some of those.

MajorBreakfast · 2018-07-25T08:34:58Z

where should 0.1 -> 0.3 combinators live?

@tinaun I think your approach to pack everything related to compatibility into futures_util::compat is the right solution.

Additionally to the stuff you mentioned above, futures-preview, our facade, needs to reexport everything if the feature is enabled

Nemo157 · 2018-07-25T18:52:21Z

Since I need them for a project I want to use as a test case for this I've got Stream and (uni-directional) Sink compat implementations ready. Once the executor stuff is sorted I'll hopefully have a small ported project to show. (Although, it's really not the nicest codebase to start with).

tinaun · 2018-07-26T03:10:21Z

besides the NeverError bikeshed and the cargo renamings this should be about done

Nemo157 · 2018-08-02T10:12:27Z

IMO the cargo renaming can wait. This compiles and works currently, we just can’t run the tests with the feature active until renaming is supported.

MajorBreakfast · 2018-08-02T17:48:47Z

I'm going to merge this now. We still need need to document it (I think cargo doc ignores it ATM) and add some further polish. However that can be done in follow-up PRs :)

Big thanks to @tinaun !

Nemo157 · 2018-08-02T17:50:30Z

Guess that means I should get onto doing the rebase of my changes I'd planned 😄

Nemo157 · 2018-08-02T17:55:44Z

futures-util/src/compat/executor.rs

+                let fut = exec_err.into_future().into_inner().unwrap_or_else(|_| ());
+                SpawnObjError {
+                    kind: SpawnErrorKind::shutdown(),
+                    task: Box::new(fut).into(),   


This field is now future.

MajorBreakfast reviewed Jul 25, 2018

View reviewed changes

Nemo157 reviewed Jul 25, 2018

View reviewed changes

MajorBreakfast reviewed Jul 25, 2018

View reviewed changes

MajorBreakfast mentioned this pull request Jul 30, 2018

ExecutorExt #1142

Closed

tinaun force-pushed the compat branch 2 times, most recently from 0b78491 to 8bdff6b Compare August 1, 2018 04:34

Nemo157 reviewed Aug 2, 2018

View reviewed changes

tinaun force-pushed the compat branch from d526cce to 9e06734 Compare August 2, 2018 19:21

MajorBreakfast force-pushed the compat branch from 9e06734 to 69fb4b1 Compare August 2, 2018 19:59

tinaun and others added 6 commits August 2, 2018 22:09

Compat implementation

feadcbe

finished executors

a5da4e5

futures 0.1 extension trait

69428db

add Compat to main lib

127c3f5

doc fixes

3380430

Refactor compatibility layer

e711c1a

Rename NeverError To UnitError

8bdb4ef

MajorBreakfast force-pushed the compat branch from bb0dee7 to 8bdb4ef Compare August 2, 2018 20:09

MajorBreakfast merged commit 1a83bba into rust-lang:master Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Compat` implementation #1119

`Compat` implementation #1119

tinaun commented Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018

Nemo157 Jul 25, 2018

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018

Nemo157 Jul 25, 2018

Nemo157 Jul 25, 2018

MajorBreakfast Jul 25, 2018

tinaun Jul 25, 2018

Nemo157 Jul 25, 2018

Nemo157 Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018

Nemo157 Jul 25, 2018

tinaun Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018

MajorBreakfast Jul 25, 2018

MajorBreakfast commented Jul 25, 2018 •

edited

Loading

Nemo157 commented Jul 25, 2018

MajorBreakfast commented Jul 25, 2018 •

edited

Loading

Nemo157 commented Jul 25, 2018

tinaun commented Jul 26, 2018

Nemo157 commented Aug 2, 2018

MajorBreakfast commented Aug 2, 2018

Nemo157 commented Aug 2, 2018

Nemo157 Aug 2, 2018

Compat implementation #1119

Compat implementation #1119

Conversation

tinaun commented Jul 25, 2018 • edited Loading

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

MajorBreakfast Jul 25, 2018 • edited Loading

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

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

Nemo157 Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinaun Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Jul 25, 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 commented Jul 25, 2018 • edited Loading

Nemo157 commented Jul 25, 2018

MajorBreakfast commented Jul 25, 2018 • edited Loading

Nemo157 commented Jul 25, 2018

tinaun commented Jul 26, 2018

Nemo157 commented Aug 2, 2018

MajorBreakfast commented Aug 2, 2018

Nemo157 commented Aug 2, 2018

Choose a reason for hiding this comment

`Compat` implementation #1119

`Compat` implementation #1119

tinaun commented Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018 •

edited

Loading

Nemo157 Jul 25, 2018 •

edited

Loading

tinaun Jul 25, 2018 •

edited

Loading

MajorBreakfast Jul 25, 2018 •

edited

Loading

MajorBreakfast commented Jul 25, 2018 •

edited

Loading

MajorBreakfast commented Jul 25, 2018 •

edited

Loading