Skip to content

Commit

Permalink
chore: starting to fix unnecessary_iter_cloned (#13848)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
blyxyas authored Dec 17, 2024
2 parents 8ea395e + 8fe39b2 commit 9f4941a
Show file tree
Hide file tree
Showing 5 changed files with 226 additions and 44 deletions.
16 changes: 8 additions & 8 deletions clippy_lints/src/methods/unnecessary_iter_cloned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use clippy_utils::ty::{get_iterator_item_ty, implements_trait};
use clippy_utils::visitors::for_each_expr_without_closures;
use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local};
use core::ops::ControlFlow;
use itertools::Itertools;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind};
Expand Down Expand Up @@ -122,14 +123,13 @@ pub fn check_for_loop_iter(
} else {
Applicability::MachineApplicable
};
diag.span_suggestion(expr.span, "use", snippet.to_owned(), applicability);
if !references_to_binding.is_empty() {
diag.multipart_suggestion(
"remove any references to the binding",
references_to_binding,
applicability,
);
}

let combined = references_to_binding
.into_iter()
.chain(vec![(expr.span, snippet.to_owned())])
.collect_vec();

diag.multipart_suggestion("remove any references to the binding", combined, applicability);
},
);
return true;
Expand Down
201 changes: 201 additions & 0 deletions tests/ui/unnecessary_iter_cloned.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
#![allow(unused_assignments)]
#![warn(clippy::unnecessary_to_owned)]

#[allow(dead_code)]
#[derive(Clone, Copy)]
enum FileType {
Account,
PrivateKey,
Certificate,
}

fn main() {
let path = std::path::Path::new("x");

let _ = check_files(&[(FileType::Account, path)]);
let _ = check_files_vec(vec![(FileType::Account, path)]);

// negative tests
let _ = check_files_ref(&[(FileType::Account, path)]);
let _ = check_files_mut(&[(FileType::Account, path)]);
let _ = check_files_ref_mut(&[(FileType::Account, path)]);
let _ = check_files_self_and_arg(&[(FileType::Account, path)]);
let _ = check_files_mut_path_buf(&[(FileType::Account, std::path::PathBuf::new())]);

check_mut_iteratee_and_modify_inner_variable();
}

// `check_files` and its variants are based on:
// https://github.com/breard-r/acmed/blob/1f0dcc32aadbc5e52de6d23b9703554c0f925113/acmed/src/storage.rs#L262
fn check_files(files: &[(FileType, &std::path::Path)]) -> bool {
for (t, path) in files {
let other = match get_file_path(t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

fn check_files_vec(files: Vec<(FileType, &std::path::Path)>) -> bool {
for (t, path) in files.iter() {
let other = match get_file_path(t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

fn check_files_ref(files: &[(FileType, &std::path::Path)]) -> bool {
for (ref t, path) in files.iter().copied() {
let other = match get_file_path(t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

#[allow(unused_assignments)]
fn check_files_mut(files: &[(FileType, &std::path::Path)]) -> bool {
for (mut t, path) in files.iter().copied() {
t = FileType::PrivateKey;
let other = match get_file_path(&t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

fn check_files_ref_mut(files: &[(FileType, &std::path::Path)]) -> bool {
for (ref mut t, path) in files.iter().copied() {
*t = FileType::PrivateKey;
let other = match get_file_path(t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

fn check_files_self_and_arg(files: &[(FileType, &std::path::Path)]) -> bool {
for (t, path) in files.iter().copied() {
let other = match get_file_path(&t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.join(path).is_file() || !other.is_file() {
return false;
}
}
true
}

#[allow(unused_assignments)]
fn check_files_mut_path_buf(files: &[(FileType, std::path::PathBuf)]) -> bool {
for (mut t, path) in files.iter().cloned() {
t = FileType::PrivateKey;
let other = match get_file_path(&t) {
Ok(p) => p,
Err(_) => {
return false;
},
};
if !path.is_file() || !other.is_file() {
return false;
}
}
true
}

fn get_file_path(_file_type: &FileType) -> Result<std::path::PathBuf, std::io::Error> {
Ok(std::path::PathBuf::new())
}

// Issue 12098
// https://github.com/rust-lang/rust-clippy/issues/12098
// no message emits
fn check_mut_iteratee_and_modify_inner_variable() {
struct Test {
list: Vec<String>,
mut_this: bool,
}

impl Test {
fn list(&self) -> &[String] {
&self.list
}
}

let mut test = Test {
list: vec![String::from("foo"), String::from("bar")],
mut_this: false,
};

for _item in test.list().to_vec() {
println!("{}", _item);

test.mut_this = true;
{
test.mut_this = true;
}
}
}

mod issue_12821 {
fn foo() {
let v: Vec<_> = "hello".chars().collect();
for c in v.iter() {
//~^ ERROR: unnecessary use of `cloned`
println!("{c}"); // should not suggest to remove `&`
}
}

fn bar() {
let v: Vec<_> = "hello".chars().collect();
for c in v.iter() {
//~^ ERROR: unnecessary use of `cloned`
let ref_c = c; //~ HELP: remove any references to the binding
println!("{ref_c}");
}
}

fn baz() {
let v: Vec<_> = "hello".chars().enumerate().collect();
for (i, c) in v.iter() {
//~^ ERROR: unnecessary use of `cloned`
let ref_c = c; //~ HELP: remove any references to the binding
let ref_i = i;
println!("{i} {ref_c}"); // should not suggest to remove `&` from `i`
}
}
}
2 changes: 0 additions & 2 deletions tests/ui/unnecessary_iter_cloned.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
#![allow(unused_assignments)]
#![warn(clippy::unnecessary_to_owned)]

//@no-rustfix: need to change the suggestion to a multipart suggestion

#[allow(dead_code)]
#[derive(Clone, Copy)]
enum FileType {
Expand Down
43 changes: 15 additions & 28 deletions tests/ui/unnecessary_iter_cloned.stderr
Original file line number Diff line number Diff line change
@@ -1,71 +1,58 @@
error: unnecessary use of `copied`
--> tests/ui/unnecessary_iter_cloned.rs:33:22
--> tests/ui/unnecessary_iter_cloned.rs:31:22
|
LL | for (t, path) in files.iter().copied() {
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::unnecessary-to-owned` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_to_owned)]`
help: use
|
LL | for (t, path) in files {
| ~~~~~
help: remove any references to the binding
|
LL - let other = match get_file_path(&t) {
LL + let other = match get_file_path(t) {
LL ~ for (t, path) in files {
LL ~ let other = match get_file_path(t) {
|

error: unnecessary use of `copied`
--> tests/ui/unnecessary_iter_cloned.rs:48:22
--> tests/ui/unnecessary_iter_cloned.rs:46:22
|
LL | for (t, path) in files.iter().copied() {
| ^^^^^^^^^^^^^^^^^^^^^
|
help: use
|
LL | for (t, path) in files.iter() {
| ~~~~~~~~~~~~
help: remove any references to the binding
|
LL - let other = match get_file_path(&t) {
LL + let other = match get_file_path(t) {
LL ~ for (t, path) in files.iter() {
LL ~ let other = match get_file_path(t) {
|

error: unnecessary use of `cloned`
--> tests/ui/unnecessary_iter_cloned.rs:179:18
--> tests/ui/unnecessary_iter_cloned.rs:177:18
|
LL | for c in v.iter().cloned() {
| ^^^^^^^^^^^^^^^^^ help: use: `v.iter()`
| ^^^^^^^^^^^^^^^^^ help: remove any references to the binding: `v.iter()`

error: unnecessary use of `cloned`
--> tests/ui/unnecessary_iter_cloned.rs:187:18
--> tests/ui/unnecessary_iter_cloned.rs:185:18
|
LL | for c in v.iter().cloned() {
| ^^^^^^^^^^^^^^^^^
|
help: use
|
LL | for c in v.iter() {
| ~~~~~~~~
help: remove any references to the binding
|
LL - let ref_c = &c;
LL + let ref_c = c;
LL ~ for c in v.iter() {
LL |
LL ~ let ref_c = c;
|

error: unnecessary use of `cloned`
--> tests/ui/unnecessary_iter_cloned.rs:196:23
--> tests/ui/unnecessary_iter_cloned.rs:194:23
|
LL | for (i, c) in v.iter().cloned() {
| ^^^^^^^^^^^^^^^^^
|
help: use
|
LL | for (i, c) in v.iter() {
| ~~~~~~~~
help: remove any references to the binding
|
LL ~ for (i, c) in v.iter() {
LL |
LL ~ let ref_c = c;
LL ~ let ref_i = i;
|
Expand Down
8 changes: 2 additions & 6 deletions tests/ui/unnecessary_to_owned.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -519,14 +519,10 @@ error: unnecessary use of `to_vec`
LL | for t in file_types.to_vec() {
| ^^^^^^^^^^^^^^^^^^^
|
help: use
|
LL | for t in file_types {
| ~~~~~~~~~~
help: remove any references to the binding
|
LL - let path = match get_file_path(&t) {
LL + let path = match get_file_path(t) {
LL ~ for t in file_types {
LL ~ let path = match get_file_path(t) {
|

error: unnecessary use of `to_vec`
Expand Down

0 comments on commit 9f4941a

Please sign in to comment.