avoid generating drops for moved operands of calls

Currently, after a CALL terminator is created in MIR, we insert DROP
statements for all of its operands -- even though they were just moved
shortly before! These spurious drops are later removed, but not before
causing borrow check errors.

This PR series modifies the drop code to track operands that were
moved and avoid creating drops for them.

Right now, I'm only using this mechanism for calls, but it seems
likely it could be used in more places.

Author: nikomatsakis

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

@@ -110,6 +110,8 @@ struct Scope {
     /// 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>,
 
@@ -159,7 +161,7 @@ struct CachedBlock {
     generator_drop: Option<BasicBlock>,
 }
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq, Eq)]
 pub(crate) enum DropKind {
     Value,
     Storage,
@@ -280,6 +282,7 @@ impl<'tcx> Scopes<'tcx> {
             region_scope: region_scope.0,
             region_scope_span: region_scope.1.span,
             drops: vec![],
+            moved_locals: vec![],
             cached_generator_drop: None,
             cached_exits: Default::default(),
             cached_unwind: CachedBlock::default(),
@@ -484,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()
@@ -576,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;
@@ -626,7 +631,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 block,
                 unwind_to,
                 self.arg_count,
-                true,
+                true, // is generator
+                true, // is cached path
             ));
         }
 
@@ -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,6 +1122,15 @@ 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;
+
+        // 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;
+        }
+
         match drop_data.kind {
             DropKind::Value => {
                 let unwind_to = get_unwind_to(scope, is_generator, drop_idx, generator_drop)

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

@@ -0,0 +1,17 @@
+// 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.
+
+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,29 @@
+// Regression test for issue #64433.
+//
+// See issue-64391-2.rs for more details, as that was fixed by the
+// same PR.
+//
+// check-pass
+
+#[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() {
+}