From 5b3c00f012dade7a1586cc8cca4b9e4a20888315 Mon Sep 17 00:00:00 2001 From: Skyler Calaman <54462713+Blckbrry-Pi@users.noreply.github.com> Date: Tue, 16 Nov 2021 09:18:49 -0500 Subject: [PATCH] Successfully generalize prevention of suggestions causing multiple mutable borrows. Also add some more tests to check that it's working. --- clippy_lints/src/loops/needless_collect.rs | 82 +++++++++++++++++----- tests/ui/needless_collect_indirect.rs | 21 +++++- 2 files changed, 84 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 22c123e9e..aeecf226b 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -3,8 +3,9 @@ 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::{can_move_expr_to_closure, is_trait_method, path_to_local_id, CaptureKind}; +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, HirIdSet, Local, Mutability, Node, PatKind, Stmt, StmtKind}; @@ -174,15 +175,23 @@ struct IterFunctionVisitor<'b, 'a> { illegal_mutable_capture_ids: HirIdSet, current_mutably_captured_ids: HirIdSet, cx: &'a LateContext<'b>, - uses: Vec, + uses: Vec>, + hir_id_uses_map: FxHashMap, + current_statement_hir_id: Option, seen_other: bool, target: HirId, } impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { fn visit_block(&mut self, block: &'txc Block<'tcx>) { - for elem in block.stmts.iter().filter_map(get_expr_from_stmt).chain(block.expr) { - self.current_mutably_captured_ids = HirIdSet::default(); - self.visit_expr(elem); + for (expr, hir_id) in block + .stmts + .iter() + .filter_map(get_expr_and_hir_id_from_stmt) + .chain(block.expr.map(|expr| (expr, None))) + { + 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); } } @@ -202,28 +211,53 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { .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(IterFunction { + "into_iter" => self.uses.push(Some(IterFunction { func: IterFunctionKind::IntoIter, span: expr.span, - }), - "len" => self.uses.push(IterFunction { + })), + "len" => self.uses.push(Some(IterFunction { func: IterFunctionKind::Len, span: expr.span, - }), - "is_empty" => self.uses.push(IterFunction { + })), + "is_empty" => self.uses.push(Some(IterFunction { func: IterFunctionKind::IsEmpty, span: expr.span, - }), - "contains" => self.uses.push(IterFunction { + })), + "contains" => self.uses.push(Some(IterFunction { func: IterFunctionKind::Contains(args[0].span), span: expr.span, - }), - _ => self.seen_other = true, + })), + _ => { + 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) { @@ -239,11 +273,17 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { } } -fn get_expr_from_stmt<'v>(stmt: &'v Stmt<'v>) -> Option<&'v Expr<'v>> { +fn get_expr_and_hir_id_from_stmt<'v>(stmt: &'v Stmt<'v>) -> Option<(&'v Expr<'v>, Option)> { match stmt.kind { - StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some(expr), + StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some((expr, None)), StmtKind::Item(..) => None, - StmtKind::Local(Local { init, .. }) => *init, + 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)) + } + }, } } @@ -284,9 +324,15 @@ fn detect_iter_and_into_iters<'tcx: 'a, 'a>( 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()) + } } #[allow(rustc::usage_of_ty_tykind)] diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index 7f2d86b09..1f11d1f8d 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -79,13 +79,32 @@ mod issue7110 { mod issue7975 { use super::*; - fn shouldnt_lint() -> Vec<()> { + 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() {