Check coroutine upvars and in dtorck constraint

3 files changed, 95 insertions, 24 deletions

diff --git a/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs b/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
index b1b331d1b61..de404532899 100644
--- a/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
+++ b/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
@@ -319,39 +319,65 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>(
         }
 
         ty::Coroutine(def_id, 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.
+            // 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);
-            if tcx.mir_coroutine_witnesses(def_id).is_some_and(|witness| {
+            let needs_drop = tcx.mir_coroutine_witnesses(def_id).is_some_and(|witness| {
                 witness.field_tys.iter().any(|field| field.ty.needs_drop(tcx, typing_env))
-            }) {
+            });
+            if needs_drop {
+                // Pushing types directly to `constraints.outlives` is equivalent
+                // to requiring them to be use-live, since if we were instead to
+                // recurse on them like we do below, we only end up collecting the
+                // types that are relevant for drop-liveness.
                 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
+                // the upvars are drop-live. This is only needed if we aren't already
+                // counting *all* of the upvars as use-live above, since use-liveness
+                // is a *stronger requirement* than drop-liveness. Recursing here
+                // unconditionally would just be collecting duplicated types for no
+                // reason.
+                for ty in args.upvar_tys() {
+                    dtorck_constraint_for_ty_inner(
+                        tcx,
+                        typing_env,
+                        span,
+                        depth + 1,
+                        ty,
+                        constraints,
+                    );
+                }
             }
         }
 
diff --git a/tests/ui/async-await/drop-live-upvar.rs b/tests/ui/async-await/drop-live-upvar.rs
new file mode 100644
index 00000000000..8e881f729b9
--- /dev/null
+++ b/tests/ui/async-await/drop-live-upvar.rs
@@ -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() {}
diff --git a/tests/ui/async-await/drop-live-upvar.stderr b/tests/ui/async-await/drop-live-upvar.stderr
new file mode 100644
index 00000000000..f804484536b
--- /dev/null
+++ b/tests/ui/async-await/drop-live-upvar.stderr
@@ -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`.