Rollup merge of #144156 - compiler-errors:dtorck-upvars, r=lcnr

Check coroutine upvars in dtorck constraint

Fix rust-lang/rust#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 the `needs_drop` function, and just calling `coroutine_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. https://github.com/rust-lang/rust/pull/144158 implements this experimentally.

This is a pretty involved fix, and conflicts with some in-flight changes (rust-lang/rust#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.

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
+            // rust-lang/rust#49918: Locals can be stored across await points in the coroutine,
-            // in the interior, and sit idle when coroutine yields
+            // called interior/witness types. Since we do not compute these witnesses until after
-            // (and is subsequently dropped).
+            // 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
+            // Here, we implement a more precise approximation for the coroutine's dtorck constraint
-            // coroutine to determine what effects dropping it might
+            // by considering whether any of the interior types needs drop. Note that this is still
-            // have (by looking at any drop effects associated with
+            // an approximation because the coroutine interior has its regions erased, so we must add
-            // its interior).
+            // *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
+            // Note also that this check requires that the coroutine's upvars are use-live, since
-            // CoroutineWitness that explicitly assume they are not
+            // a region from a type that does not have a destructor that was captured in an upvar
-            // traversed in such a manner. So instead, we will
+            // may flow into an interior type with a destructor. This is stronger than requiring
-            // simplify things for now by treating all coroutines as
+            // the upvars are drop-live.
-            // if they were like trait objects, where its upvars must
-            // all be alive for the coroutine's (potential)
-            // destructor.
             //
-            // In particular, skipping over `_interior` is safe
+            // For example, if we capture two upvar references `&'1 (), &'2 ()` and have some type
-            // because any side-effects from dropping `_interior` can
+            // in the interior, `for<'r> { NeedsDrop<'r> }`, we have no way to tell whether the
-            // only take place through references with lifetimes
+            // region `'r` came from the `'1` or `'2` region, so we require both are live. This
-            // derived from lifetimes attached to the upvars and resume
+            // could even be unnecessary if `'r` was actually a `'static` region or some region
-            // argument, and we *do* incorporate those here.
+            // 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
+            // Note that we don't care about whether the resume type has any drops since this is
-            // to avoid query cycles during MIR building, we can check the actual
+            // redundant; there is no storage for the resume type, so if it is actually stored
-            // witness during borrowck to avoid unnecessary liveness constraints.
+            // 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,
+                    );
+                }
             }
         }
 

tests/ui/async-await/drop-live-upvar-2.may_not_dangle.stderr

@@ -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`.

tests/ui/async-await/drop-live-upvar-2.rs

@@ -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);
+        };
+    }
+}

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() {}

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`.