-
Notifications
You must be signed in to change notification settings - Fork 627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
futures-util: Update Future combinators to preserve Clone #2742
base: master
Are you sure you want to change the base?
futures-util: Update Future combinators to preserve Clone #2742
Conversation
5e2f791
to
9f35f08
Compare
9f35f08
to
9ecc6b8
Compare
#2743 is a separate fix for the missing |
Fixes this error on ToT: ``` error: type does not implement `Debug`; consider adding `#[derive(Debug)]` or a manual implementation --> futures-util/src/compat/compat01as03.rs:325:1 | 325 | struct WakerToHandle<'a>(&'a task03::Waker); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D missing-debug-implementations` implied by `-D warnings` ``` Tested: - `cargo fmt`. - `cargo doc`. - `cargo test`.
This updates most `Future` combinators to preserve `Clone` when available on the input `Future`. For motivation, imagine you have some complicated `Future` that is not `Clone` and requires `.shared()` to properly share it. Then imagine you have a library function that is meant to bundle together a bunch of combinators to fulfill some semantic purpose. That library funciton will have to call `.shared()` if it wants to try to guarantee the return `Future` is `Clone`, but this might be suboptimal if the input `Future` was already `Clone`, plus it has the ability to obfuscate and hide the `.shared()` allocation. With this change, you can instead require `Future + Clone` on the input `Future` and have a guarantee the output will be `Clone` as well. The hold-out `Future` implementations are: - `Remote` / `RemoteHandle` due to their use of `futures_channel::oneshot::{Sender, Receiver}`. This seems like it is by design that these cannot be `Clone`. - `JoinAll` / `TryJoinAll` due to their use of `Stream` combinators, but also that it seems unlikely that people would expect them to offer `Clone` since they are used to performa a potentially costly sync barrier that would probably be desired to happen only once. For the hold-outs, the existing pattern of using `.shared()` allows for `Clone`, and follows the intended semantics of those combinators. Some combinators that might not make the most sense to preserve `Clone`: - `IntoStream` - `TryFlattenStream` If these changes make sense, I think it would also make sense to apply them to `Stream` combinators as well (although I don't see myself utilizing this property as much with them). If that is the case, these `Future` -> `Stream` combinators make sense to preserve `Clone`. Tested: - `cargo doc`. - `cargo fmt`. - `cargo test --all-features`.
9ecc6b8
to
ceee10f
Compare
@taiki-e Any thoughts on this? This doesn't seem to impose any oddities for me when using the However, with the ability to easily poll with things like |
Thanks for the PR. Which future combinator is actually needed for your use case? |
|
This updates most
Future
combinators to preserveClone
when available on the baseFuture
.For motivation, imagine you have some complicated
Future
that is notClone
and requires.shared()
to properly share it. Then imagine you have a library function that is meant to bundle together a bunch of combinators to fulfill some semantic purpose. That library function will have to call.shared()
if it wants to try to guarantee the returnFuture
isClone
, but this might be suboptimal if the inputFuture
was alreadyClone
, plus it obfuscates the.shared()
allocation. With this change, you can instead requireFuture + Clone
on the inputFuture
and have a guarantee the output will beClone
as well.The hold-out
Future
implementations are:Remote
/RemoteHandle
due to their use offutures_channel::oneshot::{Sender, Receiver}
. This seems like it is by design that these cannot beClone
.JoinAll
/TryJoinAll
due to their use ofStream
combinators, but also that it seems unlikely that people would expect them to offerClone
since they are used to perform a potentially costly sync barrier that would probably be desired to happen only once.For the hold-outs, the existing pattern of using
.shared()
allows forClone
, and follows the intended semantics of those combinators.Some combinators that might not make the most sense to preserve
Clone
:IntoStream
TryFlattenStream
If these changes make sense, I think it would also make sense to apply them to
Stream
combinators as well (although I don't see myself utilizing this property as much with them). If that is the case, theseFuture
->Stream
combinators make sense to preserveClone
.Tested:
cargo doc
.cargo fmt
.cargo test --all-features
.