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

compiler/rustc_borrowck/messages.ftl

@@ -16,6 +16,9 @@ borrowck_borrow_due_to_use_closure =
 borrowck_borrow_due_to_use_coroutine =
     borrow occurs due to use in coroutine
 
+borrowck_calling_operator_moves =
+    calling this operator moves the value
+
 borrowck_calling_operator_moves_lhs =
     calling this operator moves the left-hand side
 

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,

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -1050,7 +1050,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     );
                     err.subdiagnostic(self.dcx(), CaptureReasonNote::FnOnceMoveInCall { var_span });
                 }
-                CallKind::Operator { self_arg, .. } => {
+                CallKind::Operator { self_arg, trait_id, .. } => {
                     let self_arg = self_arg.unwrap();
                     err.subdiagnostic(
                         self.dcx(),
@@ -1062,9 +1062,16 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         },
                     );
                     if self.fn_self_span_reported.insert(fn_span) {
+                        let lang = self.infcx.tcx.lang_items();
                         err.subdiagnostic(
                             self.dcx(),
-                            CaptureReasonNote::LhsMoveByOperator { span: self_arg.span },
+                            if [lang.not_trait(), lang.deref_trait(), lang.neg_trait()]
+                                .contains(&Some(trait_id))
+                            {
+                                CaptureReasonNote::UnOpMoveByOperator { span: self_arg.span }
+                            } else {
+                                CaptureReasonNote::LhsMoveByOperator { span: self_arg.span }
+                            },
                         );
                     }
                 }
@@ -1226,20 +1233,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             {
                                 let msg = match &errors[..] {
                                     [] => "you can `clone` the value and consume it, but this \
-                                            might not be your desired behavior"
+                                           might not be your desired behavior"
                                         .to_string(),
                                     [error] => {
                                         format!(
-                                            "you could `clone` the value and consume it, if \
-                                                the `{}` trait bound could be satisfied",
+                                            "you could `clone` the value and consume it, if the \
+                                             `{}` trait bound could be satisfied",
                                             error.obligation.predicate,
                                         )
                                     }
                                     [errors @ .., last] => {
                                         format!(
-                                            "you could `clone` the value and consume it, if \
-                                                the following trait bounds could be satisfied: {} \
-                                                and `{}`",
+                                            "you could `clone` the value and consume it, if the \
+                                             following trait bounds could be satisfied: \
+                                             {} and `{}`",
                                             errors
                                                 .iter()
                                                 .map(|e| format!("`{}`", e.obligation.predicate))

compiler/rustc_borrowck/src/session_diagnostics.rs

@@ -368,6 +368,11 @@ pub(crate) enum CaptureReasonNote {
         #[primary_span]
         var_span: Span,
     },
+    #[note(borrowck_calling_operator_moves)]
+    UnOpMoveByOperator {
+        #[primary_span]
+        span: Span,
+    },
     #[note(borrowck_calling_operator_moves_lhs)]
     LhsMoveByOperator {
         #[primary_span]

compiler/rustc_mir_build/messages.ftl

@@ -24,7 +24,7 @@ mir_build_borrow_of_layout_constrained_field_requires_unsafe_unsafe_op_in_unsafe
 
 mir_build_borrow_of_moved_value = borrow of moved value
     .label = value moved into `{$name}` here
-    .occurs_because_label = move occurs because `{$name}` has type `{$ty}` which does not implement the `Copy` trait
+    .occurs_because_label = move occurs because `{$name}` has type `{$ty}`, which does not implement the `Copy` trait
     .value_borrowed_label = value borrowed here after move
     .suggestion = borrow this binding in the pattern to avoid moving the value
 

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`.