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 = 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 = 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 = 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 = 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 = 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 = 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 = 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 = 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 = 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 = 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| u32::clone(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` + +error: you are explicitly cloning with `.map()` + --> $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: you are explicitly cloning with `.map()` - --> $DIR/map_clone.rs:80:13 + --> $DIR/map_clone.rs:97: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| Clone::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: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