diff options
| -rw-r--r-- | clippy_lints/src/methods/unnecessary_iter_cloned.rs | 16 | ||||
| -rw-r--r-- | clippy_lints/src/methods/utils.rs | 16 | ||||
| -rw-r--r-- | tests/ui/unnecessary_iter_cloned.fixed | 29 | ||||
| -rw-r--r-- | tests/ui/unnecessary_iter_cloned.rs | 29 | ||||
| -rw-r--r-- | tests/ui/unnecessary_iter_cloned.stderr | 44 | ||||
| -rw-r--r-- | tests/ui/unnecessary_to_owned.stderr | 2 |
6 files changed, 117 insertions, 19 deletions
diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs index 7431dc1cf0b..d1300dd43c2 100644 --- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs +++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs @@ -38,7 +38,7 @@ pub fn check_for_loop_iter( ) -> bool { if let Some(grandparent) = get_parent_expr(cx, expr).and_then(|parent| get_parent_expr(cx, parent)) && let Some(ForLoop { pat, body, .. }) = ForLoop::hir(grandparent) - && let (clone_or_copy_needed, addr_of_exprs) = clone_or_copy_needed(cx, pat, body) + && let (clone_or_copy_needed, references_to_binding) = clone_or_copy_needed(cx, pat, body) && !clone_or_copy_needed && let Some(receiver_snippet) = snippet_opt(cx, receiver.span) { @@ -123,14 +123,12 @@ pub fn check_for_loop_iter( Applicability::MachineApplicable }; diag.span_suggestion(expr.span, "use", snippet, applicability); - for addr_of_expr in addr_of_exprs { - match addr_of_expr.kind { - ExprKind::AddrOf(_, _, referent) => { - let span = addr_of_expr.span.with_hi(referent.span.lo()); - diag.span_suggestion(span, "remove this `&`", "", applicability); - }, - _ => unreachable!(), - } + if !references_to_binding.is_empty() { + diag.multipart_suggestion( + "remove any references to the binding", + references_to_binding, + applicability, + ); } }, ); diff --git a/clippy_lints/src/methods/utils.rs b/clippy_lints/src/methods/utils.rs index 34d7b9acbe4..1a55b7160fb 100644 --- a/clippy_lints/src/methods/utils.rs +++ b/clippy_lints/src/methods/utils.rs @@ -9,6 +9,7 @@ use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::sym; +use rustc_span::Span; pub(super) fn derefs_to_slice<'tcx>( cx: &LateContext<'tcx>, @@ -96,15 +97,15 @@ pub(super) fn clone_or_copy_needed<'tcx>( cx: &LateContext<'tcx>, pat: &Pat<'tcx>, body: &'tcx Expr<'tcx>, -) -> (bool, Vec<&'tcx Expr<'tcx>>) { +) -> (bool, Vec<(Span, String)>) { let mut visitor = CloneOrCopyVisitor { cx, binding_hir_ids: pat_bindings(pat), clone_or_copy_needed: false, - addr_of_exprs: Vec::new(), + references_to_binding: Vec::new(), }; visitor.visit_expr(body); - (visitor.clone_or_copy_needed, visitor.addr_of_exprs) + (visitor.clone_or_copy_needed, visitor.references_to_binding) } /// Returns a vector of all `HirId`s bound by the pattern. @@ -127,7 +128,7 @@ struct CloneOrCopyVisitor<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, binding_hir_ids: Vec<HirId>, clone_or_copy_needed: bool, - addr_of_exprs: Vec<&'tcx Expr<'tcx>>, + references_to_binding: Vec<(Span, String)>, } impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { @@ -142,8 +143,11 @@ impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { if self.is_binding(expr) { if let Some(parent) = get_parent_expr(self.cx, expr) { match parent.kind { - ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => { - self.addr_of_exprs.push(parent); + ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, referent) => { + if !parent.span.from_expansion() { + self.references_to_binding + .push((parent.span.until(referent.span), String::new())); + } return; }, ExprKind::MethodCall(.., args, _) => { diff --git a/tests/ui/unnecessary_iter_cloned.fixed b/tests/ui/unnecessary_iter_cloned.fixed index 2c582c90ba8..dc5e163ff04 100644 --- a/tests/ui/unnecessary_iter_cloned.fixed +++ b/tests/ui/unnecessary_iter_cloned.fixed @@ -170,3 +170,32 @@ fn check_mut_iteratee_and_modify_inner_variable() { } } } + +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` + } + } +} diff --git a/tests/ui/unnecessary_iter_cloned.rs b/tests/ui/unnecessary_iter_cloned.rs index a28ccd1efef..8f797ac717f 100644 --- a/tests/ui/unnecessary_iter_cloned.rs +++ b/tests/ui/unnecessary_iter_cloned.rs @@ -170,3 +170,32 @@ fn check_mut_iteratee_and_modify_inner_variable() { } } } + +mod issue_12821 { + fn foo() { + let v: Vec<_> = "hello".chars().collect(); + for c in v.iter().cloned() { + //~^ 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().cloned() { + //~^ 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().cloned() { + //~^ 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` + } + } +} diff --git a/tests/ui/unnecessary_iter_cloned.stderr b/tests/ui/unnecessary_iter_cloned.stderr index fb98cfddc26..0bdb37a521f 100644 --- a/tests/ui/unnecessary_iter_cloned.stderr +++ b/tests/ui/unnecessary_iter_cloned.stderr @@ -10,7 +10,7 @@ help: use | LL | for (t, path) in files { | ~~~~~ -help: remove this `&` +help: remove any references to the binding | LL - let other = match get_file_path(&t) { LL + let other = match get_file_path(t) { @@ -26,11 +26,49 @@ help: use | LL | for (t, path) in files.iter() { | ~~~~~~~~~~~~ -help: remove this `&` +help: remove any references to the binding | LL - let other = match get_file_path(&t) { LL + let other = match get_file_path(t) { | -error: aborting due to 2 previous errors +error: unnecessary use of `cloned` + --> tests/ui/unnecessary_iter_cloned.rs:177:18 + | +LL | for c in v.iter().cloned() { + | ^^^^^^^^^^^^^^^^^ help: use: `v.iter()` + +error: unnecessary use of `cloned` + --> 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; + | + +error: unnecessary use of `cloned` + --> 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 ~ let ref_c = c; +LL ~ let ref_i = i; + | + +error: aborting due to 5 previous errors diff --git a/tests/ui/unnecessary_to_owned.stderr b/tests/ui/unnecessary_to_owned.stderr index 5475df9c7b9..2829f3cd6e9 100644 --- a/tests/ui/unnecessary_to_owned.stderr +++ b/tests/ui/unnecessary_to_owned.stderr @@ -487,7 +487,7 @@ help: use | LL | for t in file_types { | ~~~~~~~~~~ -help: remove this `&` +help: remove any references to the binding | LL - let path = match get_file_path(&t) { LL + let path = match get_file_path(t) { |