diff options
| -rw-r--r-- | clippy_lints/src/loops/needless_collect.rs | 178 | ||||
| -rw-r--r-- | tests/ui/needless_collect_indirect.rs | 31 |
2 files changed, 183 insertions, 26 deletions
diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index e87f4b66912..6f3acb45ba4 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -3,13 +3,16 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{is_trait_method, path_to_local_id}; +use clippy_utils::{can_move_expr_to_closure, is_trait_method, path_to_local, path_to_local_id, CaptureKind}; use if_chain::if_chain; +use rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor}; -use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, StmtKind}; +use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, Local, Mutability, Node, PatKind, Stmt, StmtKind}; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; +use rustc_middle::ty::subst::GenericArgKind; +use rustc_middle::ty::{self, TyS}; use rustc_span::sym; use rustc_span::{MultiSpan, Span}; @@ -83,7 +86,8 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo is_type_diagnostic_item(cx, ty, sym::VecDeque) || is_type_diagnostic_item(cx, ty, sym::BinaryHeap) || is_type_diagnostic_item(cx, ty, sym::LinkedList); - if let Some(iter_calls) = detect_iter_and_into_iters(block, id); + let iter_ty = cx.typeck_results().expr_ty(iter_source); + if let Some(iter_calls) = detect_iter_and_into_iters(block, id, cx, get_captured_ids(cx, iter_ty)); if let [iter_call] = &*iter_calls; then { let mut used_count_visitor = UsedCountVisitor { @@ -167,37 +171,89 @@ enum IterFunctionKind { Contains(Span), } -struct IterFunctionVisitor { - uses: Vec<IterFunction>, +struct IterFunctionVisitor<'a, 'tcx> { + illegal_mutable_capture_ids: HirIdSet, + current_mutably_captured_ids: HirIdSet, + cx: &'a LateContext<'tcx>, + uses: Vec<Option<IterFunction>>, + hir_id_uses_map: FxHashMap<HirId, usize>, + current_statement_hir_id: Option<HirId>, seen_other: bool, target: HirId, } -impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { +impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { + fn visit_block(&mut self, block: &'tcx Block<'tcx>) { + for (expr, hir_id) in block.stmts.iter().filter_map(get_expr_and_hir_id_from_stmt) { + self.visit_block_expr(expr, hir_id); + } + if let Some(expr) = block.expr { + self.visit_block_expr(expr, None); + } + } + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { // Check function calls on our collection if let ExprKind::MethodCall(method_name, _, [recv, args @ ..], _) = &expr.kind { + if method_name.ident.name == sym!(collect) && is_trait_method(self.cx, expr, sym::Iterator) { + self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(recv)); + self.visit_expr(recv); + return; + } + if path_to_local_id(recv, self.target) { - match &*method_name.ident.name.as_str() { - "into_iter" => self.uses.push(IterFunction { - func: IterFunctionKind::IntoIter, - span: expr.span, - }), - "len" => self.uses.push(IterFunction { - func: IterFunctionKind::Len, - span: expr.span, - }), - "is_empty" => self.uses.push(IterFunction { - func: IterFunctionKind::IsEmpty, - span: expr.span, - }), - "contains" => self.uses.push(IterFunction { - func: IterFunctionKind::Contains(args[0].span), - span: expr.span, - }), - _ => self.seen_other = true, + if self + .illegal_mutable_capture_ids + .intersection(&self.current_mutably_captured_ids) + .next() + .is_none() + { + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.insert(hir_id, self.uses.len()); + } + match &*method_name.ident.name.as_str() { + "into_iter" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::IntoIter, + span: expr.span, + })), + "len" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::Len, + span: expr.span, + })), + "is_empty" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::IsEmpty, + span: expr.span, + })), + "contains" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::Contains(args[0].span), + span: expr.span, + })), + _ => { + self.seen_other = true; + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.remove(&hir_id); + } + }, + } } return; } + + if let Some(hir_id) = path_to_local(recv) { + if let Some(index) = self.hir_id_uses_map.remove(&hir_id) { + if self + .illegal_mutable_capture_ids + .intersection(&self.current_mutably_captured_ids) + .next() + .is_none() + { + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.insert(hir_id, index); + } + } else { + self.uses[index] = None; + } + } + } } // Check if the collection is used for anything else if path_to_local_id(expr, self.target) { @@ -213,6 +269,28 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor { } } +impl<'tcx> IterFunctionVisitor<'_, 'tcx> { + fn visit_block_expr(&mut self, expr: &'tcx Expr<'tcx>, hir_id: Option<HirId>) { + self.current_statement_hir_id = hir_id; + self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(expr)); + self.visit_expr(expr); + } +} + +fn get_expr_and_hir_id_from_stmt<'v>(stmt: &'v Stmt<'v>) -> Option<(&'v Expr<'v>, Option<HirId>)> { + match stmt.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some((expr, None)), + StmtKind::Item(..) => None, + StmtKind::Local(Local { init, pat, .. }) => { + if let PatKind::Binding(_, hir_id, ..) = pat.kind { + init.map(|init_expr| (init_expr, Some(hir_id))) + } else { + init.map(|init_expr| (init_expr, None)) + } + }, + } +} + struct UsedCountVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, id: HirId, @@ -237,12 +315,60 @@ impl<'a, 'tcx> Visitor<'tcx> for UsedCountVisitor<'a, 'tcx> { /// Detect the occurrences of calls to `iter` or `into_iter` for the /// given identifier -fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, id: HirId) -> Option<Vec<IterFunction>> { +fn detect_iter_and_into_iters<'tcx: 'a, 'a>( + block: &'tcx Block<'tcx>, + id: HirId, + cx: &'a LateContext<'tcx>, + captured_ids: HirIdSet, +) -> Option<Vec<IterFunction>> { let mut visitor = IterFunctionVisitor { uses: Vec::new(), target: id, seen_other: false, + cx, + current_mutably_captured_ids: HirIdSet::default(), + illegal_mutable_capture_ids: captured_ids, + hir_id_uses_map: FxHashMap::default(), + current_statement_hir_id: None, }; visitor.visit_block(block); - if visitor.seen_other { None } else { Some(visitor.uses) } + if visitor.seen_other { + None + } else { + Some(visitor.uses.into_iter().flatten().collect()) + } +} + +fn get_captured_ids(cx: &LateContext<'tcx>, ty: &'_ TyS<'_>) -> HirIdSet { + fn get_captured_ids_recursive(cx: &LateContext<'tcx>, ty: &'_ TyS<'_>, set: &mut HirIdSet) { + match ty.kind() { + ty::Adt(_, generics) => { + for generic in *generics { + if let GenericArgKind::Type(ty) = generic.unpack() { + get_captured_ids_recursive(cx, ty, set); + } + } + }, + ty::Closure(def_id, _) => { + let closure_hir_node = cx.tcx.hir().get_if_local(*def_id).unwrap(); + if let Node::Expr(closure_expr) = closure_hir_node { + can_move_expr_to_closure(cx, closure_expr) + .unwrap() + .into_iter() + .for_each(|(hir_id, capture_kind)| { + if matches!(capture_kind, CaptureKind::Ref(Mutability::Mut)) { + set.insert(hir_id); + } + }); + } + }, + _ => (), + } + } + + let mut set = HirIdSet::default(); + + get_captured_ids_recursive(cx, ty, &mut set); + + set } diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index 2c94235b8f5..1f11d1f8d56 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -76,6 +76,37 @@ mod issue7110 { } } +mod issue7975 { + use super::*; + + fn direct_mapping_with_used_mutable_reference() -> Vec<()> { + let test_vec: Vec<()> = vec![]; + let mut vec_2: Vec<()> = vec![]; + let mut_ref = &mut vec_2; + let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect(); + collected_vec.into_iter().map(|_| mut_ref.push(())).collect() + } + + fn indirectly_mapping_with_used_mutable_reference() -> Vec<()> { + let test_vec: Vec<()> = vec![]; + let mut vec_2: Vec<()> = vec![]; + let mut_ref = &mut vec_2; + let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect(); + let iter = collected_vec.into_iter(); + iter.map(|_| mut_ref.push(())).collect() + } + + fn indirect_collect_after_indirect_mapping_with_used_mutable_reference() -> Vec<()> { + let test_vec: Vec<()> = vec![]; + let mut vec_2: Vec<()> = vec![]; + let mut_ref = &mut vec_2; + let collected_vec: Vec<_> = test_vec.into_iter().map(|_| mut_ref.push(())).collect(); + let iter = collected_vec.into_iter(); + let mapped_iter = iter.map(|_| mut_ref.push(())); + mapped_iter.collect() + } +} + fn allow_test() { #[allow(clippy::needless_collect)] let v = [1].iter().collect::<Vec<_>>(); |