From c2cc90402b6a896c3273a57e506d6c02a1ec6038 Mon Sep 17 00:00:00 2001
From: Michael Howell <michael@notriddle.com>
Date: Thu, 7 Mar 2024 20:01:28 -0700
Subject: [PATCH] diagnostics: suggest `Clone` bounds when noop `clone()`

---
 .../src/traits/error_reporting/suggestions.rs | 60 +++++++++++++++----
 tests/ui/suggestions/clone-bounds-121524.rs   | 19 ++++++
 .../ui/suggestions/clone-bounds-121524.stderr | 19 ++++++
 3 files changed, 88 insertions(+), 10 deletions(-)
 create mode 100644 tests/ui/suggestions/clone-bounds-121524.rs
 create mode 100644 tests/ui/suggestions/clone-bounds-121524.stderr

diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
index 1241227a5af..0dc59a4578a 100644
--- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
+++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
@@ -987,10 +987,6 @@ fn suggest_add_clone_to_arg(
             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| {
@@ -998,6 +994,39 @@ fn suggest_add_clone_to_arg(
                     .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 @@ fn suggest_add_clone_to_arg(
                         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!(
+                            "this `clone()` copies the reference, \
+                            which does not do anything, \
+                            because `{inner_ty}` does not implement `Clone`"
+                        ),
+                    );
+                } else {
+                    err.span_suggestion_verbose(
+                        obligation.cause.span.shrink_to_hi(),
+                        "consider using clone here",
+                        ".clone()".to_string(),
+                        Applicability::MaybeIncorrect,
+                    );
+                }
                 return true;
             }
             false
diff --git a/tests/ui/suggestions/clone-bounds-121524.rs b/tests/ui/suggestions/clone-bounds-121524.rs
new file mode 100644
index 00000000000..8cd60b452de
--- /dev/null
+++ b/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);
+}
diff --git a/tests/ui/suggestions/clone-bounds-121524.stderr b/tests/ui/suggestions/clone-bounds-121524.stderr
new file mode 100644
index 00000000000..6d60508a4a1
--- /dev/null
+++ b/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: this `clone()` copies the reference, which does not do anything, because `impl DoesAThing` does not implement `Clone`
+  --> $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`.