-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add core::future::{pending,ready} #70834
Conversation
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 |
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw variance is pretty easy to test for if you keep it simple:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fn() -> T
is probably what we want here, based on the comment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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! |
Updated the patch with @taiki-e's suggestion. |
Resolved the final discussion thread on this PR. Re-tagging @rust-lang/libs for review. |
👍 on adding these! |
@bors r+ |
📌 Commit 029515d has been approved by |
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
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)
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)
…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.
…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.
Adds two future constructors to
core
:future::ready
andfuture::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
futures
andasync-std
crates. This implementation has been adapted from thefutures
crate.Examples
In #70817 we propose adding the
ready!
macro. In the example we use anasync fn
which does not return a future that implementsUnpin
, which leads to the use ofunsafe
. Instead had we hadfuture::ready
available, we could've written the same example without usingunsafe
:Why future::ready?
Arguably
future::ready
andasync {}
can be considered equivalent. The main differences are thatfuture::ready
returns 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::ready
andfuture::pending
form a counterpart to the enum members ofPoll
:Ready
andPending
. These functions form a conceptual bridge betweenPoll
andFuture
, and can be used as a useful teaching device.References
futures::future::ready
futures::future::pending
async_std::future::pending
async_std::future::ready