Suggest cloning Arc moved into closure

``` error[E0382]: borrow of moved value: `x` --> $DIR/moves-based-on-type-capture-clause-bad.rs:9:20 | 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 | println!("{}", x); | - variable moved due to use in closure LL | }); LL | println!("{}", x); | ^ value borrowed here after move | = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) help: clone the value before moving it into the closure | LL ~ let value = x.clone(); LL ~ thread::spawn(move || { LL ~ println!("{}", value); | ``` Fix #104232.
rust-lang · May 1, 2024 · d695130 · d695130
1 parent 20aa2d8
commit d695130
Show file tree

Hide file tree

Showing 12 changed files with 90 additions and 29 deletions.
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -492,11 +492,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     ..
                 } = move_spans
                 {
-                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
+                    self.suggest_cloning(err, place.as_ref(), ty, expr, None, Some(move_spans));
                 } else if self.suggest_hoisting_call_outside_loop(err, expr) {
                     // The place where the the type moves would be misleading to suggest clone.
                     // #121466
-                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
+                    self.suggest_cloning(err, place.as_ref(), ty, expr, None, Some(move_spans));
                 }
             }
             if let Some(pat) = finder.pat {
@@ -1083,6 +1083,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
     pub(crate) fn suggest_cloning(
         &self,
         err: &mut Diag<'_>,
+        place: PlaceRef<'tcx>,
         ty: Ty<'tcx>,
         mut expr: &'cx hir::Expr<'cx>,
         mut other_expr: Option<&'cx hir::Expr<'cx>>,
@@ -1189,7 +1190,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
         let ty = ty.peel_refs();
         if self.implements_clone(ty) {
-            self.suggest_cloning_inner(err, ty, expr);
+            if self.in_move_closure(expr) {
+                if let Some(name) = self.describe_place(place) {
+                    self.suggest_clone_of_captured_var_in_move_closure(err, &name, use_spans);
+                }
+            } else {
+                self.suggest_cloning_inner(err, ty, expr);
+            }
         } else if let ty::Adt(def, args) = ty.kind()
             && def.did().as_local().is_some()
             && def.variants().iter().all(|variant| {
@@ -1441,7 +1448,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         if let Some(expr) = self.find_expr(borrow_span)
             && let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
         {
-            self.suggest_cloning(&mut err, ty, expr, self.find_expr(span), Some(move_spans));
+            self.suggest_cloning(
+                &mut err,
+                place.as_ref(),
+                ty,
+                expr,
+                self.find_expr(span),
+                Some(move_spans),
+            );
         }
         self.buffer_error(err);
     }

diff --git a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs
@@ -3,7 +3,7 @@
 
 use rustc_errors::{Applicability, Diag};
 use rustc_hir::intravisit::Visitor;
-use rustc_hir::{CaptureBy, ExprKind, HirId, Node};
+use rustc_hir::{CaptureBy, ExprKind, Node};
 use rustc_middle::bug;
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, Ty};
@@ -307,25 +307,17 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         self.cannot_move_out_of(span, &description)
     }
 
-    fn suggest_clone_of_captured_var_in_move_closure(
+    pub(in crate::diagnostics) fn suggest_clone_of_captured_var_in_move_closure(
         &self,
         err: &mut Diag<'_>,
-        upvar_hir_id: HirId,
         upvar_name: &str,
         use_spans: Option<UseSpans<'tcx>>,
     ) {
         let tcx = self.infcx.tcx;
-        let typeck_results = tcx.typeck(self.mir_def_id());
         let Some(use_spans) = use_spans else { return };
         // We only care about the case where a closure captured a binding.
         let UseSpans::ClosureUse { args_span, .. } = use_spans else { return };
         let Some(body_id) = tcx.hir_node(self.mir_hir_id()).body_id() else { return };
-        // Fetch the type of the expression corresponding to the closure-captured binding.
-        let Some(captured_ty) = typeck_results.node_type_opt(upvar_hir_id) else { return };
-        if !self.implements_clone(captured_ty) {
-            // We only suggest cloning the captured binding if the type can actually be cloned.
-            return;
-        };
         // Find the closure that captured the binding.
         let mut expr_finder = FindExprBySpan::new(args_span, tcx);
         expr_finder.include_closures = true;
@@ -501,20 +493,12 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 );
 
                 let closure_span = tcx.def_span(def_id);
-                let mut err = self
-                    .cannot_move_out_of(span, &place_description)
+                self.cannot_move_out_of(span, &place_description)
                     .with_span_label(upvar_span, "captured outer variable")
                     .with_span_label(
                         closure_span,
                         format!("captured by this `{closure_kind}` closure"),
-                    );
-                self.suggest_clone_of_captured_var_in_move_closure(
-                    &mut err,
-                    upvar_hir_id,
-                    &upvar_name,
-                    use_spans,
-                );
-                err
+                    )
             }
             _ => {
                 let source = self.borrowed_content_source(deref_base);
@@ -576,7 +560,14 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     };
 
                     if let Some(expr) = self.find_expr(span) {
-                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span), None);
+                        self.suggest_cloning(
+                            err,
+                            move_from.as_ref(),
+                            place_ty,
+                            expr,
+                            self.find_expr(other_span),
+                            None,
+                        );
                     }
 
                     err.subdiagnostic(
@@ -613,6 +604,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 if let Some(expr) = self.find_expr(use_span) {
                     self.suggest_cloning(
                         err,
+                        original_path.as_ref(),
                         place_ty,
                         expr,
                         self.find_expr(span),
@@ -730,7 +722,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 let place_desc = &format!("`{}`", self.local_names[*local].unwrap());
 
                 if let Some(expr) = self.find_expr(binding_span) {
-                    self.suggest_cloning(err, bind_to.ty, expr, None, None);
+                    let local_place: PlaceRef<'tcx> = (*local).into();
+                    self.suggest_cloning(err, local_place, bind_to.ty, expr, None, None);
                 }
 
                 err.subdiagnostic(

diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs
@@ -16,7 +16,7 @@ const ENTRY_LIMIT: usize = 900;
 // FIXME: The following limits should be reduced eventually.
 
 const ISSUES_ENTRY_LIMIT: usize = 1676;
-const ROOT_ENTRY_LIMIT: usize = 859;
+const ROOT_ENTRY_LIMIT: usize = 855;
 
 const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
     "rs",     // test source files

diff --git a/tests/ui/borrowck/borrowck-move-moved-value-into-closure.stderr b/tests/ui/borrowck/borrowck-move-moved-value-into-closure.stderr
@@ -12,6 +12,12 @@ LL |     call_f(move|| { *t + 1 });
    |            ^^^^^^   -- use occurs due to use in closure
    |            |
    |            value used here after move
+   |
+help: clone the value before moving it into the closure
+   |
+LL ~     let value = t.clone();
+LL ~     call_f(move|| { value + 1 });
+   |
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/moves/moves-based-on-type-capture-clause-bad.fixed b/tests/ui/moves/moves-based-on-type-capture-clause-bad.fixed
@@ -0,0 +1,11 @@
+//@ run-rustfix
+use std::thread;
+
+fn main() {
+    let x = "Hello world!".to_string();
+    let value = x.clone();
+    thread::spawn(move || {
+        println!("{}", value);
+    });
+    println!("{}", x); //~ ERROR borrow of moved value
+}
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
@@ -1,3 +1,4 @@
+//@ run-rustfix
 use std::thread;
 
 fn main() {

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
@@ -1,5 +1,5 @@
 error[E0382]: borrow of moved value: `x`
-  --> $DIR/moves-based-on-type-capture-clause-bad.rs:8:20
+  --> $DIR/moves-based-on-type-capture-clause-bad.rs:9:20
    |
 LL |     let x = "Hello world!".to_string();
    |         - move occurs because `x` has type `String`, which does not implement the `Copy` trait
@@ -12,6 +12,12 @@ LL |     println!("{}", x);
    |                    ^ value borrowed here after move
    |
    = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: clone the value before moving it into the closure
+   |
+LL ~     let value = x.clone();
+LL ~     thread::spawn(move || {
+LL ~         println!("{}", value);
+   |
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/no-capture-arc.rs → tests/ui/moves/no-capture-arc.rs b/tests/ui/no-capture-arc.rs → tests/ui/moves/no-capture-arc.rs
diff --git a/tests/ui/no-capture-arc.stderr → tests/ui/moves/no-capture-arc.stderr b/tests/ui/no-capture-arc.stderr → tests/ui/moves/no-capture-arc.stderr
@@ -13,6 +13,12 @@ LL |     assert_eq!((*arc_v)[2], 3);
    |                ^^^^^^^^ value borrowed here after move
    |
    = note: borrow occurs due to deref coercion to `Vec<i32>`
+help: clone the value before moving it into the closure
+   |
+LL ~     let value = arc_v.clone();
+LL ~     thread::spawn(move|| {
+LL ~         assert_eq!((*value)[3], 4);
+   |
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/moves/no-reuse-move-arc.fixed b/tests/ui/moves/no-reuse-move-arc.fixed
@@ -0,0 +1,17 @@
+//@ run-rustfix
+use std::sync::Arc;
+use std::thread;
+
+fn main() {
+    let v = vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+    let arc_v = Arc::new(v);
+
+    let value = arc_v.clone();
+    thread::spawn(move|| {
+        assert_eq!((*value)[3], 4);
+    });
+
+    assert_eq!((*arc_v)[2], 3); //~ ERROR borrow of moved value: `arc_v`
+
+    println!("{:?}", *arc_v);
+}
diff --git a/tests/ui/no-reuse-move-arc.rs → tests/ui/moves/no-reuse-move-arc.rs b/tests/ui/no-reuse-move-arc.rs → tests/ui/moves/no-reuse-move-arc.rs
@@ -1,3 +1,4 @@
+//@ run-rustfix
 use std::sync::Arc;
 use std::thread;
 

diff --git a/tests/ui/no-reuse-move-arc.stderr → tests/ui/moves/no-reuse-move-arc.stderr b/tests/ui/no-reuse-move-arc.stderr → tests/ui/moves/no-reuse-move-arc.stderr
@@ -1,5 +1,5 @@
 error[E0382]: borrow of moved value: `arc_v`
-  --> $DIR/no-reuse-move-arc.rs:12:16
+  --> $DIR/no-reuse-move-arc.rs:13:16
    |
 LL |     let arc_v = Arc::new(v);
    |         ----- move occurs because `arc_v` has type `Arc<Vec<i32>>`, which does not implement the `Copy` trait
@@ -13,6 +13,12 @@ LL |     assert_eq!((*arc_v)[2], 3);
    |                ^^^^^^^^ value borrowed here after move
    |
    = note: borrow occurs due to deref coercion to `Vec<i32>`
+help: clone the value before moving it into the closure
+   |
+LL ~     let value = arc_v.clone();
+LL ~     thread::spawn(move|| {
+LL ~         assert_eq!((*value)[3], 4);
+   |
 
 error: aborting due to 1 previous error