-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Check coroutine upvars in dtorck constraint #144156
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
base: master
Are you sure you want to change the base?
Conversation
Yeah, I guess the resume ty doesn't need to be checked since although it can contain lifetimes, it would need to be stored in a witness ty to actually be dropped (since it doesn't have its own storage) and so it's redundant wrt the witness part of this drop check. |
bde52d9
to
a6cc0fe
Compare
compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
Outdated
Show resolved
Hide resolved
a6cc0fe
to
f2e14da
Compare
compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
Outdated
Show resolved
Hide resolved
f2e14da
to
f1410f8
Compare
Tweaked the comment a bit more and implemented the modified check that only requires the upvars be drop-live if there are no drops in the interior. |
Also tweaked this comment to use "interior" rather than flopping between "witness" and "interior". TBH i kinda prefer that name over witness, and at least for the purposes of this explanation it makes it clear that it's types that are local to the body of the coroutine. |
f1410f8
to
9b0b433
Compare
Also added a test for the upvar breakage. |
Hi @rust-lang/types, I previously assumed that every type stored in a coroutine is part of its witness. This assumption caused us to only require things to be live when dropping the coroutine if the witness required drop in #117134. This is not the case as we may capture upvars by value. These upvars are not considered part of the witness, but still need to get dropped when dropping the coroutine. This PR updates the comment to better explain what's going on and fixes this issue. We now require all upvars to be drop-live regardless of whether the coroutine witness has anything that needs to be drop. @rfcbot fcp merge I think we should still do a crater run? |
Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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. |
@bors2 try |
Check coroutine upvars in dtorck constraint
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
// if they were like trait objects, where its upvars must | ||
// all be alive for the coroutine's (potential) | ||
// destructor. | ||
// Note also that this check requires that the coroutine's upvars are use-live, since |
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.
For T-types reading this: Use-live means that all of the regions in a type must be live. Drop-live means that all of regions except for those in a #[may_dangle]
arg must be live (and some rules about built-in types, e.g. &'1 ()
doesn't require '1
to be live since it is never Drop
)
constraints.outlives.extend(args.upvar_tys().iter().map(ty::GenericArg::from)); | ||
constraints.outlives.push(args.resume_ty().into()); | ||
} else { | ||
// Even if a witness type doesn't need a drop, we still require 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.
For T-types reading this: here is the "important" part of the PR. The rest is just modifying a comment to explain the behavior better, so the needs_drop
branch of this if
statement makes sense, and it can be conceptually contrasted to what I added below.
Specifically, even though the witness types don't need drop, we still need to treat this like something that captures upvars. Compare this branch to the closure case above.
But importantly, we only need to treat these upvars as drop-live. See the drop-live-upvar-2
test to demonstrate how this is important.
Fix #144155.
This PR fixes an unsoundness where we were not considering coroutine upvars as drop-live if the coroutine interior types (witness types) had nothing which required drop.
In the case that the coroutine does not have any interior types that need to be dropped, then we don't need to treat all of the upvars as use-live; instead, this PR uses the same logic as closures, and descends into the upvar types to collect anything that must be drop-live. The rest of this PR is reworking the comment to explain the behavior here.
r? @lcnr or reassign 😸
Just some thoughts --- a proper fix for this whole situation would be to consider
TypingMode
in theneeds_drop
function, and just callingcoroutine_ty.needs_drop(tcx, typing_env)
in the dtorck constraint check.During MIR building, we should probably use a typing mode that stalls the local coroutines and considers them to be unconditionally drop, or perhaps just stall all coroutines in analysis mode. Then in borrowck mode, we can re-check
needs_drop
but descend into witness types properly. #144158 implements this experimentally.This is a pretty involved fix, and conflicts with some in-flight changes (#144157) that I have around removing coroutine witnesses altogether. I'm happy to add a FIXME to rework this whole approach, but I don't want to block this quick fix since it's obviously more correct than the status-quo.