-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -319,36 +319,56 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>( | |
} | ||
|
||
ty::Coroutine(_, args) => { | ||
// rust-lang/rust#49918: types can be constructed, stored | ||
// in the interior, and sit idle when coroutine yields | ||
// (and is subsequently dropped). | ||
// rust-lang/rust#49918: Locals can be stored across await points in the coroutine, | ||
// called interior/witness types. Since we do not compute these witnesses until after | ||
// building MIR, we consider all coroutines to unconditionally require a drop during | ||
// MIR building. However, considering the coroutine to unconditionally require a drop | ||
// here may unnecessarily require its upvars' regions to be live when they don't need | ||
// to be, leading to borrowck errors: <https://github.com/rust-lang/rust/issues/116242>. | ||
// | ||
// It would be nice to descend into interior of a | ||
// coroutine to determine what effects dropping it might | ||
// have (by looking at any drop effects associated with | ||
// its interior). | ||
// Here, we implement a more precise approximation for the coroutine's dtorck constraint | ||
// by considering whether any of the interior types needs drop. Note that this is still | ||
// an approximation because the coroutine interior has its regions erased, so we must add | ||
// *all* of the upvars to live types set if we find that *any* interior type needs drop. | ||
// This is because any of the regions captured in the upvars may be stored in the interior, | ||
// which then has its regions replaced by a binder (conceptually erasing the regions), | ||
// so there's no way to enforce that the precise region in the interior type is live | ||
// since we've lost that information by this point. | ||
// | ||
// However, the interior's representation uses things like | ||
// CoroutineWitness that explicitly assume they are not | ||
// traversed in such a manner. So instead, we will | ||
// simplify things for now by treating all coroutines as | ||
// 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 | ||
// a region from a type that does not have a destructor that was captured in an upvar | ||
// may flow into an interior type with a destructor. This is stronger than requiring | ||
// the upvars are drop-live. | ||
// | ||
// In particular, skipping over `_interior` is safe | ||
// because any side-effects from dropping `_interior` can | ||
// only take place through references with lifetimes | ||
// derived from lifetimes attached to the upvars and resume | ||
// argument, and we *do* incorporate those here. | ||
// For example, if we capture two upvar references `&'1 (), &'2 ()` and have some type | ||
// in the interior, `for<'r> { NeedsDrop<'r> }`, we have no way to tell whether the | ||
// region `'r` came from the `'1` or `'2` region, so we require both are live. This | ||
// could even be unnecessary if `'r` was actually a `'static` region or some region | ||
// local to the coroutine! That's why it's an approximation. | ||
let args = args.as_coroutine(); | ||
|
||
// While we conservatively assume that all coroutines require drop | ||
// to avoid query cycles during MIR building, we can check the actual | ||
// witness during borrowck to avoid unnecessary liveness constraints. | ||
if args.witness().needs_drop(tcx, tcx.erase_regions(typing_env)) { | ||
// Note that we don't care about whether the resume type has any drops since this is | ||
// redundant; there is no storage for the resume type, so if it is actually stored | ||
// in the interior, we'll already detect the need for a drop by checking the interior. | ||
let typing_env = tcx.erase_regions(typing_env); | ||
let needs_drop = args.witness().needs_drop(tcx, typing_env); | ||
if needs_drop { | ||
compiler-errors marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 commentThe 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 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 |
||
// the upvars are drop-live. This is only needed if we aren't already | ||
// counting *all* of the upvars as use-live above. | ||
for ty in args.upvar_tys() { | ||
dtorck_constraint_for_ty_inner( | ||
tcx, | ||
typing_env, | ||
span, | ||
depth + 1, | ||
ty, | ||
constraints, | ||
); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
error[E0597]: `y` does not live long enough | ||
--> $DIR/drop-live-upvar-2.rs:31:26 | ||
| | ||
LL | let y = (); | ||
| - binding `y` declared here | ||
LL | drop_me = Droppy(&y); | ||
| ^^ borrowed value does not live long enough | ||
... | ||
LL | } | ||
| - `y` dropped here while still borrowed | ||
LL | } | ||
| - borrow might be used here, when `fut` is dropped and runs the destructor for coroutine | ||
| | ||
= note: values in a scope are dropped in the opposite order they are defined | ||
|
||
error: aborting due to 1 previous error | ||
|
||
For more information about this error, try `rustc --explain E0597`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
//@ revisions: may_dangle may_not_dangle | ||
//@[may_dangle] check-pass | ||
//@ edition: 2018 | ||
|
||
// Ensure that if a coroutine's interior has no drop types then we don't require the upvars to | ||
// be *use-live*, but instead require them to be *drop-live*. In this case, `Droppy<&'?0 ()>` | ||
// does not require that `'?0` is live for drops since the parameter is `#[may_dangle]` in | ||
// the may_dangle revision, but not in the may_not_dangle revision. | ||
|
||
#![feature(dropck_eyepatch)] | ||
|
||
struct Droppy<T>(T); | ||
|
||
#[cfg(may_dangle)] | ||
unsafe impl<#[may_dangle] T> Drop for Droppy<T> { | ||
fn drop(&mut self) { | ||
// This does not use `T` of course. | ||
} | ||
} | ||
|
||
#[cfg(may_not_dangle)] | ||
impl<T> Drop for Droppy<T> { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn main() { | ||
let drop_me; | ||
let fut; | ||
{ | ||
let y = (); | ||
drop_me = Droppy(&y); | ||
//[may_not_dangle]~^ ERROR `y` does not live long enough | ||
fut = async { | ||
std::mem::drop(drop_me); | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
//@ edition: 2018 | ||
// Regression test for <https://github.com/rust-lang/rust/issues/144155>. | ||
|
||
struct NeedsDrop<'a>(&'a Vec<i32>); | ||
|
||
async fn await_point() {} | ||
|
||
impl Drop for NeedsDrop<'_> { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn foo() { | ||
let v = vec![1, 2, 3]; | ||
let x = NeedsDrop(&v); | ||
let c = async { | ||
std::future::ready(()).await; | ||
drop(x); | ||
}; | ||
drop(v); | ||
//~^ ERROR cannot move out of `v` because it is borrowed | ||
} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
error[E0505]: cannot move out of `v` because it is borrowed | ||
--> $DIR/drop-live-upvar.rs:19:10 | ||
| | ||
LL | let v = vec![1, 2, 3]; | ||
| - binding `v` declared here | ||
LL | let x = NeedsDrop(&v); | ||
| -- borrow of `v` occurs here | ||
... | ||
LL | drop(v); | ||
| ^ move out of `v` occurs here | ||
LL | | ||
LL | } | ||
| - borrow might be used here, when `c` is dropped and runs the destructor for coroutine | ||
| | ||
help: consider cloning the value if the performance cost is acceptable | ||
| | ||
LL | let x = NeedsDrop(&v.clone()); | ||
| ++++++++ | ||
|
||
error: aborting due to 1 previous error | ||
|
||
For more information about this error, try `rustc --explain E0505`. |
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 neverDrop
)