Detect calls to .clone() on T: !Clone types on borrowck errors

When encountering a lifetime error on a type that *holds* a type that
doesn't implement `Clone`, explore the item's body for potential calls
to `.clone()` that are only cloning the reference `&T` instead of `T`
because `T: !Clone`. If we find this, suggest `T: Clone`.

```
error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
  --> $DIR/clone-on-ref.rs:7:5
   |
LL |     for v in list.iter() {
   |              ---- immutable borrow occurs here
LL |         cloned_items.push(v.clone())
   |                             ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
LL |     }
LL |     list.push(T::default());
   |     ^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
LL |
LL |     drop(cloned_items);
   |          ------------ immutable borrow later used here
   |
help: consider further restricting this bound
   |
LL | fn foo<T: Default + Clone>(list: &mut Vec<T>) {
   |                   +++++++
```
```
error[E0505]: cannot move out of `x` because it is borrowed
  --> $DIR/clone-on-ref.rs:23:10
   |
LL | fn qux(x: A) {
   |        - binding `x` declared here
LL |     let a = &x;
   |             -- borrow of `x` occurs here
LL |     let b = a.clone();
   |               ------- this call doesn't do anything, the result is still `&A` because `A` doesn't implement `Clone`
LL |     drop(x);
   |          ^ move out of `x` occurs here
LL |
LL |     println!("{b:?}");
   |               ----- borrow later used here
   |
help: consider annotating `A` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct A;
   |
```

Author: estebank

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -12,7 +12,6 @@ use rustc_hir::def::{DefKind, Res};
 use rustc_hir::intravisit::{walk_block, walk_expr, Visitor};
 use rustc_hir::{CoroutineDesugaring, PatField};
 use rustc_hir::{CoroutineKind, CoroutineSource, LangItem};
-use rustc_infer::traits::ObligationCause;
 use rustc_middle::hir::nested_filter::OnlyBodies;
 use rustc_middle::mir::tcx::PlaceTy;
 use rustc_middle::mir::{
@@ -21,16 +20,21 @@ use rustc_middle::mir::{
     PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
     VarBindingForm,
 };
-use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty, TyCtxt};
+use rustc_middle::ty::{
+    self, suggest_constraining_type_params, PredicateKind, ToPredicate, Ty, TyCtxt,
+    TypeSuperVisitable, TypeVisitor,
+};
 use rustc_middle::util::CallKind;
 use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
+use rustc_span::def_id::DefId;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::hygiene::DesugaringKind;
 use rustc_span::symbol::{kw, sym, Ident};
 use rustc_span::{BytePos, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
+use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt;
 use rustc_trait_selection::traits::error_reporting::FindExprBySpan;
-use rustc_trait_selection::traits::ObligationCtxt;
+use rustc_trait_selection::traits::{Obligation, ObligationCause, ObligationCtxt};
 use std::iter;
 
 use crate::borrow_set::TwoPhaseActivation;
@@ -283,7 +287,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 // something that already has `Fn`-like bounds (or is a closure), so we can't
                 // restrict anyways.
             } else {
-                self.suggest_adding_copy_bounds(&mut err, ty, span);
+                let copy_did = self.infcx.tcx.require_lang_item(LangItem::Copy, Some(span));
+                self.suggest_adding_bounds(&mut err, ty, copy_did, span);
             }
 
             if needs_note {
@@ -775,7 +780,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
-    fn suggest_adding_copy_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, span: Span) {
+    fn suggest_adding_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, def_id: DefId, span: Span) {
         let tcx = self.infcx.tcx;
         let generics = tcx.generics_of(self.mir_def_id());
 
@@ -788,10 +793,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         };
         // Try to find predicates on *generic params* that would allow copying `ty`
         let ocx = ObligationCtxt::new(self.infcx);
-        let copy_did = tcx.require_lang_item(LangItem::Copy, Some(span));
         let cause = ObligationCause::misc(span, self.mir_def_id());
 
-        ocx.register_bound(cause, self.param_env, ty, copy_did);
+        ocx.register_bound(cause, self.param_env, ty, def_id);
         let errors = ocx.select_all_or_error();
 
         // Only emit suggestion if all required predicates are on generic
@@ -877,6 +881,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 Some(borrow_span),
                 None,
             );
+        self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
         self.buffer_error(err);
     }
 
@@ -1215,10 +1220,104 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         );
 
         self.suggest_using_local_if_applicable(&mut err, location, issued_borrow, explanation);
+        self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
 
         err
     }
 
+    fn suggest_copy_for_type_in_cloned_ref(&self, err: &mut Diag<'tcx>, place: Place<'tcx>) {
+        let tcx = self.infcx.tcx;
+        let hir = tcx.hir();
+        let Some(body_id) = tcx.hir_node(self.mir_hir_id()).body_id() else { return };
+        struct FindUselessClone<'hir> {
+            pub clones: Vec<&'hir hir::Expr<'hir>>,
+        }
+        impl<'hir> FindUselessClone<'hir> {
+            pub fn new() -> Self {
+                Self { clones: vec![] }
+            }
+        }
+
+        impl<'v> Visitor<'v> for FindUselessClone<'v> {
+            fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
+                // FIXME: use `lookup_method_for_diagnostic`?
+                if let hir::ExprKind::MethodCall(segment, _rcvr, args, _span) = ex.kind
+                    && segment.ident.name == sym::clone
+                    && args.len() == 0
+                {
+                    self.clones.push(ex);
+                }
+                hir::intravisit::walk_expr(self, ex);
+            }
+        }
+        let mut expr_finder = FindUselessClone::new();
+
+        let body = hir.body(body_id).value;
+        expr_finder.visit_expr(body);
+
+        pub struct Holds<'tcx> {
+            ty: Ty<'tcx>,
+            holds: bool,
+        }
+
+        impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for Holds<'tcx> {
+            type Result = std::ops::ControlFlow<()>;
+
+            fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
+                if t == self.ty {
+                    self.holds = true;
+                }
+                t.super_visit_with(self)
+            }
+        }
+
+        let mut types_to_constrain = FxIndexSet::default();
+
+        let local_ty = self.body.local_decls[place.local].ty;
+        let typeck_results = tcx.typeck(self.mir_def_id());
+        let clone = tcx.require_lang_item(LangItem::Clone, Some(body.span));
+        for expr in expr_finder.clones {
+            if let hir::ExprKind::MethodCall(_, rcvr, _, span) = expr.kind
+                && let Some(rcvr_ty) = typeck_results.node_type_opt(rcvr.hir_id)
+                && let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
+                && rcvr_ty == ty
+                && let ty::Ref(_, inner, _) = rcvr_ty.kind()
+                && let inner = inner.peel_refs()
+                && let mut v = (Holds { ty: inner, holds: false })
+                && let _ = v.visit_ty(local_ty)
+                && v.holds
+                && let None = self.infcx.type_implements_trait_shallow(clone, inner, self.param_env)
+            {
+                err.span_label(
+                    span,
+                    format!(
+                        "this call doesn't do anything, the result is still `{rcvr_ty}` \
+                             because `{inner}` doesn't implement `Clone`",
+                    ),
+                );
+                types_to_constrain.insert(inner);
+            }
+        }
+        for ty in types_to_constrain {
+            self.suggest_adding_bounds(err, ty, clone, body.span);
+            if let ty::Adt(..) = ty.kind() {
+                // The type doesn't implement Clone.
+                let trait_ref = ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, clone, [ty]));
+                let obligation = Obligation::new(
+                    self.infcx.tcx,
+                    ObligationCause::dummy(),
+                    self.param_env,
+                    trait_ref,
+                );
+                self.infcx.err_ctxt().suggest_derive(
+                    &obligation,
+                    err,
+                    trait_ref.to_predicate(self.infcx.tcx),
+                );
+            }
+        }
+    }
+
     #[instrument(level = "debug", skip(self, err))]
     fn suggest_using_local_if_applicable(
         &self,

tests/ui/borrowck/clone-on-ref.fixed

@@ -0,0 +1,32 @@
+//@ run-rustfix
+fn foo<T: Default + Clone>(list: &mut Vec<T>) {
+    let mut cloned_items = Vec::new();
+    for v in list.iter() {
+        cloned_items.push(v.clone())
+    }
+    list.push(T::default());
+    //~^ ERROR cannot borrow `*list` as mutable because it is also borrowed as immutable
+    drop(cloned_items);
+}
+fn bar<T: std::fmt::Display + Clone>(x: T) {
+    let a = &x;
+    let b = a.clone();
+    drop(x);
+    //~^ ERROR cannot move out of `x` because it is borrowed
+    println!("{b}");
+}
+#[derive(Debug)]
+#[derive(Clone)]
+struct A;
+fn qux(x: A) {
+    let a = &x;
+    let b = a.clone();
+    drop(x);
+    //~^ ERROR cannot move out of `x` because it is borrowed
+    println!("{b:?}");
+}
+fn main() {
+    foo(&mut vec![1, 2, 3]);
+    bar("");
+    qux(A);
+}

tests/ui/borrowck/clone-on-ref.rs

@@ -0,0 +1,31 @@
+//@ run-rustfix
+fn foo<T: Default>(list: &mut Vec<T>) {
+    let mut cloned_items = Vec::new();
+    for v in list.iter() {
+        cloned_items.push(v.clone())
+    }
+    list.push(T::default());
+    //~^ ERROR cannot borrow `*list` as mutable because it is also borrowed as immutable
+    drop(cloned_items);
+}
+fn bar<T: std::fmt::Display>(x: T) {
+    let a = &x;
+    let b = a.clone();
+    drop(x);
+    //~^ ERROR cannot move out of `x` because it is borrowed
+    println!("{b}");
+}
+#[derive(Debug)]
+struct A;
+fn qux(x: A) {
+    let a = &x;
+    let b = a.clone();
+    drop(x);
+    //~^ ERROR cannot move out of `x` because it is borrowed
+    println!("{b:?}");
+}
+fn main() {
+    foo(&mut vec![1, 2, 3]);
+    bar("");
+    qux(A);
+}

tests/ui/borrowck/clone-on-ref.stderr

@@ -0,0 +1,64 @@
+error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
+  --> $DIR/clone-on-ref.rs:7:5
+   |
+LL |     for v in list.iter() {
+   |              ---- immutable borrow occurs here
+LL |         cloned_items.push(v.clone())
+   |                             ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
+LL |     }
+LL |     list.push(T::default());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
+LL |
+LL |     drop(cloned_items);
+   |          ------------ immutable borrow later used here
+   |
+help: consider further restricting this bound
+   |
+LL | fn foo<T: Default + Clone>(list: &mut Vec<T>) {
+   |                   +++++++
+
+error[E0505]: cannot move out of `x` because it is borrowed
+  --> $DIR/clone-on-ref.rs:14:10
+   |
+LL | fn bar<T: std::fmt::Display>(x: T) {
+   |                              - binding `x` declared here
+LL |     let a = &x;
+   |             -- borrow of `x` occurs here
+LL |     let b = a.clone();
+   |               ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
+LL |     drop(x);
+   |          ^ move out of `x` occurs here
+LL |
+LL |     println!("{b}");
+   |               --- borrow later used here
+   |
+help: consider further restricting this bound
+   |
+LL | fn bar<T: std::fmt::Display + Clone>(x: T) {
+   |                             +++++++
+
+error[E0505]: cannot move out of `x` because it is borrowed
+  --> $DIR/clone-on-ref.rs:23:10
+   |
+LL | fn qux(x: A) {
+   |        - binding `x` declared here
+LL |     let a = &x;
+   |             -- borrow of `x` occurs here
+LL |     let b = a.clone();
+   |               ------- this call doesn't do anything, the result is still `&A` because `A` doesn't implement `Clone`
+LL |     drop(x);
+   |          ^ move out of `x` occurs here
+LL |
+LL |     println!("{b:?}");
+   |               ----- borrow later used here
+   |
+help: consider annotating `A` with `#[derive(Clone)]`
+   |
+LL + #[derive(Clone)]
+LL | struct A;
+   |
+
+error: aborting due to 3 previous errors
+
+Some errors have detailed explanations: E0502, E0505.
+For more information about an error, try `rustc --explain E0502`.