-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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::task::ready! macro #70817
Add core::task::ready! macro #70817
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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 latest upstream changes (presumably #70826) made this pull request unmergeable. Please resolve the merge conflicts. |
cc @petrochenkov I think this is actually possible, but maybe it requires |
Yes, either |
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 addition to me. I imagine that it would come up a lot when implementing futures by hand. I personally would prefer to see |
The implementation of this macro is eerily similar to that of
Even in Rust 2018, it’s surprisingly involved to export a |
@SimonSapin Was |
We already use Macros 2.0 can use #[rustc_macro_transparency = "semitransparent"]
pub macro ready() {} |
@eddyb I did not attempt to use |
@SimonSapin Oh, that's probably because most of the differences are semantic. Using the shorthand syntax (otherwise it's just pub macro matches($expression:expr, $( $pattern:pat )|+ $( if $guard: expr )?) {
match $expression {
$( $pattern )|+ $( if $guard )? => true,
_ => false
}
} |
@SimonSapin Hmm -- so, we did deliberate for quite some time whether the Now that |
|
Ah, ok, that is indeed the 'multiple possible uses of Try' I was referring to, just applied to the composed |
Thanks for the explanations! I was not aware this had been discussed before. |
…=sfackler Add core::future::{pending,ready} Adds two future constructors to `core`: `future::ready` and `future::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` and `async-std` crates. This implementation has been adapted from the `futures` crate. ## Examples In rust-lang#70817 we propose adding the `ready!` macro. In the example we use an `async fn` which does not return a future that implements `Unpin`, which leads to the use of `unsafe`. Instead had we had `future::ready` available, we could've written the same example without using `unsafe`: ```rust use core::task::{Context, Poll}; use core::future::{self, Future}; use core::pin::Pin; pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> { let mut fut = future::ready(42_u8); let num = ready!(Pin::new(fut).poll(cx)); // ... use num Poll::Ready(()) } ``` ## 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](https://docs.rs/http-service/0.4.0/http_service/trait.HttpService.html#associatedtype.ConnectionFuture)). The final, minor argument is that `future::ready` and `future::pending` form a counterpart to the enum members of `Poll`: `Ready` and `Pending`. These functions form a conceptual bridge between `Poll` and `Future`, and can be used as a useful teaching device. ## References - [`futures::future::ready`](https://docs.rs/futures/0.3.4/futures/future/fn.ready.html) - [`futures::future::pending`](https://docs.rs/futures/0.3.4/futures/future/fn.pending.html) - [`async_std::future::pending`](https://docs.rs/async-std/1.5.0/async_std/future/fn.pending.html) - [`async_std::future::ready`](https://docs.rs/async-std/1.5.0/async_std/future/fn.ready.html)
@yoshuawuyts once you rebase this, i'll get it reviewed |
@Dylan-DPC rebased! |
r? @dtolnay |
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.
Thanks, seems good to me.
I think I would prefer an import path for this macro as core::task::ready
since it is only applicable when used in conjunction with core::task::Context
and core::task::Poll
. Take a look at #72279 for an existing example of a macro not at the crate root; that one is exposed as core::ptr::raw_ref
.
I agree that we should expose this under the task module. |
Updated with @dtolnay's feedback! |
Note that this PR is affected by #74355 which means it won't show up in the docs. Imo that shouldn't block this PR from being merged in nightly, though probably wait for that to be resolved before stabilizing. |
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.
Looks good!
@bors r+ |
📌 Commit 18be370 has been approved by |
…arth Rollup of 7 pull requests Successful merges: - rust-lang#70817 (Add core::task::ready! macro) - rust-lang#73762 (Document the trait keyword) - rust-lang#74021 (impl Index<RangeFrom> for CStr) - rust-lang#74071 (rustc_metadata: Make crate loading fully speculative) - rust-lang#74445 (add test for rust-lang#62878) - rust-lang#74459 (Make unreachable_unchecked a const fn) - rust-lang#74478 (Revert "Use an UTF-8 locale for the linker.") Failed merges: r? @ghost
☔ The latest upstream changes (presumably #74493) made this pull request unmergeable. Please resolve the merge conflicts. |
…=m-ou-se Stabilize core::task::ready! _Tracking issue: https://github.com/rust-lang/rust/issues/70922_ This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed. The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library. r? `@tmandry` cc/ `@rust-lang/wg-async-foundations` `@rust-lang/libs` ## Example ```rust use core::task::{Context, Poll}; use core::future::Future; use core::pin::Pin; async fn get_num() -> usize { 42 } pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> { let mut f = get_num(); let f = unsafe { Pin::new_unchecked(&mut f) }; let num = ready!(f.poll(cx)); // ... use num Poll::Ready(()) } ```
…=m-ou-se Stabilize core::task::ready! _Tracking issue: https://github.com/rust-lang/rust/issues/70922_ This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed. The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library. r? ``@tmandry`` cc/ ``@rust-lang/wg-async-foundations`` ``@rust-lang/libs`` ## Example ```rust use core::task::{Context, Poll}; use core::future::Future; use core::pin::Pin; async fn get_num() -> usize { 42 } pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> { let mut f = get_num(); let f = unsafe { Pin::new_unchecked(&mut f) }; let num = ready!(f.poll(cx)); // ... use num Poll::Ready(()) } ```
This PR adds
ready!
as a top-level macro tolibcore
following the implementation offutures_core::ready
, tracking issue #70922. This macro is commonly used when implementingFuture
,AsyncRead
,AsyncWrite
andStream
. And being only 5 lines, it seems like a useful and straight forward addition to std.Example
Naming
In
async-std
we chose to nest the macro under thetask
module instead of having the macro at the top-level. This is a pattern that currently does not occur in std, mostly due to this not being possible prior to Rust 2018.This PR proposes to add the
ready
macro ascore::ready
. But another option would be to introduce it ascore::task::ready
since it's really only useful when used in conjunction withtask::{Context, Poll}
.Implementation questions
I tried rendering the documentation locally but the macro didn't show up under
core
. I'm not sure if I quite got this right. I used thetodo!
macro PR as a reference, and our approaches look similar.References
futures::ready
async_std::task::ready
futures_core::ready