Add core::future::{pending,ready} #70834
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matklad
reviewed
Apr 6, 2020
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
2 tasks
|
Do the (I personally find it a bit confusing when looking for things to stumble into many of these types related to iterator for example. These were implemented before the time Note: I'm not a reviewer. Just a user. |
Yes, else we cannot guarantee that they're |
JohnTitor
added
the
S-waiting-on-review
eddyb
reviewed
Apr 12, 2020
src/libcore/future/pending.rs
Outdated
| pub struct Pending<T> { | ||
| // We only ever produce T. By marking we don't store it we can bypass | ||
| // dropck and be covariant. | ||
| _data: marker::PhantomData<dyn Fn() -> T>, |
There was a problem hiding this comment.
Isn't dyn Fn() -> T invariant over T because it's an associated type constraint?
We don't do this anywhere else AFAICT, there's a PhantomData<fn() -> T> in core::mem::Discriminant but no PhantomData<Fn or PhantomData<dyn Fn in the codebase.
There was a problem hiding this comment.
Btw variance is pretty easy to test for if you keep it simple:
There was a problem hiding this comment.
futures::future::Pending<T> uses PhantomData<T> so it's covariant (but also interacts with dropck).
async-std uses pub async fn pending<T>() -> T which means it's necessarily invariant (although the private struct Pending<T>, that it relies on, looks identical to the futures one).
Given that a value of T never exists at any point and the parameter only exists for trait dispatch (I don't know what "We only ever produce T." means in the comment), I'd argue that T could be bivariant here, but that's not an option nowadays (cc @nikomatsakis this may be an interesting edge case).
There was a problem hiding this comment.
This comment was added based on @matklad's review in #70834 (comment). I don't think I have enough understanding of variance to participate in this discussion. Is there a change we should make here to enable this PR to move forward?
There was a problem hiding this comment.
I think fn() -> T is probably what we want here, based on the comment anyway.
There was a problem hiding this comment.
@yoshuawuyts Ah, @matklad's comment is entirely correct, but note that it said fn() -> T, not dyn Fn() -> T. If I had to guess, maybe fn was typo'd as Fn and then the compiler asked for dyn to be added for the new, more explicit, trait object syntax.
Like @nikomatsakis said, dyn Fn needs to be replaced with fn.
PhantomData<fn() -> T> and PhantomData<T> have the same ("normal" aka "co-") variance, it's just that the former is more relaxed in terms of reasoning about drops (whereas the latter would be needed if you had, say, an owning raw pointer to a heap-allocated T value, like Box<T> or even Rc<T>).
PhantomData<dyn Fn() -> T>, OTOH, being a trait object, is necessarily (because we know nothing of the concrete type, which may have arbitrary restrictions) invariant over T .
That makes it similar to PhantomData<Cell<T>> and PhantomData<&'a mut T>.
If Pending<T> was invariant over T, then Pending<&'static ()> would not be able to become Pending<&'a ()> for some 'a smaller than 'static.
There was a problem hiding this comment.
That said, we just saw in the context of Discriminant<T> that we may wish we had been invariant here in the future. Perhaps we should leave it as is. (For that matter, I suspect that PhantomData<T> would suffice, even if it's more conservative with respect to dropck.) Seems like best practices here are not entirely obvious.
There was a problem hiding this comment.
I suspect that
PhantomData<T>would suffice (...) best practices here are not entirely obvious.
Alright; moved the PR to use PhantomData<T> since that also happens to be the simplest impl. All of this is internal anyway and doesn't risk having accidental soundness implications; it can always be changed later. I think at the very least this should be good enough to release on nightly. Thanks all!
nikomatsakis
added
the
T-libs-api
|
I'm tagging this for @rust-lang/libs, since that seems like the most appropriate team to make decisions here, but I think we should also have some discussion about the best way to manage "async-related conveniences" (not on this PR, I'll fire up a discussion on Zulip under #wg-async-foundations perhaps). In any case, this seems like a pretty logical set of primitives to include to me! |
taiki-e
reviewed
Apr 13, 2020
|
Updated the patch with @taiki-e's suggestion. |
|
Resolved the final discussion thread on this PR. Re-tagging @rust-lang/libs for review. |
sfackler
reviewed
May 8, 2020
sfackler
reviewed
May 8, 2020
|
👍 on adding these! |
|
@bors r+ |
|
📌 Commit 029515d has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 9, 2020
Rollup of 8 pull requests Successful merges: - rust-lang#70834 (Add core::future::{pending,ready}) - rust-lang#71839 (Make BTreeMap::new and BTreeSet::new const) - rust-lang#71890 (Simplify the error Registry methods a little) - rust-lang#71942 (Shrink `LocalDecl`) - rust-lang#71947 (Dead-code pass highlights too much of impl functions) - rust-lang#71981 (Fix `strip-priv-imports` pass name in the rustdoc documentation) - rust-lang#72018 (Fix canonicalization links) - rust-lang#72031 (Better documentation for io::Read::read() return value) Failed merges: r? @ghost
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 10, 2020
Add core::future::{poll_fn, PollFn}
This is a sibling PR to rust-lang#70834, adding `future::poll_fn`. This is a small helper function that helps bridge the gap between "poll state machines" and "async/await". It was first introduced in [futures@0.1.7](https://docs.rs/futures/0.1.7/futures/future/fn.poll_fn.html) in December of 2016, and has been tried and tested as part of the ecosystem for the past 3.5 years.
## Implementation
Much of the same reasoning from rust-lang#70834 applies: by returning a concrete struct rather than an `async fn` we get to mark the future as `Unpin`. It also becomes named which allows storing it in structs without boxing. This implementation has been modified from the implementation in `futures-rs`.
## References
- [`futures::future::poll_fn`](https://docs.rs/futures/0.3.5/futures/future/fn.poll_fn.html)
- [`async_std::future::poll_fn`](https://docs.rs/async-std/1.5.0/async_std/future/fn.poll_fn.html)
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 11, 2020
Add core::future::{poll_fn, PollFn}
This is a sibling PR to rust-lang#70834, adding `future::poll_fn`. This is a small helper function that helps bridge the gap between "poll state machines" and "async/await". It was first introduced in [futures@0.1.7](https://docs.rs/futures/0.1.7/futures/future/fn.poll_fn.html) in December of 2016, and has been tried and tested as part of the ecosystem for the past 3.5 years.
## Implementation
Much of the same reasoning from rust-lang#70834 applies: by returning a concrete struct rather than an `async fn` we get to mark the future as `Unpin`. It also becomes named which allows storing it in structs without boxing. This implementation has been modified from the implementation in `futures-rs`.
## References
- [`futures::future::poll_fn`](https://docs.rs/futures/0.3.5/futures/future/fn.poll_fn.html)
- [`async_std::future::poll_fn`](https://docs.rs/async-std/1.5.0/async_std/future/fn.poll_fn.html)
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Sep 12, 2020
…ess-fns, r=sfackler
Stabilize core::future::{pending,ready}
This PR stabilizes `core::future::{pending,ready}`, tracking issue rust-lang#70921.
## Motivation
These functions have been on nightly for three months now, and have lived as part of the futures ecosystem for several years. In that time these functions have undergone several iterations, with [the `async-std` impls](https://docs.rs/async-std/1.6.2/async_std/future/index.html) probably diverging the most (using `async fn`, which in hindsight was a mistake).
It seems the space around these functions has been _thoroughly_ explored over the last couple of years, and the ecosystem has settled on the current shape of the functions. It seems highly unlikely we'd want to make any further changes to these functions, so I propose we stabilize.
## Implementation notes
This stabilization PR was fairly straightforward; this feature has already thoroughly been reviewed by the libs team already in rust-lang#70834. So all this PR does is remove the feature gate.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 12, 2020
…s-fns, r=sfackler
Stabilize core::future::{pending,ready}
This PR stabilizes `core::future::{pending,ready}`, tracking issue rust-lang#70921.
## Motivation
These functions have been on nightly for three months now, and have lived as part of the futures ecosystem for several years. In that time these functions have undergone several iterations, with [the `async-std` impls](https://docs.rs/async-std/1.6.2/async_std/future/index.html) probably diverging the most (using `async fn`, which in hindsight was a mistake).
It seems the space around these functions has been _thoroughly_ explored over the last couple of years, and the ecosystem has settled on the current shape of the functions. It seems highly unlikely we'd want to make any further changes to these functions, so I propose we stabilize.
## Implementation notes
This stabilization PR was fairly straightforward; this feature has already thoroughly been reviewed by the libs team already in rust-lang#70834. So all this PR does is remove the feature gate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds two future constructors to
core:future::readyandfuture::pending. These functions enable constructing futures of any type that either immediately resolve, or never resolve which is an incredible useful tool when writing documentation.These functions have prior art in both the
futuresandasync-stdcrates. This implementation has been adapted from thefuturescrate.Examples
In #70817 we propose adding the
ready!macro. In the example we use anasync fnwhich does not return a future that implementsUnpin, which leads to the use ofunsafe. Instead had we hadfuture::readyavailable, we could've written the same example without usingunsafe:Why future::ready?
Arguably
future::readyandasync {}can be considered equivalent. The main differences are thatfuture::readyreturns a future that implementsUnpin, and the returned future is a concrete type. This is useful for traits that require a future as an associated type that can sometimes be a no-op (example).The final, minor argument is that
future::readyandfuture::pendingform a counterpart to the enum members ofPoll:ReadyandPending. These functions form a conceptual bridge betweenPollandFuture, and can be used as a useful teaching device.References
futures::future::readyfutures::future::pendingasync_std::future::pendingasync_std::future::ready