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.

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

Stabilize core::future::{pending,ready} #74328

Merged
merged 1 commit into from
Sep 12, 2020
Merged

Stabilize core::future::{pending,ready} #74328

merged 1 commit into from
Sep 12, 2020

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Jul 14, 2020

This PR stabilizes core::future::{pending,ready}, tracking issue #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 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 #70834. So all this PR does is remove the feature gate.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2020
@jonas-schievink jonas-schievink added A-async-await Area: Async & Await relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 14, 2020
@jonas-schievink jonas-schievink added this to the 1.46 milestone Jul 14, 2020
@Mark-Simulacrum
Copy link
Member

with the async-std impls probably diverging the most (using async fn, which in hindsight was a mistake).

Can you elaborate on this? Is there some issue that we could reference?

@taiki-e
Copy link
Member

taiki-e commented Jul 14, 2020

have lived as part of the futures ecosystem for several years

Note that most of these periods are before the async-await stabilizes.
Now that async-await is stable, most of the use of ready can actually be replaced with async blocks.
In some cases, it's more convenient than async blocks, but I don't think it's as common as it used to be.

@yoshuawuyts
Copy link
Member Author

with the async-std impls probably diverging the most (using async fn, which in hindsight was a mistake).

Can you elaborate on this? Is there some issue that we could reference?

@Mark-Simulacrum I guess I hinted at this in the text of my original PR but didn't cover it explicitly (#70834):

Why future::ready?

Arguably future::ready and async {} can be considered equivalent. The main differences are that future::ready returns a future that implements Unpin, 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 future returned from an async fn or async {} cannot be stored inside structs without boxing. This is not very common, but has come up before when trying to implement a no-op for traits that expect associated futures (example trait, workaround). Prefering manual futures over async fn in the stdlib feel similar to how the stdlib prefers named generics over impl trait because it's more widely applicable (e.g. enables the use of Turbofish) [1].


Now that async-await is stable, most of the use of ready can actually be replaced with async blocks. In some cases, it's more convenient than async blocks, but I don't think it's as common as it used to be.

@taiki-e Yeah I agree; future::ready is definitely mostly a helper function. It's useful when needing to store futures inside structs, but most predominantly when writing examples. Especially as a counterpart to future::pending, because while std::future::ready may be replaced by async {} in many cases, there is no replacement for future::pending.

Together they make a good pairing to convey the Future state inside examples, as for example in async_std::future::race:

use async_std::future;

let a = future::pending();
let b = future::ready(1u8);

assert_eq!(a.race(b).await, 1u8);

This example wouldn't be possible without some form of future::pending. And future::ready forms a natural counterpart to it that clearly conveys the state of the future as part of the name.


[1]: I'd be happy to elaborate more on this if desired. It may be useful to establish guidelines about async fn vs manual future impls in the stdlib at some point.

@withoutboats
Copy link
Contributor

@rfcbot fcp merge

These two functions return, respectively, a future that is ready and a future that is pending. The ready function takes an argument, which will be returned.

The pending version would have to be implemented basically as it is.

The ready version is importantly distinct from the async identity function because it implements Unpin and has a named future that can be stored in a struct.

@rfcbot
Copy link

rfcbot commented Jul 15, 2020

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 15, 2020
@rfcbot
Copy link

rfcbot commented Jul 17, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 17, 2020
@taiki-e
Copy link
Member

taiki-e commented Jul 18, 2020

@yoshuawuyts
Could you add a description about the difference between ready and async identity function to ready docs?
(They were also described in the PR that implemented this feature, but they were not documented, so the same question was repeated in this PR.)

most predominantly when writing examples

This depends on the content of the example. async block is more powerful, flexible, and allows delayed execution, so I'd basically recommend using async block unless you need Unpin or a named future.

@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.46, 1.47 Jul 18, 2020
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jul 27, 2020
@rfcbot
Copy link

rfcbot commented Jul 27, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 27, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@yoshuawuyts
Copy link
Member Author

Rebased to address the merge conflict and addressed @taiki-e's final comments.

@Nemo157
Copy link
Member

Nemo157 commented Aug 17, 2020

impl<T> Debug for Pending<T> where
    T: Debug, 

T: Debug seems unnecessary here, this should probably have an explicit impl instead of a derive.

@yoshuawuyts
Copy link
Member Author

@Nemo157 I'm not sure I follow? #[derive(Debug)] has the same behavior as impl<T: Debug> for Pending<T> (playground)?

@sfackler
Copy link
Member

@Nemo157's saying that the impl can just be impl<T> Debug for Pending<T> since there isn't actually a T value to print.

@yoshuawuyts
Copy link
Member Author

ah yes that makes sense! Updated with @Nemo157's feedback!

@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.47, 1.48 Aug 24, 2020
@sfackler
Copy link
Member

Needs an ./x.py fmt, but r=me after tidy passes.

@yoshuawuyts
Copy link
Member Author

@sfackler build passed!

@Dylan-DPC-zz
Copy link

@bors r=sfackler

@bors
Copy link
Contributor

bors commented Sep 11, 2020

📌 Commit 688f447 has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2020
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
Copy link
Contributor

bors commented Sep 12, 2020

⌛ Testing commit 688f447 with merge 94a7ea2...

@bors
Copy link
Contributor

bors commented Sep 12, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: sfackler
Pushing 94a7ea2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2020
@bors bors merged commit 94a7ea2 into rust-lang:master Sep 12, 2020
@yoshuawuyts yoshuawuyts deleted the stabilize-future-readiness-fns branch September 14, 2020 15:18
@yoshuawuyts
Copy link
Member Author

@Dylan-DPC is the 1.47 feature flag here correct? Shouldn't this be 1.48 since the first beta has already been cut?

@Dylan-DPC-zz
Copy link

Ah ye. Can you submit a new PR with that small change? thanks

tmandry added a commit to tmandry/rust that referenced this pull request Sep 16, 2020
…y-stabilization-label, r=Dylan-DPC

Fix stabilization marker for future_readiness_fns

Updated the rustc version in which this will be stabilized from `1.47.0 -> 1.48.0`. Fixes rust-lang#74328 (comment). Ref rust-lang#70921.

r? @Dylan-DPC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.