Skip to content
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.

Sign up for GitHub

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

Jump to bottom

futures-util: Update Future combinators to preserve Clone #2742

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

futures-util: Update Future combinators to preserve Clone #2742

wants to merge 2 commits into from

Commits on May 9, 2023

  1. futures-util: Allow missing_debug_implementations on WakerToHandle 

    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`.
    @zjijz
    zjijz committed May 9, 2023
    Configuration menu
    Copy the full SHA
    ce0e7c9 View commit details
    Browse the repository at this point in the history

  2. Update Future combinators to preserve Clone 

    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`.
    @zjijz
    zjijz committed May 9, 2023
    Configuration menu
    Copy the full SHA
    ceee10f View commit details
    Browse the repository at this point in the history