Stabilize core::task::if_ready! by yoshuawuyts · Pull Request #81050 · rust-lang...

Copy link

Member

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(())
}

jyn514 commented on Jan 15

Looking at the implementation of the macro, I would expect to use ? instead, since that makes it more clear that it short-circuits and returns if the task isn't ready. Have you considered implementing Try for Poll?

cc @scottmcm since I know you've been thinking a lot about Try recently

yoshuawuyts commented on Jan 15 •

@jyn514 Poll already implements Try if the inner type is an Option, Result, or Result of Option. I forget the reason, but I believe this may simply not be possible to implement on any type T without specialization.

Actually that makes for an interesting proposition. When I introduced this API early last year specialization seemed unstable enough that I didn't feel comfortable to even suggest it. However now.. perhaps? I haven't kept up with the progress of min_specialization enough to know whether it's feasible. If we could just make this work with ? that may indeed be preferable.

scottmcm commented on Jan 16

@jyn514 For the Poll impls, @cramertj knows more than I do.

sfackler commented on Jan 16

I'd be pretty hesitant to make ? behave wildly differently for different variants of Polls. I think we're just stuck with the fact that ? on Poll maps through to the inner value.

yoshuawuyts commented on Jan 16

I'd be pretty hesitant to make ? behave wildly differently for different variants of Polls. I think we're just stuck with the fact that ? on Poll maps through to the inner value.

That's fair; in which case I think we should proceed with stabilizing task::ready! as per this PR.

KodrAus commented on Jan 22

@rfcbot fcp merge

This stabilizes the task::ready! macro, which is used in manual futures implementations to early-return on Poll::Pending.

rfcbot commented on Jan 22 •

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

Concerns:

• naming (#81050 (comment))

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.

m-ou-se commented on Jan 22

Poll already implements Try if the inner type is an Option, Result, or Result of Option.

? on Poll types like Poll<Option<Result>> working on the innermost None/Err instead of the outermost Pending was stabilized as part of the futures_api, but was never really discussed much separately in the context of stabilization (or FCP'd separately). This behaviour was called out in one comment on the futures_api stabilization PR, but that comment seems to have been ignored. :(

KodrAus commented on Jan 22

Do we expect manual futures implementations to continue to become more niche as async becomes available in more contexts?

yoshuawuyts commented on Jan 22

Do we expect manual futures implementations to continue to become more niche as async becomes available in more contexts?

To some degree. For example async gen fn may replace manual Stream impls just like gen fn might for Iter, but for industrial-grade libraries it seems likely manual implementations will continue to be used. Returning anonymous futures/streams are not nearly as flexible as named impls.

Also for some async traits like AsyncRead and AsyncWrite there is no clear path yet how language level features could obsolete the need to implement these by hand.

I think the usage of ready! will likely roughly remain where it is now. Arguably it's already a bit of a niche: understanding how to implement futures takes a fair bit of learning. I suspect more language features will not so much take away from existing implementations, as make async Rust accessible to a greater number of people.

tmandry commented on Feb 2

My concern with stabilizing this (and with the futures API it's based on) is that std::task::ready! is easily confused with std::future::ready, a function with very different semantics.

I remember thinking of other names in the past, like try_poll!. But I didn't feel like opening a bikeshed back then. So I guess I'm doing that now

yoshuawuyts commented on Feb 3

@tmandry are you able to file a concern about naming? We probably should ensure we don't merge this until that's resolved.

I don't feel strongly about either ready! or try_poll!. The reasoning in favor of try_poll! makes sense to me and I wouldn't be opposed to going with that instead.

taiki-e commented on Feb 3

@tmandry

I remember thinking of other names in the past, like try_poll!. But I didn't feel like opening a bikeshed back then. So I guess I'm doing that now

Personally, I don't think the name try_poll is easy-to-understand. AFAIK, many (most?) of the use of the term "try" in the APIs of the standard library (and futures) relates to results, options, or Try traits. I think it's confusing to add another try_* that behaves wildly differently from the Try implementation of Poll.

FWIW, In the past futures had a macro called try_poll that did something wildly different. rust-lang/futures-rs#1698. (There was also a macro called try_ready.)

tmandry commented on Feb 5

@tmandry are you able to file a concern about naming?

Not sure, but I can try...

@rfcbot concern naming

tmandry commented on Feb 5

Another idea I remember is poll_await!, since you're basically awaiting a future inside of a poll implementation.

m-ou-se commented on Feb 5

@rfcbot concern naming

taiki-e commented on Feb 13

Another idea I remember is poll_await!, since you're basically awaiting a future inside of a poll implementation.

I'm not sure it's preferable to use names that include await.
Unlike .await, ready returns early rather than suspending.
I know that some people are already writing broken code using ready like .await (i.e., the code that misunderstands that the state when ready is called is maintained in the next poll* call), but names that include await may be more misleading.

m-ou-se commented 16 days ago

@rfcbot merge

rfcbot commented 16 days ago •

Team member @m-ou-se 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.

joshtriplett commented 16 days ago

To confirm: this new FCP is based on the name if_ready!.

rfcbot commented 16 days ago

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

withoutboats commented 15 days ago •

I want to register why I think this choice is a mistake. I don't think its a disaster or anything and I don't have time to engage much, but I think it would be better to not rename this. I think the cost of renaming has been sharply underestimated in this process.

This isn't a new API. A macro with this functionality is provided by all of these crates: futures/futures_core, tokio, async_std, smol, and futures_lite. It's basically impossible to write a crate in the async ecosystem today without depending on at least one of these. In all of these crates, this macro is called ready!. And most of the time that I've personally written a poll method, I've used this macro, under that name.

Setting aside the question of whether this strong consensus is evidence that ready! is a good name or just normalcy bias (though I note that the maintainers of most of these crates 'd my earlier comment in support of ready!), renaming this macro when stabilizing it in std increases substantially the churn costs. All of those libraries I cited have a pretty strong stability commitment, which means they will all continue to provide a macro with this functionality under the name ready!. ready! isn't going anywhere therefore; users will have to learn that the ready! macro from all of these libraries has the same functionality as if_ready! from std, and the stated clarity benefits of if_ready over ready (which I personally think are minimal to non-existent) will be reduced by the fact that users working in this area will have to read and understand code containing both forms.

I basically think that in the abstract the difference is a wash, and I would have no strong opinion, but that in the concrete the churn costs outweigh any difference between the two names. I would encourage the teams to be a bit more quietist and only change the naming of ecosystem-consensus APIs with a really strong motivation. I don't think the motivation here is strong enough.

BurntSushi commented 15 days ago

One blind spot I have is that I don't actually know who the stakeholders are here. I mean, I realize they are, at minimum, the maintainers of the crates that @withoutboats listed and I do know a couple of the names involved there, but that's it. So I guess I would definitely be curious to hear from the stakeholders about this. What do you think we should do here?

I think that if ready! is going to have a continued and prominent spot in the ecosystem regardless of what name we pick here, then there are perhaps more severe downsides to choosing a different name than has been considered here I think. (Which I think is just echoing @withoutboats.)

hawkw commented 15 days ago

So I guess I would definitely be curious to hear from the stakeholders about this. What do you think we should do here?

So, here's my two cents, although please note that this is just my personal opinion and not that of the entire Tokio team.

It's worth noting that in Tokio's stability policy, we've stated that we're not going to publish a 2.0 release until at least 3 years after 1.0 was published, and that we'll continue actively supporting 1.0 for at least 5 years regardless of whether or not a 2.0 is released. So, while we could certainly deprecate ready! in favor of if_ready!, ready! is going to remain in Tokio's API surface for a while, even if it's deprecated. I can't speak for other async ecosystem crates, but I imagine that, for example, async-std isn't particularly eager to release a 2.0 just to remove ready!, either. :)

Now, I don't think that having a deprecated ready! and an if_ready! in the API is a huge issue. It makes things a little bit more confusing for new users, but IMO, it's far from the biggest learnability obstacle, so the impact of that is fairly negligible. On the other hand, while I don't particularly want to restart a bikeshed over the name of the macro, I also don't find if_ready! to be a significant improvement over ready! (personally, I actually think the if_ready! naming is less clear>1, but it's not a big deal).

Finally, there's the amount of work to update all the various downstream crates that use ready!, the documentation, examples, etc, to switch from ready! to if_ready!. This would potentially be a lot of code to change, but changing it is quite easy --- in most cases, maintainers could just do a find-and-replace and everything would be fine. So I would characterize the cost of this as "annoying", but not terrible.

My personal perspective is that, overall, the gain from renaming ready! to if_ready! is pretty negligible, and the impact of keeping deprecated API ready! macros around is not great but also pretty negligible. So, all things being equal, if it doesn't really matter, I would err on the side of avoiding ecosystem churn for a name change that doesn't really feel like a meaningful improvement --- I'm inclined to agree with @withoutboats. But...if a majority of other folks agree that the renaming is important and has real value, the resulting ecosystem churn is going to be an annoyance, not a disaster.

1I'd expect a macro of the form if_<whatever>!(<expr>) to only evaluate <expr> if <cond> is true, which is...kind of the opposite of what this macro does.

withoutboats commented 14 days ago •

Yea, speaking as someone who maintains a big pile of downstream user code, I would find these libraries deprecating their ready macro to be annoying, not a disaster, but definitely annoying. I think it's important to keep in mind that async/await has been stable for almost 2 years now during which async Rust has had explosive growth. There's a lot of users out there with the ready macro in their code. It's not the same phase anymore as 2018/2019.

I think the misinterpretation @hawkw describes is an important downside of if_ready, but I have a more abstract concern along the same lines: I don't think there's any succinct name for this macro that will make its operation immediately clear. Using a slightly opaque name like "ready" is helpful in this case because it makes it more obviously a "term of art" macro that users realize they need to look up to understand. With a name that tries to be self-explanatory, but could have misinterpretations, users are more likely to persist in their misunderstanding because they think they intuitively understand what the macro does without looking it up, but they are maybe wrong.

yoshuawuyts commented 14 days ago

So I guess I would definitely be curious to hear from the stakeholders about this. What do you think we should do here?

I've shared my own thoughts on this before (I'm okay with either name), but not yet from the perspective of an async-std maintainer.

I don't think there's anything stopping us from deprecating async_std::task::ready! and adding async_std::task::if_ready! within the current major. We probably wouldn't want to use the #[deprecated] tag since it would cause warnings (which in turn may break downstream user's CI if they disallow warnings). Instead we'd likely want to mark it as a deprecation through docs-only, and possibly sprinkle in some CSS to make it stand out. In a future 2.0 version (which we plan to do once more async lang feature stabilize) we would then complete the transition.

I believe the async-std project could work with both ready! and if_ready!, and as such poses no technical restrictions on the choice of naming.

joshtriplett commented 9 days ago

We discussed this in today's @rust-lang/libs-api meeting. We found the most recent round of arguments compelling: neither ready nor if_ready is especially clear, and ready! at least has precedent. In addition, ready! being a macro does make it clear that it could affect control flow. It also helps that this is std::task::ready!, with no plans to add it to any kind of prelude or similar.

Not everyone was concerned about the issue of deprecation; some of us felt that whether this was called ready! or if_ready! there'd still be a long transition period from framework-specific paths to the standard library path. But we all roughly agreed that renaming doesn't really seem worthwhile.

@rfcbot cancel

rfcbot commented 9 days ago

@joshtriplett proposal cancelled.

joshtriplett commented 9 days ago

Shall we merge this as std::task::ready!?

@rfcbot merge

rfcbot commented 9 days ago •

Team member @joshtriplett 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 commented 9 days ago

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

Kestrer commented 8 days ago

ready! is going to remain in Tokio's API surface for a while, even if it's deprecated

FWIW Tokio doesn't actually expose ready!. But so many other crates do that the point still stands.