Do not consider async task_context to be alive across yields
#105668
Conversation
rustbot
added
S-waiting-on-review
compiler-errors
left a comment
compiler-errors
left a comment
There was a problem hiding this comment.
Interesting solution to this problem. Left a few ideas for ways to harden it, because I'm worried about subtle bugs...
There was a problem hiding this comment.
Is it possible to more strongly assert that the thing that we're intentionally not considering live across the yield is a &mut Context<'_>? Maybe assert on the value of ty: Ty<'tcx> above?
There was a problem hiding this comment.
How do I best assert that though without manually constructing a matching type? I could probably get the expected resume_ty from the generator somehow and use that?
There was a problem hiding this comment.
Can't you destructure the type, check if it's a reference to a specific ADT? The Context type is a lang item, right?
There was a problem hiding this comment.
Yes, did that now, though its a bit more complicated than hoped.
Did also rename some things and added more comments.
There was a problem hiding this comment.
Can you explain why we need both of these checks? Shouldn't we always be accessing this context type through the same local variable? Could you drop in_await_yield and maybe match on expr instead?
I'm thinking that it should be a ExprKind::Path or something... you could see that it resolved to the argument we're expecting it to be.
There was a problem hiding this comment.
I tried matching just on the ExprKind::Path of the lhs of the assignment. The problem is also the yield itself, more specifically the () going into it, and the &mut Context<'_> coming out of it.
Fun fact, by skipping the whole expression, it also avoids putting a () into the generator witness in some cases.
Swatinem
force-pushed
the
async-resume-interior
branch
from
1251ec1 to
c8b1bb1
Compare
|
This does not fix #105501. |
Swatinem
force-pushed
the
async-resume-interior
branch
from
c8b1bb1 to
a653e28
Compare
When lowering async constructs to generators, the resume argument is guaranteed not to be alive across yield points. However the simple `generator_interior` analysis thinks it is. Because of that, the resume ty was part of the `GeneratorWitness` and considered to be part of the generator type, even though it is not really. This prevented async blocks from being `UnwindSafe`, and possibly `Send` in some cases. The code now special cases the fact that the `task_context` of async blocks is never alive across yield points.
Swatinem
force-pushed
the
async-resume-interior
branch
from
a653e28 to
34faed0
Compare
|
Given #105915 reverting the PR that regressed the root issue, not sure if we want to land this as-is. |
|
☔ The latest upstream changes (presumably #105918) made this pull request unmergeable. Please resolve the merge conflicts. |
|
That revert PR probably broke cg_clif again. It is fine for the beta branch, but something like this PR + reverting the revert is necessary for cg_clif. |
|
I do have an idea for cg_clif. Since it is mostly concerned with the MIR after the generators have been properly transformed, which happens after type checking, it should be possible to:
|
|
That would work I think. |
|
Going to close this for now, I will open a separate PR with just the testcase that is good to have. |
6 participants
When lowering async constructs to generators, the resume argument is guaranteed not to be alive across yield points. However the simple
generator_interioranalysis thinks it is. Because of that, the resume ty was part of theGeneratorWitnessand considered to be part of the generator type, even though it is not really.This prevented async blocks from being
UnwindSafe, and possiblySendin some cases.The code now special cases the fact that the
task_contextof async blocks is never alive across yield points.This fixes one regression from #105250.
r? @compiler-errors