From e936071fbf22e8fb48e7e473d35b01df453278e2 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 9 Dec 2021 19:27:27 +0100 Subject: [PATCH 1/6] Minor improvements to `future::join!`'s implementation This is a follow-up from #91645, regarding [some remarks I made](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/join!/near/264293660). Mainly: - it hides the recursive munching through a private `macro`, to avoid leaking such details (a corollary is getting rid of the need to use `@` to disambiguate); - it uses a `match` binding, _outside_ the `async move` block, to better match the semantics from function-like syntax; - it pre-pins the future before calling into `poll_fn`, since `poll_fn`, alone, cannot guarantee that its capture does not move; - it uses `.ready()?` since it's such a neat pattern; - it renames `Took` to `Taken` for consistency with `Done`. --- library/core/src/future/join.rs | 150 +++++++++++++++++++++----------- 1 file changed, 97 insertions(+), 53 deletions(-) diff --git a/library/core/src/future/join.rs b/library/core/src/future/join.rs index aadff103ebab4..afab1bab7421a 100644 --- a/library/core/src/future/join.rs +++ b/library/core/src/future/join.rs @@ -45,59 +45,102 @@ use crate::task::{Context, Poll}; /// # }; /// ``` #[unstable(feature = "future_join", issue = "91642")] -pub macro join { - ( $($fut:expr),* $(,)?) => { - join! { @count: (), @futures: {}, @rest: ($($fut,)*) } - }, - // Recurse until we have the position of each future in the tuple +pub macro join( $($fut:expr),+ $(,)? ) { + // Funnel through an internal macro not to leak implementation details. + join_internal! { + current_position[] + futures_and_positions[] + munching[ $($fut)+ ] + } +} + +/// To be able to *name* the i-th future in the tuple (say we want the .4-th), +/// the following trick will be used: `let (_, _, _, _, it, ..) = tuple;` +/// In order to do that, we need to generate a `i`-long repetition of `_`, +/// for each i-th fut. Hence the recursive muncher approach. +macro join_internal { + // Recursion step: map each future with its "position" (underscore count). ( - // A token for each future that has been expanded: "_ _ _" - @count: ($($count:tt)*), - // Futures and their positions in the tuple: "{ a => (_), b => (_ _)) }" - @futures: { $($fut:tt)* }, - // Take a future from @rest to expand - @rest: ($current:expr, $($rest:tt)*) - ) => { - join! { - @count: ($($count)* _), - @futures: { $($fut)* $current => ($($count)*), }, - @rest: ($($rest)*) + // Accumulate a token for each future that has been expanded: "_ _ _". + current_position[ + $($underscores:tt)* + ] + // Accumulate Futures and their positions in the tuple: `_0th () _1st ( _ ) …`. + futures_and_positions[ + $($acc:tt)* + ] + // Munch one future. + munching[ + $current:tt + $($rest:tt)* + ] + ) => ( + join_internal! { + current_position[ + $($underscores)* + _ + ] + futures_and_positions[ + $($acc)* + $current ( $($underscores)* ) + ] + munching[ + $($rest)* + ] } - }, - // Now generate the output future - ( - @count: ($($count:tt)*), - @futures: { - $( $(@$f:tt)? $fut:expr => ( $($pos:tt)* ), )* - }, - @rest: () - ) => { - async move { - let mut futures = ( $( MaybeDone::Future($fut), )* ); + ), + // End of recursion: generate the output future. + ( + current_position $_:tt + futures_and_positions[ + $( + $fut_expr:tt ( $($pos:tt)* ) + )* + ] + // Nothing left to munch. + munching[] + ) => ( + match ( $( MaybeDone::Future($fut_expr), )* ) { futures => async { + let mut futures = futures; + // SAFETY: this is `pin_mut!`. + let futures = unsafe { Pin::new_unchecked(&mut futures) }; poll_fn(move |cx| { let mut done = true; - + // For each `fut`, pin-project to it, and poll it. $( - let ( $($pos,)* fut, .. ) = &mut futures; - - // SAFETY: The futures are never moved - done &= unsafe { Pin::new_unchecked(fut).poll(cx).is_ready() }; + // SAFETY: pinning projection + let fut = unsafe { + futures.as_mut().map_unchecked_mut(|it| { + let ( $($pos,)* fut, .. ) = it; + fut + }) + }; + // Despite how tempting it may be to `let () = fut.poll(cx).ready()?;` + // doing so would defeat the point of `join!`: to start polling eagerly all + // of the futures, to allow parallelizing the waits. + done &= fut.poll(cx).is_ready(); )* - - if done { - // Extract all the outputs - Poll::Ready(($({ - let ( $($pos,)* fut, .. ) = &mut futures; - - fut.take_output().unwrap() - }),*)) - } else { - Poll::Pending + if !done { + return Poll::Pending; } + // All ready; time to extract all the outputs. + + // SAFETY: `.take_output()` does not break the `Pin` invariants for that `fut`. + let futures = unsafe { + futures.as_mut().get_unchecked_mut() + }; + Poll::Ready( + ($( + { + let ( $($pos,)* fut, .. ) = &mut *futures; + fut.take_output().unwrap() + } + ),*) // <- no trailing comma since we don't want 1-tuples. + ) }).await - } - } + }} + ), } /// Future used by `join!` that stores it's output to @@ -109,14 +152,14 @@ pub macro join { pub enum MaybeDone { Future(F), Done(F::Output), - Took, + Taken, } #[unstable(feature = "future_join", issue = "91642")] impl MaybeDone { pub fn take_output(&mut self) -> Option { - match &*self { - MaybeDone::Done(_) => match mem::replace(self, Self::Took) { + match *self { + MaybeDone::Done(_) => match mem::replace(self, Self::Taken) { MaybeDone::Done(val) => Some(val), _ => unreachable!(), }, @@ -132,13 +175,14 @@ impl Future for MaybeDone { fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { // SAFETY: pinning in structural for `f` unsafe { - match self.as_mut().get_unchecked_mut() { - MaybeDone::Future(f) => match Pin::new_unchecked(f).poll(cx) { - Poll::Ready(val) => self.set(Self::Done(val)), - Poll::Pending => return Poll::Pending, - }, + // Do not mix match ergonomics with unsafe. + match *self.as_mut().get_unchecked_mut() { + MaybeDone::Future(ref mut f) => { + let val = Pin::new_unchecked(f).poll(cx).ready()?; + self.set(Self::Done(val)); + } MaybeDone::Done(_) => {} - MaybeDone::Took => unreachable!(), + MaybeDone::Taken => unreachable!(), } } From 846cb9c583712928e66658486a6066b60b6197a4 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 9 Dec 2021 19:50:22 +0100 Subject: [PATCH 2/6] Fix two false positive lints --- library/core/src/future/join.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/core/src/future/join.rs b/library/core/src/future/join.rs index afab1bab7421a..20336bc9e4c69 100644 --- a/library/core/src/future/join.rs +++ b/library/core/src/future/join.rs @@ -1,4 +1,4 @@ -#![allow(unused_imports)] // items are used by the macro +#![allow(unused_imports, unused_macros)] // items are used by the macro use crate::cell::UnsafeCell; use crate::future::{poll_fn, Future}; @@ -54,6 +54,8 @@ pub macro join( $($fut:expr),+ $(,)? ) { } } +// FIXME(danielhenrymantilla): a private macro should need no stability guarantee. +#[unstable(feature = "future_join", issue = "91642")] /// To be able to *name* the i-th future in the tuple (say we want the .4-th), /// the following trick will be used: `let (_, _, _, _, it, ..) = tuple;` /// In order to do that, we need to generate a `i`-long repetition of `_`, From 07bcf4aad3774e8b796a0a2b0141784ede3eb88a Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 9 Dec 2021 20:12:01 +0100 Subject: [PATCH 3/6] Bring back the colon separators for the macro munching. Co-Authored-By: Ibraheem Ahmed --- library/core/src/future/join.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/library/core/src/future/join.rs b/library/core/src/future/join.rs index 20336bc9e4c69..2d27b8edfd800 100644 --- a/library/core/src/future/join.rs +++ b/library/core/src/future/join.rs @@ -48,9 +48,9 @@ use crate::task::{Context, Poll}; pub macro join( $($fut:expr),+ $(,)? ) { // Funnel through an internal macro not to leak implementation details. join_internal! { - current_position[] - futures_and_positions[] - munching[ $($fut)+ ] + current_position: [] + futures_and_positions: [] + munching: [ $($fut)+ ] } } @@ -64,29 +64,29 @@ macro join_internal { // Recursion step: map each future with its "position" (underscore count). ( // Accumulate a token for each future that has been expanded: "_ _ _". - current_position[ + current_position: [ $($underscores:tt)* ] // Accumulate Futures and their positions in the tuple: `_0th () _1st ( _ ) …`. - futures_and_positions[ + futures_and_positions: [ $($acc:tt)* ] // Munch one future. - munching[ + munching: [ $current:tt $($rest:tt)* ] ) => ( join_internal! { - current_position[ + current_position: [ $($underscores)* _ ] - futures_and_positions[ + futures_and_positions: [ $($acc)* $current ( $($underscores)* ) ] - munching[ + munching: [ $($rest)* ] } @@ -94,14 +94,14 @@ macro join_internal { // End of recursion: generate the output future. ( - current_position $_:tt - futures_and_positions[ + current_position: $_:tt + futures_and_positions: [ $( $fut_expr:tt ( $($pos:tt)* ) )* ] // Nothing left to munch. - munching[] + munching: [] ) => ( match ( $( MaybeDone::Future($fut_expr), )* ) { futures => async { let mut futures = futures; From e277a987585f7e841b2b2c38f9f90fe5defda828 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 9 Dec 2021 21:21:37 +0100 Subject: [PATCH 4/6] Fix missing `mut` typo Co-authored-by: Ibraheem Ahmed --- library/core/src/future/join.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/future/join.rs b/library/core/src/future/join.rs index 2d27b8edfd800..a6ffbe07d91b0 100644 --- a/library/core/src/future/join.rs +++ b/library/core/src/future/join.rs @@ -106,7 +106,7 @@ macro join_internal { match ( $( MaybeDone::Future($fut_expr), )* ) { futures => async { let mut futures = futures; // SAFETY: this is `pin_mut!`. - let futures = unsafe { Pin::new_unchecked(&mut futures) }; + let mut futures = unsafe { Pin::new_unchecked(&mut futures) }; poll_fn(move |cx| { let mut done = true; // For each `fut`, pin-project to it, and poll it. From f8dc13db43d61bba73f68cfcccfe4771864f6d78 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 9 Dec 2021 22:19:57 +0100 Subject: [PATCH 5/6] Add tests asserting the function-like semantics of `join!()` --- library/core/tests/future.rs | 34 ++++++++++++++++++++++++++++++++++ library/core/tests/lib.rs | 1 + 2 files changed, 35 insertions(+) diff --git a/library/core/tests/future.rs b/library/core/tests/future.rs index 73249b1b8a435..8f4a991ed2d23 100644 --- a/library/core/tests/future.rs +++ b/library/core/tests/future.rs @@ -64,6 +64,40 @@ fn test_join() { }); } +/// Tests that `join!(…)` behaves "like a function": evaluating its arguments +/// before applying any of its own logic. +/// +/// _e.g._, `join!(async_fn(&borrowed), …)` does not consume `borrowed`; +/// and `join!(opt_fut?, …)` does let that `?` refer to the callsite scope. +mod test_join_function_like_value_arg_semantics { + use super::*; + + async fn async_fn(_: impl Sized) {} + + // no need to _run_ this test, just to compile it. + fn _join_does_not_unnecessarily_move_mentioned_bindings() { + let not_copy = vec![()]; + let _ = join!(async_fn(¬_copy)); // should not move `not_copy` + let _ = not_copy; // OK + } + + #[test] + fn join_lets_control_flow_effects_such_as_try_flow_through() { + let maybe_fut = None; + if false { + *&mut { maybe_fut } = Some(async {}); + loop {} + } + assert!(Option::is_none(&try { join!(maybe_fut?, async { unreachable!() }) })); + } + + #[test] + fn join_is_able_to_handle_temporaries() { + let _ = join!(async_fn(&String::from("temporary"))); + let () = block_on(join!(async_fn(&String::from("temporary")))); + } +} + fn block_on(fut: impl Future) { struct Waker; impl Wake for Waker { diff --git a/library/core/tests/lib.rs b/library/core/tests/lib.rs index 73a3a1fc3e0af..012e6e5b57ad0 100644 --- a/library/core/tests/lib.rs +++ b/library/core/tests/lib.rs @@ -49,6 +49,7 @@ #![feature(str_internals)] #![feature(test)] #![feature(trusted_len)] +#![feature(try_blocks)] #![feature(try_trait_v2)] #![feature(slice_internals)] #![feature(slice_partition_dedup)] From 67ab53daee2fd42a8ae643225acfc7c7c074e0f7 Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Fri, 10 Dec 2021 05:07:52 -0800 Subject: [PATCH 6/6] Update library/core/tests/future.rs Co-authored-by: Daniel Henry-Mantilla --- library/core/tests/future.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/tests/future.rs b/library/core/tests/future.rs index 8f4a991ed2d23..e8b83b5cbc21e 100644 --- a/library/core/tests/future.rs +++ b/library/core/tests/future.rs @@ -78,7 +78,7 @@ mod test_join_function_like_value_arg_semantics { fn _join_does_not_unnecessarily_move_mentioned_bindings() { let not_copy = vec![()]; let _ = join!(async_fn(¬_copy)); // should not move `not_copy` - let _ = not_copy; // OK + let _ = ¬_copy; // OK } #[test]