Better account for more cases involving closures

estebank · estebank · commit dea9b5031ccc · 2024-04-12T04:46:31.000Z
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -203,13 +203,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
 
                 if !seen_spans.contains(&move_span) {
                     if !closure {
-                        self.suggest_ref_or_clone(
-                            mpi,
-                            move_span,
-                            &mut err,
-                            &mut in_pattern,
-                            move_spans,
-                        );
+                        self.suggest_ref_or_clone(mpi, &mut err, &mut in_pattern, move_spans);
                     }
 
                     let msg_opt = CapturedMessageOpt {
@@ -351,18 +345,28 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
     fn suggest_ref_or_clone(
         &self,
         mpi: MovePathIndex,
-        move_span: Span,
         err: &mut Diag<'tcx>,
         in_pattern: &mut bool,
         move_spans: UseSpans<'_>,
     ) {
+        let move_span = match move_spans {
+            UseSpans::ClosureUse { capture_kind_span, .. } => capture_kind_span,
+            _ => move_spans.args_or_use(),
+        };
         struct ExpressionFinder<'hir> {
             expr_span: Span,
             expr: Option<&'hir hir::Expr<'hir>>,
             pat: Option<&'hir hir::Pat<'hir>>,
             parent_pat: Option<&'hir hir::Pat<'hir>>,
+            hir: rustc_middle::hir::map::Map<'hir>,
         }
         impl<'hir> Visitor<'hir> for ExpressionFinder<'hir> {
+            type NestedFilter = OnlyBodies;
+
+            fn nested_visit_map(&mut self) -> Self::Map {
+                self.hir
+            }
+
             fn visit_expr(&mut self, e: &'hir hir::Expr<'hir>) {
                 if e.span == self.expr_span {
                     self.expr = Some(e);
@@ -397,8 +401,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             let expr = hir.body(body_id).value;
             let place = &self.move_data.move_paths[mpi].place;
             let span = place.as_local().map(|local| self.body.local_decls[local].source_info.span);
-            let mut finder =
-                ExpressionFinder { expr_span: move_span, expr: None, pat: None, parent_pat: None };
+            let mut finder = ExpressionFinder {
+                expr_span: move_span,
+                expr: None,
+                pat: None,
+                parent_pat: None,
+                hir,
+            };
             finder.visit_expr(expr);
             if let Some(span) = span
                 && let Some(expr) = finder.expr
@@ -479,12 +488,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 } else if let UseSpans::ClosureUse {
                     closure_kind:
                         ClosureKind::Coroutine(CoroutineKind::Desugared(_, CoroutineSource::Block)),
-                    args_span: _,
-                    capture_kind_span: _,
-                    path_span,
+                    ..
                 } = move_spans
                 {
-                    self.suggest_cloning(err, ty, expr, path_span);
+                    self.suggest_cloning(err, ty, expr, None);
                 } else if self.suggest_hoisting_call_outside_loop(err, expr) {
                     // The place where the the type moves would be misleading to suggest clone.
                     // #121466
@@ -1233,6 +1240,18 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
+    fn in_move_closure(&self, expr: &hir::Expr<'_>) -> bool {
+        for (_, node) in self.infcx.tcx.hir().parent_iter(expr.hir_id) {
+            if let hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Closure(closure), .. }) = node
+                && let hir::CaptureBy::Value { .. } = closure.capture_clause
+            {
+                // `move || x.clone()` will not work. FIXME: suggest `let y = x.clone(); move || y`
+                return true;
+            }
+        }
+        false
+    }
+
     fn suggest_cloning_inner(
         &self,
         err: &mut Diag<'_>,
@@ -1245,6 +1264,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             // See `tests/ui/moves/needs-clone-through-deref.rs`
             return false;
         }
+        if self.in_move_closure(expr) {
+            return false;
+        }
         // Try to find predicates on *generic params* that would allow copying `ty`
         let suggestion =
             if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) {
diff --git a/tests/ui/borrowck/borrowck-closures-slice-patterns.stderr b/tests/ui/borrowck/borrowck-closures-slice-patterns.stderr
@@ -38,6 +38,11 @@ LL |         let [y, z @ ..] = x;
 LL |     };
 LL |     &x;
    |     ^^ value borrowed here after move
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |         let [y, z @ ..] = x.clone();
+   |                            ++++++++
 
 error[E0502]: cannot borrow `*x` as mutable because it is also borrowed as immutable
   --> $DIR/borrowck-closures-slice-patterns.rs:33:13
@@ -79,6 +84,12 @@ LL |         let [y, z @ ..] = *x;
 LL |     };
 LL |     &x;
    |     ^^ value borrowed here after move
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL -         let [y, z @ ..] = *x;
+LL +         let [y, z @ ..] = x.clone();
+   |
 
 error[E0502]: cannot borrow `*x` as mutable because it is also borrowed as immutable
   --> $DIR/borrowck-closures-slice-patterns.rs:59:13
diff --git a/tests/ui/borrowck/issue-101119.stderr b/tests/ui/borrowck/issue-101119.stderr
@@ -4,11 +4,25 @@ error[E0382]: use of moved value: `state`
 LL | fn fill_memory_blocks_mt(state: &mut State) {
    |                          ----- move occurs because `state` has type `&mut State`, which does not implement the `Copy` trait
 LL |     loop {
+   |     ---- inside of this loop
 LL |         once(move || {
    |              ^^^^^^^ value moved into closure here, in previous iteration of loop
 LL |
 LL |             fill_segment(state);
    |                          ----- use occurs due to use in closure
+   |
+note: consider changing this parameter type in function `fill_segment` to borrow instead if owning the value isn't necessary
+  --> $DIR/issue-101119.rs:14:20
+   |
+LL | fn fill_segment(_: &mut State) {}
+   |    ------------    ^^^^^^^^^^ this parameter takes ownership of the value
+   |    |
+   |    in this function
+note: if `State` implemented `Clone`, you could clone the value
+  --> $DIR/issue-101119.rs:1:1
+   |
+LL | struct State;
+   | ^^^^^^^^^^^^
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/issues/issue-24357.rs b/tests/ui/issues/issue-24357.rs
@@ -1,4 +1,4 @@
-struct NoCopy;
+struct NoCopy; //~ NOTE if `NoCopy` implemented `Clone`, you could clone the value
 fn main() {
    let x = NoCopy;
    //~^ NOTE move occurs because `x` has type `NoCopy`
diff --git a/tests/ui/issues/issue-24357.stderr b/tests/ui/issues/issue-24357.stderr
@@ -11,6 +11,12 @@ LL |    let f = move || { let y = x; };
 ...
 LL |    let z = x;
    |            ^ value used here after move
+   |
+note: if `NoCopy` implemented `Clone`, you could clone the value
+  --> $DIR/issue-24357.rs:1:1
+   |
+LL | struct NoCopy;
+   | ^^^^^^^^^^^^^
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/moves/issue-75904-move-closure-loop.stderr b/tests/ui/moves/issue-75904-move-closure-loop.stderr
@@ -4,11 +4,18 @@ error[E0382]: use of moved value: `a`
 LL |     let mut a = NotCopy;
    |         ----- move occurs because `a` has type `NotCopy`, which does not implement the `Copy` trait
 LL |     loop {
+   |     ---- inside of this loop
 LL |         || {
    |         ^^ value moved into closure here, in previous iteration of loop
 LL |             &mut a;
 LL |             a;
    |             - use occurs due to use in closure
+   |
+note: if `NotCopy` implemented `Clone`, you could clone the value
+  --> $DIR/issue-75904-move-closure-loop.rs:5:1
+   |
+LL | struct NotCopy;
+   | ^^^^^^^^^^^^^^
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/moves/moves-based-on-type-capture-clause-bad.rs b/tests/ui/moves/moves-based-on-type-capture-clause-bad.rs
@@ -2,7 +2,7 @@ use std::thread;
 
 fn main() {
     let x = "Hello world!".to_string();
-    thread::spawn(move|| {
+    thread::spawn(move || {
         println!("{}", x);
     });
     println!("{}", x); //~ ERROR borrow of moved value
diff --git a/tests/ui/moves/moves-based-on-type-capture-clause-bad.stderr b/tests/ui/moves/moves-based-on-type-capture-clause-bad.stderr
@@ -3,8 +3,8 @@ error[E0382]: borrow of moved value: `x`
    |
 LL |     let x = "Hello world!".to_string();
    |         - move occurs because `x` has type `String`, which does not implement the `Copy` trait
-LL |     thread::spawn(move|| {
-   |                   ------ value moved into closure here
+LL |     thread::spawn(move || {
+   |                   ------- value moved into closure here
 LL |         println!("{}", x);
    |                        - variable moved due to use in closure
 LL |     });
diff --git a/tests/ui/nll/closure-move-spans.fixed b/tests/ui/nll/closure-move-spans.fixed
@@ -0,0 +1,23 @@
+// check that moves due to a closure capture give a special note
+//@ run-rustfix
+#![allow(unused_variables, unused_must_use, dead_code)]
+
+fn move_after_move(x: String) {
+    || x.clone();
+    let y = x; //~ ERROR
+}
+
+fn borrow_after_move(x: String) {
+    || x.clone();
+    let y = &x; //~ ERROR
+}
+
+fn borrow_mut_after_move(mut x: String) {
+    || x.clone();
+    let y = &mut x; //~ ERROR
+}
+
+fn fn_ref<F: Fn()>(f: F) -> F { f }
+fn fn_mut<F: FnMut()>(f: F) -> F { f }
+
+fn main() {}
diff --git a/tests/ui/nll/closure-move-spans.rs b/tests/ui/nll/closure-move-spans.rs
@@ -1,4 +1,6 @@
 // check that moves due to a closure capture give a special note
+//@ run-rustfix
+#![allow(unused_variables, unused_must_use, dead_code)]
 
 fn move_after_move(x: String) {
     || x;
diff --git a/tests/ui/nll/closure-move-spans.stderr b/tests/ui/nll/closure-move-spans.stderr
@@ -1,5 +1,5 @@
 error[E0382]: use of moved value: `x`
-  --> $DIR/closure-move-spans.rs:5:13
+  --> $DIR/closure-move-spans.rs:7:13
    |
 LL | fn move_after_move(x: String) {
    |                    - move occurs because `x` has type `String`, which does not implement the `Copy` trait
@@ -9,9 +9,14 @@ LL |     || x;
    |     value moved into closure here
 LL |     let y = x;
    |             ^ value used here after move
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     || x.clone();
+   |         ++++++++
 
 error[E0382]: borrow of moved value: `x`
-  --> $DIR/closure-move-spans.rs:10:13
+  --> $DIR/closure-move-spans.rs:12:13
    |
 LL | fn borrow_after_move(x: String) {
    |                      - move occurs because `x` has type `String`, which does not implement the `Copy` trait
@@ -21,9 +26,14 @@ LL |     || x;
    |     value moved into closure here
 LL |     let y = &x;
    |             ^^ value borrowed here after move
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     || x.clone();
+   |         ++++++++
 
 error[E0382]: borrow of moved value: `x`
-  --> $DIR/closure-move-spans.rs:15:13
+  --> $DIR/closure-move-spans.rs:17:13
    |
 LL | fn borrow_mut_after_move(mut x: String) {
    |                          ----- move occurs because `x` has type `String`, which does not implement the `Copy` trait
@@ -33,6 +43,11 @@ LL |     || x;
    |     value moved into closure here
 LL |     let y = &mut x;
    |             ^^^^^^ value borrowed here after move
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     || x.clone();
+   |         ++++++++
 
 error: aborting due to 3 previous errors
 
diff --git a/tests/ui/nll/closures-in-loops.stderr b/tests/ui/nll/closures-in-loops.stderr
@@ -4,10 +4,16 @@ error[E0382]: use of moved value: `x`
 LL | fn repreated_move(x: String) {
    |                   - move occurs because `x` has type `String`, which does not implement the `Copy` trait
 LL |     for i in 0..10 {
+   |     -------------- inside of this loop
 LL |         || x;
    |         ^^ - use occurs due to use in closure
    |         |
    |         value moved into closure here, in previous iteration of loop
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |         || x.clone();
+   |             ++++++++
 
 error[E0499]: cannot borrow `x` as mutable more than once at a time
   --> $DIR/closures-in-loops.rs:13:16
diff --git a/tests/ui/nll/issue-27282-move-match-input-into-guard.stderr b/tests/ui/nll/issue-27282-move-match-input-into-guard.stderr
@@ -10,6 +10,11 @@ LL |         _ if { (|| { let bar = b; *bar = false; })();
    |                 --             - variable moved due to use in closure
    |                 |
    |                 value moved into closure here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |         _ if { (|| { let bar = b.clone(); *bar = false; })();
+   |                                 ++++++++
 
 error[E0382]: use of moved value: `b`
   --> $DIR/issue-27282-move-match-input-into-guard.rs:24:5
@@ -23,6 +28,11 @@ LL |             (|| { let bar = b; *bar = false; })();
    |              --             - variable moved due to use in closure
    |              |
    |              value moved into closure here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |             (|| { let bar = b.clone(); *bar = false; })();
+   |                              ++++++++
 
 error: aborting due to 2 previous errors
 
diff --git a/tests/ui/unboxed-closures/unboxed-closure-illegal-move.stderr b/tests/ui/unboxed-closures/unboxed-closure-illegal-move.stderr
@@ -37,11 +37,6 @@ LL |         let f = to_fn(move || drop(x));
    |                       -------      ^ move occurs because `x` has type `Box<i32>`, which does not implement the `Copy` trait
    |                       |
    |                       captured by this `Fn` closure
-   |
-help: consider cloning the value if the performance cost is acceptable
-   |
-LL |         let f = to_fn(move || drop(x.clone()));
-   |                                     ++++++++
 
 error[E0507]: cannot move out of `x`, a captured variable in an `FnMut` closure
   --> $DIR/unboxed-closure-illegal-move.rs:32:40
@@ -52,11 +47,6 @@ LL |         let f = to_fn_mut(move || drop(x));
    |                           -------      ^ move occurs because `x` has type `Box<i32>`, which does not implement the `Copy` trait
    |                           |
    |                           captured by this `FnMut` closure
-   |
-help: consider cloning the value if the performance cost is acceptable
-   |
-LL |         let f = to_fn_mut(move || drop(x.clone()));
-   |                                         ++++++++
 
 error: aborting due to 4 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-struct NoCopy;`
	`1`	+struct NoCopy; //~ NOTE if `NoCopy` implemented `Clone`, you could clone the value
`2`	`2`	`fn main() {`
`3`	`3`	`let x = NoCopy;`
`4`	`4`	//~^ NOTE move occurs because `x` has type `NoCopy`