Auto merge of rust-lang#122603 - estebank:clone-o-rama, r=lcnr

Detect borrow checker errors where `.clone()` would be an appropriate user action

When a value is moved twice, suggest cloning the earlier move:

```
error[E0509]: cannot move out of type `U2`, which implements the `Drop` trait
  --> $DIR/union-move.rs:49:18
   |
LL |         move_out(x.f1_nocopy);
   |                  ^^^^^^^^^^^
   |                  |
   |                  cannot move out of here
   |                  move occurs because `x.f1_nocopy` has type `ManuallyDrop<RefCell<i32>>`, which does not implement the `Copy` trait
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL |         move_out(x.f1_nocopy.clone());
   |                             ++++++++
```

When a value is borrowed by an `fn` call, consider if cloning the result of the call would be reasonable, and suggest cloning that, instead of the argument:

```
error[E0505]: cannot move out of `a` because it is borrowed
  --> $DIR/variance-issue-20533.rs:53:14
   |
LL |         let a = AffineU32(1);
   |             - binding `a` declared here
LL |         let x = bat(&a);
   |                     -- borrow of `a` occurs here
LL |         drop(a);
   |              ^ move out of `a` occurs here
LL |         drop(x);
   |              - borrow later used here
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL |         let x = bat(&a).clone();
   |                        ++++++++
```

otherwise, suggest cloning the argument:

```
error[E0505]: cannot move out of `a` because it is borrowed
  --> $DIR/variance-issue-20533.rs:59:14
   |
LL |         let a = ClonableAffineU32(1);
   |             - binding `a` declared here
LL |         let x = foo(&a);
   |                     -- borrow of `a` occurs here
LL |         drop(a);
   |              ^ move out of `a` occurs here
LL |         drop(x);
   |              - borrow later used here
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL -         let x = foo(&a);
LL +         let x = foo(a.clone());
   |
```

This suggestion doesn't attempt to square out the types between what's cloned and what the `fn` expects, to allow the user to make a determination on whether to change the `fn` call or `fn` definition themselves.

Special case move errors caused by `FnOnce`:

```
error[E0382]: use of moved value: `blk`
  --> $DIR/once-cant-call-twice-on-heap.rs:8:5
   |
LL | fn foo<F:FnOnce()>(blk: F) {
   |                    --- move occurs because `blk` has type `F`, which does not implement the `Copy` trait
LL |     blk();
   |     ----- `blk` moved due to this call
LL |     blk();
   |     ^^^ value used here after move
   |
note: `FnOnce` closures can only be called once
  --> $DIR/once-cant-call-twice-on-heap.rs:6:10
   |
LL | fn foo<F:FnOnce()>(blk: F) {
   |          ^^^^^^^^ `F` is made to be an `FnOnce` closure here
LL |     blk();
   |     ----- this value implements `FnOnce`, which causes it to be moved when called
```

Account for redundant `.clone()` calls in resulting suggestions:

```
error[E0507]: cannot move out of dereference of `S`
  --> $DIR/needs-clone-through-deref.rs:15:18
   |
LL |         for _ in self.clone().into_iter() {}
   |                  ^^^^^^^^^^^^ ----------- value moved due to this method call
   |                  |
   |                  move occurs because value has type `Vec<usize>`, which does not implement the `Copy` trait
   |
note: `into_iter` takes ownership of the receiver `self`, which moves value
  --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
help: you can `clone` the value and consume it, but this might not be your desired behavior
   |
LL |         for _ in <Vec<usize> as Clone>::clone(&self).into_iter() {}
   |                  ++++++++++++++++++++++++++++++    ~
```

We use the presence of `&mut` values in a move error as a proxy for the user caring about side effects, so we don't emit a clone suggestion in that case:

```
error[E0505]: cannot move out of `s` because it is borrowed
  --> $DIR/borrowck-overloaded-index-move-index.rs:53:7
   |
LL |     let mut s = "hello".to_string();
   |         ----- binding `s` declared here
LL |     let rs = &mut s;
   |              ------ borrow of `s` occurs here
...
LL |     f[s] = 10;
   |       ^ move out of `s` occurs here
...
LL |     use_mut(rs);
   |             -- borrow later used here
```

We properly account for `foo += foo;` errors where we *don't* suggest `foo.clone() += foo;`, instead suggesting `foo += foo.clone();`.

---

Each commit can be reviewed in isolation. There are some "cleanup" commits, but kept them separate in order to show *why* specific changes were being made, and their effect on tests' output.

Fix rust-lang#49693, CC rust-lang#64167.

Author: bors

compiler/rustc_borrowck/messages.ftl

@@ -87,6 +87,12 @@ borrowck_move_unsized =
 borrowck_moved_a_fn_once_in_call =
     this value implements `FnOnce`, which causes it to be moved when called
 
+borrowck_moved_a_fn_once_in_call_call =
+    `FnOnce` closures can only be called once
+
+borrowck_moved_a_fn_once_in_call_def =
+    `{$ty}` is made to be an `FnOnce` closure here
+
 borrowck_moved_due_to_await =
     {$place_name} {$is_partial ->
         [true] partially moved

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -5,6 +5,7 @@ use crate::session_diagnostics::{
     CaptureVarKind, CaptureVarPathUseCause, OnClosureNote,
 };
 use rustc_errors::{Applicability, Diag};
+use rustc_errors::{DiagCtxt, MultiSpan};
 use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, Namespace};
 use rustc_hir::CoroutineKind;
@@ -29,6 +30,8 @@ use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt as _;
 use rustc_trait_selection::traits::type_known_to_meet_bound_modulo_regions;
 
+use crate::fluent_generated as fluent;
+
 use super::borrow_set::BorrowData;
 use super::MirBorrowckCtxt;
 
@@ -587,7 +590,7 @@ impl UseSpans<'_> {
     #[allow(rustc::diagnostic_outside_of_impl)]
     pub(super) fn args_subdiag(
         self,
-        dcx: &rustc_errors::DiagCtxt,
+        dcx: &DiagCtxt,
         err: &mut Diag<'_>,
         f: impl FnOnce(Span) -> CaptureArgLabel,
     ) {
@@ -601,7 +604,7 @@ impl UseSpans<'_> {
     #[allow(rustc::diagnostic_outside_of_impl)]
     pub(super) fn var_path_only_subdiag(
         self,
-        dcx: &rustc_errors::DiagCtxt,
+        dcx: &DiagCtxt,
         err: &mut Diag<'_>,
         action: crate::InitializationRequiringAction,
     ) {
@@ -639,7 +642,7 @@ impl UseSpans<'_> {
     #[allow(rustc::diagnostic_outside_of_impl)]
     pub(super) fn var_subdiag(
         self,
-        dcx: &rustc_errors::DiagCtxt,
+        dcx: &DiagCtxt,
         err: &mut Diag<'_>,
         kind: Option<rustc_middle::mir::BorrowKind>,
         f: impl FnOnce(hir::ClosureKind, Span) -> CaptureVarCause,
@@ -1034,7 +1037,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 .map(|n| format!("`{n}`"))
                 .unwrap_or_else(|| "value".to_owned());
             match kind {
-                CallKind::FnCall { fn_trait_id, .. }
+                CallKind::FnCall { fn_trait_id, self_ty }
                     if Some(fn_trait_id) == self.infcx.tcx.lang_items().fn_once_trait() =>
                 {
                     err.subdiagnostic(
@@ -1046,7 +1049,79 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             is_loop_message,
                         },
                     );
-                    err.subdiagnostic(self.dcx(), CaptureReasonNote::FnOnceMoveInCall { var_span });
+                    // Check if the move occurs on a value because of a call on a closure that comes
+                    // from a type parameter `F: FnOnce()`. If so, we provide a targeted `note`:
+                    // ```
+                    // error[E0382]: use of moved value: `blk`
+                    //   --> $DIR/once-cant-call-twice-on-heap.rs:8:5
+                    //    |
+                    // LL | fn foo<F:FnOnce()>(blk: F) {
+                    //    |                    --- move occurs because `blk` has type `F`, which does not implement the `Copy` trait
+                    // LL | blk();
+                    //    | ----- `blk` moved due to this call
+                    // LL | blk();
+                    //    | ^^^ value used here after move
+                    //    |
+                    // note: `FnOnce` closures can only be called once
+                    //   --> $DIR/once-cant-call-twice-on-heap.rs:6:10
+                    //    |
+                    // LL | fn foo<F:FnOnce()>(blk: F) {
+                    //    |        ^^^^^^^^ `F` is made to be an `FnOnce` closure here
+                    // LL | blk();
+                    //    | ----- this value implements `FnOnce`, which causes it to be moved when called
+                    // ```
+                    if let ty::Param(param_ty) = self_ty.kind()
+                        && let generics = self.infcx.tcx.generics_of(self.mir_def_id())
+                        && let param = generics.type_param(param_ty, self.infcx.tcx)
+                        && let Some(hir_generics) = self
+                            .infcx
+                            .tcx
+                            .typeck_root_def_id(self.mir_def_id().to_def_id())
+                            .as_local()
+                            .and_then(|def_id| self.infcx.tcx.hir().get_generics(def_id))
+                        && let spans = hir_generics
+                            .predicates
+                            .iter()
+                            .filter_map(|pred| match pred {
+                                hir::WherePredicate::BoundPredicate(pred) => Some(pred),
+                                _ => None,
+                            })
+                            .filter(|pred| {
+                                if let Some((id, _)) = pred.bounded_ty.as_generic_param() {
+                                    id == param.def_id
+                                } else {
+                                    false
+                                }
+                            })
+                            .flat_map(|pred| pred.bounds)
+                            .filter_map(|bound| {
+                                if let Some(trait_ref) = bound.trait_ref()
+                                    && let Some(trait_def_id) = trait_ref.trait_def_id()
+                                    && trait_def_id == fn_trait_id
+                                {
+                                    Some(bound.span())
+                                } else {
+                                    None
+                                }
+                            })
+                            .collect::<Vec<Span>>()
+                        && !spans.is_empty()
+                    {
+                        let mut span: MultiSpan = spans.clone().into();
+                        for sp in spans {
+                            span.push_span_label(sp, fluent::borrowck_moved_a_fn_once_in_call_def);
+                        }
+                        span.push_span_label(
+                            fn_call_span,
+                            fluent::borrowck_moved_a_fn_once_in_call,
+                        );
+                        err.span_note(span, fluent::borrowck_moved_a_fn_once_in_call_call);
+                    } else {
+                        err.subdiagnostic(
+                            self.dcx(),
+                            CaptureReasonNote::FnOnceMoveInCall { var_span },
+                        );
+                    }
                 }
                 CallKind::Operator { self_arg, trait_id, .. } => {
                     let self_arg = self_arg.unwrap();
@@ -1212,13 +1287,21 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                                 .iter_projections()
                                 .any(|(_, elem)| matches!(elem, ProjectionElem::Deref))
                             {
+                                let (start, end) = if let Some(expr) = self.find_expr(move_span)
+                                    && let Some(_) = self.clone_on_reference(expr)
+                                    && let hir::ExprKind::MethodCall(_, rcvr, _, _) = expr.kind
+                                {
+                                    (move_span.shrink_to_lo(), move_span.with_lo(rcvr.span.hi()))
+                                } else {
+                                    (move_span.shrink_to_lo(), move_span.shrink_to_hi())
+                                };
                                 vec![
                                     // We use the fully-qualified path because `.clone()` can
                                     // sometimes choose `<&T as Clone>` instead of `<T as Clone>`
                                     // when going through auto-deref, so this ensures that doesn't
                                     // happen, causing suggestions for `.clone().clone()`.
-                                    (move_span.shrink_to_lo(), format!("<{ty} as Clone>::clone(&")),
-                                    (move_span.shrink_to_hi(), ")".to_string()),
+                                    (start, format!("<{ty} as Clone>::clone(&")),
+                                    (end, ")".to_string()),
                                 ]
                             } else {
                                 vec![(move_span.shrink_to_hi(), ".clone()".to_string())]

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -435,7 +435,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
 
     fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
         match error {
-            GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
+            GroupedMoveError::MovesFromPlace {
+                mut binds_to, move_from, span: other_span, ..
+            } => {
                 self.add_borrow_suggestions(err, span);
                 if binds_to.is_empty() {
                     let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
@@ -444,6 +446,10 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                         None => "value".to_string(),
                     };
 
+                    if let Some(expr) = self.find_expr(span) {
+                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span));
+                    }
+
                     err.subdiagnostic(
                         self.dcx(),
                         crate::session_diagnostics::TypeNoCopy::Label {
@@ -468,19 +474,24 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
             }
             // No binding. Nothing to suggest.
             GroupedMoveError::OtherIllegalMove { ref original_path, use_spans, .. } => {
-                let span = use_spans.var_or_use();
+                let use_span = use_spans.var_or_use();
                 let place_ty = original_path.ty(self.body, self.infcx.tcx).ty;
                 let place_desc = match self.describe_place(original_path.as_ref()) {
                     Some(desc) => format!("`{desc}`"),
                     None => "value".to_string(),
                 };
+
+                if let Some(expr) = self.find_expr(use_span) {
+                    self.suggest_cloning(err, place_ty, expr, self.find_expr(span));
+                }
+
                 err.subdiagnostic(
                     self.dcx(),
                     crate::session_diagnostics::TypeNoCopy::Label {
                         is_partial_move: false,
                         ty: place_ty,
                         place: &place_desc,
-                        span,
+                        span: use_span,
                     },
                 );
 
@@ -582,6 +593,11 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
 
             if binds_to.len() == 1 {
                 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);
+                }
+
                 err.subdiagnostic(
                     self.dcx(),
                     crate::session_diagnostics::TypeNoCopy::Label {

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -3,10 +3,10 @@
 // fn body, causing this (invalid) code to be accepted.
 
 pub trait Foo<'a> {
-    type Bar;
+    type Bar: Clone;
 }
 
-impl<'a, T:'a> Foo<'a> for T {
+impl<'a, T: 'a> Foo<'a> for T {
     type Bar = &'a T;
 }
 

tests/ui/associated-types/associated-types-outlives.rs

@@ -10,6 +10,11 @@ LL |         drop(x);
    |              ^ move out of `x` occurs here
 LL |         return f(y);
    |                  - borrow later used here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |         's: loop { y = denormalise(&x).clone(); break }
+   |                                       ++++++++
 
 error: aborting due to 1 previous error
 

tests/ui/associated-types/associated-types-outlives.stderr

@@ -7,6 +7,12 @@ LL |     drop(t);
    |          - value moved here
 LL |     drop(t);
    |          ^ value used here after move
+   |
+note: if `S<()>` implemented `Clone`, you could clone the value
+  --> $DIR/issue-25700.rs:1:1
+   |
+LL | struct S<T: 'static>(#[allow(dead_code)] Option<&'static T>);
+   | ^^^^^^^^^^^^^^^^^^^^
 
 error: aborting due to 1 previous error
 

tests/ui/associated-types/issue-25700.stderr

@@ -1,5 +1,6 @@
 use std::ops::AddAssign;
 
+#[derive(Clone)]
 struct Int(i32);
 
 impl AddAssign for Int {
@@ -16,6 +17,7 @@ fn main() {
     x;
     //~^ ERROR cannot move out of `x` because it is borrowed
     //~| move out of `x` occurs here
+    //~| HELP consider cloning
 
     let y = Int(2);
     //~^ HELP consider changing this to be mutable

tests/ui/augmented-assignments.rs

@@ -1,5 +1,5 @@
 error[E0505]: cannot move out of `x` because it is borrowed
-  --> $DIR/augmented-assignments.rs:16:5
+  --> $DIR/augmented-assignments.rs:17:5
    |
 LL |     let mut x = Int(1);
    |         ----- binding `x` declared here
@@ -8,9 +8,14 @@ LL |     x
 ...
 LL |     x;
    |     ^ move out of `x` occurs here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     x.clone();
+   |      ++++++++
 
 error[E0596]: cannot borrow `y` as mutable, as it is not declared as mutable
-  --> $DIR/augmented-assignments.rs:23:5
+  --> $DIR/augmented-assignments.rs:25:5
    |
 LL |     y
    |     ^ cannot borrow as mutable

tests/ui/augmented-assignments.stderr

@@ -10,6 +10,12 @@ LL |     let y = x;
 LL |
 LL |     r.use_ref();
    |     - borrow later used here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL -     let r = &x.0;
+LL +     let r = x.0.clone();
+   |
 
 error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
   --> $DIR/borrow-tuple-fields.rs:18:13
@@ -42,6 +48,12 @@ LL |     let y = x;
    |             ^ move out of `x` occurs here
 LL |     r.use_ref();
    |     - borrow later used here
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL -     let r = &x.0;
+LL +     let r = x.0.clone();
+   |
 
 error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
   --> $DIR/borrow-tuple-fields.rs:33:13