Auto merge of #64584 - nikomatsakis:issue-64477-generator-capture-types, r=eddyb

record fewer adjustment types in generator witnesses, avoid spurious drops in MIR construction

Don't record all intermediate adjustment types -- That's way more than is needed, and winds up recording types that will never appear in MIR.

Note: I'm like 90% sure that this logic is correct, but this stuff is subtle and can be hard to keep straight.  However, the risk of this PR is fairly low -- if we miss types here, I believe the most common outcome is an ICE.

This fixes the original issue cited by #64477, but I'm leaving the issue open for now since there may be other cases we can detect and improve in a targeted way.

r? @Zoxc

src/librustc/mir/mod.rs

@@ -1917,6 +1917,15 @@ impl<'tcx> Place<'tcx> {
         }
     }
 
+    /// If this place represents a local variable like `_X` with no
+    /// projections, return `Some(_X)`.
+    pub fn as_local(&self) -> Option<Local> {
+        match self {
+            Place { projection: box [], base: PlaceBase::Local(l) } => Some(*l),
+            _ => None,
+        }
+    }
+
     pub fn as_ref(&self) -> PlaceRef<'_, 'tcx> {
         PlaceRef {
             base: &self.base,

src/librustc_mir/build/expr/into.rs

@@ -244,6 +244,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                     let success = this.cfg.start_new_block();
                     let cleanup = this.diverge_cleanup();
+
+                    this.record_operands_moved(&args);
+
                     this.cfg.terminate(
                         block,
                         source_info,

src/librustc_mir/build/scope.rs

@@ -104,25 +104,14 @@ struct Scope {
     /// the span of that region_scope
     region_scope_span: Span,
 
-    /// Whether there's anything to do for the cleanup path, that is,
-    /// when unwinding through this scope. This includes destructors,
-    /// but not StorageDead statements, which don't get emitted at all
-    /// for unwinding, for several reasons:
-    ///  * clang doesn't emit llvm.lifetime.end for C++ unwinding
-    ///  * LLVM's memory dependency analysis can't handle it atm
-    ///  * polluting the cleanup MIR with StorageDead creates
-    ///    landing pads even though there's no actual destructors
-    ///  * freeing up stack space has no effect during unwinding
-    /// Note that for generators we do emit StorageDeads, for the
-    /// use of optimizations in the MIR generator transform.
-    needs_cleanup: bool,
-
     /// set of places to drop when exiting this scope. This starts
     /// out empty but grows as variables are declared during the
     /// building process. This is a stack, so we always drop from the
     /// end of the vector (top of the stack) first.
     drops: Vec<DropData>,
 
+    moved_locals: Vec<Local>,
+
     /// The cache for drop chain on “normal” exit into a particular BasicBlock.
     cached_exits: FxHashMap<(BasicBlock, region::Scope), BasicBlock>,
 
@@ -172,7 +161,7 @@ struct CachedBlock {
     generator_drop: Option<BasicBlock>,
 }
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq)]
 pub(crate) enum DropKind {
     Value,
     Storage,
@@ -202,8 +191,7 @@ pub enum BreakableTarget {
 
 impl CachedBlock {
     fn invalidate(&mut self) {
-        self.generator_drop = None;
-        self.unwind = None;
+        *self = CachedBlock::default();
     }
 
     fn get(&self, generator_drop: bool) -> Option<BasicBlock> {
@@ -261,6 +249,25 @@ impl Scope {
             scope: self.source_scope
         }
     }
+
+
+    /// Whether there's anything to do for the cleanup path, that is,
+    /// when unwinding through this scope. This includes destructors,
+    /// but not StorageDead statements, which don't get emitted at all
+    /// for unwinding, for several reasons:
+    ///  * clang doesn't emit llvm.lifetime.end for C++ unwinding
+    ///  * LLVM's memory dependency analysis can't handle it atm
+    ///  * polluting the cleanup MIR with StorageDead creates
+    ///    landing pads even though there's no actual destructors
+    ///  * freeing up stack space has no effect during unwinding
+    /// Note that for generators we do emit StorageDeads, for the
+    /// use of optimizations in the MIR generator transform.
+    fn needs_cleanup(&self) -> bool {
+        self.drops.iter().any(|drop| match drop.kind {
+            DropKind::Value => true,
+            DropKind::Storage => false,
+        })
+    }
 }
 
 impl<'tcx> Scopes<'tcx> {
@@ -274,8 +281,8 @@ impl<'tcx> Scopes<'tcx> {
             source_scope: vis_scope,
             region_scope: region_scope.0,
             region_scope_span: region_scope.1.span,
-            needs_cleanup: false,
             drops: vec![],
+            moved_locals: vec![],
             cached_generator_drop: None,
             cached_exits: Default::default(),
             cached_unwind: CachedBlock::default(),
@@ -295,7 +302,7 @@ impl<'tcx> Scopes<'tcx> {
 
     fn may_panic(&self, scope_count: usize) -> bool {
         let len = self.len();
-        self.scopes[(len - scope_count)..].iter().any(|s| s.needs_cleanup)
+        self.scopes[(len - scope_count)..].iter().any(|s| s.needs_cleanup())
     }
 
     /// Finds the breakable scope for a given label. This is used for
@@ -480,7 +487,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             block,
             unwind_to,
             self.arg_count,
-            false,
+            false, // not generator
+            false, // not unwind path
         ));
 
         block.unit()
@@ -572,7 +580,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block,
                 unwind_to,
                 self.arg_count,
-                false,
+                false, // not generator
+                false, // not unwind path
             ));
 
             scope = next_scope;
@@ -622,7 +631,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block,
                 unwind_to,
                 self.arg_count,
-                true,
+                true, // is generator
+                true, // is cached path
             ));
         }
 
@@ -801,10 +811,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // cache of outer scope stays intact.
             scope.invalidate_cache(!needs_drop, self.is_generator, this_scope);
             if this_scope {
-                if let DropKind::Value = drop_kind {
-                    scope.needs_cleanup = true;
-                }
-
                 let region_scope_span = region_scope.span(self.hir.tcx(),
                                                           &self.hir.region_scope_tree);
                 // Attribute scope exit drops to scope's closing brace.
@@ -822,6 +828,75 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         span_bug!(span, "region scope {:?} not in scope to drop {:?}", region_scope, local);
     }
 
+    /// Indicates that the "local operand" stored in `local` is
+    /// *moved* at some point during execution (see `local_scope` for
+    /// more information about what a "local operand" is -- in short,
+    /// it's an intermediate operand created as part of preparing some
+    /// MIR instruction). We use this information to suppress
+    /// redundant drops on the non-unwind paths. This results in less
+    /// MIR, but also avoids spurious borrow check errors
+    /// (c.f. #64391).
+    ///
+    /// Example: when compiling the call to `foo` here:
+    ///
+    /// ```rust
+    /// foo(bar(), ...)
+    /// ```
+    ///
+    /// we would evaluate `bar()` to an operand `_X`. We would also
+    /// schedule `_X` to be dropped when the expression scope for
+    /// `foo(bar())` is exited. This is relevant, for example, if the
+    /// later arguments should unwind (it would ensure that `_X` gets
+    /// dropped). However, if no unwind occurs, then `_X` will be
+    /// unconditionally consumed by the `call`:
+    ///
+    /// ```
+    /// bb {
+    ///   ...
+    ///   _R = CALL(foo, _X, ...)
+    /// }
+    /// ```
+    ///
+    /// However, `_X` is still registered to be dropped, and so if we
+    /// do nothing else, we would generate a `DROP(_X)` that occurs
+    /// after the call. This will later be optimized out by the
+    /// drop-elaboation code, but in the meantime it can lead to
+    /// spurious borrow-check errors -- the problem, ironically, is
+    /// not the `DROP(_X)` itself, but the (spurious) unwind pathways
+    /// that it creates. See #64391 for an example.
+    pub fn record_operands_moved(
+        &mut self,
+        operands: &[Operand<'tcx>],
+    ) {
+        let scope = match self.local_scope() {
+            None => {
+                // if there is no local scope, operands won't be dropped anyway
+                return;
+            }
+
+            Some(local_scope) => {
+                self.scopes.iter_mut().find(|scope| scope.region_scope == local_scope)
+                    .unwrap_or_else(|| bug!("scope {:?} not found in scope list!", local_scope))
+            }
+        };
+
+        // look for moves of a local variable, like `MOVE(_X)`
+        let locals_moved = operands.iter().flat_map(|operand| match operand {
+            Operand::Copy(_) | Operand::Constant(_) => None,
+            Operand::Move(place) => place.as_local(),
+        });
+
+        for local in locals_moved {
+            // check if we have a Drop for this operand and -- if so
+            // -- add it to the list of moved operands. Note that this
+            // local might not have been an operand created for this
+            // call, it could come from other places too.
+            if scope.drops.iter().any(|drop| drop.local == local && drop.kind == DropKind::Value) {
+                scope.moved_locals.push(local);
+            }
+        }
+    }
+
     // Other
     // =====
     /// Branch based on a boolean condition.
@@ -1020,6 +1095,7 @@ fn build_scope_drops<'tcx>(
     last_unwind_to: BasicBlock,
     arg_count: usize,
     generator_drop: bool,
+    is_cached_path: bool,
 ) -> BlockAnd<()> {
     debug!("build_scope_drops({:?} -> {:?})", block, scope);
 
@@ -1046,8 +1122,17 @@ fn build_scope_drops<'tcx>(
         let drop_data = &scope.drops[drop_idx];
         let source_info = scope.source_info(drop_data.span);
         let local = drop_data.local;
+
         match drop_data.kind {
             DropKind::Value => {
+                // If the operand has been moved, and we are not on an unwind
+                // path, then don't generate the drop. (We only take this into
+                // account for non-unwind paths so as not to disturb the
+                // caching mechanism.)
+                if !is_cached_path && scope.moved_locals.iter().any(|&o| o == local) {
+                    continue;
+                }
+
                 let unwind_to = get_unwind_to(scope, is_generator, drop_idx, generator_drop)
                     .unwrap_or(last_unwind_to);
 

src/librustc_typeck/check/generator_interior.rs

@@ -181,13 +181,34 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
 
         let scope = self.region_scope_tree.temporary_scope(expr.hir_id.local_id);
 
-        // Record the unadjusted type
+        // If there are adjustments, then record the final type --
+        // this is the actual value that is being produced.
+        if let Some(adjusted_ty) = self.fcx.tables.borrow().expr_ty_adjusted_opt(expr) {
+            self.record(adjusted_ty, scope, Some(expr), expr.span);
+        }
+
+        // Also record the unadjusted type (which is the only type if
+        // there are no adjustments). The reason for this is that the
+        // unadjusted value is sometimes a "temporary" that would wind
+        // up in a MIR temporary.
+        //
+        // As an example, consider an expression like `vec![].push()`.
+        // Here, the `vec![]` would wind up MIR stored into a
+        // temporary variable `t` which we can borrow to invoke
+        // `<Vec<_>>::push(&mut t)`.
+        //
+        // Note that an expression can have many adjustments, and we
+        // are just ignoring those intermediate types. This is because
+        // those intermediate values are always linearly "consumed" by
+        // the other adjustments, and hence would never be directly
+        // captured in the MIR.
+        //
+        // (Note that this partly relies on the fact that the `Deref`
+        // traits always return references, which means their content
+        // can be reborrowed without needing to spill to a temporary.
+        // If this were not the case, then we could conceivably have
+        // to create intermediate temporaries.)
         let ty = self.fcx.tables.borrow().expr_ty(expr);
         self.record(ty, scope, Some(expr), expr.span);
-
-        // Also include the adjusted types, since these can result in MIR locals
-        for adjustment in self.fcx.tables.borrow().expr_adjustments(expr) {
-            self.record(adjustment.target, scope, Some(expr), expr.span);
-        }
     }
 }

src/test/mir-opt/box_expr.rs

@@ -57,25 +57,18 @@ impl Drop for S {
 //     }
 //
 //     bb5: {
-//         drop(_4) -> [return: bb8, unwind: bb6];
-//     }
-//
-//     bb6 (cleanup): {
-//         drop(_1) -> bb1;
-//     }
-//
-//     bb7 (cleanup): {
-//         drop(_4) -> bb6;
-//     }
-//
-//     bb8: {
 //         StorageDead(_4);
 //         StorageDead(_3);
 //         _0 = ();
-//         drop(_1) -> bb9;
+//         drop(_1) -> bb8;
 //     }
-//
-//     bb9: {
+//     bb6 (cleanup): {
+//         drop(_1) -> bb1;
+//     }
+//     bb7 (cleanup): {
+//         drop(_4) -> bb6;
+//     }
+//     bb8: {
 //         StorageDead(_1);
 //         return;
 //     }

src/test/mir-opt/generator-storage-dead-unwind.rs

@@ -57,7 +57,7 @@ fn main() {
 //     StorageLive(_6);
 //     StorageLive(_7);
 //     _7 = move _2;
-//     _6 = const take::<Foo>(move _7) -> [return: bb9, unwind: bb8];
+//     _6 = const take::<Foo>(move _7) -> [return: bb7, unwind: bb9];
 // }
 // bb3 (cleanup): {
 //     StorageDead(_2);
@@ -75,17 +75,7 @@ fn main() {
 // bb6: {
 //     generator_drop;
 // }
-// bb7 (cleanup): {
-//     StorageDead(_3);
-//     StorageDead(_2);
-//     drop(_1) -> bb1;
-// }
-// bb8 (cleanup): {
-//     StorageDead(_7);
-//     StorageDead(_6);
-//     goto -> bb7;
-// }
-// bb9: {
+// bb7: {
 //     StorageDead(_7);
 //     StorageDead(_6);
 //     StorageLive(_8);
@@ -93,6 +83,16 @@ fn main() {
 //     _9 = move _3;
 //     _8 = const take::<Bar>(move _9) -> [return: bb10, unwind: bb11];
 // }
+// bb8 (cleanup): {
+//     StorageDead(_3);
+//     StorageDead(_2);
+//     drop(_1) -> bb1;
+// }
+// bb9 (cleanup): {
+//     StorageDead(_7);
+//     StorageDead(_6);
+//     goto -> bb8;
+// }
 // bb10: {
 //     StorageDead(_9);
 //     StorageDead(_8);
@@ -104,7 +104,7 @@ fn main() {
 // bb11 (cleanup): {
 //     StorageDead(_9);
 //     StorageDead(_8);
-//     goto -> bb7;
+//     goto -> bb8;
 // }
 // bb12: {
 //     return;

src/test/mir-opt/no-spurious-drop-after-call.rs

@@ -0,0 +1,24 @@
+// ignore-wasm32-bare compiled with panic=abort by default
+
+// Test that after the call to `std::mem::drop` we do not generate a
+// MIR drop of the argument. (We used to have a `DROP(_2)` in the code
+// below, as part of bb3.)
+
+fn main() {
+    std::mem::drop("".to_string());
+}
+
+// END RUST SOURCE
+// START rustc.main.ElaborateDrops.before.mir
+//    bb2: {
+//        StorageDead(_3);
+//        _1 = const std::mem::drop::<std::string::String>(move _2) -> [return: bb3, unwind: bb4];
+//    }
+//    bb3: {
+//        StorageDead(_2);
+//        StorageDead(_4);
+//        StorageDead(_1);
+//        _0 = ();
+//        return;
+//    }
+// END rustc.main.ElaborateDrops.before.mir

src/test/ui/async-await/issues/issue-64391-2.rs

@@ -0,0 +1,20 @@
+// Regression test for #64391
+//
+// As described on the issue, the (spurious) `DROP` inserted for the
+// `"".to_string()` value was causing a (spurious) unwind path that
+// led us to believe that the future might be dropped after `config`
+// had been dropped. This cannot, in fact, happen.
+//
+// check-pass
+// edition:2018
+
+async fn connect() {
+    let config = 666;
+    connect2(&config, "".to_string()).await
+}
+
+async fn connect2(_config: &u32, _tls: String) {
+    unimplemented!()
+}
+
+fn main() { }

src/test/ui/async-await/issues/issue-64433.rs

@@ -0,0 +1,30 @@
+// Regression test for issue #64433.
+//
+// See issue-64391-2.rs for more details, as that was fixed by the
+// same PR.
+//
+// check-pass
+// edition:2018
+
+#[derive(Debug)]
+struct A<'a> {
+    inner: Vec<&'a str>,
+}
+
+struct B {}
+
+impl B {
+    async fn something_with_a(&mut self, a: A<'_>) -> Result<(), String> {
+        println!("{:?}", a);
+        Ok(())
+    }
+}
+
+async fn can_error(some_string: &str) -> Result<(), String> {
+    let a = A { inner: vec![some_string, "foo"] };
+    let mut b = B {};
+    Ok(b.something_with_a(a).await.map(|_| ())?)
+}
+
+fn main() {
+}

src/test/ui/async-await/issues/issue-64477.rs

@@ -0,0 +1,20 @@
+// Regression test for #64477.
+//
+// We were incorrectly claiming that the `f(x).await` future captured
+// a value of type `T`, and hence that `T: Send` would have to hold.
+//
+// check-pass
+// edition:2018
+
+use std::future::Future;
+use std::pin::Pin;
+
+fn f<T>(_: &T) -> Pin<Box<dyn Future<Output = ()> + Send>> {
+    unimplemented!()
+}
+
+pub fn g<T: Sync>(x: &'static T) -> impl Future<Output = ()> + Send {
+    async move { f(x).await }
+}
+
+fn main() { }