diagnostics: suggest Clone bounds when noop clone()

Author: notriddle

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -975,29 +975,58 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
         err: &mut Diag<'_>,
         trait_pred: ty::PolyTraitPredicate<'tcx>,
     ) -> bool {
+        let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } =
+            obligation.cause.code()
+        else {
+            return false;
+        };
+        let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None);
         let self_ty = self.resolve_vars_if_possible(trait_pred.self_ty());
         self.enter_forall(self_ty, |ty: Ty<'_>| {
             let Some(generics) = self.tcx.hir().get_generics(obligation.cause.body_id) else {
                 return false;
             };
             let ty::Ref(_, inner_ty, hir::Mutability::Not) = ty.kind() else { return false };
             let ty::Param(param) = inner_ty.kind() else { return false };
-            let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } =
-                obligation.cause.code()
-            else {
-                return false;
-            };
-            let arg_node = self.tcx.hir_node(*arg_hir_id);
-            let Node::Expr(Expr { kind: hir::ExprKind::Path(_), .. }) = arg_node else {
-                return false;
-            };
 
-            let clone_trait = self.tcx.require_lang_item(LangItem::Clone, None);
             let has_clone = |ty| {
                 self.type_implements_trait(clone_trait, [ty], obligation.param_env)
                     .must_apply_modulo_regions()
             };
 
+            let existing_clone_call = match self.tcx.hir_node(*arg_hir_id) {
+                // It's just a variable. Propose cloning it.
+                Node::Expr(Expr { kind: hir::ExprKind::Path(_), .. }) => None,
+                // It's already a call to `clone()`. We might be able to suggest
+                // adding a `+ Clone` bound, though.
+                Node::Expr(Expr {
+                    kind:
+                        hir::ExprKind::MethodCall(
+                            hir::PathSegment { ident, .. },
+                            _receiver,
+                            &[],
+                            call_span,
+                        ),
+                    hir_id,
+                    ..
+                }) if ident.name == sym::clone
+                    && !call_span.from_expansion()
+                    && !has_clone(*inner_ty) =>
+                {
+                    // We only care about method calls corresponding to the real `Clone` trait.
+                    let Some(typeck_results) = self.typeck_results.as_ref() else { return false };
+                    let Some((DefKind::AssocFn, did)) = typeck_results.type_dependent_def(*hir_id)
+                    else {
+                        return false;
+                    };
+                    if self.tcx.trait_of_item(did) != Some(clone_trait) {
+                        return false;
+                    }
+                    Some(ident.span)
+                }
+                _ => return false,
+            };
+
             let new_obligation = self.mk_trait_obligation_with_new_self_ty(
                 obligation.param_env,
                 trait_pred.map_bound(|trait_pred| (trait_pred, *inner_ty)),
@@ -1015,12 +1044,23 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
                         None,
                     );
                 }
-                err.span_suggestion_verbose(
-                    obligation.cause.span.shrink_to_hi(),
-                    "consider using clone here",
-                    ".clone()".to_string(),
-                    Applicability::MaybeIncorrect,
-                );
+                if let Some(existing_clone_call) = existing_clone_call {
+                    err.span_note(
+                        existing_clone_call,
+                        format!(
+                            "because `{inner_ty}` does not implement `Clone`, \
+                            this call to `clone()` copies the reference, \
+                            which does not do anything"
+                        ),
+                    );
+                } else {
+                    err.span_suggestion_verbose(
+                        obligation.cause.span.shrink_to_hi(),
+                        "consider using clone here",
+                        ".clone()".to_string(),
+                        Applicability::MaybeIncorrect,
+                    );
+                }
                 return true;
             }
             false

tests/ui/suggestions/clone-bounds-121524.rs

@@ -0,0 +1,19 @@
+#[derive(Clone)]
+struct ThingThatDoesAThing;
+
+trait DoesAThing {}
+
+impl DoesAThing for ThingThatDoesAThing {}
+
+fn clones_impl_ref_inline(thing: &impl DoesAThing) {
+    //~^ HELP consider further restricting this bound
+    drops_impl_owned(thing.clone()); //~ ERROR E0277
+    //~^ NOTE copies the reference
+    //~| NOTE the trait `DoesAThing` is not implemented for `&impl DoesAThing`
+}
+
+fn drops_impl_owned(_thing: impl DoesAThing) { }
+
+fn main() {
+    clones_impl_ref_inline(&ThingThatDoesAThing);
+}

tests/ui/suggestions/clone-bounds-121524.stderr

@@ -0,0 +1,19 @@
+error[E0277]: the trait bound `&impl DoesAThing: DoesAThing` is not satisfied
+  --> $DIR/clone-bounds-121524.rs:10:22
+   |
+LL |     drops_impl_owned(thing.clone());
+   |                      ^^^^^^^^^^^^^ the trait `DoesAThing` is not implemented for `&impl DoesAThing`
+   |
+note: because `impl DoesAThing` does not implement `Clone`, this call to `clone()` copies the reference, which does not do anything
+  --> $DIR/clone-bounds-121524.rs:10:28
+   |
+LL |     drops_impl_owned(thing.clone());
+   |                            ^^^^^
+help: consider further restricting this bound
+   |
+LL | fn clones_impl_ref_inline(thing: &impl DoesAThing + Clone) {
+   |                                                   +++++++
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0277`.