Auto merge of #117134 - lcnr:dropck_outlives-coroutine, r=compiler-errors

dropck_outlives check whether generator witness needs_drop

see https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/.23116242.3A.20Code.20no.20longer.20compiles.20after.20-Zdrop-tracking-mir.20.E2.80.A6/near/398311627 for an explanation.

Fixes #116242 (or well, the repro by `@jamuraa` in #116242 (comment)). I did not add a regression test as it depends on other crates. We do have 1 test going from fail to pass, showing the intended behavior.

r? types

Author: bors

compiler/rustc_middle/src/ty/util.rs

@@ -1107,8 +1107,10 @@ impl<'tcx> Ty<'tcx> {
                 // This doesn't depend on regions, so try to minimize distinct
                 // query keys used.
                 // If normalization fails, we just use `query_ty`.
-                let query_ty =
-                    tcx.try_normalize_erasing_regions(param_env, query_ty).unwrap_or(query_ty);
+                debug_assert!(!param_env.has_infer());
+                let query_ty = tcx
+                    .try_normalize_erasing_regions(param_env, query_ty)
+                    .unwrap_or_else(|_| tcx.erase_regions(query_ty));
 
                 tcx.needs_drop_raw(param_env.and(query_ty))
             }
@@ -1297,7 +1299,6 @@ pub fn needs_drop_components<'tcx>(
         | ty::FnDef(..)
         | ty::FnPtr(_)
         | ty::Char
-        | ty::CoroutineWitness(..)
         | ty::RawPtr(_)
         | ty::Ref(..)
         | ty::Str => Ok(SmallVec::new()),
@@ -1337,7 +1338,8 @@ pub fn needs_drop_components<'tcx>(
         | ty::Placeholder(..)
         | ty::Infer(_)
         | ty::Closure(..)
-        | ty::Coroutine(..) => Ok(smallvec![ty]),
+        | ty::Coroutine(..)
+        | ty::CoroutineWitness(..) => Ok(smallvec![ty]),
     }
 }
 

compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs

@@ -133,7 +133,7 @@ pub fn compute_dropck_outlives_inner<'tcx>(
             result.overflows.len(),
             ty_stack.len()
         );
-        dtorck_constraint_for_ty_inner(tcx, DUMMY_SP, for_ty, depth, ty, &mut constraints)?;
+        dtorck_constraint_for_ty_inner(tcx, param_env, DUMMY_SP, depth, ty, &mut constraints)?;
 
         // "outlives" represent types/regions that may be touched
         // by a destructor.
@@ -185,16 +185,15 @@ pub fn compute_dropck_outlives_inner<'tcx>(
 
 /// Returns a set of constraints that needs to be satisfied in
 /// order for `ty` to be valid for destruction.
+#[instrument(level = "debug", skip(tcx, param_env, span, constraints))]
 pub fn dtorck_constraint_for_ty_inner<'tcx>(
     tcx: TyCtxt<'tcx>,
+    param_env: ty::ParamEnv<'tcx>,
     span: Span,
-    for_ty: Ty<'tcx>,
     depth: usize,
     ty: Ty<'tcx>,
     constraints: &mut DropckConstraint<'tcx>,
 ) -> Result<(), NoSolution> {
-    debug!("dtorck_constraint_for_ty_inner({:?}, {:?}, {:?}, {:?})", span, for_ty, depth, ty);
-
     if !tcx.recursion_limit().value_within_limit(depth) {
         constraints.overflows.push(ty);
         return Ok(());
@@ -224,13 +223,13 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>(
         ty::Array(ety, _) | ty::Slice(ety) => {
             // single-element containers, behave like their element
             rustc_data_structures::stack::ensure_sufficient_stack(|| {
-                dtorck_constraint_for_ty_inner(tcx, span, for_ty, depth + 1, *ety, constraints)
+                dtorck_constraint_for_ty_inner(tcx, param_env, span, depth + 1, *ety, constraints)
             })?;
         }
 
         ty::Tuple(tys) => rustc_data_structures::stack::ensure_sufficient_stack(|| {
             for ty in tys.iter() {
-                dtorck_constraint_for_ty_inner(tcx, span, for_ty, depth + 1, ty, constraints)?;
+                dtorck_constraint_for_ty_inner(tcx, param_env, span, depth + 1, ty, constraints)?;
             }
             Ok::<_, NoSolution>(())
         })?,
@@ -249,7 +248,14 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>(
 
             rustc_data_structures::stack::ensure_sufficient_stack(|| {
                 for ty in args.as_closure().upvar_tys() {
-                    dtorck_constraint_for_ty_inner(tcx, span, for_ty, depth + 1, ty, constraints)?;
+                    dtorck_constraint_for_ty_inner(
+                        tcx,
+                        param_env,
+                        span,
+                        depth + 1,
+                        ty,
+                        constraints,
+                    )?;
                 }
                 Ok::<_, NoSolution>(())
             })?
@@ -278,8 +284,8 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>(
             // only take place through references with lifetimes
             // derived from lifetimes attached to the upvars and resume
             // argument, and we *do* incorporate those here.
-
-            if !args.as_coroutine().is_valid() {
+            let args = args.as_coroutine();
+            if !args.is_valid() {
                 // By the time this code runs, all type variables ought to
                 // be fully resolved.
                 tcx.sess.delay_span_bug(
@@ -289,10 +295,13 @@ pub fn dtorck_constraint_for_ty_inner<'tcx>(
                 return Err(NoSolution);
             }
 
-            constraints
-                .outlives
-                .extend(args.as_coroutine().upvar_tys().iter().map(ty::GenericArg::from));
-            constraints.outlives.push(args.as_coroutine().resume_ty().into());
+            // 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(param_env)) {
+                constraints.outlives.extend(args.upvar_tys().iter().map(ty::GenericArg::from));
+                constraints.outlives.push(args.resume_ty().into());
+            }
         }
 
         ty::Adt(def, args) => {

compiler/rustc_traits/src/dropck_outlives.rs

@@ -34,6 +34,7 @@ pub(crate) fn adt_dtorck_constraint(
 ) -> Result<&DropckConstraint<'_>, NoSolution> {
     let def = tcx.adt_def(def_id);
     let span = tcx.def_span(def_id);
+    let param_env = tcx.param_env(def_id);
     debug!("dtorck_constraint: {:?}", def);
 
     if def.is_manually_drop() {
@@ -55,7 +56,7 @@ pub(crate) fn adt_dtorck_constraint(
     let mut result = DropckConstraint::empty();
     for field in def.all_fields() {
         let fty = tcx.type_of(field.did).instantiate_identity();
-        dtorck_constraint_for_ty_inner(tcx, span, fty, 0, fty, &mut result)?;
+        dtorck_constraint_for_ty_inner(tcx, param_env, span, 0, fty, &mut result)?;
     }
     result.outlives.extend(tcx.destructor_constraints(def));
     dedup_dtorck_constraint(&mut result);

compiler/rustc_ty_utils/src/needs_drop.rs

@@ -66,6 +66,9 @@ fn has_significant_drop_raw<'tcx>(
 struct NeedsDropTypes<'tcx, F> {
     tcx: TyCtxt<'tcx>,
     param_env: ty::ParamEnv<'tcx>,
+    // Whether to reveal coroutine witnesses, this is set
+    // to `false` unless we compute `needs_drop` for a coroutine witness.
+    reveal_coroutine_witnesses: bool,
     query_ty: Ty<'tcx>,
     seen_tys: FxHashSet<Ty<'tcx>>,
     /// A stack of types left to process, and the recursion depth when we
@@ -89,6 +92,7 @@ impl<'tcx, F> NeedsDropTypes<'tcx, F> {
         Self {
             tcx,
             param_env,
+            reveal_coroutine_witnesses: false,
             seen_tys,
             query_ty: ty,
             unchecked_tys: vec![(ty, 0)],
@@ -133,8 +137,31 @@ where
                     // The information required to determine whether a coroutine has drop is
                     // computed on MIR, while this very method is used to build MIR.
                     // To avoid cycles, we consider that coroutines always require drop.
-                    ty::Coroutine(..) => {
-                        return Some(Err(AlwaysRequiresDrop));
+                    //
+                    // HACK: Because we erase regions contained in the coroutine witness, we
+                    // have to conservatively assume that every region captured by the
+                    // coroutine has to be live when dropped. This results in a lot of
+                    // undesirable borrowck errors. During borrowck, we call `needs_drop`
+                    // for the coroutine witness and check whether any of the contained types
+                    // need to be dropped, and only require the captured types to be live
+                    // if they do.
+                    ty::Coroutine(_, args, _) => {
+                        if self.reveal_coroutine_witnesses {
+                            queue_type(self, args.as_coroutine().witness());
+                        } else {
+                            return Some(Err(AlwaysRequiresDrop));
+                        }
+                    }
+                    ty::CoroutineWitness(def_id, args) => {
+                        if let Some(witness) = tcx.mir_coroutine_witnesses(def_id) {
+                            self.reveal_coroutine_witnesses = true;
+                            for field_ty in &witness.field_tys {
+                                queue_type(
+                                    self,
+                                    EarlyBinder::bind(field_ty.ty).instantiate(tcx, args),
+                                );
+                            }
+                        }
                     }
 
                     _ if component.is_copy_modulo_regions(tcx, self.param_env) => (),
@@ -191,7 +218,6 @@ where
                     | ty::FnPtr(..)
                     | ty::Tuple(_)
                     | ty::Bound(..)
-                    | ty::CoroutineWitness(..)
                     | ty::Never
                     | ty::Infer(_)
                     | ty::Error(_) => {

compiler/rustc_type_ir/src/ty_kind.rs

@@ -218,7 +218,6 @@ pub enum TyKind<I: Interner> {
     /// the type of the coroutine, we convert them to higher ranked
     /// lifetimes bound by the witness itself.
     ///
-    /// This variant is only using when `drop_tracking_mir` is set.
     /// This contains the `DefId` and the `GenericArgsRef` of the coroutine.
     /// The actual witness types are computed on MIR by the `mir_coroutine_witnesses` query.
     ///

tests/ui/coroutine/borrowing.stderr

@@ -1,24 +1,16 @@
 error[E0597]: `a` does not live long enough
   --> $DIR/borrowing.rs:9:33
    |
+LL |     let _b = {
+   |         -- borrow later stored here
+LL |         let a = 3;
 LL |         Pin::new(&mut || yield &a).resume(())
-   |                       ----------^
-   |                       |         |
-   |                       |         borrowed value does not live long enough
+   |                       --        ^ borrowed value does not live long enough
+   |                       |
    |                       value captured here by coroutine
-   |                       a temporary with access to the borrow is created here ...
 LL |
 LL |     };
-   |     -- ... and the borrow might be used here, when that temporary is dropped and runs the destructor for coroutine
-   |     |
-   |     `a` dropped here while still borrowed
-   |
-   = note: the temporary is part of an expression at the end of a block;
-           consider forcing this temporary to be dropped sooner, before the block's local variables are dropped
-help: for example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block
-   |
-LL |         let x = Pin::new(&mut || yield &a).resume(()); x
-   |         +++++++                                      +++
+   |     - `a` dropped here while still borrowed
 
 error[E0597]: `a` does not live long enough
   --> $DIR/borrowing.rs:16:20

tests/ui/coroutine/issue-110929-coroutine-conflict-error-ice.rs

@@ -1,11 +1,11 @@
 // edition:2021
+// check-pass
 #![feature(coroutines)]
 
 fn main() {
     let x = &mut ();
     || {
         let _c = || yield *&mut *x;
         || _ = &mut *x;
-        //~^ cannot borrow `*x` as mutable more than once at a time
     };
 }

tests/ui/coroutine/issue-110929-coroutine-conflict-error-ice.stderr

@@ -4,10 +4,9 @@ error[E0499]: cannot borrow `thing` as mutable more than once at a time
 LL |     gen.as_mut().resume(&mut thing);
    |                         ---------- first mutable borrow occurs here
 LL |     gen.as_mut().resume(&mut thing);
-   |                         ^^^^^^^^^^ second mutable borrow occurs here
-LL |
-LL | }
-   | - first borrow might be used here, when `gen` is dropped and runs the destructor for coroutine
+   |                  ------ ^^^^^^^^^^ second mutable borrow occurs here
+   |                  |
+   |                  first borrow later used by call
 
 error: aborting due to previous error
 

tests/ui/coroutine/retain-resume-ref.stderr

@@ -0,0 +1,18 @@
+// check-pass
+// edition: 2021
+
+// regression test for #116242.
+use std::future;
+
+fn main() {
+    let mut recv = future::ready(());
+    let _combined_fut = async {
+        let _ = || read(&mut recv);
+    };
+
+    drop(recv);
+}
+
+fn read<F: future::Future>(_: &mut F) -> F::Output {
+    todo!()
+}

tests/ui/dropck/coroutine-liveness-1.rs

@@ -0,0 +1,23 @@
+// check-pass
+// edition: 2021
+
+// regression test found while working on #117134.
+use std::future;
+
+fn main() {
+    let mut recv = future::ready(());
+    let _combined_fut = async {
+        let _ = || read(&mut recv);
+    };
+
+    let _uwu = (String::new(), _combined_fut);
+    // Dropping a coroutine as part of a more complex
+    // types should not add unnecessary liveness
+    // constraints.
+
+    drop(recv);
+}
+
+fn read<F: future::Future>(_: &mut F) -> F::Output {
+    todo!()
+}