Fix redundant_iter_cloned false positive with move closures and coroutines (#16494)

Fixes rust-lang/rust-clippy#16428

changelog: [`redundant_iter_cloned`]: fix false positive with move
closures and coroutines

clippy_lints/src/methods/iter_overeager_cloned.rs

@@ -1,13 +1,15 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{implements_trait, is_copy};
+use clippy_utils::visitors::for_each_expr_without_closures;
+use core::ops::ControlFlow;
 use rustc_ast::BindingMode;
 use rustc_errors::Applicability;
-use rustc_hir::{Body, Expr, ExprKind, HirId, HirIdSet, PatKind};
+use rustc_hir::{Body, CaptureBy, Closure, Expr, ExprKind, HirId, HirIdSet, Param, PatKind};
 use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 use rustc_lint::LateContext;
 use rustc_middle::mir::{FakeReadCause, Mutability};
-use rustc_middle::ty::{self, BorrowKind};
+use rustc_middle::ty::{self, BorrowKind, UpvarCapture};
 use rustc_span::{Symbol, sym};
 
 use super::{ITER_OVEREAGER_CLONED, REDUNDANT_ITER_CLONED};
@@ -64,6 +66,11 @@ pub(super) fn check<'tcx>(
             let body @ Body { params: [p], .. } = cx.tcx.hir_body(closure.body) else {
                 return;
             };
+
+            if param_captured_by_move_block(cx, body.value, p) {
+                return;
+            }
+
             let mut delegate = MoveDelegate {
                 used_move: HirIdSet::default(),
             };
@@ -140,6 +147,34 @@ struct MoveDelegate {
     used_move: HirIdSet,
 }
 
+/// Checks if the expression contains a closure or coroutine with `move` capture semantics that
+/// captures the given parameter.
+fn param_captured_by_move_block(cx: &LateContext<'_>, expr: &Expr<'_>, param: &Param<'_>) -> bool {
+    let mut param_hir_ids = HirIdSet::default();
+    param.pat.walk(|pat| {
+        param_hir_ids.insert(pat.hir_id);
+        true
+    });
+
+    for_each_expr_without_closures(expr, |e| {
+        if let ExprKind::Closure(Closure {
+            capture_clause: CaptureBy::Value { .. },
+            def_id,
+            ..
+        }) = e.kind
+            && cx.tcx.closure_captures(*def_id).iter().any(|capture| {
+                matches!(capture.info.capture_kind, UpvarCapture::ByValue)
+                    && matches!(capture.place.base, PlaceBase::Upvar(upvar) if param_hir_ids.contains(&upvar.var_path.hir_id))
+            })
+        {
+            return ControlFlow::Break(());
+        }
+
+        ControlFlow::Continue(())
+    })
+    .is_some()
+}
+
 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 {

tests/ui/iter_overeager_cloned.fixed

@@ -102,3 +102,63 @@ fn main() {
 fn cloned_flatten(x: Option<&Option<String>>) -> Option<String> {
     x.cloned().flatten()
 }
+
+mod issue_16428 {
+    #[derive(Clone)]
+    struct Foo;
+
+    impl Foo {
+        async fn do_async(&self) {}
+    }
+
+    fn async_move_map() -> Vec<impl std::future::Future<Output = ()>> {
+        let map: std::collections::HashMap<(), Foo> = std::collections::HashMap::new();
+
+        // Should NOT lint: async move block captures `item` by value
+        map.values()
+            .cloned()
+            .map(|item| async move { item.do_async().await })
+            .collect::<Vec<_>>()
+    }
+
+    fn async_move_for_each() {
+        let map: std::collections::HashMap<(), Foo> = std::collections::HashMap::new();
+
+        // Should NOT lint: async move block captures `item` by value
+        map.values()
+            .cloned()
+            .for_each(|item| drop(async move { item.do_async().await }));
+    }
+
+    fn move_closure() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should NOT lint: move closure captures `x` by value
+        let _: Vec<_> = vec.iter().cloned().map(|x| move || x.len()).collect();
+    }
+
+    fn async_move_not_capturing_param() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should lint: async move captures `y`, not `x`
+        let _ = vec.iter().map(|x| {
+            //~^ redundant_iter_cloned
+            let y = x.len();
+            async move { y }
+        });
+    }
+
+    fn move_closure_not_capturing_param() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should lint: move closure captures `y`, not `x`
+        let _: Vec<_> = vec
+            //~^ redundant_iter_cloned
+            .iter()
+            .map(|x| {
+                let y = x.len();
+                move || y
+            })
+            .collect();
+    }
+}

tests/ui/iter_overeager_cloned.rs

@@ -103,3 +103,64 @@ fn bar<'a>(iter: impl Iterator<Item = &'a S<'a>> + 'a, target: String) -> impl I
 fn cloned_flatten(x: Option<&Option<String>>) -> Option<String> {
     x.cloned().flatten()
 }
+
+mod issue_16428 {
+    #[derive(Clone)]
+    struct Foo;
+
+    impl Foo {
+        async fn do_async(&self) {}
+    }
+
+    fn async_move_map() -> Vec<impl std::future::Future<Output = ()>> {
+        let map: std::collections::HashMap<(), Foo> = std::collections::HashMap::new();
+
+        // Should NOT lint: async move block captures `item` by value
+        map.values()
+            .cloned()
+            .map(|item| async move { item.do_async().await })
+            .collect::<Vec<_>>()
+    }
+
+    fn async_move_for_each() {
+        let map: std::collections::HashMap<(), Foo> = std::collections::HashMap::new();
+
+        // Should NOT lint: async move block captures `item` by value
+        map.values()
+            .cloned()
+            .for_each(|item| drop(async move { item.do_async().await }));
+    }
+
+    fn move_closure() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should NOT lint: move closure captures `x` by value
+        let _: Vec<_> = vec.iter().cloned().map(|x| move || x.len()).collect();
+    }
+
+    fn async_move_not_capturing_param() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should lint: async move captures `y`, not `x`
+        let _ = vec.iter().cloned().map(|x| {
+            //~^ redundant_iter_cloned
+            let y = x.len();
+            async move { y }
+        });
+    }
+
+    fn move_closure_not_capturing_param() {
+        let vec = vec!["1".to_string(), "2".to_string()];
+
+        // Should lint: move closure captures `y`, not `x`
+        let _: Vec<_> = vec
+            //~^ redundant_iter_cloned
+            .iter()
+            .cloned()
+            .map(|x| {
+                let y = x.len();
+                move || y
+            })
+            .collect();
+    }
+}

tests/ui/iter_overeager_cloned.stderr

@@ -165,5 +165,47 @@ LL |     let _ = vec.iter().cloned().any(|x| x.len() == 1);
    |                       |
    |                       help: try: `.any(|x| x.len() == 1)`
 
-error: aborting due to 19 previous errors
+error: unneeded cloning of iterator items
+  --> tests/ui/iter_overeager_cloned.rs:145:17
+   |
+LL |           let _ = vec.iter().cloned().map(|x| {
+   |  _________________^
+LL | |
+LL | |             let y = x.len();
+LL | |             async move { y }
+LL | |         });
+   | |__________^
+   |
+help: try
+   |
+LL ~         let _ = vec.iter().map(|x| {
+LL +
+LL +             let y = x.len();
+LL +             async move { y }
+LL ~         });
+   |
+
+error: unneeded cloning of iterator items
+  --> tests/ui/iter_overeager_cloned.rs:156:25
+   |
+LL |           let _: Vec<_> = vec
+   |  _________________________^
+LL | |
+LL | |             .iter()
+LL | |             .cloned()
+...  |
+LL | |                 move || y
+LL | |             })
+   | |______________^
+   |
+help: try
+   |
+LL ~             .iter()
+LL +             .map(|x| {
+LL +                 let y = x.len();
+LL +                 move || y
+LL +             })
+   |
+
+error: aborting due to 21 previous errors