Skip to content

Commit 7714e67

Browse files
committed
Chore: starting to fix unnecessary_iter_cloned
1 parent 0f9cc8d commit 7714e67

6 files changed

+250
-39
lines changed

clippy_lints/src/methods/unnecessary_iter_cloned.rs

+26-2
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,35 @@ pub fn check_for_loop_iter(
122122
} else {
123123
Applicability::MachineApplicable
124124
};
125-
diag.span_suggestion(expr.span, "use", snippet.to_owned(), applicability);
125+
126+
let combined_suggestions: Vec<_> = references_to_binding
127+
.iter()
128+
.filter_map(|(span, binding)| {
129+
// Determine if removing & causes a type mismatch
130+
let expected_ty = cx.typeck_results().expr_ty(expr);
131+
let actual_ty = cx.typeck_results().expr_ty_adjusted(expr);
132+
133+
// Only suggest removing the reference if types match
134+
if expected_ty != actual_ty {
135+
// If removing & is invalid, suggest keeping it and removing `cloned`
136+
println!("{:?}", span);
137+
Some((*span, format!("&{}", binding)))
138+
} else {
139+
// Otherwise, suggest removing cloned directly
140+
println!("Poop");
141+
Some((*span, binding.to_owned()))
142+
}
143+
})
144+
.chain(std::iter::once((expr.span, snippet.to_owned())))
145+
.collect();
146+
147+
148+
// diag.span_suggestion(expr.span, "use", snippet.to_owned(), applicability);
149+
126150
if !references_to_binding.is_empty() {
127151
diag.multipart_suggestion(
128152
"remove any references to the binding",
129-
references_to_binding,
153+
combined_suggestions,
130154
applicability,
131155
);
132156
}
+201
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
#![allow(unused_assignments)]
2+
#![warn(clippy::unnecessary_to_owned)]
3+
4+
#[allow(dead_code)]
5+
#[derive(Clone, Copy)]
6+
enum FileType {
7+
Account,
8+
PrivateKey,
9+
Certificate,
10+
}
11+
12+
fn main() {
13+
let path = std::path::Path::new("x");
14+
15+
let _ = check_files(&[(FileType::Account, path)]);
16+
let _ = check_files_vec(vec![(FileType::Account, path)]);
17+
18+
// negative tests
19+
let _ = check_files_ref(&[(FileType::Account, path)]);
20+
let _ = check_files_mut(&[(FileType::Account, path)]);
21+
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
22+
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
23+
let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);
24+
25+
check_mut_iteratee_and_modify_inner_variable();
26+
}
27+
28+
// `check_files` and its variants are based on:
29+
// https://github.com/breard-r/acmed/blob/1f0dcc32aadbc5e52de6d23b9703554c0f925113/acmed/src/storage.rs#L262
30+
fn check_files(files: &[(FileType, &std::path::Path)]) -> bool {
31+
for (t, path) in files {
32+
let other = match get_file_path(t) {
33+
Ok(p) => p,
34+
Err(_) => {
35+
return false;
36+
},
37+
};
38+
if !path.is_file() || !other.is_file() {
39+
return false;
40+
}
41+
}
42+
true
43+
}
44+
45+
fn check_files_vec(files: Vec<(FileType, &std::path::Path)>) -> bool {
46+
for (t, path) in files.iter() {
47+
let other = match get_file_path(t) {
48+
Ok(p) => p,
49+
Err(_) => {
50+
return false;
51+
},
52+
};
53+
if !path.is_file() || !other.is_file() {
54+
return false;
55+
}
56+
}
57+
true
58+
}
59+
60+
fn check_files_ref(files: &[(FileType, &std::path::Path)]) -> bool {
61+
for (ref t, path) in files.iter().copied() {
62+
let other = match get_file_path(t) {
63+
Ok(p) => p,
64+
Err(_) => {
65+
return false;
66+
},
67+
};
68+
if !path.is_file() || !other.is_file() {
69+
return false;
70+
}
71+
}
72+
true
73+
}
74+
75+
#[allow(unused_assignments)]
76+
fn check_files_mut(files: &[(FileType, &std::path::Path)]) -> bool {
77+
for (mut t, path) in files.iter().copied() {
78+
t = FileType::PrivateKey;
79+
let other = match get_file_path(&t) {
80+
Ok(p) => p,
81+
Err(_) => {
82+
return false;
83+
},
84+
};
85+
if !path.is_file() || !other.is_file() {
86+
return false;
87+
}
88+
}
89+
true
90+
}
91+
92+
fn check_files_ref_mut(files: &[(FileType, &std::path::Path)]) -> bool {
93+
for (ref mut t, path) in files.iter().copied() {
94+
*t = FileType::PrivateKey;
95+
let other = match get_file_path(t) {
96+
Ok(p) => p,
97+
Err(_) => {
98+
return false;
99+
},
100+
};
101+
if !path.is_file() || !other.is_file() {
102+
return false;
103+
}
104+
}
105+
true
106+
}
107+
108+
fn check_files_self_and_arg(files: &[(FileType, &std::path::Path)]) -> bool {
109+
for (t, path) in files.iter().copied() {
110+
let other = match get_file_path(&t) {
111+
Ok(p) => p,
112+
Err(_) => {
113+
return false;
114+
},
115+
};
116+
if !path.join(path).is_file() || !other.is_file() {
117+
return false;
118+
}
119+
}
120+
true
121+
}
122+
123+
#[allow(unused_assignments)]
124+
fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool {
125+
for (mut t, path) in files.iter().cloned() {
126+
t = FileType::PrivateKey;
127+
let other = match get_file_path(&t) {
128+
Ok(p) => p,
129+
Err(_) => {
130+
return false;
131+
},
132+
};
133+
if !path.is_file() || !other.is_file() {
134+
return false;
135+
}
136+
}
137+
true
138+
}
139+
140+
fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
141+
Ok(std::path::PathBuf::new())
142+
}
143+
144+
// Issue 12098
145+
// https://github.com/rust-lang/rust-clippy/issues/12098
146+
// no message emits
147+
fn check_mut_iteratee_and_modify_inner_variable() {
148+
struct Test {
149+
list: Vec<String>,
150+
mut_this: bool,
151+
}
152+
153+
impl Test {
154+
fn list(&self) -> &[String] {
155+
&self.list
156+
}
157+
}
158+
159+
let mut test = Test {
160+
list: vec![String::from("foo"), String::from("bar")],
161+
mut_this: false,
162+
};
163+
164+
for _item in test.list().to_vec() {
165+
println!("{}", _item);
166+
167+
test.mut_this = true;
168+
{
169+
test.mut_this = true;
170+
}
171+
}
172+
}
173+
174+
mod issue_12821 {
175+
fn foo() {
176+
let v: Vec<_> = "hello".chars().collect();
177+
for c in v.iter().cloned() {
178+
//~^ ERROR: unnecessary use of `cloned`
179+
println!("{c}"); // should not suggest to remove `&`
180+
}
181+
}
182+
183+
fn bar() {
184+
let v: Vec<_> = "hello".chars().collect();
185+
for c in v.iter() {
186+
//~^ ERROR: unnecessary use of `cloned`
187+
let ref_c = c; //~ HELP: remove any references to the binding
188+
println!("{ref_c}");
189+
}
190+
}
191+
192+
fn baz() {
193+
let v: Vec<_> = "hello".chars().enumerate().collect();
194+
for (i, c) in v.iter() {
195+
//~^ ERROR: unnecessary use of `cloned`
196+
let ref_c = c; //~ HELP: remove any references to the binding
197+
let ref_i = i;
198+
println!("{i} {ref_c}"); // should not suggest to remove `&` from `i`
199+
}
200+
}
201+
}

tests/ui/unnecessary_iter_cloned.rs

-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
#![allow(unused_assignments)]
22
#![warn(clippy::unnecessary_to_owned)]
33

4-
//@no-rustfix: need to change the suggestion to a multipart suggestion
5-
64
#[allow(dead_code)]
75
#[derive(Clone, Copy)]
86
enum FileType {

tests/ui/unnecessary_iter_cloned.stderr

+15-28
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,58 @@
11
error: unnecessary use of `copied`
2-
--> tests/ui/unnecessary_iter_cloned.rs:33:22
2+
--> tests/ui/unnecessary_iter_cloned.rs:31:22
33
|
44
LL | for (t, path) in files.iter().copied() {
55
| ^^^^^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::unnecessary-to-owned` implied by `-D warnings`
88
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_to_owned)]`
9-
help: use
10-
|
11-
LL | for (t, path) in files {
12-
| ~~~~~
139
help: remove any references to the binding
1410
|
15-
LL - let other = match get_file_path(&t) {
16-
LL + let other = match get_file_path(t) {
11+
LL ~ for (t, path) in files {
12+
LL ~ let other = match get_file_path(t) {
1713
|
1814

1915
error: unnecessary use of `copied`
20-
--> tests/ui/unnecessary_iter_cloned.rs:48:22
16+
--> tests/ui/unnecessary_iter_cloned.rs:46:22
2117
|
2218
LL | for (t, path) in files.iter().copied() {
2319
| ^^^^^^^^^^^^^^^^^^^^^
2420
|
25-
help: use
26-
|
27-
LL | for (t, path) in files.iter() {
28-
| ~~~~~~~~~~~~
2921
help: remove any references to the binding
3022
|
31-
LL - let other = match get_file_path(&t) {
32-
LL + let other = match get_file_path(t) {
23+
LL ~ for (t, path) in files.iter() {
24+
LL ~ let other = match get_file_path(t) {
3325
|
3426

3527
error: unnecessary use of `cloned`
36-
--> tests/ui/unnecessary_iter_cloned.rs:179:18
28+
--> tests/ui/unnecessary_iter_cloned.rs:177:18
3729
|
3830
LL | for c in v.iter().cloned() {
39-
| ^^^^^^^^^^^^^^^^^ help: use: `v.iter()`
31+
| ^^^^^^^^^^^^^^^^^
4032

4133
error: unnecessary use of `cloned`
42-
--> tests/ui/unnecessary_iter_cloned.rs:187:18
34+
--> tests/ui/unnecessary_iter_cloned.rs:185:18
4335
|
4436
LL | for c in v.iter().cloned() {
4537
| ^^^^^^^^^^^^^^^^^
4638
|
47-
help: use
48-
|
49-
LL | for c in v.iter() {
50-
| ~~~~~~~~
5139
help: remove any references to the binding
5240
|
53-
LL - let ref_c = &c;
54-
LL + let ref_c = c;
41+
LL ~ for c in v.iter() {
42+
LL |
43+
LL ~ let ref_c = c;
5544
|
5645

5746
error: unnecessary use of `cloned`
58-
--> tests/ui/unnecessary_iter_cloned.rs:196:23
47+
--> tests/ui/unnecessary_iter_cloned.rs:194:23
5948
|
6049
LL | for (i, c) in v.iter().cloned() {
6150
| ^^^^^^^^^^^^^^^^^
6251
|
63-
help: use
64-
|
65-
LL | for (i, c) in v.iter() {
66-
| ~~~~~~~~
6752
help: remove any references to the binding
6853
|
54+
LL ~ for (i, c) in v.iter() {
55+
LL |
6956
LL ~ let ref_c = c;
7057
LL ~ let ref_i = i;
7158
|
+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Poop
2+
Poop
3+
Poop
4+
Poop
5+
Poop

tests/ui/unnecessary_to_owned.stderr

+3-7
Original file line numberDiff line numberDiff line change
@@ -519,14 +519,10 @@ error: unnecessary use of `to_vec`
519519
LL | for t in file_types.to_vec() {
520520
| ^^^^^^^^^^^^^^^^^^^
521521
|
522-
help: use
522+
help: remove unnecessary cloning and any references to the binding
523523
|
524-
LL | for t in file_types {
525-
| ~~~~~~~~~~
526-
help: remove any references to the binding
527-
|
528-
LL - let path = match get_file_path(&t) {
529-
LL + let path = match get_file_path(t) {
524+
LL ~ for t in file_types {
525+
LL ~ let path = match get_file_path(t) {
530526
|
531527

532528
error: unnecessary use of `to_vec`

0 commit comments

Comments
 (0)