Tracking Issue for Ready::into_inner()
#101196
Comments
…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.
…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.
|
@joshtriplett would it be acceptable to start an FCP for this? Or are there any outstanding concerns? |
|
@rust-lang/libs-api: Simple function for unpackaging a The contentious part is that this can panic ("Called `into_inner()` on `Ready` after completion") which is unusual for a function named I feel this was addressed convincingly enough by #101189 (comment): "in the
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 |
|
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. |
|
cc @rust-lang/wg-async |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
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! |
|
@rfcbot concern async wg review |
|
WG-async discussed this in our recent triage meeting, and we plan to discuss it again later this week. |
|
@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 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 We felt that In that light, we discussed that it may be reasonable someday to have We were also persuaded by the fact that other 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 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 In the WG-async discussion, @tmandry had concerns about this being the only Regarding that, the point that @daxpedda advanced here is that this is a fundamental property of
So the question is, what's the argument against naming this The thinking here is as follows: There are concerns about the If |
|
I actually believe that The only reason why
I don't have one and I'm actually in favor of changing the name to |
|
I did raise concerns about 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
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. |
|
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 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 |
Sorry, I'd forgotten about this or I would have mentioned it. Agreed that it seemed like there was consensus by the end.
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. |
|
@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
This is not the parallel we're drawing, or at least, it's now how I see it. When we analogize Similarly, and discussed below, There's really no conceptual reason that we couldn't someday directly implement Speaking more generally, the fundamental parallel we're drawing is that the method consumes Put differently, if there were 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 Given these traits, it seems clear that 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 once<T>(value: T) -> Once<T>(This analogy is closer than it may first appear. The state machine of a If we were adding a similar method to Perhaps I'm wrong, but it just seems difficult to imagine that we'd call it Footnotes |
I agree that if we were to add such a method to The choice, as I see it, is between breaking one of two conventions:
I would place more weight on the
Yeah, we should really make sure the motivation is clear before diving too deeply into bikeshedding 😅 |
|
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 An alternative we should strongly consider is naming this 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 conventionConsider the signature of /// Consumes the cell, returning the wrapped value.
///
/// Returns `None` if the cell was empty.
pub fn into_inner(self) -> Option<T> { .. }As with APIs that similarly return fallibly from
How this applies to asyncThere is a strong analogy between There is a weaker but still compelling analogy between Given this, it would seem strange for And if that's true, then it would also be inconsistent for the asynchronous version, PracticalityWe'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 Returning Since we'd be returning If we didn't return Returning fallibly here is more expressive, as otherwise there is no zero-cost way to construct this ProposalIn the earlier comment above, I argued why the method should be called This would address both the considerations that @dtolnay raised above and (I would suggest) the concerns raised by WG-async. As @tmandry noted:
As we often do in Rust, let's choose having both good things by breaking neither convention. Returning |
|
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 |
|
E.g. GitHub search doesn't show a single use of 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 |
|
At $dayjob we have a custom 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 There is also one function that returns 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 In either case this code will only run before the |
|
@Arnavion: For your first example, could you talk about where the call to If you could, it may also be worth discussing why this code makes sense at a higher level. E.g., why is the Finally, perhaps you could discuss why you would prefer to use 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 |
Yes sorry, that was indeed the point. I had edited my post to move that "this first code could use
The
The reason to use
"Demand" sure, but at least it's not "requires". In my opinion switching to |
|
I have re-read the counterproposal in #101196 (comment) a couple times in the intervening months, and the analogy being drawn there of The API contract for iterators is that you can call Using 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 Polling a future, having it return 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 Now looking at the real-world example we have from Arnavion in #101196 (comment), it shows 2 places that the signature of Having an Having an The scenario of code being given a |
|
At this point, mainly I'm just not sure there's much of a case to be made for We all, I think, agree that if In terms of whether a correct program can be written that makes use of // 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 Now, of course, we could argue that maybe the futures should each be wrapped in an Since the motivation for Footnotes
|
|
@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. |
|
@BurntSushi This has been reviewed by WG-async (multiple times) so the concern should be resolved now. |
|
@rustbot labels -I-async-nominated Agreed there's nothing more for async to discuss here. Unnominating. |
|
@rfcbot resolve async wg review |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
Which version is currently proposed? With all that back and forth I am confused which is current.
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. |
|
Number 2, I believe. |
|
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. |
…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.
Feature gate:
#![feature(ready_into_inner)]This is a tracking issue for
Ready::into_inner().Adds a method to
Readythat unwraps the value without an asynchronous context. See thefutureslibrary for an equivalent function.Public API
Steps / History
Ready::into_inner()#101189Ready::into_inner()#116528Unresolved Questions
Footnotes
https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩
The text was updated successfully, but these errors were encountered: