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 Ready::into_inner() #101196

Closed
1 of 3 tasks
daxpedda opened this issue Aug 30, 2022 · 36 comments · Fixed by #116528
Closed
1 of 3 tasks

Tracking Issue for Ready::into_inner() #101196

daxpedda opened this issue Aug 30, 2022 · 36 comments · Fixed by #116528
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-async Working group: Async & await

Comments

@daxpedda
Copy link
Contributor

daxpedda commented Aug 30, 2022

Feature gate: #![feature(ready_into_inner)]

This is a tracking issue for Ready::into_inner().

Adds a method to Ready that unwraps the value without an asynchronous context. See the futures library for an equivalent function.

Public API

impl<T> Ready<T> {
    #[must_use]
    #[inline]
    pub fn into_inner(self) -> T;
}

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@daxpedda daxpedda added 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. labels Aug 30, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2022
…riplett

Implement `Ready::into_inner()`

Tracking issue: rust-lang#101196.

This implements a method to unwrap the value inside a `Ready` outside an async context.
See https://docs.rs/futures/0.3.24/futures/future/struct.Ready.html#method.into_inner for previous work.

This was discussed in [Zulip beforehand](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60Ready.3A.3Ainto_inner.28.29.60):
> An example I'm hitting right now:
I have a cross-platform library that provides a functions that returns a `Future`. The only reason why it returns a `Future` is because the WASM platform requires it, but the native doesn't, to make a cross-platform API that is equal for all I just return a `Ready` on the native targets.
>
> Now I would like to expose native-only functions that aren't async, that users can use to avoid having to deal with async when they are targeting native. With `into_inner` that's easily solvable now.
>
> I want to point out that some internal restructuring could be used to solve that problem too, but in this case it's not that simple, the library uses internal traits that return the `Future` already and playing around with that would introduce unnecessary `cfg` in a lot more places. So it is really only a quality-of-life feature.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 4, 2022
…riplett

Implement `Ready::into_inner()`

Tracking issue: rust-lang#101196.

This implements a method to unwrap the value inside a `Ready` outside an async context.
See https://docs.rs/futures/0.3.24/futures/future/struct.Ready.html#method.into_inner for previous work.

This was discussed in [Zulip beforehand](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60Ready.3A.3Ainto_inner.28.29.60):
> An example I'm hitting right now:
I have a cross-platform library that provides a functions that returns a `Future`. The only reason why it returns a `Future` is because the WASM platform requires it, but the native doesn't, to make a cross-platform API that is equal for all I just return a `Ready` on the native targets.
>
> Now I would like to expose native-only functions that aren't async, that users can use to avoid having to deal with async when they are targeting native. With `into_inner` that's easily solvable now.
>
> I want to point out that some internal restructuring could be used to solve that problem too, but in this case it's not that simple, the library uses internal traits that return the `Future` already and playing around with that would introduce unnecessary `cfg` in a lot more places. So it is really only a quality-of-life feature.
@daxpedda
Copy link
Contributor Author

@joshtriplett would it be acceptable to start an FCP for this? Or are there any outstanding concerns?

@dtolnay
Copy link
Member

dtolnay commented Oct 10, 2023

@rust-lang/libs-api:
@rfcbot fcp merge

Simple function for unpackaging a core::future::Ready<T> future that has not already been polled into a T.

The contentious part is that this can panic ("Called `into_inner()` on `Ready` after completion") which is unusual for a function named .into_inner(). This design is based on the identical API in the futures crate: https://docs.rs/futures/0.3.28/futures/future/struct.Ready.html#method.into_inner. It does not return Option<T>.

I feel this was addressed convincingly enough by #101189 (comment): "in the async world I have encountered the attitude that doing anything with a Future that was already polled to completion by returning Poll::Ready will panic". The next comment by a different user reinforces that -> T is frequently used while they have never seen a case that called for -> Option<T>.

ready.into_inner() is expressible in a more roundabout way (but still zero-cost, probably) by polling once with a noop waker. (Waker::noop() tracking issue: #98286. But you can write your own noop waker today on stable with some unsafe code.)

use core::future::{self, Future as _};
use core::pin::Pin;
use core::task::{Context, Poll, Waker};

let mut ready = future::ready(...);
let value = match Pin::new(&mut ready).poll(&mut Context::from_waker(&Waker::noop())) {
    Poll::Ready(ready) => ready,
    Poll::Pending => unreachable!(),
};

Be aware that a zero-cost conversion to Option<T> remains not expressible. If unwinding is enabled, one may use panic::catch_unwind(|| ready.into_inner()).ok(). Otherwise, someone with that use case will need to propose it under a different name — .take() was discussed for this in the implementation PR.

@rfcbot
Copy link

rfcbot commented Oct 10, 2023

Team member @dtolnay 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 Oct 10, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Nov 7, 2023

cc @rust-lang/wg-async

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 7, 2023
@rfcbot
Copy link

rfcbot commented Nov 7, 2023

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

@yoshuawuyts
Copy link
Member

Thanks for the ping @m-ou-se! - I am surprised to learn this API exists. Apparently WG Async hadn't been pinged on the implementation in #101189, and it never came up in any triage issue either. Can we please file a blocking concern on WG Async having a chance to discuss this in a meeting?

I don't know whether there are any issues, but I'd like to give ourselves some time to look this over and discuss it. Thank you!

@BurntSushi
Copy link
Member

@rfcbot concern async wg review

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 7, 2023
@traviscross traviscross added WG-async Working group: Async & await I-async-nominated Nominated for discussion during an async working group meeting. labels Nov 7, 2023
@traviscross
Copy link
Contributor

WG-async discussed this in our recent triage meeting, and we plan to discuss it again later this week.

@traviscross
Copy link
Contributor

traviscross commented Dec 14, 2023

@rustbot labels +I-libs-api-nominated

(Edit 2023-12-18:)

Nominating for T-libs-api to discuss the 2023-12-18 consensus recommendation from WG-async that this be named Ready::unwrap:

We discussed this in the WG-async triage meeting on 2023-12-18 and reached a consensus:

Consensus: WG-async would prefer that this be called Ready::unwrap.

We felt that Ready is akin to an async variant of Option, and that, put in those terms, the functionality we're describing here is most similar to unwrap.

In that light, we discussed that it may be reasonable someday to have Ready::expect. We're not proposing that here, and we discussed how #[track_caller] may lessen the need for this, but keeping this option open further persuaded us that unwrap may be more correct.

We were also persuaded by the fact that other into_ methods do not panic (e.g. the currently-unstable Result::into_ok that has an E: Into<!> bound).

It has generally been the position of WG-async to prefer that async versions of sync things follow the conventions of the sync thing rather than doing something different. In terms of user experience, we want people to be able to carry over their intuitions from synchronous Rust. And conversely, we'd prefer that people not hit unpleasant surprises that only happen when using the asynchronous version of a thing, as that may color their view of async Rust in general.

That's the broader context in which WG-async is expressing a preference here for unwrap.


Edit 2023-12-18: Below follows the original nomination which is superseded by the one above.

We discussed this briefly in the WG-async call on 2023-12-14 but did not yet reach consensus. We plan to discuss further.

Nominating this for T-libs-api to address the question, "what is the argument against calling this unwrap?"

In the WG-async discussion, @tmandry had concerns about this being the only into_inner method that would panic.

Regarding that, the point that @daxpedda advanced here is that this is a fundamental property of Future. However, @daxpedda also noted:

...I would propose if we introduce a take method, that we might want to rename into_inner into unwrap?

So the question is, what's the argument against naming this unwrap?

The thinking here is as follows: There are concerns about the into_inner name. While we may have justifications about why panicking may be OK under that name, it would be reasonable to ask whether another name would need no justification, especially if that name would be more parsimonious with later extensions such as a take method. If this had been named unwrap originally, would we have had concerns that would have led us to calling it into_inner instead?

If unwrap as a name wouldn't need any justification and would raise no concerns, perhaps we should consider doing that. If that could be done, it would likely resolve all of the remaining concerns that members of WG-async had about this.

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 14, 2023
@daxpedda
Copy link
Contributor Author

daxpedda commented Dec 14, 2023

I actually believe that unwrap would be the better name because it is already used to describe similar behavior on Option, Result, Rc and Arc in Std.

The only reason why into_inner was chosen initially was for historical reasons, as this API was first explored in futures, which uses that name. I did check if there was any previous discussion about this in futures, but it seems not: rust-lang/futures-rs#2055.

So the question is, what's the argument against naming this unwrap?

I don't have one and I'm actually in favor of changing the name to unwrap.

@tmandry
Copy link
Member

tmandry commented Dec 15, 2023

I did raise concerns about into_inner during the meeting, but they were mostly alleviated by the argument that it would be very hard to create a situation where the call panics. You would have to do something like

let fut = pin!(ready(100));
let _ = fut.poll(cx);
let fut_unpinned = mem::replace(ready(0));
let value = fut_unpinned.into_inner(); // panic!

That said, I think unwrap would also be fine!

what's the argument against naming this unwrap?

Other than the precedent of the futures crate, the only argument I can think of against it is that it would confuse the reader into thinking there were a Result or Option involved.


In any case, there was no concern in WG-async about having this method in the first place. Ultimately it's up to @rust-lang/libs-api to decide what to name it.

@traviscross
Copy link
Contributor

Agreed. In prior discussion and on the call, some did have reservations about this API due to it peeking through the abstraction by giving a way to get the output of the Future other than by polling or awaiting it, but we discussed on the call how, in Rust, methods often poke through such abstractions when we know the specific type. On that basis, I think everyone did end up agreeing that this was OK.

So if you're now happy with the fact that this can panic, then it's likely there exists a WG-async consensus that this is a fine thing to have in some form, though I'm not entirely certain whether any other members had concerns about the panic behavior or about the name as we didn't drive this discussion to consensus.

While it is of course T-libs-api's final call here, it's also probably OK for WG-async, for things within our scope, to provide input or raise concerns on the name also, if we have them, especially as the name relates to semantics like panicking. Speaking just for myself, I'm still interested in an answer to the nominated question or alternatively consideration of naming this unwrap, especially on the basis that the author here, @daxpedda, agrees with that.

@tmandry
Copy link
Member

tmandry commented Dec 16, 2023

reservations about this API due to it peeking through the abstraction

Sorry, I'd forgotten about this or I would have mentioned it. Agreed that it seemed like there was consensus by the end.

it's also probably OK for WG-async, for things within our scope, to provide input or raise concerns on the name also, if we have them

Of course, I agree, and I hope that input is useful! I just don't want to imply that @rust-lang/libs-api should block on us reaching full consensus about something that isn't ultimately our decision, if they already feel prepared to decide.

@tmandry
Copy link
Member

tmandry commented Jan 4, 2024

I agree with @dtolnay. The parallel to Option is not compelling to me because the use case is so different.

That said, the rest of wg-async should have a chance to respond.

@rustbot label I-async-nominated

@rustbot rustbot added the I-async-nominated Nominated for discussion during an async working group meeting. label Jan 4, 2024
@traviscross
Copy link
Contributor

traviscross commented Jan 4, 2024

@rustbot labels +I-async-nominated

Nominating for WG-async so we can discuss @dtolnay's thoughtful response above.

[Edit: Didn't see @tmandry's comment above until after posting.]

Regarding use cases, we'd be interested to see those also. It would be fair to say that in our discussions, no-one on WG-async expressed a strong feeling that this method was particularly needed. We agreed only that we could imagine use cases.


Speaking just for myself here:

[Edit 2024-01-07]: This post goes on to argue why we should call this unwrap. But I no longer believe that's the best path forward. Instead, I propose here that we should indeed call this Ready::into_inner (including for the reasons that @dtolnay clearly explained) but that it should return Option. The new proposal does reference some arguments made here about the close analogy between Ready and std::iter::Once.

I think the application of "async things should follow the conventions of the sync thing" to Ready::unwrap does not point in such a conclusive direction, or at least, that connection was not compelling to me. The failure mode of this reasoning is that it acts as if the pre-existing synchronous version of:

async fn ready() -> T {
    T
}

is:

fn ready() -> Option<T> {
   Some(T)
} 

which does not seem like the right parallel to draw.

This is not the parallel we're drawing, or at least, it's now how I see it.

When we analogize Ready<T> to Option<T>, we're comparing the types, not the function ready. Ready is essentially just an Option on which we've implemented Future.

Similarly, and discussed below, std::iter::Once is an Option on which we've implemented Iterator.

There's really no conceptual reason that we couldn't someday directly implement Future<Output = T> for Option<T>.1 This is the sense in which we expect analogous behavior from Option::whatever() and Ready::whatever(). Ready is the "async-flavored version of Option" in the same way that Once in the "iterative-flavored version of Option.


Speaking more generally, the fundamental parallel we're drawing is that the method consumes self exposing the wrapped value and may panic.

Put differently, if there were Unwrap and IntoInner traits, based on existing use in the standard library and the ecosystem, it seems likely they'd be defined as:

pub trait Unwrap<T> {
    /// Returns the wrapped value, consuming the `self` value.
    ///
    /// # Panics
    ///
    /// This function may panic if the wrapped value cannot be returned.
    fn unwrap(self) -> T;
}

pub trait IntoInner<T> {
    /// Consumes the `self` value, returning the wrapped value.
    fn into_inner(self) -> T;
}

Whether something can panic, even if unlikely, is still important. In certain safety-relevant contexts, people try to statically ensure that panics cannot occur. A language like Rust (or maybe Rust itself one day) could conceivably want to encode whether a function can panic as part of its type signature (e.g. whether a function is total).

Given these traits, it seems clear that Ready would implement Unwrap and not IntoInner.

If someone using Rust observes that we've violated the contract of those implicitly-understood (but not type system enforced) "traits" (i.e. contracts), but only for async Rust, then we've failed to allow users to "carry over their intuitions from synchronous Rust." And we've made it possible that people will "hit unpleasant surprises that only happen when using the asynchronous version of a thing."


As touched on above, the best synchronous analogy to pub fn ready<T>(t: T) -> Ready<T> in the standard library is actually std::iter::once:

pub fn once<T>(value: T) -> Once<T>

(This analogy is closer than it may first appear. The state machine of a Future, as exposed by poll, is necessarily iterative.)

If we were adding a similar method to Once, would we call it unwrap or into_inner?

Perhaps I'm wrong, but it just seems difficult to imagine that we'd call it into_inner despite the fact that it would panic if it were iterated prior to the method call.2

Footnotes

  1. Though conventionally and perhaps for practical reasons we may be more likely to implement IntoFuture for it.

  2. If we were to go ahead and add Once::into_inner, I'd object to that name personally on the grounds of convention, but I would withdraw the async-flavored part of my objection.

@tmandry
Copy link
Member

tmandry commented Jan 5, 2024

If we were adding a similar method to Once, would we call it unwrap or into_inner?

Perhaps I'm wrong, but it just seems difficult to imagine that we'd call it into_inner despite the fact that it would panic if it were iterated prior to the method call.2

I agree that if we were to add such a method to Once it would probably be called the same thing as in the Ready case, but think it would still make sense to call it into_inner for all the same reasons discussed in this thread.

The choice, as I see it, is between breaking one of two conventions:

  • unwrap() is only used on Result and Option types.
  • into_inner() never panics, returning an Option instead.

I would place more weight on the unwrap() convention because it is so fundamental to Rust code, which I think is what @dtolnay was getting at.

Regarding use cases, we'd be interested to see those also. It would be fair to say that in our discussions, no-one on WG-async expressed a strong feeling that this method was particularly needed. We agreed only that we could imagine use cases.

Yeah, we should really make sure the motivation is clear before diving too deeply into bikeshedding 😅

@traviscross
Copy link
Contributor

traviscross commented Jan 7, 2024

Setting aside for the moment that we should really see use cases here, I'd like to supersede my proposal above with this proposal that we name it Ready::into_inner and return Option.


An alternative we should strongly consider is naming this into_inner (for the reasons @dtolnay described in his excellent analysis) and then returning Option.

This was discussed earlier, but perhaps not enough, and not in this context. Let me make this case on the grounds of existing convention, how that applies to preserving symmetry as we make things async, and on practicality.

Existing convention

Consider the signature of OnceCell::into_inner:

    /// Consumes the cell, returning the wrapped value.
    ///
    /// Returns `None` if the cell was empty.
    pub fn into_inner(self) -> Option<T> { .. }

As with Ready or with iter::Once, the OnceCell may or may not have a value contained within. If it does not, then we return Option rather than panicking. This preserves the convention that things named "into" don't panic.

APIs that similarly return fallibly from into_inner include:

How this applies to async

There is a strong analogy between std::iter::Once and Ready, as described earlier.

There is a weaker but still compelling analogy between OnceCell and iter::Once. Where Once can be read once (by iteration) and its into_inner would fail after that read, OnceCell can be written once and its into_inner fails before that write (or after take has been called on it).

Given this, it would seem strange for OnceCell::into_inner to return fallibly but for iter::Once::into_inner to panic.

And if that's true, then it would also be inconsistent for the asynchronous version, Ready::into_inner, to panic.

Practicality

We'll hopefully find out when we see more use cases, but it would be surprising if calls to this method were so widespread that writing .into_inner().unwrap() (instead of into_inner()) would be too much of an imposition.

Returning Option here means that people can also write .into_inner().expect("..").

Since we'd be returning Option, we don't have to consider adding various other methods from Option onto Ready.

If we didn't return Option (but called this into_inner), then later decided that we did need a method that returned Option, we would have already used up the appropriate name.

Returning fallibly here is more expressive, as otherwise there is no zero-cost way to construct this Option from a Ready value, as @dtolnay pointed out in an earlier comment.

Proposal

In the earlier comment above, I argued why the method should be called unwrap. But actually, I would no longer propose that. I'd instead propose that it should be called Ready::into_inner but that it should return Option.

This would address both the considerations that @dtolnay raised above and (I would suggest) the concerns raised by WG-async.

As @tmandry noted:

The choice, as I see it, is between breaking one of two conventions:

  • unwrap() is only used on Result and Option types.
  • into_inner() never panics, returning an Option instead.

As we often do in Rust, let's choose having both good things by breaking neither convention. Returning Option from into_inner does that.

@traviscross
Copy link
Contributor

WG-async discussed this today and agreed that we would like to see example use cases of either the proposed standard library version or the comparable version in futures-rs.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 16, 2024

E.g. GitHub search doesn't show a single use of Ready::into_inner():
https://github.com/search?q=language%3ARust+symbol%3AReady%3A%3Ainto_inner&type=code

Personally I don't know of one either, the one I mentioned before was a potential use-case that was prevented by the fact that Ready::into_inner() isn't in Std and depending on futures-util wasn't an option.

@Arnavion
Copy link

At $dayjob we have a custom Future impl that looks like:

enum Request<A: Future> {
    Ready { value: Option<A::Output> },
    Timeout { inner: Box<tokio::time::Timeout<A>> },
    InfiniteTimeout { inner: A },
}

impl<A: Future> Future for Request<A> {
    type Output = Result<A::Output, Error>;

    fn poll(...) {
        match self {
            Self::Ready { value } => Poll::Ready(Ok(value.take().expect("polled after completion"))),
            ...

If std::future::Ready::into_inner (or whatever you want to call it) is stabilized, we could change the Request::Ready variant to use inner: std::future::Ready<A::Output> instead of value: Option<A::Output>, and delegate to inner.poll() instead of implementing it manually with Poll::Ready(value.take().expect(...)).


There is also one function that returns Request<impl Future<Output = TypeA>>, and another function that returns Request<impl Future<Output = TypeB>> where TypeB is convertible from TypeA using .into():

fn request_a(...) -> Request<impl Future<Output = TypeA>> { ... }

fn request_b(...) -> Request<impl Future<Output = TypeB>> {
    let a = request_a(...);
    match a {
        Request::Ready { value } => Request::Ready { value: /* Option<TypeA> -> Option<TypeB> */ value.map(Into::into) },
        ...
    }
}

This would become more complicated if we switched from value: Option<A::Output> to inner: std::future::Ready<A::Output> since it would need to become std::future::ready(inner.into_inner().into()). But this is acceptable, and would become better if Ready gained an fn Ready::<T>::map(self, impl FnOnce(T) -> U)) -> Ready<U> in the future.


In either case this code will only run before the Request future has resolved. So it is okay for Ready::into_inner (or whatever you call it) to be infallible (ie panic if already resolved) instead of returning Option / Result. The second case above happens to work even if the Request future has already been resolved (the value: None::<TypeA> will become value: None::<TypeB> without panicking), but our code doesn't use that fact.

@traviscross
Copy link
Contributor

@Arnavion: For your first example, could you talk about where the call to into_inner would be? I see where you would delegate to inner.poll(cx), but not where into_inner comes into play, unless the point is that due to code such as in the second example, you can't do the delegation unless into_inner is available to use elsewhere.

If you could, it may also be worth discussing why this code makes sense at a higher level. E.g., why is the Request holding on to the output in that Option rather than just resolving immediately once it's available?

Finally, perhaps you could discuss why you would prefer to use Ready here. To my eyes, your code already looks optimal given what it's trying to achieve. While the delegation in the first example might save a few characters, it complicates the second example as you mention, and it starts to demand importing other methods on Option such as map onto Ready, as you suggest.

In the first example, if we wanted to make that future fused, which would be a reasonable thing to want, then we'd be back to either writing it as you did there or needing into_inner to return Option (and using that instead of delegating to poll).

@Arnavion
Copy link

Arnavion commented Feb 3, 2024

unless the point is that due to code such as in the second example, you can't do the delegation unless into_inner is available to use elsewhere.

Yes sorry, that was indeed the point. I had edited my post to move that "this first code could use inner.poll" paragraph up near the code block, but missed that the context would be lost.

E.g., why is the Request holding on to the output in that Option rather than just resolving immediately once it's available?

The Request::Ready variant functions the same as std::future::Ready, so it uses an Option for the same reason, ie because Future::poll only gets &mut self.

Finally, perhaps you could discuss why you would prefer to use Ready here. To my eyes, your code already looks optimal given what it's trying to achieve.

The reason to use Ready in the code is purely to make it obvious what it's doing. Currently we have to "manually implement" the functionality of Ready using .take().expect(). Your question of "Why does it need to use an Option?" would not have happened if the code used Ready. The .expect() causes any dev reading the code to be interrupted by the possibility of a panic, and take a second to confirm to themselves that it's okay. If it was just Self::Ready { inner } => inner.poll(cx) then it's obviously a wrapper delegating to its wrappee and there's nothing complicated to think about.

and it starts to demand importing other methods on Option such as map onto Ready, as you suggest.

"Demand" sure, but at least it's not "requires". In my opinion switching to Ready and dealing with ready(.into_inner().into()) is still preferable than sticking with Option and dealing with .take().expect().

@dtolnay
Copy link
Member

dtolnay commented Mar 31, 2024

I have re-read the counterproposal in #101196 (comment) a couple times in the intervening months, and the analogy being drawn there of core::future::Ready to core::iter::Once has not been compelling to me.

The API contract for iterators is that you can call next on them, which will return Some zero or more (potentially infinite) times followed by None once, after which subsequent calls might perpetually return None or might start returning Some again. If the iterator has a FusedIterator impl, the calls after the first None are guaranteed to perpetually return more None.

Using next to consume an item, then handing off the iterator to other code to operate with the remaining items, is 100% normal and correct usage, such as in the below code I wrote the other day. (If the iterator might already have produced None before you hand it off to elsewhere, then whether doing so is sensible hinges on whether there is a FusedIterator impl, but this is not the case here because we know there has not been a None yet.)

let rustc_wrapper = env::var_os("RUSTC_WRAPPER");
let rustc_workspace_wrapper = env::var_os("RUSTC_WORKSPACE_WRAPPER");
let rustc = env::var_os("RUSTC").unwrap();

let mut wrapped_rustc = rustc_wrapper
    .into_iter()
    .chain(rustc_workspace_wrapper)
    .chain(iter::once(rustc));

let mut cmd = Command::new(wrapped_rustc.next().unwrap());
cmd.args(wrapped_rustc);

The API contract for futures is that you can call poll on them, which will return Pending zero or more (potentially infinite) times followed by Ready once, after which the future is no longer considered useful. Subsequent poll calls are expected to panic or block forever or cause other kinds of problems, although individual implementations may uphold additional bespoke guarantees such as futures::future::Fuse or core::future::poll_fn.

Polling a future, having it return Ready, then (unpinning and) handing off that future to elsewhere is not 100% normal and correct usage, unlike the equivalent iterator scenario. This is in "doing it wrong" territory, i.e. "bug in the program" territory. Panicks are the exact mechanism designated for signalling that a bug in the program has been detected, whereas Result/Option are the mechanisms for handling anticipated failure modes in a correct program.

Adapted from Tyler's comment in #101196 (comment), this is the kind of code we are concerned with:

#![feature(noop_waker, ready_into_inner)]

use core::future::{self, Future as _};
use core::mem;
use core::pin::pin;
use core::task::{Context, Waker};

fn main() {
    let mut ready = future::ready("...");
    let mut fut = pin!(ready);
    let _ready = fut.as_mut().poll(&mut Context::from_waker(&Waker::noop()));
    let fut_unpinned = mem::replace(&mut *fut, future::ready(""));
    let _crash = fut_unpinned.into_inner();
}

For a future like Fuse, post-Ready stuff can be done in a way that is not a bug, but there is no general expectation for other futures (including Ready<T>) that anything other than dropping would perform a useful function.

Now looking at the real-world example we have from Arnavion in #101196 (comment), it shows 2 places that the signature of into_inner would come into play: in Request::poll and in request_b.

Having an into_inner signature that returns Option<T> is useful if there is an anticipated failure mode that the code inside Request::poll and request_b is responsible for handling. This is not the case here. The way a "None" would end up here is if someone took a Ready<T>, polled it once to obtain Poll::Ready, unpinned it, stuck it into Request, then pinned and polled that. It's a bug but it's not a bug in Request::poll, and it's not an "expected failure mode in a correct program" that Request::poll should be expected to consider.

Having an into_inner signature that returns T is useful if it leaves error handling (expect, unwrap, match, ?, ...) to just codepaths that are anticipated failure modes in a correct program, such as the ? in #101196 (comment) and similar scenario in wgpu from #101196 (comment). Making the user reach for expect/? for something that we know would not be an expected failure mode wouldn't be a good design.

The scenario of code being given a Ready<T> that has already been polled, without being informed in some other way whether it refers to a usable Ready<T> or not, is a bug in whatever thing failed to arrange for the Ready<T> to be dropped promptly after it had been polled and returned ready. That same bug would have been surfaced just by calling .await on the errant Ready<T>, which is an interface that likewise does not make the user accommodate the case that the future being awaited has previously completed already. I am advocating that into_inner also should not require the caller to accommodate the presence of this bug in other code.

@traviscross
Copy link
Contributor

traviscross commented May 26, 2024

At this point, mainly I'm just not sure there's much of a case to be made for Ready::into_inner in any form. The use cases that have been described so far don't seem particularly compelling; as discussed here, it seems not particularly worse to write these in other ways.

We all, I think, agree that if None cannot be returned from a function except if the program is incorrect, then that function should instead panic.1

In terms of whether a correct program can be written that makes use of None being returned from Ready::into_inner, here's the first one that came to mind:

// Here, we demonstrate a *correct* program that uses the `None` return
// value of `Ready::into_inner`.
fn main() {
    let mut xs: Vec<Ready<u8>> = (0..=255).map(ready).collect();
    let mut idx = rand(1);
    // First, for whatever reason, we want to poll 128 random futures
    // in the vector.
    for _ in 0..128 {
        idx = rand(idx); // The index is guaranteed to not repeat.
        assert_eq!(Poll::Ready(idx), poll_once(&mut xs[idx as usize]));
    }
    // Then, for whatever reason, we want to print out the values from
    // the ready futures we *didn't* poll.  We can do this because
    // `Ready::into_inner` returns an `Option`.
    for x in xs.drain(..).filter_map(Ready::into_inner) {
        //                           ^^^^^^^^^^^^^^^^^
        //                           ^ Returns `Option<u8>`.
        println!("{x:?}");
    }
}

(Full details and supporting code here: Playground link. Note in particular that there is no unsafe code and we do not rely on any extensions to the contract of Future.)

Now, of course, we could argue that maybe the futures should each be wrapped in an Option and dropped immediately after they're used in that first loop. That's probably how I'd write it in general. But maybe there's some reason to do this as above. Maybe we care about the drop order, or maybe we're trying to hit some performance goal by batching our drop operations. Maybe we care about the overhead of the "extra" Option. Who knows? The only point is that the program above is correct and at least minimally plausible.

Since the motivation for Ready::into_inner in any form seems itself only barely plausible to me currently, that's enough to make me want to do the more expressive thing, if we were to add this at all. But of course, maybe the right path is just to not do it, at all. As in the linked demo here, it's simple enough to write one's own Ready future.

Footnotes

  1. As an interesting side note, regarding the contract of Iterator, I'd highly recommend this thread. Many plugged-in people, it seems, expect that next may panic if called again after returning None despite this not being stated (at least not clearly) in the documentation.

@joshtriplett
Copy link
Member

@traviscross The proposed use case is this: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60Ready.3A.3Ainto_inner.28.29.60/near/295675503 - making an async API that isn't actually async into a sync API.

@Amanieu
Copy link
Member

Amanieu commented Aug 6, 2024

@BurntSushi This has been reviewed by WG-async (multiple times) so the concern should be resolved now.

@traviscross
Copy link
Contributor

@rustbot labels -I-async-nominated

Agreed there's nothing more for async to discuss here. Unnominating.

@rustbot rustbot removed the I-async-nominated Nominated for discussion during an async working group meeting. label Aug 6, 2024
@BurntSushi
Copy link
Member

@rfcbot resolve async wg review

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 6, 2024
@rfcbot
Copy link

rfcbot commented Aug 6, 2024

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

@Skgland
Copy link
Contributor

Skgland commented Aug 7, 2024

Which version is currently proposed? With all that back and forth I am confused which is current.

  1. Ready<T>::unwrap() -> T
  2. Ready<T>::into_inner() -> T
  3. Ready<T>::into_inner() -> Option<T>

Currently in nightly appears to be 2. the stabilization PR currently includes changes to make it 1. and the discussion reads to me as if the current proposal is 3.

@dtolnay
Copy link
Member

dtolnay commented Aug 7, 2024

Number 2, I believe.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 16, 2024
@rfcbot
Copy link

rfcbot commented Aug 16, 2024

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.

This will be merged soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 16, 2024
…r, r=dtolnay

Stabilize `Ready::into_inner()`

This PR stabilizes `Ready::into_inner()`.

Tracking issue: rust-lang#101196.
Implementation PR: rust-lang#101189.

Closes rust-lang#101196.
@bors bors closed this as completed in 67d0973 Aug 17, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
Development

Successfully merging a pull request may close this issue.