From 03bb7908b964fd461b2ba277dcf4c04608b083ef Mon Sep 17 00:00:00 2001 From: Ethiraric Date: Tue, 27 Feb 2024 11:41:38 +0100 Subject: [PATCH] [`map_entry`]: Check insert expression for map use The lint makes sure that the map is not used (borrowed) before the call to `insert`. Since the lint creates a mutable borrow on the map with the `Entry`, it wouldn't be possible to replace such code with `Entry`. However, expressions up to the `insert` call are checked, but not expressions for the arguments of the `insert` call itself. This commit fixes that. Fixes #11935 --- clippy_lints/src/entry.rs | 55 +++++++++++++++++++++++++++++++++++++-- tests/ui/entry.fixed | 11 ++++++++ tests/ui/entry.rs | 11 ++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index 0b4bc375df0..de6073c2723 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -214,12 +214,31 @@ fn entry_path(self) -> &'static str { } } +/// Details on an expression checking whether a map contains a key. +/// +/// For instance, with the following: +/// ```ignore +/// !!!self.the_map.contains_key("the_key") +/// ``` +/// +/// - `negated` will be set to `true` (the 3 `!` negate the condition) +/// - `map` will be the `self.the_map` expression +/// - `key` will be the `"the_key"` expression struct ContainsExpr<'tcx> { + /// Whether the check for `contains_key` was negated. negated: bool, + /// The map on which the check is performed. map: &'tcx Expr<'tcx>, + /// The key that is checked to be contained. key: &'tcx Expr<'tcx>, + /// The context of the whole condition expression. call_ctxt: SyntaxContext, } + +/// Inspect the given expression and return details about the `contains_key` check. +/// +/// If the given expression is not a `contains_key` check against a `BTreeMap` or a `HashMap`, +/// return `None`. fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(MapType, ContainsExpr<'tcx>)> { let mut negated = false; let expr = peel_hir_expr_while(expr, |e| match e.kind { @@ -229,6 +248,7 @@ fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Optio }, _ => None, }); + match expr.kind { ExprKind::MethodCall( _, @@ -261,11 +281,28 @@ fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Optio } } +/// Details on an expression inserting a key into a map. +/// +/// For instance, on the following: +/// ```ignore +/// self.the_map.insert("the_key", 3 + 4); +/// ``` +/// +/// - `map` will be the `self.the_map` expression +/// - `key` will be the `"the_key"` expression +/// - `value` will be the `3 + 4` expression struct InsertExpr<'tcx> { + /// The map into which the insertion is performed. map: &'tcx Expr<'tcx>, + /// The key at which to insert. key: &'tcx Expr<'tcx>, + /// The value to insert. value: &'tcx Expr<'tcx>, } + +/// Inspect the given expression and return details about the `insert` call. +/// +/// If the given expression is not an `insert` call into a `BTreeMap` or a `HashMap`, return `None`. fn try_parse_insert<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { if let ExprKind::MethodCall(_, map, [key, value], _) = expr.kind { let id = cx.typeck_results().type_dependent_def_id(expr.hir_id)?; @@ -298,7 +335,7 @@ struct Insertion<'tcx> { value: &'tcx Expr<'tcx>, } -/// This visitor needs to do a multiple things: +/// This visitor needs to do multiple things: /// * Find all usages of the map. An insertion can only be made before any other usages of the map. /// * Determine if there's an insertion using the same key. There's no need for the entry api /// otherwise. @@ -346,7 +383,7 @@ fn visit_cond_arm(&mut self, e: &'tcx Expr<'_>) -> bool { res } - /// Visits an expression which is not itself in a tail position, but other sibling expressions + /// Visit an expression which is not itself in a tail position, but other sibling expressions /// may be. e.g. if conditions fn visit_non_tail_expr(&mut self, e: &'tcx Expr<'_>) { let in_tail_pos = self.in_tail_pos; @@ -354,6 +391,19 @@ fn visit_non_tail_expr(&mut self, e: &'tcx Expr<'_>) { self.visit_expr(e); self.in_tail_pos = in_tail_pos; } + + /// Visit the key and value expression of an insert expression. + /// There may not be uses of the map in either of those two either. + fn visit_insert_expr_arguments(&mut self, e: &InsertExpr<'tcx>) { + let in_tail_pos = self.in_tail_pos; + let allow_insert_closure = self.allow_insert_closure; + let is_single_insert = self.is_single_insert; + walk_expr(self, e.key); + walk_expr(self, e.value); + self.in_tail_pos = in_tail_pos; + self.allow_insert_closure = allow_insert_closure; + self.is_single_insert = is_single_insert; + } } impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { @@ -425,6 +475,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { match try_parse_insert(self.cx, expr) { Some(insert_expr) if SpanlessEq::new(self.cx).eq_expr(self.map, insert_expr.map) => { + self.visit_insert_expr_arguments(&insert_expr); // Multiple inserts, inserts with a different key, and inserts from a macro can't use the entry api. if self.is_map_used || !SpanlessEq::new(self.cx).eq_expr(self.key, insert_expr.key) diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index 4099fe7e139..71ec13f4610 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -165,4 +165,15 @@ pub fn issue_10331() { } } +/// Issue 11935 +/// Do not suggest using entries if the map is used inside the `insert` expression. +pub fn issue_11935() { + let mut counts: HashMap = HashMap::new(); + if !counts.contains_key(&1) { + counts.insert(1, 1); + } else { + counts.insert(1, counts.get(&1).unwrap() + 1); + } +} + fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 409be0aa060..86092b7c055 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -169,4 +169,15 @@ pub fn issue_10331() { } } +/// Issue 11935 +/// Do not suggest using entries if the map is used inside the `insert` expression. +pub fn issue_11935() { + let mut counts: HashMap = HashMap::new(); + if !counts.contains_key(&1) { + counts.insert(1, 1); + } else { + counts.insert(1, counts.get(&1).unwrap() + 1); + } +} + fn main() {}