From d695130d9795ed82646c6593afaf07bdb0a578be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 1 May 2024 20:46:06 +0000 Subject: [PATCH] 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. --- .../src/diagnostics/conflict_errors.rs | 22 +++++++++-- .../src/diagnostics/move_errors.rs | 37 ++++++++----------- src/tools/tidy/src/ui_tests.rs | 2 +- ...rowck-move-moved-value-into-closure.stderr | 6 +++ ...ves-based-on-type-capture-clause-bad.fixed | 11 ++++++ .../moves-based-on-type-capture-clause-bad.rs | 1 + ...es-based-on-type-capture-clause-bad.stderr | 8 +++- tests/ui/{ => moves}/no-capture-arc.rs | 0 tests/ui/{ => moves}/no-capture-arc.stderr | 6 +++ tests/ui/moves/no-reuse-move-arc.fixed | 17 +++++++++ tests/ui/{ => moves}/no-reuse-move-arc.rs | 1 + tests/ui/{ => moves}/no-reuse-move-arc.stderr | 8 +++- 12 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 tests/ui/moves/moves-based-on-type-capture-clause-bad.fixed rename tests/ui/{ => moves}/no-capture-arc.rs (100%) rename tests/ui/{ => moves}/no-capture-arc.stderr (80%) create mode 100644 tests/ui/moves/no-reuse-move-arc.fixed rename tests/ui/{ => moves}/no-reuse-move-arc.rs (95%) rename tests/ui/{ => moves}/no-reuse-move-arc.stderr (76%) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 6c82dd1489ca2..e0c62b5ebd7d3 100644 --- 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 index 1d844c3d6a295..d91ade610c62b 100644 --- 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>, ) { 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 index 94a0eee154d93..f8a8246f661d9 100644 --- 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 index 6a77d86f250a1..79e7203175488 100644 --- 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 new file mode 100644 index 0000000000000..04a183ca96be4 --- /dev/null +++ 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 index 9d7277c1c2499..c9a7f2c8ed805 100644 --- 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 index c2b9aeab23748..c323e2436d243 100644 --- 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 b/tests/ui/moves/no-capture-arc.rs similarity index 100% rename from tests/ui/no-capture-arc.rs rename to tests/ui/moves/no-capture-arc.rs diff --git a/tests/ui/no-capture-arc.stderr b/tests/ui/moves/no-capture-arc.stderr similarity index 80% rename from tests/ui/no-capture-arc.stderr rename to tests/ui/moves/no-capture-arc.stderr index 38432c851c522..605e87519519f 100644 --- a/tests/ui/no-capture-arc.stderr +++ b/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` +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 new file mode 100644 index 0000000000000..a5dac8cc14bf2 --- /dev/null +++ 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 b/tests/ui/moves/no-reuse-move-arc.rs similarity index 95% rename from tests/ui/no-reuse-move-arc.rs rename to tests/ui/moves/no-reuse-move-arc.rs index 9c957a4e01b41..0d67aa56489ce 100644 --- a/tests/ui/no-reuse-move-arc.rs +++ b/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 b/tests/ui/moves/no-reuse-move-arc.stderr similarity index 76% rename from tests/ui/no-reuse-move-arc.stderr rename to tests/ui/moves/no-reuse-move-arc.stderr index cdeb6eadc174f..841bb7fda9b65 100644 --- a/tests/ui/no-reuse-move-arc.stderr +++ b/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>`, 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` +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