When possible, suggest cloning the result of a call instead of an argument

```
error[E0505]: cannot move out of `a` because it is borrowed
  --> $DIR/variance-issue-20533.rs:28:14
   |
LL |         let a = AffineU32(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).clone();
   |                        ++++++++
```

Author: estebank

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -464,7 +464,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 {
                     // We already suggest cloning for these cases in `explain_captures`.
                 } else {
-                    self.suggest_cloning(err, ty, expr);
+                    self.suggest_cloning(err, ty, expr, None);
                 }
             }
             if let Some(pat) = finder.pat {
@@ -747,7 +747,78 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         true
     }
 
-    pub(crate) fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>) {
+    pub(crate) fn suggest_cloning(
+        &self,
+        err: &mut Diag<'_>,
+        ty: Ty<'tcx>,
+        expr: &hir::Expr<'_>,
+        other_expr: Option<&hir::Expr<'_>>,
+    ) {
+        'outer: {
+            if let ty::Ref(..) = ty.kind() {
+                // We check for either `let binding = foo(expr, other_expr);` or
+                // `foo(expr, other_expr);` and if so we don't suggest an incorrect
+                // `foo(expr, other_expr).clone()`
+                if let Some(other_expr) = other_expr
+                    && let Some(parent_let) =
+                        self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
+                            if let (hir_id, hir::Node::Local(_) | hir::Node::Stmt(_)) = n {
+                                Some(hir_id)
+                            } else {
+                                None
+                            }
+                        })
+                    && let Some(other_parent_let) =
+                        self.infcx.tcx.hir().parent_iter(other_expr.hir_id).find_map(|n| {
+                            if let (hir_id, hir::Node::Local(_) | hir::Node::Stmt(_)) = n {
+                                Some(hir_id)
+                            } else {
+                                None
+                            }
+                        })
+                    && parent_let == other_parent_let
+                {
+                    // Explicitly check that we don't have `foo(&*expr, other_expr)`, as cloning the
+                    // result of `foo(...)` won't help.
+                    break 'outer;
+                }
+
+                // We're suggesting `.clone()` on an borrowed value. See if the expression we have
+                // is an argument to a function or method call, and try to suggest cloning the
+                // *result* of the call, instead of the argument. This is closest to what people
+                // would actually be looking for in most cases, with maybe the exception of things
+                // like `fn(T) -> T`, but even then it is reasonable.
+                let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
+                let mut prev = expr;
+                while let hir::Node::Expr(parent) = self.infcx.tcx.parent_hir_node(prev.hir_id) {
+                    if let hir::ExprKind::Call(..) | hir::ExprKind::MethodCall(..) = parent.kind
+                        && let Some(call_ty) = typeck_results.node_type_opt(parent.hir_id)
+                        && let call_ty = call_ty.peel_refs()
+                        && (!call_ty
+                            .walk()
+                            .any(|t| matches!(t.unpack(), ty::GenericArgKind::Lifetime(_)))
+                            || if let ty::Alias(ty::Projection, _) = call_ty.kind() {
+                                // FIXME: this isn't quite right with lifetimes on assoc types,
+                                // but ignore for now. We will only suggest cloning if
+                                // `<Ty as Trait>::Assoc: Clone`, which should keep false positives
+                                // down to a managable ammount.
+                                true
+                            } else {
+                                false
+                            })
+                        && let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
+                        && self
+                            .infcx
+                            .type_implements_trait(clone_trait_def, [call_ty], self.param_env)
+                            .must_apply_modulo_regions()
+                        && self.suggest_cloning_inner(err, call_ty, parent)
+                    {
+                        return;
+                    }
+                    prev = parent;
+                }
+            }
+        }
         let ty = ty.peel_refs();
         if let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
             && self
@@ -774,12 +845,17 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
-    fn suggest_cloning_inner(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>) {
+    fn suggest_cloning_inner(
+        &self,
+        err: &mut Diag<'_>,
+        ty: Ty<'tcx>,
+        expr: &hir::Expr<'_>,
+    ) -> bool {
         let tcx = self.infcx.tcx;
         if let Some(_) = self.clone_on_reference(expr) {
             // Avoid redundant clone suggestion already suggested in `explain_captures`.
             // See `tests/ui/moves/needs-clone-through-deref.rs`
-            return;
+            return false;
         }
         // Try to find predicates on *generic params* that would allow copying `ty`
         let suggestion =
@@ -796,7 +872,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             if let hir::ExprKind::AddrOf(_, hir::Mutability::Mut, _) = inner_expr.kind {
                 // We assume that `&mut` refs are desired for their side-effects, so cloning the
                 // value wouldn't do what the user wanted.
-                return;
+                return false;
             }
             inner_expr = inner;
         }
@@ -819,6 +895,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             "consider cloning the value if the performance cost is acceptable"
         };
         err.multipart_suggestion_verbose(msg, sugg, Applicability::MachineApplicable);
+        true
     }
 
     fn suggest_adding_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, def_id: DefId, span: Span) {
@@ -927,7 +1004,7 @@ 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.suggest_cloning(&mut err, ty, expr, self.find_expr(span));
         }
         self.buffer_error(err);
     }

compiler/rustc_borrowck/src/diagnostics/move_errors.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;
@@ -445,7 +447,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     };
 
                     if let Some(expr) = self.find_expr(span) {
-                        self.suggest_cloning(err, place_ty, expr);
+                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span));
                     }
 
                     err.subdiagnostic(
@@ -472,15 +474,15 @@ 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(span) {
-                    self.suggest_cloning(err, place_ty, expr);
+                if let Some(expr) = self.find_expr(use_span) {
+                    self.suggest_cloning(err, place_ty, expr, self.find_expr(span));
                 }
 
                 err.subdiagnostic(
@@ -489,7 +491,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                         is_partial_move: false,
                         ty: place_ty,
                         place: &place_desc,
-                        span,
+                        span: use_span,
                     },
                 );
 
@@ -593,7 +595,7 @@ 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);
+                    self.suggest_cloning(err, bind_to.ty, expr, None);
                 }
 
                 err.subdiagnostic(

tests/ui/associated-types/associated-types-outlives.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.stderr

@@ -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/variance/variance-issue-20533.rs

@@ -19,8 +19,15 @@ fn baz<'a, T>(_x: &'a T) -> Baked<'a> {
     Baked(PhantomData)
 }
 
+fn bat(x: &AffineU32) -> &u32 {
+    &x.0
+}
+
 struct AffineU32(u32);
 
+#[derive(Clone)]
+struct ClonableAffineU32(u32);
+
 fn main() {
     {
         let a = AffineU32(1);
@@ -40,4 +47,16 @@ fn main() {
         drop(a); //~ ERROR cannot move out of `a`
         drop(x);
     }
+    {
+        let a = AffineU32(1);
+        let x = bat(&a);
+        drop(a); //~ ERROR cannot move out of `a`
+        drop(x);
+    }
+    {
+        let a = ClonableAffineU32(1);
+        let x = foo(&a);
+        drop(a); //~ ERROR cannot move out of `a`
+        drop(x);
+    }
 }

tests/ui/variance/variance-issue-20533.stderr

@@ -1,5 +1,5 @@
 error[E0505]: cannot move out of `a` because it is borrowed
-  --> $DIR/variance-issue-20533.rs:28:14
+  --> $DIR/variance-issue-20533.rs:35:14
    |
 LL |         let a = AffineU32(1);
    |             - binding `a` declared here
@@ -11,7 +11,7 @@ LL |         drop(x);
    |              - borrow later used here
 
 error[E0505]: cannot move out of `a` because it is borrowed
-  --> $DIR/variance-issue-20533.rs:34:14
+  --> $DIR/variance-issue-20533.rs:41:14
    |
 LL |         let a = AffineU32(1);
    |             - binding `a` declared here
@@ -23,7 +23,7 @@ LL |         drop(x);
    |              - borrow later used here
 
 error[E0505]: cannot move out of `a` because it is borrowed
-  --> $DIR/variance-issue-20533.rs:40:14
+  --> $DIR/variance-issue-20533.rs:47:14
    |
 LL |         let a = AffineU32(1);
    |             - binding `a` declared here
@@ -34,6 +34,41 @@ LL |         drop(a);
 LL |         drop(x);
    |              - borrow later used here
 
-error: aborting due to 3 previous errors
+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();
+   |                        ++++++++
+
+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());
+   |
+
+error: aborting due to 5 previous errors
 
 For more information about this error, try `rustc --explain E0505`.