More tests 4 #1069

MajorBreakfast · 2018-07-04T13:35:24Z

No description provided.

MajorBreakfast · 2018-07-04T13:35:49Z

futures-core/src/lib.rs

@@ -30,57 +30,8 @@ macro_rules! if_std {
    )*)
 }

-#[macro_export]
-macro_rules! pinned_deref {


I removed this macro. It was never used anywhere

MajorBreakfast · 2018-07-04T13:37:11Z

futures-core/src/lib.rs

-}
-
-#[macro_export]
-macro_rules! pinned_field {


I removed this macro because it's an anti pattern. Better alternatives are:

Usage of PinMut::map_unchecked

unsafe_pinned! macro

MajorBreakfast · 2018-07-04T13:40:43Z

futures-core/src/lib.rs

-    )* }
-}
+#[macro_use]
+mod macros;


I've put them in a macro module This is cleaner, but ultimately when macro imports are implemented I'd like to:

Put the poll macros under futures::task

Put the pin macros... not sure where. We need to decide that once macros can be imported via use statements

Correction: Macro imports via use are implemented. They‘re just feature gated. We could use them.

MajorBreakfast · 2018-07-04T13:42:02Z

futures-core/src/macros/pin.rs

+            unsafe {
+                $crate::core_reexport::mem::PinMut::map_unchecked(
+                    self.reborrow(), |x| &mut x.$f
+                )


I've changed this to map_unchecked. It works the same.

Also, here and in the following macros the $crate::core_reexport:: prefixes were partly missing.

MajorBreakfast · 2018-07-04T13:42:46Z

futures-util/src/future/flatten.rs

@@ -43,6 +53,6 @@ impl<A> Future for Flatten<A>
    type Output = <A::Output as Future>::Output;

    fn poll(mut self: PinMut<Self>, cx: &mut task::Context) -> Poll<Self::Output> {
-        unsafe { pinned_field!(self, state) }.poll(cx, |a, ()| a)
+        self.state().poll(cx, |a, ()| a)


I removed all occurences of the pinned_field! macro

MajorBreakfast · 2018-07-04T13:43:49Z

futures/src/macros/mod.rs

@@ -0,0 +1,7 @@
+// Macros redefined here because macro re-exports are unstable.


That's the reason why all macros are duplicated

MajorBreakfast · 2018-07-04T13:44:53Z

futures-util/src/sink/send_all.rs

@@ -65,7 +65,7 @@ where
 impl<'a, T, U> Future for SendAll<'a, T, U>
 where
    T: Sink + Unpin + 'a + ?Sized,
-    U: Stream<Item = Result<T::SinkItem, T::SinkError>> + Unpin + 'a + ?Sized,
+    U: Stream<Item = T::SinkItem> + Unpin + 'a + ?Sized,


Signature was wrong

MajorBreakfast · 2018-07-04T15:06:48Z

futures/tests/recurse.rs

+    ::std::thread::spawn(|| {
+        block_on(do_it((1_000, 0)).map(move |x| tx.send(x).unwrap()))
+    });
+    assert_eq!(500_500, rx.recv().unwrap());


I made it a bit fancier than it was before. Now it adds the numbers 0 + 1 + ... + 1000 = 500,500

MajorBreakfast · 2018-07-04T18:04:28Z

futures/tests_disabled/unfold.rs

-}
-
-#[test]
-fn unfold_err1() {


Streams are no longer error-aware, therefore this test doesn't apply anymore.

MajorBreakfast · 2018-07-04T18:28:55Z

@aturon This can be reviewed

MajorBreakfast · 2018-07-05T22:40:56Z

futures-util/src/try_future/try_chain.rs

+
+#[must_use = "futures do nothing unless polled"]
+#[derive(Debug)]
+crate enum TryChain<Fut1, Fut2, Data> {


TryChain is a new mechanism so that and_then and or_else can share a single implementation.

MajorBreakfast · 2018-07-05T22:42:44Z

futures-util/src/try_future/try_chain.rs

+                }
+            };
+
+            *this = TryChain::Empty; // Drop fut1


The eager_drop tests want this: They expect that the old futures is dropped before the closure runs.

MajorBreakfast · 2018-07-05T22:44:13Z

futures-util/src/try_future/mod.rs

              Self: Sized,
    {
-        map_ok::new(self, f)
+        MapOk::new(self, op)


I made these new functions methods

MajorBreakfast · 2018-07-05T22:45:16Z

futures-util/src/try_future/map_err.rs

-    f: Option<F>,
+pub struct MapErr<Fut, F> {
+    future: Fut,
+    op: Option<F>,


op is how the standard library's Result methods call this

MajorBreakfast · 2018-07-05T22:48:29Z

futures-util/src/try_future/into_future.rs

+/// Converts a `TryFuture` into a normal `Future`
+#[derive(Debug)]
+#[must_use = "futures do nothing unless polled"]
+pub struct IntoFuture<Fut> {


This is a new invention. I originally planned to use it for something, but my plan then changed and I did it another way. I suggest to keep it, though. I think it could be useful.

MajorBreakfast · 2018-07-06T22:09:01Z

Added tests for basic combinators and oneshot::channel()

MajorBreakfast · 2018-07-07T13:09:49Z

Woohoo. Green CI 🎉

And I didn't cheat. To the contrary: The docs now actually build and the CI runs the tests again

MajorBreakfast · 2018-07-07T14:32:18Z

futures/src/lib.rs

@@ -23,126 +25,25 @@

 #![no_std]

-#![deny(missing_docs, missing_debug_implementations, warnings)]
+#![deny(missing_docs, missing_debug_implementations)]


Rustdoc for reexports seems broken. The crates all build fine individually (e.g. cargo doc -p futures-core). But when reexported in the futures crate we get a bunch of warnings

MajorBreakfast · 2018-07-07T14:33:52Z

futures-channel/src/mpsc/mod.rs

+//! [`Receiver`]: struct.Receiver.html
+//! [`Stream`]: ../../futures_core/stream/trait.Stream.html
+//! [`Receiver::poll_next`]:
+//!     ../../futures_core/stream/trait.Stream.html#tymethod.poll_next


I added these links like so. I wasn't able to make it build otherwise. Seems to be a bug in all //! comments

MajorBreakfast · 2018-07-07T16:45:54Z

futures/src/lib.rs

-pub use futures_core::stream::Stream;
-pub use futures_util::stream::StreamExt;
-pub use futures_sink::Sink;
-pub use futures_util::sink::SinkExt;


I removed these top level exports because using them is an anti-pattern. It's much easier to either import from the right location like futures::task::Poll or to import the prelude futures::prelude::*. Anything in-between doesn't make sense in my opinion, it just makes the layout of the futures crate less clear.

I disagree. I don't see any anti-pattern. I frequently have use futures::{Future, Stream} in modules. Making them harder to import doesn't improve anything.

Glob imports and preludes, however, definitely are an anti-pattern.

I agree with @seanmonstar, having to begin every file with

use futures::{core::{future::Future, stream::Stream}, utils::{future::FutureExt, stream::StreamExt}};

Instead of

use futures::{Future, Stream, FutureExt, StreamExt};

Would be very annoying.

@Nemo157 @seanmonstar Thanks for your comments!

My reaction is that all this top-level stuff accumulates. There is also:

Context, ContextExt, CoreFutureExt, FutureObj, LocalFutureObj, TryFuture, TryFutureExt and TryStream, TryStreamExt, Sink, SinkExt

These are just the most common imports, but there are obviously more that we could add. (FutureObj exists because dyn Trait is currently impossible for traits that use arbitrary self types) My intention behind removing these top-level imports is that it makes the docs better. Top level exports are a bad thing because they destroy the structure. If everything is exported at the top level anyway, there is no real reason to have submodules at all. How can we decide which things are considered important enough to be exported at the top level? How will it grow over time? How would it look like if the standard library did it? I think that it doesn't scale and we shouldn't do it.
The hierarchy I'm proposing here is the same as in libcore: libcore also differentiates between the submodules future and task. And a stream submodule will probably exist in the future.

@Nemo157 FutureExt is defined in the utils crate, but there's no such module. All extensions live in the respective submodules future, stream and sink

This is how I imagine it:

use futures::future::{Future, FutureExt}; use futures::stream::{Stream, StreamExt}; use futures::task::{Context, Poll};

I think that this actually looks pretty nice. That said, the approach I'm proposing here is just a suggestion. This PR has become rather large, so I explictly highlighted this change to make it absolutely obvious. I certainly don't want to decide that for everyone. If you all really dislike it, we can revert to the old approach. I'm confident, though, that the approach of clear separation is better. Now I want to sell it :) If you're on board, then we change it, otherwise we change it back to the old approach. But, take a little time and think about it. I really think this approach is better.

I think idea of top-level re-exports serves both convenience for common case (easy to reach things you need without clutter) and discoverability. It's hard to argue that Future, Stream and Sink (and their extension traits since 0.2) are core concepts to futures crates. In my view, public submodules still make sense for things that are more specialized, e.g. deeper dive into streaming functionality for writing your own combinators. Being able to import things through futures::{Future, FutureExt, Stream, StreamExt} and futures::stream::{Stream, StreamExt, FilterMap, Empty} is both useful in my opinion if only in different contexts.

Clean:

Cluttered:

Consider the impression on a new user when he/she opens the docs for the first time.

I don't see any problem with the traits list there at the bottom. If it were a big deal, they can be doc(hidden) from the main page, but I don't think it's an issue.

I definitely wouldn't make actually using these traits harder just for the docs change.

One thing to note is that futures 0.2 didn't export Context to the top-level. As far as I am aware the standard was to use task::Context everywhere. That doesn't reduce it much though 😦

I wonder if trait aliases could help with all those extension traits. In futures have something like

trait Future = ::core::future::Future + CoreFutureExt + FutureExt;

(although currently the RFC doesn't specify that trait aliases bring methods into scope, I left a comment on the tracking issue to find out).

@seanmonstar My main concern is the impression a new user gets when looking at the docs for the first time. I've added a commit that adds the reexports as doc(hidden). As long as there is no big pile of unsorted stuff visible to users, it's fine I guess.

@Nemo157 I think it would be confusing if more than one thing was called a Future. Would impl Future for MyType even work for a trait alias?

MajorBreakfast · 2018-07-07T20:07:43Z

futures/src/lib.rs

+    pub use futures_util::try_future::{
+        TryFutureExt,
+        AndThen, ErrInto, FlattenSink, IntoFuture, MapErr, MapOk, OrElse,
+        UnwrapOrElse,


I've put the stuff for TryFuture into the future module (they previously weren't exported)

Currently the TryFutures do not implement the Future trait. But, this is planned to change as soon as it becomes technically possible. Therefore, putting the extensions for both into a single module makes sense to me. What do you think?

MajorBreakfast · 2018-07-08T20:28:44Z

@cramertj In case you're wondering: I moved the futures-util/benches/bilock.rs to futures-util/benches_disabled/bilock.rs in order to disable it. It hasn't been converted to 0.3 yet (e.g. has fn poll_next(&mut self, ... in it)

cramertj · 2018-07-09T20:05:50Z

futures-util/src/future/map.rs

-    unsafe_pinned!(future -> A);
-    unsafe_unpinned!(f -> Option<F>);
+    /// Creates a new Map.
+    pub(super) fn new(future: Fut, op: F) -> Map<Fut, F> {


I'm fine with changing these new functions to pub(super) methods, but we should keep it consistent-- there are a bunch of other places where this pattern is used (most combinators) and those should be changed as well.

I'll definitely do those in another PR. You know that I love consistency 🤓

MajorBreakfast · 2018-07-09T22:36:50Z

🎉

MajorBreakfast added 4 commits July 4, 2018 12:52

Remove no longer relevant FIXME

d4c271c

Fix send all

aa84e98

Test for split

c6362e1

Restructure macros

b6ba956

MajorBreakfast commented Jul 4, 2018

View reviewed changes

MajorBreakfast added 3 commits July 4, 2018 16:41

FutureExt::inspect test

c656939

Scope free functions

0661769

recurse test

df75825

MajorBreakfast commented Jul 4, 2018

View reviewed changes

MajorBreakfast added 3 commits July 4, 2018 17:13

Same features attribute for all the tests

468943b

Remove extern crate futures; from tests

824a29f

Extract contents of await module into submodules

0c74e93

MajorBreakfast force-pushed the more-tests-4 branch from ee0dc8f to 0c74e93 Compare July 4, 2018 15:54

MajorBreakfast added 2 commits July 4, 2018 20:02

Split test support module into submodules

30a17a0

Unfold test

b5c3aa5

MajorBreakfast commented Jul 4, 2018

View reviewed changes

MajorBreakfast added 2 commits July 4, 2018 20:26

Remove unused macro use warning

337b8c0

Rename await module to async_await

05a6b99

MajorBreakfast commented Jul 5, 2018

View reviewed changes

MajorBreakfast added 3 commits July 7, 2018 00:12

Align naming with mpsc::channel() docs

2a78234

Compatibility with new nightly: FutureObj

39c3e47

Make docs build, light refactoring, enable tests in CI

3722454

MajorBreakfast commented Jul 7, 2018

View reviewed changes

Streamline use statements

4f1653f

MajorBreakfast commented Jul 7, 2018

View reviewed changes

MajorBreakfast added 3 commits July 7, 2018 18:54

result() is now called ready()

4e7c924

Tweak macro documentation and document pin macros

dd92921

Ensure all future and stream reexports

cc3c9b0

MajorBreakfast commented Jul 7, 2018

View reviewed changes

Add benchmarks and no_std build to CI

ad744ac

MajorBreakfast force-pushed the more-tests-4 branch from 942b033 to ad744ac Compare July 7, 2018 21:23

Final tweaks to use statement order

9cb2f85

MajorBreakfast mentioned this pull request Jul 9, 2018

Reorganize subcrates for futures 0.3 #1071

Closed

doc(hidden) top-level reexports

b66bfdf

MajorBreakfast force-pushed the more-tests-4 branch from 401d527 to b66bfdf Compare July 9, 2018 18:11

futures_core hidden top level exports

ade4c92

cramertj reviewed Jul 9, 2018

View reviewed changes

MajorBreakfast added 3 commits July 9, 2018 22:32

task::Context

0f82429

Remove Context from top-level exports

a89c0dd

Change to task::Context in docs for oneshot

a5222ba

cramertj merged commit 0dce221 into rust-lang:0.3 Jul 9, 2018

taiki-e mentioned this pull request Sep 1, 2019

Remove top-level re-export of Poll? #1854

Closed

taiki-e mentioned this pull request Apr 25, 2020

Refactor to reduce the amount of unsafe and duplicated code. #2128

Merged

taiki-e mentioned this pull request Aug 12, 2021

doc(hidden) reexports #2472

Closed

		@@ -0,0 +1,7 @@
		// Macros redefined here because macro re-exports are unstable.

More tests 4 #1069

More tests 4 #1069

Conversation

MajorBreakfast commented Jul 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Jul 4, 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

MajorBreakfast Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

MajorBreakfast commented Jul 6, 2018

MajorBreakfast commented Jul 7, 2018

MajorBreakfast Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Jul 9, 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

MajorBreakfast Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

MajorBreakfast Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

MajorBreakfast commented Jul 8, 2018 • edited Loading

cramertj Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Jul 9, 2018

MajorBreakfast Jul 4, 2018 •

edited

Loading

MajorBreakfast Jul 5, 2018 •

edited

Loading

MajorBreakfast Jul 7, 2018 •

edited

Loading

MajorBreakfast Jul 7, 2018 •

edited

Loading

MajorBreakfast Jul 7, 2018 •

edited

Loading

MajorBreakfast Jul 9, 2018 •

edited

Loading

MajorBreakfast Jul 7, 2018 •

edited

Loading

MajorBreakfast Jul 7, 2018 •

edited

Loading

MajorBreakfast commented Jul 8, 2018 •

edited

Loading

cramertj Jul 9, 2018 •

edited

Loading