Skip to content

Commit 88b5d51

Browse files
borsbors
committed
Auto merge of #12129 - GuillaumeGomez:map-clone-copy, r=llogiq
Fix suggestion for `map_clone` lint on types implementing `Copy` Follow-up of #12104. It was missing this check to suggest the correct method. r? `@llogiq` changelog: Fix suggestion for `map_clone` lint on types implementing `Copy`
2 parents c537134 + 74db4b7 commit 88b5d51

File tree

4 files changed

+111
-13
lines changed

4 files changed

+111
-13
lines changed

‎clippy_lints/src/methods/map_clone.rs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,15 @@ fn handle_path(
113113
if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id()
114114
&& match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD)
115115
{
116-
// FIXME: It would be better to infer the type to check if it's copyable or not
117-
// to suggest to use `.copied()` instead of `.cloned()` where applicable.
118-
lint_path(cx, e.span, recv.span);
116+
// The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option`
117+
// and `Result`.
118+
if let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind()
119+
&& let args = args.as_slice()
120+
&& let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type())
121+
&& ty.is_ref()
122+
{
123+
lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
124+
}
119125
}
120126
}
121127

@@ -139,17 +145,19 @@ fn lint_needless_cloning(cx: &LateContext<'_>, root: Span, receiver: Span) {
139145
);
140146
}
141147

142-
fn lint_path(cx: &LateContext<'_>, replace: Span, root: Span) {
148+
fn lint_path(cx: &LateContext<'_>, replace: Span, root: Span, is_copy: bool) {
143149
let mut applicability = Applicability::MachineApplicable;
144150

151+
let replacement = if is_copy { "copied" } else { "cloned" };
152+
145153
span_lint_and_sugg(
146154
cx,
147155
MAP_CLONE,
148156
replace,
149157
"you are explicitly cloning with `.map()`",
150-
"consider calling the dedicated `cloned` method",
158+
&format!("consider calling the dedicated `{replacement}` method"),
151159
format!(
152-
"{}.cloned()",
160+
"{}.{replacement}()",
153161
snippet_with_applicability(cx, root, "..", &mut applicability),
154162
),
155163
applicability,

‎tests/ui/map_clone.fixed‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,15 +69,48 @@ fn main() {
6969
//~^ ERROR: you are explicitly cloning with `.map()`
7070
let y = x.cloned();
7171
//~^ ERROR: you are explicitly cloning with `.map()`
72+
//~| HELP: consider calling the dedicated `cloned` method
7273
let y = x.cloned();
7374
//~^ ERROR: you are explicitly cloning with `.map()`
75+
//~| HELP: consider calling the dedicated `cloned` method
76+
77+
let x: Option<u32> = Some(0);
78+
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
79+
let y = x.copied();
80+
//~^ ERROR: you are explicitly cloning with `.map()`
81+
//~| HELP: consider calling the dedicated `copied` method
82+
let y = x.copied();
83+
//~^ ERROR: you are explicitly cloning with `.map()`
84+
//~| HELP: consider calling the dedicated `copied` method
85+
86+
// Should not suggest `copied` or `cloned` here since `T` is not a reference.
87+
let x: Option<u32> = Some(0);
88+
let y = x.map(|x| u32::clone(&x));
89+
let y = x.map(|x| Clone::clone(&x));
7490

7591
// Testing with `Result` now.
7692
let x: Result<String, ()> = Ok(String::new());
7793
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
7894
let y = x.cloned();
7995
//~^ ERROR: you are explicitly cloning with `.map()`
96+
//~| HELP: consider calling the dedicated `cloned` method
8097
let y = x.cloned();
98+
//~^ ERROR: you are explicitly cloning with `.map()`
99+
//~| HELP: consider calling the dedicated `cloned` method
100+
101+
let x: Result<u32, ()> = Ok(0);
102+
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
103+
let y = x.copied();
104+
//~^ ERROR: you are explicitly cloning with `.map()`
105+
//~| HELP: consider calling the dedicated `copied` method
106+
let y = x.copied();
107+
//~^ ERROR: you are explicitly cloning with `.map()`
108+
//~| HELP: consider calling the dedicated `copied` method
109+
110+
// Should not suggest `copied` or `cloned` here since `T` is not a reference.
111+
let x: Result<u32, ()> = Ok(0);
112+
let y = x.map(|x| u32::clone(&x));
113+
let y = x.map(|x| Clone::clone(&x));
81114

82115
// We ensure that no warning is emitted here because `useless_asref` is taking over.
83116
let x = Some(String::new());

‎tests/ui/map_clone.rs‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,15 +69,48 @@ fn main() {
6969
//~^ ERROR: you are explicitly cloning with `.map()`
7070
let y = x.map(Clone::clone);
7171
//~^ ERROR: you are explicitly cloning with `.map()`
72+
//~| HELP: consider calling the dedicated `cloned` method
7273
let y = x.map(String::clone);
7374
//~^ ERROR: you are explicitly cloning with `.map()`
75+
//~| HELP: consider calling the dedicated `cloned` method
76+
77+
let x: Option<u32> = Some(0);
78+
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
79+
let y = x.map(|x| u32::clone(x));
80+
//~^ ERROR: you are explicitly cloning with `.map()`
81+
//~| HELP: consider calling the dedicated `copied` method
82+
let y = x.map(|x| Clone::clone(x));
83+
//~^ ERROR: you are explicitly cloning with `.map()`
84+
//~| HELP: consider calling the dedicated `copied` method
85+
86+
// Should not suggest `copied` or `cloned` here since `T` is not a reference.
87+
let x: Option<u32> = Some(0);
88+
let y = x.map(|x| u32::clone(&x));
89+
let y = x.map(|x| Clone::clone(&x));
7490

7591
// Testing with `Result` now.
7692
let x: Result<String, ()> = Ok(String::new());
7793
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
7894
let y = x.map(|x| String::clone(x));
7995
//~^ ERROR: you are explicitly cloning with `.map()`
80-
let y = x.map(|x| String::clone(x));
96+
//~| HELP: consider calling the dedicated `cloned` method
97+
let y = x.map(|x| Clone::clone(x));
98+
//~^ ERROR: you are explicitly cloning with `.map()`
99+
//~| HELP: consider calling the dedicated `cloned` method
100+
101+
let x: Result<u32, ()> = Ok(0);
102+
let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint.
103+
let y = x.map(|x| u32::clone(x));
104+
//~^ ERROR: you are explicitly cloning with `.map()`
105+
//~| HELP: consider calling the dedicated `copied` method
106+
let y = x.map(|x| Clone::clone(x));
107+
//~^ ERROR: you are explicitly cloning with `.map()`
108+
//~| HELP: consider calling the dedicated `copied` method
109+
110+
// Should not suggest `copied` or `cloned` here since `T` is not a reference.
111+
let x: Result<u32, ()> = Ok(0);
112+
let y = x.map(|x| u32::clone(&x));
113+
let y = x.map(|x| Clone::clone(&x));
81114

82115
// We ensure that no warning is emitted here because `useless_asref` is taking over.
83116
let x = Some(String::new());

‎tests/ui/map_clone.stderr‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,22 +50,46 @@ LL | let y = x.map(Clone::clone);
5050
| ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
5151

5252
error: you are explicitly cloning with `.map()`
53-
--> $DIR/map_clone.rs:72:13
53+
--> $DIR/map_clone.rs:73:13
5454
|
5555
LL | let y = x.map(String::clone);
5656
| ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
5757

5858
error: you are explicitly cloning with `.map()`
59-
--> $DIR/map_clone.rs:78:13
59+
--> $DIR/map_clone.rs:79:13
6060
|
61-
LL | let y = x.map(|x| String::clone(x));
62-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
61+
LL | let y = x.map(|x| u32::clone(x));
62+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
6363

6464
error: you are explicitly cloning with `.map()`
65-
--> $DIR/map_clone.rs:80:13
65+
--> $DIR/map_clone.rs:82:13
66+
|
67+
LL | let y = x.map(|x| Clone::clone(x));
68+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
69+
70+
error: you are explicitly cloning with `.map()`
71+
--> $DIR/map_clone.rs:94:13
6672
|
6773
LL | let y = x.map(|x| String::clone(x));
6874
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
6975

70-
error: aborting due to 11 previous errors
76+
error: you are explicitly cloning with `.map()`
77+
--> $DIR/map_clone.rs:97:13
78+
|
79+
LL | let y = x.map(|x| Clone::clone(x));
80+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()`
81+
82+
error: you are explicitly cloning with `.map()`
83+
--> $DIR/map_clone.rs:103:13
84+
|
85+
LL | let y = x.map(|x| u32::clone(x));
86+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
87+
88+
error: you are explicitly cloning with `.map()`
89+
--> $DIR/map_clone.rs:106:13
90+
|
91+
LL | let y = x.map(|x| Clone::clone(x));
92+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
93+
94+
error: aborting due to 15 previous errors
7195

0 commit comments

Comments
 (0)