Account for .clone() when suggesting <T as Clone>::clone

Author: estebank

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -977,6 +977,21 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         can_suggest_clone
     }
 
+    pub(crate) fn clone_on_reference(&self, expr: &hir::Expr<'_>) -> Option<Span> {
+        let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
+        if let hir::ExprKind::MethodCall(segment, rcvr, args, span) = expr.kind
+            && let Some(expr_ty) = typeck_results.node_type_opt(expr.hir_id)
+            && let Some(rcvr_ty) = typeck_results.node_type_opt(rcvr.hir_id)
+            && rcvr_ty == expr_ty
+            && segment.ident.name == sym::clone
+            && args.is_empty()
+        {
+            Some(span)
+        } else {
+            None
+        }
+    }
+
     fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>, span: Span) {
         let tcx = self.infcx.tcx;
         // Try to find predicates on *generic params* that would allow copying `ty`
@@ -1644,6 +1659,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         );
     }
 
+    pub(crate) fn find_expr(&self, span: Span) -> Option<&hir::Expr<'_>> {
+        let tcx = self.infcx.tcx;
+        let body_id = tcx.hir_node(self.mir_hir_id()).body_id()?;
+        let mut expr_finder = FindExprBySpan::new(span);
+        expr_finder.visit_expr(tcx.hir().body(body_id).value);
+        expr_finder.result
+    }
+
     fn suggest_slice_method_if_applicable(
         &self,
         err: &mut Diag<'_>,

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -1214,13 +1214,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())]

tests/ui/moves/needs-clone-through-deref.fixed

@@ -12,7 +12,7 @@ impl Deref for S {
 impl S {
     fn foo(&self) {
         // `self.clone()` returns `&S`, not `Vec`
-        for _ in <Vec<usize> as Clone>::clone(&self.clone()).into_iter() {} //~ ERROR cannot move out of dereference of `S`
+        for _ in <Vec<usize> as Clone>::clone(&self).into_iter() {} //~ ERROR cannot move out of dereference of `S`
     }
 }
 fn main() {}

tests/ui/moves/needs-clone-through-deref.stderr

@@ -10,8 +10,8 @@ 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.clone()).into_iter() {}
-   |                  ++++++++++++++++++++++++++++++            +
+LL |         for _ in <Vec<usize> as Clone>::clone(&self).into_iter() {}
+   |                  ++++++++++++++++++++++++++++++    ~
 
 error: aborting due to 1 previous error
 

tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.fixed

@@ -18,7 +18,7 @@ pub fn hashmap_copy<T, U>(
     map: &HashMap<T, U, Hash128_1>,
 ) where T: Hash + Clone, U: Clone
 {
-    let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map.clone()).into_values().collect(); //~ ERROR
+    let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map).into_values().collect(); //~ ERROR
 }
 
 pub fn make_map() -> HashMap<String, i64, Hash128_1>

tests/ui/moves/suggest-clone-when-some-obligation-is-unmet.stderr

@@ -10,8 +10,8 @@ note: `HashMap::<K, V, S>::into_values` takes ownership of the receiver `self`,
   --> $SRC_DIR/std/src/collections/hash/map.rs:LL:COL
 help: you could `clone` the value and consume it, if the `Hash128_1: Clone` trait bound could be satisfied
    |
-LL |     let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map.clone()).into_values().collect();
-   |                            ++++++++++++++++++++++++++++++++++++++++++++           +
+LL |     let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map).into_values().collect();
+   |                            ++++++++++++++++++++++++++++++++++++++++++++   ~
 help: consider annotating `Hash128_1` with `#[derive(Clone)]`
    |
 LL + #[derive(Clone)]