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

Tracking Issue for poll.ready()? #89780

Closed
dtolnay opened this issue Oct 11, 2021 · 45 comments
Closed

Tracking Issue for poll.ready()? #89780

dtolnay opened this issue Oct 11, 2021 · 45 comments
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Oct 11, 2021

Feature gate: #![feature(poll_ready)]

This is a tracking issue for the core::task::Poll::ready method, which combined with ? potentially supplants the ready! macro of #70922.

- let val = ready!(fut.poll(cx));
+ let val = fut.poll(cx).ready()?;

Public API

// core::task

impl<T> Poll<T> {
    pub fn ready(self) -> Ready<T>;
}

pub struct Ready<T> {...}

impl<T> Try for Ready<T> {
    type Output = T;
    type Residual = Ready<Infallible>;
    ...
}

impl<T> FromResidual for Ready<T> {...}

impl<T> FromResidual<Ready<Infallible>> for Poll<T> {...}

impl<T> Debug for Ready<T> {...}

Steps / History

Unresolved Questions

  • ?
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 11, 2021
@ibraheemdev
Copy link
Member

One trivial item, should the debug implementation for Ready show the inner Poll<T>? I left it out initially without a good reason.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 12, 2021

One trivial item, should the debug implementation for Ready show the inner Poll<T>? I left it out initially without a good reason.

It doesn't matter much since it's unlikely that the Debug implementation for this type will be used much anyway, but the default #[derive(Debug)] implementation is probably fine here.

@BurntSushi
Copy link
Member

Do we have a way of pinging async stakeholders here? There was a lot of discussion among several participants in #81050 for example. I can just pick out names from there, but if there's a better way...

@m-ou-se
Copy link
Member

m-ou-se commented Oct 12, 2021

cc @rust-lang/wg-async-foundations

@cramertj
Copy link
Member

Personally I find this less clear than the ready! macro:

With the .ready()? method

fn some_future_impl() -> Poll<Result<(), ()>> {
    some_check().ready()??;
    Poll::Ready(Ok(()))
}

fn some_stream_impl() -> Poll<Option<Result<(), ()>>> {
    some_check().ready()??;
    Poll::Ready(Some(Ok(())))
}

With the ready!(...) macro

fn some_future_impl() -> Poll<Result<(), ()>> {
    ready!(some_check())?;
    Poll::Ready(Ok(()))
}

fn some_stream_impl() -> Poll<Option<Result<(), ()>>> {
    ready!(some_check())?;
    Poll::Ready(Some(Ok(())))
}

The ?? in the former looks curious to me and it's not clear what order the various Try-like things are being processed (Result-like vs. Poll-like), while the latter looks more straightforward to me. That said, I also have a lot of experience using the system that exists today and never minded the ready! macro, so I'd appreciate hearing from folks who are newer to these APIs about their preferences.

@ibraheemdev
Copy link
Member

You could write it this way:

fn some_future_impl() -> Poll<Result<(), ()>> {
    some_check()?.ready()?;
    Poll::Ready(Ok(()))
}

fn some_stream_impl() -> Poll<Option<Result<(), ()>>> {
    some_check()?.ready()?;
    Poll::Ready(Some(Ok(())))
}

Which makes it clearer IMO, because ? is generally used for Err propagation, and the .ready()? makes it clear that it is for Poll::Pending propagation.

The fact that there are two ways to use ? with Poll definitely adds complexity though.

@LucioFranco
Copy link
Member

I have to agree with @cramertj I think the ready! macro provides a super clear "yield" point that the ready method does not have.

@tmandry tmandry added the A-async-await Area: Async & Await label Oct 17, 2021
@eholk
Copy link
Contributor

eholk commented Oct 18, 2021

@rustbot label +AsyncAwait-triaged

@rustbot rustbot added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 18, 2021
@songzhi
Copy link

songzhi commented Oct 21, 2021

In the coding experience aspect, since rust-analyzer supports custom postfix-snippets, just define a snippet like the following one, you can write ready! as postfix. But this snippet completion takes effect globally, maybe confusing and annoying when writing unrelative codes.

"rust-analyzer.completion.snippets": {
    "ready!": {
      "postfix": "ready",
      "body": [
        "ready!(${receiver})",
      ],
      "requires": "std::task::ready",
      "description": "ready!()",
      "scope": "expr",
    }
}

@mjbshaw
Copy link
Contributor

mjbshaw commented Jan 10, 2022

Drive by comment: I have to disagree with statements that ready! is clearer. As someone relatively new to async stuff, I was reading some code that used ready!. Later, after some more review, I was surprised to learn that ready! affected the control flow of the function. I was not expecting that when I was reading the code. On the other hand, I know ? will affect the control flow: that's the whole point of the ? operator. I know this is subjective and others may not agree, but I wanted to add the experience of a non-async expert here. I'd love to see poll.ready()? stabilized!

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 2, 2022

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

Concerns:

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. labels Mar 2, 2022
@yaahc
Copy link
Member

yaahc commented Mar 22, 2022

@rfcbot concern try impl contradicts existing try impls

I'm really worried that introducing a Try impl on Ready and adding this API around ? will significantly increase the risk of accidentally introducing logic errors. The existing try impls, undesirable as they are, propagate out a Poll::Ready. This new impl will instead propagate out a Poll::Pending. Now, using the specific example @cramertj and @ibraheemdev linked, we have two equivalent snippets:

fn some_stream_impl() -> Poll<Option<Result<(), ()>>> {
    some_check()?.ready()?;
    Poll::Ready(Some(Ok(())))
}

// and 

fn some_stream_impl() -> Poll<Option<Result<(), ()>>> {
    some_check().ready()??;
    Poll::Ready(Some(Ok(())))
}

The first example attempts to more clearly communicate what is being propagated, but there's an assumption that some_check() will return a Result, or a Poll<Result<..>> or a Poll<Option<Result<..>> or something for which the FromResidual impl on Poll<...> ends up being set to Ready(Err) of some shape. These question marks have almost exactly contradictory meanings, one is saying we're ready, the other is saying we're not. In the second examples the question marks are right next to each other, which I think only increases how confusing this will be to read, and the compiler does absolutely nothing to push you towards the former example, this would be handled purely by convention as people become more aware of this footgun. I think this will significantly increase how confusing the intersection between futures and error handling is, and I don't think we should add this unless we can find some way to disallow the currently stable ? behavior on Poll<Result<...>> and friends.

cc @scottmcm

@scottmcm
Copy link
Member

I don't have sufficient experience with the ?s on Poll to really have an informed opinion about how important they are. I just made sure they kept working in TryV2 🙃 That said, there's some obvious logic to me in just having Poll<T>: Try<Continue = T, Break = Poll<!>>, especially with ok-wrapping in try blocks.

Disallowing the current behaviour might be doable with an approach like the one in https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#compatibility-with-accidental-interconversions-if-needed -- make a new trait for ? in different editions, and just never stabilize the 2015+2018+2021 version that has the special Poll<Result<T, E>> impls.

A big question here, though, might be if anything is using them in try_fold calls. It doesn't seem to me like they'd be a good fit in that, but I don't know. That's a type system thing that would be hard to remove, particularly since an impl for Poll overlaps with those for Poll<Result<..>>.

@joshtriplett
Copy link
Member

@yaahc Thank you for catching this. I was aware we had impls for Poll, but I had always naturally assumed that they would propagate Pending; the idea that they would instead propagate Ready would never have occurred to me.

I agree that we need some way to disable those instances. An edition seems like a fine approach.

@cramertj
Copy link
Member

I guess I'll speak up as the person who added those impls. I still think that adding them was critical to making the transition from -> Result<Poll<...>, E>-returning Future::poll to a version which returned Poll<T>, as at that time it was very common to write Future and Stream impls by hand, and without these impls it would've been extraordinarily difficult and confusing to propagate errors in those contexts.

Personally, I also still find the "? propagates errors" intuition the most obvious, though I understand that my preference there is less and less popular as time goes on. I personally think that using ? to propagate Pending signals of a Poll<Result<...>> inside a Poll<Result<...>>-returning future impl or Poll<Option<Result<...>>>-returning Stream impl incredibly confusing, as it would be unclear to me which parts of the code represented possible "yield"-like points and which represented early returns due to errors. These cases frequently require very different types of handling, and I would have a hard time reading and understanding which ?s in which functions corresponded to Pending-s rather than Errors.

You could disambiguate by entirely removing the ability to use ? on Result<..> inside Poll<Result<...>>-returning future impls or Poll<Option<Result<...>>>-returning Stream impls, but I think that would be a significant ergonomic loss, as folks now have to learn to use some other macro to early return on errors within these contexts.

@yaahc
Copy link
Member

yaahc commented Mar 23, 2022

@cramertj that makes sense and I do want to highlight that I only briefly spent time writing much async and haven't used that portion of the language much recently, particularly not to the point of writing Poll impls, so I definitely want to lean on y'all's experience as much as possible here. I do not have a great intuition for how much people do or don't like these Try impls on Poll or how big of a priority moving away from them should be.

I talked to scottmcm a bit more about the ways we could fix this if we wanted to. He mentioned it above but basically making two Try traits, having one be permanently unstable for 2015-2021, then stabilizing the new one and the API in this issue in 2024. There was some confusion since he wasn't sure if we wanted to be able to use ? directly on Poll to propagate pending, and I guessed that it would be fine to require a .ready() first in order to get a Try type you could use ? on, since it resolved an issue with trait overlap. He quickly threw together the commit1 which would do this which we would need to run through crater since technically people can currently use Poll<...> in try_fold on stable, though it's hard to imagine why they would.

My question for the async experts in the room is, how big of an issue is this confusion really? I see a few options forward here and some pretty substantial tradeoffs:

  1. We add this API and potentially stabilize try in the current edition when it's ready
  • Pro: immediately get to use ready()?
  • Pro: try stabilization is not delayed
  • Con: potential confusion on ? behavior in Poll fns
  • Con: probably stuck with that forever, short of special casing ? to disallow using it for propagating Ready while still allowing the trait usage
  • Con: generally more confusing/complex try impls around Poll (maybe not that bad, since the new Impls aren't actually on Poll, they're on Ready, but this may be confusing in it's own right)
  1. We never add this impl and potentially stabilize try in the current edition when it's ready
  • Pro: try stabilization is not delayed
  • Pro: ? behavior is consistent on Poll
  • Pro: Poll's Try impls are less complex than previous example
  • Con: we probably go back and stabilize ready!, stuck with macro for propagating Poll::Pending forever, representing custom control flow with a macro instead of ?
  1. We introduce a permanently unstable TryForOldEditions, stabilize the current unstable Try in 2024, add and stabilize this API in 2024
  • Pro: we eventually move away from the ready propagating Poll
  • Pro: Significantly simpler stable Try impl for Ready
  • Con: We have a whole bunch of extra permanently unstable Try impls on Poll, possible source of confusion
  • Con: Possibly significantly delay Try stabilization
  • Con: Significant delay on stabilization of this feature
  • Con: ? behavior on Poll changes across editions, depending on how you look at it. Technically its going away and we're instead having ? on Ready, so that may be clear enough to avoid confusion

I think its fair to say that (1) is the worst option here. But I'm not certain which is better between (2) and (3).

Footnotes

  1. https://github.com/rust-lang/rust/commit/369ac82d416c56a86e616cfb411337c5e8b2f09e

@joshtriplett
Copy link
Member

FWIW, I do favor option 3 here, and I think people's intuitions for what .ready()? means will tend to give the right answer, inspired by what ? means on Result. .ready()? to me reads as "if this is ready, extract the contents of the ready variant; if it isn't, bail".

@yaahc
Copy link
Member

yaahc commented Mar 23, 2022

I think I agree but I really want to make sure we have a strong consensus from async users as well. My feeling is that if we pick (2) we're prioritizing short term improvements over long term, and I generally think the long term maximum has a much greater overall impact since we're talking about sacrificing usability for ~2.5 years for overall better(I think???) usability forever after. The idea of delaying Try and delaying ready()?/ready! both for all of 2021 really feels bad tho.

@scottmcm
Copy link
Member

Just brainstorming:

A potential 1b would be to add the impl immediately, but add some as-yet-unknown lints or errors to help mitigate the cons. For example, we could have edition-specific lints or errors that look for specifically using ? on one or the other case, or even for using both of them.

As a sortof 3b that's also mixing in 1, we could maybe add a .error()? to cover the pull-the-error-out cases, and just remove the ? directly on Poll. Would would sortof be saying that "both of these are reasonable so you have to pick which one you're meaning".

That latter one might end up just being like a transpose, as it might be able to just return a Result, rather than needing a new type and Try impl. Which would be kinda nice, actually, since then the Try impls are all simple; the "digging deep" functionality is in the error inherent method, not part of the ?.

@cramertj
Copy link
Member

cramertj commented Mar 23, 2022

For those that prefer (3) here, what would the following code samples look like under your preferred replacement? These were (are?) common patterns in manual Future/Stream implementations.

fn poll_next(...) -> Poll<Option<Result<T, E>>> {   // and also `poll(...) -> Poll<Result<T, E>>`
    // Return early with errors, don't propagate pending
    let ...: Poll<T> = some_future.poll(cx)?
    let ...: Poll<Option<T>> = some_child_stream.poll_next(cx)?;
    // Return early on either errors or pending
    let ...: T = ready!(some_future.poll(cx))?;
    let ...: Option<T> = ready!(some_child_stream.poll_next(cx))?;
    ...
}

@yaahc
Copy link
Member

yaahc commented Mar 24, 2022

For those that prefer (3) here, what would the following code samples look like under your preferred replacement? These were (are?) common patterns in manual Future/Stream implementations.

Leaving aside bikeshed-y questions about the naming of functions, I'm gonna lean on @scottmcm's suggestion for having a transpose equivalent on Poll, which I think is more of an option (4) at this point because it still means we'd have all the current FromResidual impls for Poll, even if we remove the Try impl and can only use ? on Result or Ready:

fn poll_next(...) -> Poll<Option<Result<T, E>>> {   // and also `poll(...) -> Poll<Result<T, E>>`
    // Return early with errors, don't propagate pending
    let ...: Poll<T> = some_future.poll(cx).bikeshed_transpose_result()?
    let ...: Poll<Option<T>> = some_child_stream.poll_next(cx).bikeshed_transpose_result()?;
    // Return early on either errors or pending
    let ...: T = some_future.poll(cx).ready()?;
    let ...: Option<T> = some_child_stream.poll_next(cx).ready()??;
    ...
}

I think this is possibly clear enough. ? always means early return, and in the context of a Poll function it still often means propagating Ready implicitly, but all propagations of Pending are immediately preceded by a ready() call. All other ?s are propagating errors. Still not super in love with it, but I think this is just another example of the fundamental problems with composibility for abstractions that represent effects. The interaction of Result and Iterator is just as much of a mess as this. I think we're going to have to settle for a compromise here.

Edit: One more question actually @cramertj, in your example did you expect that the return type for some_child_stream.poll_next(cx) would be Poll<Option<Result>> or Poll<Result<Option>>? If it's the former then I'm guessing we'd need a separate transpose function for this deeper case. This is getting messier and messier X_X

@cramertj
Copy link
Member

@yaahc Can you explain in more detail how you imagine the Option<Result<...>> conversions working? What does calling bikeshed_transpose_result on a Poll<Option<Result<T, E>> do?

@scottmcm
Copy link
Member

One thing that came to mind that opens up as a possibility if some form of (3) happens, removing the Poll: Try from the going-forward version of ?: This might make the ops::Try+ops::Residual combination easier to understand by allowing them to be more tightly tied together.

Right now there's basically no trait restriction on residuals, and that has caused some confusion -- see the third unresolved question in #84277. Part of why that is is that the Poll residuals are more interesting than the rest of them.

If we didn't have those, then a restriction like

trait Try {
    type Output;
    type Residual: Residual<Self::Output, TryType = Self>;
    ...
}

Could tie things together more strictly, making mistakes less likely and thus possibly also making it easier to understand since there'd be less freedom in how the types all relate to each other.

Basically, it'd mean the all the standard library break/continue splits would be simple. Whereas right now the ops::Residual behaviour when you're dealing with Poll<Option<Result<T, E>>>: Try is not necessarily obvious. (Which I've been thinking about a bunch lately in the context of tweaking try{}.)

Now, I don't know for sure whether we'd want to do that, I don't want to decide it here, and I think that something similar might also be doable in the other solutions too. But @yaahc suggested I post more thoughts here 🙂

@withoutboats
Copy link
Contributor

(NOT A CONTRIBUTION)

In my opinion, this is worse than the ready macro in every way. I have a strong preference that the ready macro be stabilized and this API be removed from std.

First, I second @cramertj's comment that low level poll methods return result the majority of the time, so the ready operation is being combined with ? more often than not, leading to a confusing and unclear ready()??. I do not think std should promote an idiom that leads to double question mark as a frequent expression. And I agree with @yaahc that it gives ? a double meaning inside these functions, obscuring the two different distinct operations.

Second, I think the ready operator needs to be put into its proper context. Poll methods are functionally a rarely used DSL for a specific category of low level code. The various special operations needed to understand and implement them (i.e. the apis in std::pin and std::task) are therefore not a part of ordinary Rust usage, but something people will need to understand to drop down into them. Therefore, being easy to learn is far more important than being elegant to write. The ready macro is easier to understand than this API in several ways.

The first way its easier to understand is that it is far easier to read the source of. The ready macro is a simple match statement. To understand Poll::ready, first you read a method, which constructs a struct, then you have to understand that its the Try impl on the struct that you are interested in, then you have to know how the Try API is related to the ? match syntax. All of this is very opaque, and while documentation helps it is certainly less simple than the ready macro.

The second is that it is composed of two operations (a method call and a ? operator) when it is only a single operator. There is no reason to ever called .ready() without following it with ?. This introduces a decision point for the user when none exists, inherently increasing the complexity of the API for users. Sometimes this can be justified, but I don't think its justified here.

And I think that there is genuine merit to the ready! macro from a readability perspective over this method or even just "making ? work." Drawing a distinction between the Result and Poll operations in poll methods is valuable to understand whats going on, having ? mean two things would make it more confusing. And these poll methods, being very low level and hopefully few and far between (this depends on the async WG shipping async traits and so on), tend to be the kind of code that is made more clear by a somewhat more C-like API, where operations like waiting for write readiness of the underlying IO object benefit from being called out very clearly.

Finally, when it comes to "creating a new Try in the next edition" I feel I must again raise that I do not think the project is taking churn seriously enough; this would be hugely disruptive for any project with poll implementations and make it much harder for them to transition editions. What justification is there to create this churn for so many of your users?

I don't really understand the impetus behind these delays and ideas. Is there some user experience maintaining these methods that suggests the current ecosystem consensus is unsatisfactory and needs improvement? To me seems based on a theoretical desire for an API that matches the aesthetic sensibilities of the current libs team (use Try for everything, postfix everything, etc). I'd push for instead a more pragmatic attitude, returning to Graydon's original FAQ: "We do not prize expressiveness, minimalism or elegance above other goals ... We do not intend to be ... too dogmatic in any other sense. Trade-offs exist."

Speaking as someone who maintains poll methods, I strongly prefer to stabilize the ecosystem's consensus API: the ready macro, which is simple and straightforward and has existed and been in use for years already.

@yaahc
Copy link
Member

yaahc commented Jun 13, 2022

Finally, when it comes to "creating a new Try in the next edition" I feel I must again raise that I do not think the project is taking churn seriously enough; this would be hugely disruptive for any project with poll implementations and make it much harder for them to transition editions. What justification is there to create this churn for so many of your users?

Going back to my original quote

I think this will significantly increase how confusing the intersection between futures and error handling is, and I don't think we should add this unless we can find some way to disallow the currently stable ? behavior on Poll<Result<...>> and friends.

The conversation may have gotten away from us but I don't think we ever progressed far enough into the follow up discussion to get to a point where anyone should be expected to have a fully fleshed out proposal. I pointed out an issue that I thought should block the API, scott suggested some potential paths to resolving that blocker, and since then nobody has taken those ideas and converted them into a proposal.

So going back to your quote

I feel I must again raise that I do not think the project is taking churn seriously enough; this would be hugely disruptive for any project with poll implementations and make it much harder for them to transition editions. What justification is there to create this churn for so many of your users?

I do not believe this is a fair characterization. You're mischaracterizing a brainstorming session as if it were the stance of the project, and not two individuals going back and forth on unblocking potential issues as they came up so that the conversation could continue.

@withoutboats
Copy link
Contributor

withoutboats commented Jun 14, 2022

(NOT A CONTRIBUTION)

@yaahc When I talk about the project's relationship to churn I don't mean this issue in particular, but the attitude I see expressed again and again that if you can find a technical way to meet the edition requirements, you can make a change that makes existing code invalid in the next edition. Instead, it should always be recognized and centrally considered that the social cost of invalidating existing code is high, and should only be done with a compelling reason. EDIT: I check my other notification and what I'm immediately shown is you advocating idiom churn as a general rule; what exactly are you protesting?

What is the compelling reason for doing that, or for adding this API instead of stabilizing the API that everyone already is importing from futures or tokio? Here is the argument that has been made so far for this API in this and the PR that introduced it:

It looks better than ready! in my opinion, and it composes well

This is quite nice. Worth considering at least.

While I don't see much harm in having both, this does seem better in every way.

Seriously?

@jplatte
Copy link
Contributor

jplatte commented Jun 14, 2022

@withoutboats I agree with you that the macro is overall the better solution given ? has an existing meaning and both ?? and getting rid of the existing meaning of ? across an edition boundary are rather problematic, but this:

Seriously?

is rather dismissive of other people's opinions.

@withoutboats
Copy link
Contributor

(NOT A CONTRIBUTION)

Yes, I am dismissive of an argument for blocking stabilization of a widely used API and considering major idiom churn that has never been expressed in more convincing detail than "it looks better," "its quite nice," and "it seems better in every way." This should not be the standard for decision making in this project.

@yaahc
Copy link
Member

yaahc commented Jun 14, 2022

EDIT: I check my other notification and what I'm immediately shown is you advocating idiom churn as a general rule; what exactly are you protesting?

Again, the comment you linked is my opinion, not the opinion of the libs team. If I was representing the stance of the libs team I would have used "we" instead of "I".

I don't agree with your assessment that I was advocating for idiomatic churn. I was advocating for more strongly de-duplicating APIs when we add new APIs that supersede old APIs rather than just marking them deprecated and softly encouraging people to upgrade. In the quote you linked I specifically addressed the issue of churn

I wouldn't want to introduce this churn carelessly, we'd need to have mechanical ways to update code when they move to the new edition so the only churn that happens is in the diff rather than requiring repetitive manual updates across the entire ecosystem. But between a small amount of diff churn and having to endlessly maintain two ways to do the same thing, I'd choose the diff churn and the smaller library API every time.

You mentioned "Instead, it should always be recognized and centrally considered that the social cost of invalidating existing code is high, and should only be done with a compelling reason.", and I feel like I did clearly outline a compelling reason in that case, we have an API that we're seeming likely to accept with suggestions to deprecate an existing API in the process, and assuming that deprecation does get accepted I advocated for limiting the number of distinct dialects of rust that develop and to maintain consistency in the language.

The disconnect seems to be a difference in the recognized cost of invalidating existing code. I think with automatic migrations the upgrade cost of churn can be small, and that this small churn can be justified when it reduces the surface area of the language long term, since either way you still have to pay the cost of interacting with and understanding both idioms. You seem to think there are additional concerns I'm not considering, could you elaborate on those other costs?

@withoutboats
Copy link
Contributor

(NOT A CONTRIBUTION)

The disconnect seems to be a difference in the recognized cost of invalidating existing code. I think with automatic migrations the upgrade cost of churn can be small, and that this small churn can be justified when it reduces the surface area of the language long term, since either way you still have to pay the cost of interacting with and understanding both idioms. You seem to think there are additional concerns I'm not considering, could you elaborate on those other costs?

We shouldn't conflate these two issues, there's no doubt the OnceCell<()> API will exist, so users could use that instead of Once, and people will have to be familiar with both (though in my opinion this a bit is like saying we should deprecate HashSet since its just an alias for HashMap<T, ()>). In this case, the situation is dramatically different: you would change the meaning of ? on Poll between editions; the alternative is not that both idioms co-exist, but that the existing idiom which you think is worse continues to exist and the idiom that you think is better never comes into being.

Just because you can mechanically migrate code doesn't mean that the social cost becomes 0. At the time we introduced the mechanism of cargo fix, we held that this was the bare minimum to make any idiom churn acceptable, not that it now makes idiom churn free. Such idiom churn still needs to be justified by being a substantial improvement that makes everyones' lives better enough that it outweighs the cost of social migration - meaning that people who maintain the existing and functional code need to understand how their code is changed by cargo fix, make improvements for readability and maintainability that are not guaranteed by the mechanical update (especially in this case), and generally do work to keep their existing code up to date with modern Rust. This is a real cost for many existing users, the kind of thing that will keep people from following edition upgrades, and especially in a case like poll (which is supposed to be something fewer and fewer people interact with over time) it needs to be weighed heavily.

This churn can be justified, but justified it must be. Here the justification seems to be an unquestioned commitment to the idea that postfix is always better, that macros are always worse, that ? should be the early return operator for every use case. The fact that Taylor, one of the world experts in maintaining poll methods, didn't think this improves the code, has not seemed to be taken as seriously as I feel it should be: a libs team member literally proposed to merge after Taylor's criticism without even responding to it or making any case at all for the merge.

Given the argument I have already made at length, I think both the ready method and changing ? would be worse than the ready! macro (I can make additional arguments against the ? change at length, my previous post focused mainly on ready()?). What argument is there that it will actually make peoples' experience writing Rust better? Again, I see a sort of aesthetic commitment to a certain code style that some project members prefer, which is not supported by genuine complaints from users about the existing idioms.

Even setting aside how egregious this particular issue is, no I do not think "it reduces the surface of the std API" is ever a good enough reason to remove an API, or even to lint-deprecate it. The old API has to be in some way substantially inferior to the new API, prone to error, less efficient, something like this - not simple an idiom the current group around the project has decided they don't care for.

@m-ou-se m-ou-se added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jun 14, 2022
@jplatte
Copy link
Contributor

jplatte commented Jun 14, 2022

At the expense of possibly creating yet another parallel track to an already non-trivial discussion, I would like to point out that ready!() might be a prime use case for simple postfix macros (RFC 2442).

@m-ou-se
Copy link
Member

m-ou-se commented Jun 14, 2022

@withoutboats I think you're providing valuable feedback, and you make a lot of great points, but I agree with Jane that you're mischaracterizing what's happening by saying

[..] the attitude I see expressed again and again that if you can find a technical way to meet the edition requirements, you can make a change that makes existing code invalid in the next edition.

That's not quite what happened for Rust 2021 at all. We've been extremely careful, and rejected quite a few ideas based on the same points that you're making in this thread. The transition to Rust 2021 has been very smooth. I have no reason to believe this will be any different for the next edition.

Just because we're exploring some (sometimes wild) ideas, does not mean we'll not be careful when making decisions. Importantly, much of such exploration happens by individuals, while decisions that affect stable Rust are made by the full team through FCP. It'd not be helpful if every idea any of us explore is seen as a team decision.

For the .ready()? issue: this isn't in FCP yet, and I don't see this passing FCP without some good reasons to dismiss your arguments. For now, I don't think you have to worry much, or spend a lot more energy writing down more detailed arguments against it. As for the delay created by the discussion and exploration here.. I'm sure you remember from your time on the libs team that some seemingly simple features can take a long time to find a consensus on. This can be incredibly frustrating, but I don't think your last few contributions are very productive or helpful towards speeding things up.

I've nominated the issue for discussion by the library api team. There's a ton of new insights to be considered, for which I thank you. If you have any concerns about the general way the team operates or the direction the standard library is going into, I'd love to hear them. But dismissing the (sometimes controversial) ideas of one of our most ambitious and hard working team members is not a good look, and does not benefit the quality of future Rust in any way.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2022

@rust-lang/wg-async Hey Async WG! We (@rust-lang/libs-api) would like your input and ideally a recommendation on what to do here. There are good arguments for and against both ready!() and .ready()? and both have pros and cons. We'd like to hear from more stakeholders and hear what y'all think is best. Thanks!

@yoshuawuyts
Copy link
Member

Hey Async WG! We would like your input and ideally a recommendation on what to do here.

We've been discussing this on Zulip, and it seems we're seeing many of the same tradeoffs you are. Though it seems so far what we mostly agree on though is that stabilizing something, with clear semantics, in the nearer term is the higher order bit.

Personally task::ready! looks to me as the option with the lowest friction 1. From reading the comments I don't think anyone's critiqued it as having unclear semantics, and it would also make the transition from futures_core::ready! to std::task::ready! easier since it just requires changing imports.

I do want to explicitly call out that just because I'm choosing to support the stabilization of task::ready! sooner, I do not believe that should shut the door on further experimentation with the Try trait. As should be clear from this thread there are plenty of open questions and plenty of tradeoffs. I want to make sure that the error handling WG feels like they're able to freely experiment without taking this as a vote against that.

Footnotes

  1. I should highlight that I originally authored the task::ready! PR for the stdlib, and subsequently shepherded it through the FCP process. The stabilization of task::ready! had been reverted after it had been scheduled to land in 1.56. But at the time I did support the decision to reverse the stabilization so we could explore the poll::Poll::ready API because the direction seemed promising. I designed neither of the APIs though, and don't really feel ownership over them.

@nrc
Copy link
Member

nrc commented Jun 27, 2022

Yep, @yoshuawuyts's comment sums up the WG's discussion pretty well. I just want to add that I am pretty unsure about using ? for Poll in general (not necessarily against it, just very unsure) and so I prefer ready! since it doesn't force us further down that road. Again though, these are weakly held opinions so I (nor others on the WG) won't be mad if y'all decide to go the ready() route.

@withoutboats
Copy link
Contributor

withoutboats commented Jun 28, 2022

(NOT A CONTRIBUTION)

Another remark has occurred to me that I realized hasn't been made quite explicit here: it's not just that Poll and Result are different types and its important to distinguish which is being returned, but it also has a big impact on how you write a poll method. When a future early returns Err with ?, it is incorrect for that future to be called again. However, when a future early returns Pending with ready!, that future will be called again unless it is cancelled.

For this reason, when returning Pending, the future needs to be in a state that everything that's already executed in the future won't have an effect when the future is called again, either because it was idempotent or because the future's been put in a state to branch to the point where it returned Pending. This is a very different requirement from when a future returns Err, which case it is common to leave futures in a state they would panic if called again.

This is what it really means to be different between saying a future has errored and saying it is pending. It has a huge impact on the code you have to write, and it should be syntactically distinct to highlight what is going on.

@yaahc
Copy link
Member

yaahc commented Jul 5, 2022

When a future early returns Err with ?, it is incorrect for that future to be called again.

Am I right in thinking that this doesn't just apply to futures that return Result but that the more general property is that it is incorrect to poll futures that have returned "Ready"? In other words, the question is not pending vs. errored, but rather pending vs. finished.

I'm basing this off of this line in https://doc.rust-lang.org/stable/std/future/trait.Future.html#tymethod.poll

Once a future has finished, clients should not poll it again.

@withoutboats
Copy link
Contributor

withoutboats commented Jul 5, 2022

(NOT A CONTRIBUTION)

Yea, that's correct.

It's just we don't have a need for an early return operator for the successfully completed case. But since we do have use for an easy way to early return for both completed with error and pending, an important distinction about the pending case is they need to be left in a state to be called again and pick up where they left off.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 6, 2022

I've started an FCP on #70922 to see if there's consensus on stabilizing the ready!() macro.

@joshtriplett
Copy link
Member

Canceling the FCP on this in favor of ready!(), which seems to have consensus. We're not ruling out the possibility of something like this in the future, but it clearly needs more exploration, and shouldn't sit in FCP while that exploration takes place.

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Jul 6, 2022

@joshtriplett proposal cancelled.

@rfcbot rfcbot removed 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. labels Jul 6, 2022
@joshtriplett joshtriplett removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 6, 2022
@yoshuawuyts
Copy link
Member

The FCP on #70922 has completed and the task::ready! macro will be available from Rust 1.64 onwards.

bors added a commit to rust-lang-ci/rust that referenced this issue May 19, 2023
Remove unstable `Poll::ready`

Based on the discussion in rust-lang#89780, this API is problematic and would likely require changes over an edition. Now that `task::ready!` is stabilized, this seems unlikely to happen, so I think we should just go ahead and remove it.

ACP: rust-lang/libs-team#214
@dtolnay
Copy link
Member Author

dtolnay commented Apr 7, 2024

This API was removed in #107060 in favor of ready!.

@dtolnay dtolnay closed this as completed Apr 7, 2024
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 AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests