[iter_overeager_cloned]: detect .cloned().map() and .cloned().for_each()

clippy_lints/src/methods/iter_overeager_cloned.rs

@@ -1,21 +1,29 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{implements_trait, is_copy};
+use rustc_ast::BindingAnnotation;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{Body, Expr, ExprKind, HirId, HirIdSet, PatKind};
+use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::mir::{FakeReadCause, Mutability};
+use rustc_middle::ty::{self, BorrowKind};
 use rustc_span::sym;
 
 use super::ITER_OVEREAGER_CLONED;
 use crate::redundant_clone::REDUNDANT_CLONE;
+use crate::rustc_trait_selection::infer::TyCtxtInferExt;
 
 #[derive(Clone, Copy)]
 pub(super) enum Op<'a> {
     // rm `.cloned()`
     // e.g. `count`
     RmCloned,
 
+    // rm `.cloned()`
+    // e.g. `map` `for_each`
+    NeedlessMove(&'a str, &'a Expr<'a>),
+
     // later `.cloned()`
     // and add `&` to the parameter of closure parameter
     // e.g. `find` `filter`
@@ -51,8 +59,46 @@ pub(super) fn check<'tcx>(
             return;
         }
 
+        if let Op::NeedlessMove(_, expr) = op {
+            let rustc_hir::ExprKind::Closure(closure) = expr.kind else { return } ;
+            let body @ Body { params: [p], .. } = cx.tcx.hir().body(closure.body) else { return };
+            let mut delegate = MoveDelegate {used_move : HirIdSet::default()};
+            let infcx = cx.tcx.infer_ctxt().build();
+
+            ExprUseVisitor::new(
+                &mut delegate,
+                &infcx,
+                closure.body.hir_id.owner.def_id,
+                cx.param_env,
+                cx.typeck_results(),
+            )
+            .consume_body(body);
+
+            let mut to_be_discarded = false;
+
+            p.pat.walk(|it| {
+                if delegate.used_move.contains(&it.hir_id){
+                    to_be_discarded = true;
+                    return false;
+                }
+
+                match it.kind {
+                    PatKind::Binding(BindingAnnotation(_, Mutability::Mut), _, _, _)
+                    | PatKind::Ref(_, Mutability::Mut) => {
+                        to_be_discarded = true;
+                        false
+                    }
+                    _ => { true }
+                }
+            });
+
+            if to_be_discarded {
+                return;
+            }
+        }
+
         let (lint, msg, trailing_clone) = match op {
-            Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
+            Op::RmCloned | Op::NeedlessMove(_, _) => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
             Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"),
         };
 
@@ -83,8 +129,33 @@ pub(super) fn check<'tcx>(
                             diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
                         }
                     }
+                    Op::NeedlessMove(_, _) => {
+                        let method_span = expr.span.with_lo(cloned_call.span.hi());
+                        if let Some(snip) = snippet_opt(cx, method_span) {
+                            let replace_span = expr.span.with_lo(cloned_recv.span.hi());
+                            diag.span_suggestion(replace_span, "try", snip, Applicability::MaybeIncorrect);
+                        }
+                    }
                 }
             }
         );
     }
 }
+
+struct MoveDelegate {
+    used_move: HirIdSet,
+}
+
+impl<'tcx> Delegate<'tcx> for MoveDelegate {
+    fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
+        if let PlaceBase::Local(l) = place_with_id.place.base {
+            self.used_move.insert(l);
+        }
+    }
+
+    fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: BorrowKind) {}
+
+    fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
+
+    fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
+}

clippy_lints/src/methods/mod.rs

@@ -4001,9 +4001,11 @@ impl Methods {
                     manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv);
                     unnecessary_fold::check(cx, expr, init, acc, span);
                 },
-                ("for_each", [_]) => {
-                    if let Some(("inspect", _, [_], span2, _)) = method_call(recv) {
-                        inspect_for_each::check(cx, expr, span2);
+                ("for_each", [arg]) => {
+                    match method_call(recv) {
+                        Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2),
+                        Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, arg), false),
+                        _ => {}
                     }
                 },
                 ("get", [arg]) => {
@@ -4038,8 +4040,10 @@ impl Methods {
                 (name @ ("map" | "map_err"), [m_arg]) => {
                     if name == "map" {
                         map_clone::check(cx, expr, recv, m_arg, &self.msrv);
-                        if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) = method_call(recv) {
-                            iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
+                        match method_call(recv) {
+                            Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => iter_kv_map::check(cx, map_name, expr, recv2, m_arg),
+                            Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, m_arg), false),
+                            _ => {}
                         }
                     } else {
                         map_err_ignore::check(cx, expr, m_arg);

tests/ui/iter_overeager_cloned.fixed

@@ -56,14 +56,12 @@ fn main() {
         }
     }
 
-    // Not implemented yet
-    let _ = vec.iter().cloned().map(|x| x.len());
+    let _ = vec.iter().map(|x| x.len());
 
     // This would fail if changed.
     let _ = vec.iter().cloned().map(|x| x + "2");
 
-    // Not implemented yet
-    let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
+    let _ = vec.iter().for_each(|x| assert!(!x.is_empty()));
 
     // Not implemented yet
     let _ = vec.iter().cloned().all(|x| x.len() == 1);

tests/ui/iter_overeager_cloned.rs

@@ -57,13 +57,11 @@ fn main() {
         }
     }
 
-    // Not implemented yet
     let _ = vec.iter().cloned().map(|x| x.len());
 
     // This would fail if changed.
     let _ = vec.iter().cloned().map(|x| x + "2");
 
-    // Not implemented yet
     let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
 
     // Not implemented yet

tests/ui/iter_overeager_cloned.stderr

@@ -130,5 +130,21 @@ LL |             iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target
    |                 |
    |                 help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()`
 
-error: aborting due to 15 previous errors
+error: unneeded cloning of iterator items
+  --> $DIR/iter_overeager_cloned.rs:60:13
+   |
+LL |     let _ = vec.iter().cloned().map(|x| x.len());
+   |             ^^^^^^^^^^--------------------------
+   |                       |
+   |                       help: try: `.map(|x| x.len())`
+
+error: unneeded cloning of iterator items
+  --> $DIR/iter_overeager_cloned.rs:65:13
+   |
+LL |     let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
+   |             ^^^^^^^^^^----------------------------------------------
+   |                       |
+   |                       help: try: `.for_each(|x| assert!(!x.is_empty()))`
+
+error: aborting due to 17 previous errors