-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Introduce support for async gen
blocks
#118420
Conversation
Importantly, this still needs a test 💀 I'll come up with one that uses a no-op waker or something. |
This comment has been minimized.
This comment has been minimized.
fbd39f9
to
654e0ba
Compare
☔ The latest upstream changes (presumably #118473) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot labels +I-lang-nominated Nominating based on discussion to confirm that we're happy for |
654e0ba
to
b45e065
Compare
This comment has been minimized.
This comment has been minimized.
We discussed this briefly in the lang triage meeting today, and agreed that this is free to continue for nightly. (It would, of course, need an RFC before it could be considered for stabilization.) Personally, I'd say that combining existing things is almost always fine for experimentation in nightly, especially if it's something that T-compiler considers "not difficult". |
@rustbot labels -I-lang-nominated As scottmcm said, the consensus was that this is OK to land. Removing the nomination. |
b45e065
to
4155db2
Compare
I believe this is ready for review r? @eholk, though please reassign if you're busy |
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
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.
Looks good. Just some minor questions/suggestions that you can take or not. Nothing blocking, so feel free to r=me after making any changes you want to take.
pub fn async_gen_pending() -> Self { | ||
Poll::Pending | ||
} |
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.
Out of curiosity, could this be a constant like FINISHED
? Or is there not much benefit to doing that?
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.
Yes, but in the process, I found a bug in hir typeck.
4155db2
to
d9d9aa3
Compare
This comment has been minimized.
This comment has been minimized.
d9d9aa3
to
a8c7761
Compare
@@ -771,6 +771,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
let args = self.fresh_args_for_item(span, def_id); | |||
let ty = item_ty.instantiate(self.tcx, args); | |||
|
|||
self.write_args(hir_id, args); |
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.
@bors r=eholk |
a8c7761
to
11375c8
Compare
Rebased @bors r=eholk |
☀️ Test successful - checks-actions |
Finished benchmarking commit (f967532): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.909s -> 676.179s (0.19%) |
I have literally no idea why this would be a perf regression. Any ideas, @rust-lang/wg-compiler-performance? |
In any case, I don't think that the regression is especially bad, cycles and wall-time are a wash, and this PR does introduce new functionality/feature after all. |
Introduce support for `async gen` blocks I'm delighted to demonstrate that `async gen` block are not very difficult to support. They're simply coroutines that yield `Poll<Option<T>>` and return `()`. **This PR is WIP and in draft mode for now** -- I'm mostly putting it up to show folks that it's possible. This PR needs a lang-team experiment associated with it or possible an RFC, since I don't think it falls under the jurisdiction of the `gen` RFC that was recently authored by oli (rust-lang/rfcs#3513, rust-lang#117078). ### Technical note on the pre-generator-transform yield type: The reason that the underlying coroutines yield `Poll<Option<T>>` and not `Poll<T>` (which would make more sense, IMO, for the pre-transformed coroutine), is because the `TransformVisitor` that is used to turn coroutines into built-in state machine functions would have to destructure and reconstruct the latter into the former, which requires at least inserting a new basic block (for a `switchInt` terminator, to match on the `Poll` discriminant). This does mean that the desugaring (at the `rustc_ast_lowering` level) of `async gen` blocks is a bit more involved. However, since we already need to intercept both `.await` and `yield` operators, I don't consider it much of a technical burden. r? `@ghost`
…ler-errors Hide async_gen_internals from standard library documentation These are from rust-lang#118420. It doesn't appear that there is any intention to ever make these APIs available to user code. These are just conveniences meant for the compiler's implementation of `async gen`. I don't think having them featured in documentation in <https://doc.rust-lang.org/1.77.1/core/task/enum.Poll.html> is appropriate. ![image](https://github.com/rust-lang/rust/assets/1940490/0a8ae90d-5c83-4ab1-b08a-50bad2433d69)
Rollup merge of rust-lang#123528 - dtolnay:asyncgeninternals, r=compiler-errors Hide async_gen_internals from standard library documentation These are from rust-lang#118420. It doesn't appear that there is any intention to ever make these APIs available to user code. These are just conveniences meant for the compiler's implementation of `async gen`. I don't think having them featured in documentation in <https://doc.rust-lang.org/1.77.1/core/task/enum.Poll.html> is appropriate. ![image](https://github.com/rust-lang/rust/assets/1940490/0a8ae90d-5c83-4ab1-b08a-50bad2433d69)
Introduce support for `async gen` blocks I'm delighted to demonstrate that `async gen` block are not very difficult to support. They're simply coroutines that yield `Poll<Option<T>>` and return `()`. **This PR is WIP and in draft mode for now** -- I'm mostly putting it up to show folks that it's possible. This PR needs a lang-team experiment associated with it or possible an RFC, since I don't think it falls under the jurisdiction of the `gen` RFC that was recently authored by oli (rust-lang/rfcs#3513, rust-lang#117078). ### Technical note on the pre-generator-transform yield type: The reason that the underlying coroutines yield `Poll<Option<T>>` and not `Poll<T>` (which would make more sense, IMO, for the pre-transformed coroutine), is because the `TransformVisitor` that is used to turn coroutines into built-in state machine functions would have to destructure and reconstruct the latter into the former, which requires at least inserting a new basic block (for a `switchInt` terminator, to match on the `Poll` discriminant). This does mean that the desugaring (at the `rustc_ast_lowering` level) of `async gen` blocks is a bit more involved. However, since we already need to intercept both `.await` and `yield` operators, I don't consider it much of a technical burden. r? `@ghost`
I'm delighted to demonstrate that
async gen
block are not very difficult to support. They're simply coroutines that yieldPoll<Option<T>>
and return()
.This PR is WIP and in draft mode for now -- I'm mostly putting it up to show folks that it's possible. This PR needs a lang-team experiment associated with it or possible an RFC, since I don't think it falls under the jurisdiction of thegen
RFC that was recently authored by oli (rust-lang/rfcs#3513, #117078).Technical note on the pre-generator-transform yield type:
The reason that the underlying coroutines yield
Poll<Option<T>>
and notPoll<T>
(which would make more sense, IMO, for the pre-transformed coroutine), is because theTransformVisitor
that is used to turn coroutines into built-in state machine functions would have to destructure and reconstruct the latter into the former, which requires at least inserting a new basic block (for aswitchInt
terminator, to match on thePoll
discriminant).This does mean that the desugaring (at the
rustc_ast_lowering
level) ofasync gen
blocks is a bit more involved. However, since we already need to intercept both.await
andyield
operators, I don't consider it much of a technical burden.r? @ghost