diff --git a/clippy_lints/src/methods/iter_overeager_cloned.rs b/clippy_lints/src/methods/iter_overeager_cloned.rs index e3bcca64e923..241791e57c8e 100644 --- a/clippy_lints/src/methods/iter_overeager_cloned.rs +++ b/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 { diff --git a/tests/ui/iter_overeager_cloned.fixed b/tests/ui/iter_overeager_cloned.fixed index 4171f19469a4..520b4f8be8cf 100644 --- a/tests/ui/iter_overeager_cloned.fixed +++ b/tests/ui/iter_overeager_cloned.fixed @@ -102,3 +102,63 @@ fn main() { fn cloned_flatten(x: Option<&Option>) -> Option { x.cloned().flatten() } + +mod issue_16428 { + #[derive(Clone)] + struct Foo; + + impl Foo { + async fn do_async(&self) {} + } + + fn async_move_map() -> Vec> { + 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::>() + } + + 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(); + } +} diff --git a/tests/ui/iter_overeager_cloned.rs b/tests/ui/iter_overeager_cloned.rs index fe6aba24dd3e..3e79675dd7c6 100644 --- a/tests/ui/iter_overeager_cloned.rs +++ b/tests/ui/iter_overeager_cloned.rs @@ -103,3 +103,64 @@ fn bar<'a>(iter: impl Iterator> + 'a, target: String) -> impl I fn cloned_flatten(x: Option<&Option>) -> Option { x.cloned().flatten() } + +mod issue_16428 { + #[derive(Clone)] + struct Foo; + + impl Foo { + async fn do_async(&self) {} + } + + fn async_move_map() -> Vec> { + 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::>() + } + + 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(); + } +} diff --git a/tests/ui/iter_overeager_cloned.stderr b/tests/ui/iter_overeager_cloned.stderr index f234d19e4aaa..72b00ca2e32c 100644 --- a/tests/ui/iter_overeager_cloned.stderr +++ b/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