-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Rename AsyncIterator
back to Stream
, introduce an AFIT-based AsyncIterator
trait
#119550
base: master
Are you sure you want to change the base?
Conversation
The current formulation of `AsyncIterator` is not "the async version of iterator". It is closer to: "the async version of lending iterator". This is because the `poll_next` method pins the future states, as well as the generator state. This is the first in two commits: in the second commit we will reintroduce the `AsyncIterator` trait which does not pin the generator state - accurately encoding an "async version of iterator". The intent is for both implementations to exist side-by-side on nightly, giving us an opportunity to experiment with the different formulations and analyze the trade-offs.
This is the second commit in the series, introducing an Async Functions in Traits based `AsyncIterator` trait. This is a necessary step towards experimenting with AFIT-based `async gen {}` blocks, and `for await..in` loops.
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there going to be any bridging impls between the AFIT-based AsyncIterator
and async gen
/Stream
impls? Can such a bridging impl even be written soundly?
library/core/tests/async_iter/mod.rs
Outdated
let mut async_iter = async_iter.into_async_iter(); | ||
|
||
let waker = core::task::Waker::noop(); | ||
let mut cx = &mut core::task::Context::from_waker(&waker); | ||
|
||
assert_eq!(async_iter.as_mut().poll_next(&mut cx), Poll::Ready(Some(0))); | ||
assert_eq!(async_iter.as_mut().poll_next(&mut cx), Poll::Ready(Some(1))); | ||
assert_eq!(async_iter.as_mut().poll_next(&mut cx), Poll::Ready(Some(2))); | ||
assert_eq!(async_iter.as_mut().poll_next(&mut cx), Poll::Ready(None)); | ||
assert_eq!(pin!(async_iter.next()).poll(&mut cx), Poll::Ready(Some(0))); | ||
assert_eq!(pin!(async_iter.next()).poll(&mut cx), Poll::Ready(Some(1))); | ||
assert_eq!(pin!(async_iter.next()).poll(&mut cx), Poll::Ready(Some(2))); | ||
assert_eq!(pin!(async_iter.next()).poll(&mut cx), Poll::Ready(None)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this diff does a pretty good job showing the differences in pinning between the two formulations. Just like with the non-async Iterator
trait, the AFIT-based AsyncIterator
trait doesn't require the iterator to be pinned. Instead it's only the individual futures which have to be pinned. While the Stream
trait requires the entire iterator to be pinned for the entire duration.
One of the things we want to investigate is the precise lowering of async gen
blocks and functions to the AFIT-based AsyncIterator
trait impls. The theory is that both traits should have equivalent lowerings, modulo Stream
being able to hold local borrows across yield
points. This is something that is currently also not allowed in non-async gen
blocks and functions, so by also not allowing it AsyncIterator
is merely consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that is currently also not allowed in non-async gen blocks and functions, so by also not allowing it AsyncIterator is merely consistent.
I don't think this is "merely consistent" -- I consider it to be a really big mistake of the Iterator
trait and a major ergonomic issue that is not present in async {}
blocks (so it's consistent with gen
, but not async
blocks). If we can avoid this for async gen
from the get-go, I have no idea why we wouldn't just do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: On second thought, let me condense what I wanted to say more briefly: I don't think we should get into the details of the different approaches in this PR, and instead focus on answering the question how we can reach a consensus.
I believe that the most productive way of achieving that is by gaining actual implementation experience. That way we can compare both approaches side by side, and decide based on that how we want to proceed.
edit: Unless you're worried that it's not possible to hold values across .await
points in this formulation of AsyncIterator
? Because this explicitly does support that. I agree it would be really bad if that didn't work.
@@ -13,42 +9,15 @@ use crate::task::{Context, Poll}; | |||
#[unstable(feature = "async_iterator", issue = "79024")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've chosen to keep the same issue for both traits, but given them different feature gates. Since stabilizing one means not stabilizing the other, this seemed appropriate to me. Or should we open separate tracking issues for both traits?
This comment has been minimized.
This comment has been minimized.
Since this is just for experimentation, maybe it would be better to instead rename the AFIT-based version to |
@Kobzol It seems confusing to name "AsyncIterator" for something with the same signature as Stream, which is widely used in the ecosystem, and "Stream" for something completely different. Also, the results you linked to seem to include quite a few irrelevant and forked repositories. |
That's orthogonal to what I was proposing I think. What you're describing is the current state. The
That's true, but I also used a very simple query for a simple use statement. We can't know exactly how many people use the current trait (without doing e.g. a crater run, which would be overkill), but there are definitely some users, and I thought that it might be a good idea to not break them unnecessarily. |
In order to serve this goal well, I propose that the documentation for both traits should explicitly mention that they both exist and that one might try using the other one. This will encourage experimentation and feedback from people who merely use whatever they find in nightly docs without also following rust-lang development activity — who would then probably use whichever one they met first without exploring alternatives. |
@compiler-errors I was thinking we shouldn't - given we would only want to stabilize one of the traits, it seems preferable to me if both impls would work standalone. That way we can accurately evaluate them as competing designs without accidentally being limited by one another. I'm not sure to which degree that can be done though - I'm not a compiler expert - so please feel free to provide a reality check here. |
This comment has been minimized.
This comment has been minimized.
@kpreid good call! - done |
This comment has been minimized.
This comment has been minimized.
@rustbot labels +T-lang In the same spirit that the However, I'm going to withhold a T-lang nomination because... @rustbot labels +WG-async +I-async-nominated The async working group should first discuss this and develop a consensus on a direction here. |
Its unclear to me how adding an additional nightly trait will help us gain information about which approach is preferred. A vast majority of real usages of The main advantage of the AFIT approach (aiui) is that implementing the trait is significantly easier, but |
@guswynn I went into some detail about why
This is one of the main lessons to take away from the stabilization process of |
@yoshuawuyts I missed that blog post, thanks for the context! Also, TIL about arguments maps (I also wasn't aware they were used previously for consensus-building in the async world). Will take a look! |
|
The plan is for async functions in traits to be able to work with This is what a potential RTN feature would cover, which is an essential building block for all AFITs to work as expected. |
I'm going to T-libs-api nominate this. I'd love to hear their thoughts on this experimentation and if they have any opinions on the trait split here. |
Discussed offline briefly. I'll postpone on nominating this for T-libs-api to give a longer chance to elaborate on an WG-async consensus on how best to move forward with @rustbot labels -I-libs-api-nominated |
After some offline conversations I'm converting this to a draft PR until we have consensus within WG Async how we want to proceed. We may decide that this is the right direction for us, but we may want further code changes. A draft PR better reflects that the shape of the patch may not yet be final. |
Given that these are nightly-only, and given that Why don't we try the experiment of switching AsyncIerator over to |
So one reason that |
@eholk I published a crate for that in 2022 when AFITs first became available on nightly. I wrote a post announcing it was available to try out, and a follow-up nine months later evaluating it against the I don't really know what we would gain from having the crates up for even longer in the ecosystem. The main questions I've been seeing revolve around the integration of the traits with various language features. And the best way I can think of answering those questions is by actually implementing them. |
It's possible to experiment with AFIT and also |
Clarifying something here: I do think we should experiment with an AFIT-based |
It is also already possible to experiment with the usage and API of |
☔ The latest upstream changes (presumably #120136) made this pull request unmergeable. Please resolve the merge conflicts. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #120251) made this pull request unmergeable. Please resolve the merge conflicts. |
Nice, I didn't know |
If we want If we want to be able to compare the two lowerings, then it seems necessary to have both lowerings and both traits in nightly. |
@rustbot labels -T-compiler -T-libs-api This is waiting on lang work, so let's untag other teams for the moment to get it off of their agendas. We'll retag it later as appropriate. |
This PR introduces a new AFIT-based
AsyncIterator
trait, and renames the existingAsyncIterator
trait back toStream
. WG Async does not yet have internal consensus on what the right API for "async iterator" should be. What seems to be missing is actual feedback from implementation and usage. So to that end I'm proposing we temporarily introduce both formulations of the "async iterator" trait.Personally I've been wanting to make this change for several years now, but the lack of stable AFITs has been a limiting factor. Now that AFITs have landed in Rust 1.75 (December 2023), we should be OK to begin experimenting with this in-tree. The reason for choosing the
Stream
name is because in its current form the existing trait isn't actually the "async version of iterator". It's closer to the "async version of pinned iterator", but with future state machine internals exposed. Contrasted with the AFIT-basedAsyncIterator
trait, this formulation seems distinct enough that it warrants having its own name. AndStream
seems like the obvious choice for that.The deep connection between the "async iterator" trait and various language features is why we can't just continue experimenting with it in the ecosystem. Once this PR lands, this will enable WG Async to integrate the AFIT-based version of
AsyncIterator
withasync gen {}
blocks,async gen fn
functions, andfor..await in
loops.ref #79024
cc/ @rust-lang/wg-async @rust-lang/libs-api
r? @compiler-errors, both you and Eric have most recently been working on this code. I've mostly just renamed things, but I figured you might want to take a look. No worries if you're busy though.