Auto merge of #12843 - mdm:fix-unnecessary-to-owned-println-interaction, r=y21

Fix `unnecessary_to_owned` interaction with macro expansion

fixes #12821

In the case of an unnecessary `.iter().cloned()`, the lint `unnecessary_to_owned` might suggest to remove the `&` from references without checking if such references are inside a macro expansion. This can lead to unexpected behavior or even broken code if the lint suggestion is applied blindly. See issue #12821 for an example.

This PR checks if such references are inside macro expansions and skips this part of the lint suggestion in these cases.

changelog: [`unnecessary_to_owned`]: Don't suggest to remove `&` inside macro expansion

Author: bors

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,
+                    );
                 }
             },
         );

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, _) => {

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`
+        }
+    }
+}

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`
+        }
+    }
+}

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
 

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) {