Skip to content

Commit 9f4941a

Browse files
authored
chore: starting to fix unnecessary_iter_cloned (#13848)
This will address #13099 for the unnecessary_iter_cloned test. changelog: [unnecessary_iter_cloned]: unnecessary_iter_cloned manual_assert to use multipart_suggestions where appropriate
2 parents 8ea395e + 8fe39b2 commit 9f4941a

5 files changed

+226
-44
lines changed

clippy_lints/src/methods/unnecessary_iter_cloned.rs

+8-8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
66
use clippy_utils::visitors::for_each_expr_without_closures;
77
use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local};
88
use core::ops::ControlFlow;
9+
use itertools::Itertools;
910
use rustc_errors::Applicability;
1011
use rustc_hir::def_id::DefId;
1112
use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind};
@@ -122,14 +123,13 @@ pub fn check_for_loop_iter(
122123
} else {
123124
Applicability::MachineApplicable
124125
};
125-
diag.span_suggestion(expr.span, "use", snippet.to_owned(), applicability);
126-
if !references_to_binding.is_empty() {
127-
diag.multipart_suggestion(
128-
"remove any references to the binding",
129-
references_to_binding,
130-
applicability,
131-
);
132-
}
126+
127+
let combined = references_to_binding
128+
.into_iter()
129+
.chain(vec![(expr.span, snippet.to_owned())])
130+
.collect_vec();
131+
132+
diag.multipart_suggestion("remove any references to the binding", combined, applicability);
133133
},
134134
);
135135
return true;
+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() {
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+
| ^^^^^^^^^^^^^^^^^ help: remove any references to the binding: `v.iter()`
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
|

tests/ui/unnecessary_to_owned.stderr

+2-6
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
523-
|
524-
LL | for t in file_types {
525-
| ~~~~~~~~~~
526522
help: remove any references to the binding
527523
|
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)