Auto merge of #12129 - GuillaumeGomez:map-clone-copy, r=llogiq

Fix suggestion for `map_clone` lint on types implementing `Copy`

Follow-up of https://github.com/rust-lang/rust-clippy/pull/12104.

It was missing this check to suggest the correct method.

r? `@llogiq`

changelog: Fix suggestion for `map_clone` lint on types implementing `Copy`

4 files changed, 111 insertions, 13 deletions

diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs
index f9f636bbbf7..27e17b43b01 100644
--- a/clippy_lints/src/methods/map_clone.rs
+++ b/clippy_lints/src/methods/map_clone.rs
@@ -113,9 +113,15 @@ fn handle_path(
     if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id()
         && match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD)
     {
-        // FIXME: It would be better to infer the type to check if it's copyable or not
-        // to suggest to use `.copied()` instead of `.cloned()` where applicable.
-        lint_path(cx, e.span, recv.span);
+        // The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option`
+        // and `Result`.
+        if let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind()
+            && let args = args.as_slice()
+            && let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type())
+            && ty.is_ref()
+        {
+            lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
+        }
     }
 }
 
@@ -139,17 +145,19 @@ fn lint_needless_cloning(cx: &LateContext<'_>, root: Span, receiver: Span) {
     );
 }
 
-fn lint_path(cx: &LateContext<'_>, replace: Span, root: Span) {
+fn lint_path(cx: &LateContext<'_>, replace: Span, root: Span, is_copy: bool) {
     let mut applicability = Applicability::MachineApplicable;
 
+    let replacement = if is_copy { "copied" } else { "cloned" };
+
     span_lint_and_sugg(
         cx,
         MAP_CLONE,
         replace,
         "you are explicitly cloning with `.map()`",
-        "consider calling the dedicated `cloned` method",
+        &format!("consider calling the dedicated `{replacement}` method"),
         format!(
-            "{}.cloned()",
+            "{}.{replacement}()",
             snippet_with_applicability(cx, root, "..", &mut applicability),
         ),
         applicability,
diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed
index 08b155a1aea..395eea69294 100644
--- a/tests/ui/map_clone.fixed
+++ b/tests/ui/map_clone.fixed
@@ -69,15 +69,48 @@ fn main() {
     //~^ ERROR: you are explicitly cloning with `.map()`
     let y = x.cloned();
     //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
     let y = x.cloned();
     //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
+
+    let x: Option<u32> = Some(0);
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.copied();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+    let y = x.copied();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+
+    // Should not suggest `copied` or `cloned` here since `T` is not a reference.
+    let x: Option<u32> = Some(0);
+    let y = x.map(|x| u32::clone(&x));
+    let y = x.map(|x| Clone::clone(&x));
 
     // Testing with `Result` now.
     let x: Result<String, ()> = Ok(String::new());
     let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
     let y = x.cloned();
     //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
     let y = x.cloned();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
+
+    let x: Result<u32, ()> = Ok(0);
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.copied();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+    let y = x.copied();
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+
+    // Should not suggest `copied` or `cloned` here since `T` is not a reference.
+    let x: Result<u32, ()> = Ok(0);
+    let y = x.map(|x| u32::clone(&x));
+    let y = x.map(|x| Clone::clone(&x));
 
     // We ensure that no warning is emitted here because `useless_asref` is taking over.
     let x = Some(String::new());
diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs
index 901d9b278b4..82a103edf5a 100644
--- a/tests/ui/map_clone.rs
+++ b/tests/ui/map_clone.rs
@@ -69,15 +69,48 @@ fn main() {
     //~^ ERROR: you are explicitly cloning with `.map()`
     let y = x.map(Clone::clone);
     //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
     let y = x.map(String::clone);
     //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
+
+    let x: Option<u32> = Some(0);
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.map(|x| u32::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+    let y = x.map(|x| Clone::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+
+    // Should not suggest `copied` or `cloned` here since `T` is not a reference.
+    let x: Option<u32> = Some(0);
+    let y = x.map(|x| u32::clone(&x));
+    let y = x.map(|x| Clone::clone(&x));
 
     // Testing with `Result` now.
     let x: Result<String, ()> = Ok(String::new());
     let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
     let y = x.map(|x| String::clone(x));
     //~^ ERROR: you are explicitly cloning with `.map()`
-    let y = x.map(|x| String::clone(x));
+    //~| HELP: consider calling the dedicated `cloned` method
+    let y = x.map(|x| Clone::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `cloned` method
+
+    let x: Result<u32, ()> = Ok(0);
+    let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
+    let y = x.map(|x| u32::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+    let y = x.map(|x| Clone::clone(x));
+    //~^ ERROR: you are explicitly cloning with `.map()`
+    //~| HELP: consider calling the dedicated `copied` method
+
+    // Should not suggest `copied` or `cloned` here since `T` is not a reference.
+    let x: Result<u32, ()> = Ok(0);
+    let y = x.map(|x| u32::clone(&x));
+    let y = x.map(|x| Clone::clone(&x));
 
     // We ensure that no warning is emitted here because `useless_asref` is taking over.
     let x = Some(String::new());
diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr
index 9d7e9317b58..2c86a67fab8 100644
--- a/tests/ui/map_clone.stderr
+++ b/tests/ui/map_clone.stderr
@@ -50,22 +50,46 @@ LL |     let y = x.map(Clone::clone);
    |             ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
 
 error: you are explicitly cloning with `.map()`
-  --> $DIR/map_clone.rs:72:13
+  --> $DIR/map_clone.rs:73:13
    |
 LL |     let y = x.map(String::clone);
    |             ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
 
 error: you are explicitly cloning with `.map()`
-  --> $DIR/map_clone.rs:78:13
+  --> $DIR/map_clone.rs:79:13
    |
-LL |     let y = x.map(|x| String::clone(x));
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+LL |     let y = x.map(|x| u32::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
 
 error: you are explicitly cloning with `.map()`
-  --> $DIR/map_clone.rs:80:13
+  --> $DIR/map_clone.rs:82:13
+   |
+LL |     let y = x.map(|x| Clone::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:94:13
    |
 LL |     let y = x.map(|x| String::clone(x));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
 
-error: aborting due to 11 previous errors
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:97:13
+   |
+LL |     let y = x.map(|x| Clone::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:103:13
+   |
+LL |     let y = x.map(|x| u32::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
+
+error: you are explicitly cloning with `.map()`
+  --> $DIR/map_clone.rs:106:13
+   |
+LL |     let y = x.map(|x| Clone::clone(x));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
+
+error: aborting due to 15 previous errors