From 816cc5443099e779c6204e51ca135f2223cdae7b Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 24 Sep 2025 22:18:35 +0200 Subject: [PATCH 1/2] clean-up, add some comments --- clippy_lints/src/entry.rs | 83 +++++++++++++++++++-------------- tests/ui/entry.fixed | 2 +- tests/ui/entry.rs | 2 +- tests/ui/entry_btree.fixed | 1 - tests/ui/entry_btree.rs | 1 - tests/ui/entry_btree.stderr | 2 +- tests/ui/entry_unfixable.rs | 68 +++++++++++++-------------- tests/ui/entry_unfixable.stderr | 24 +++++----- tests/ui/entry_with_else.fixed | 2 +- tests/ui/entry_with_else.rs | 2 +- 10 files changed, 98 insertions(+), 89 deletions(-) diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index a5ec6777b434..ea5dbd3a4156 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -87,8 +87,8 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let lint_msg = format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()); let mut app = Applicability::MachineApplicable; - let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0; - let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0; + let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "(_)", &mut app).0; + let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "_", &mut app).0; let sugg = if let Some(else_expr) = else_expr { let Some(else_search) = find_insert_calls(cx, &contains_expr, else_expr) else { @@ -142,8 +142,11 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let indent_str = snippet_indent(cx, expr.span); let indent_str = indent_str.as_deref().unwrap_or(""); Some(format!( - "match {map_str}.entry({key_str}) {{\n{indent_str} {entry}::{then_entry} => {}\n\ - {indent_str} {entry}::{else_entry} => {}\n{indent_str}}}", + "\ + match {map_str}.entry({key_str}) {{ +{indent_str} {entry}::{then_entry} => {} +{indent_str} {entry}::{else_entry} => {} +{indent_str}}}", reindent_multiline(&then_str, true, Some(4 + indent_str.len())), reindent_multiline(&else_str, true, Some(4 + indent_str.len())), entry = map_ty.entry_path(), @@ -167,7 +170,7 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { map_ty.entry_path(), )) } else if let Some(insertion) = then_search.as_single_insertion() { - let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "..", &mut app).0; + let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "_", &mut app).0; if contains_expr.negated { if insertion.value.can_have_side_effects() { Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {value_str});")) @@ -312,15 +315,12 @@ struct InsertExpr<'tcx> { /// /// 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)?; - if let Some(insert) = cx.tcx.get_diagnostic_name(id) - && matches!(insert, sym::btreemap_insert | sym::hashmap_insert) - { - Some(InsertExpr { map, key, value }) - } else { - None - } + if let ExprKind::MethodCall(_, map, [key, value], _) = expr.kind + && let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let Some(insert) = cx.tcx.get_diagnostic_name(id) + && matches!(insert, sym::btreemap_insert | sym::hashmap_insert) + { + Some(InsertExpr { map, key, value }) } else { None } @@ -524,7 +524,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { ExprKind::If(cond_expr, then_expr, Some(else_expr)) => { self.is_single_insert = false; self.visit_non_tail_expr(cond_expr); - // Each branch may contain it's own insert expression. + // Each branch may contain its own insert expression. let mut is_map_used = self.visit_cond_arm(then_expr); is_map_used |= self.visit_cond_arm(else_expr); self.is_map_used = is_map_used; @@ -532,7 +532,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { ExprKind::Match(scrutinee_expr, arms, _) => { self.is_single_insert = false; self.visit_non_tail_expr(scrutinee_expr); - // Each branch may contain it's own insert expression. + // Each branch may contain its own insert expression. let mut is_map_used = self.is_map_used; for arm in arms { self.visit_pat(arm.pat); @@ -609,6 +609,25 @@ impl<'tcx> InsertSearchResults<'tcx> { self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap()) } + /// Create a snippet with all the `insert`s on the map replaced with `insert`s on the entry. + /// + /// `write_wrapped` will be called whenever the `insert` is an expression that is used, or has + /// its type unified with another branch -- for example, in cases like: + /// ```ignore + /// let _ = if !m.contains(&k) { + /// m.insert(k, v) + /// } else { + /// None + /// }; + /// + /// let _ = if m.contains(&k) { + /// if true { m.insert(k, v) } else { m.insert(k, v2) } + /// } else { + /// m.insert(k, v3) + /// } + /// ``` + /// The idea is that it will wrap the `insert` call so that it still type-checks with the code + /// surrounding it. fn snippet( &self, cx: &LateContext<'_>, @@ -619,6 +638,7 @@ impl<'tcx> InsertSearchResults<'tcx> { let ctxt = span.ctxt(); let mut res = String::new(); for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { + // Take everything until the call verbatim res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), @@ -628,11 +648,8 @@ impl<'tcx> InsertSearchResults<'tcx> { if is_expr_used_or_unified(cx.tcx, insertion.call) { write_wrapped(&mut res, insertion, ctxt, app); } else { - let _: fmt::Result = write!( - res, - "e.insert({})", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 - ); + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + let _: fmt::Result = write!(res, "e.insert({value_str})"); } span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); } @@ -644,11 +661,8 @@ impl<'tcx> InsertSearchResults<'tcx> { ( self.snippet(cx, span, app, |res, insertion, ctxt, app| { // Insertion into a map would return `Some(&mut value)`, but the entry returns `&mut value` - let _: fmt::Result = write!( - res, - "Some(e.insert({}))", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 - ); + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + let _: fmt::Result = write!(res, "Some(e.insert({value_str}))"); }), "Occupied(mut e)", ) @@ -658,19 +672,15 @@ impl<'tcx> InsertSearchResults<'tcx> { ( self.snippet(cx, span, app, |res, insertion, ctxt, app| { // Insertion into a map would return `None`, but the entry returns a mutable reference. + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; let _: fmt::Result = if is_expr_final_block_expr(cx.tcx, insertion.call) { write!( res, - "e.insert({});\n{}None", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, - snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""), + "e.insert({value_str});\n{indent}None", + indent = snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""), ) } else { - write!( - res, - "{{ e.insert({}); None }}", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, - ) + write!(res, "{{ e.insert({value_str}); None }}") }; }), "Vacant(e)", @@ -683,14 +693,15 @@ impl<'tcx> InsertSearchResults<'tcx> { for edit in &self.edits { match *edit { Edit::Insertion(insertion) => { - // Cut out the value from `map.insert(key, value)` + // Take everything until the call verbatim res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), "..", app, )); - res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + res.push_str(&value_str); span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); }, Edit::RemoveSemi(semi_span) => { diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index f2df9f0204ea..087e70696210 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -1,6 +1,6 @@ //@needs-asm-support -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::arch::asm; diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 166eea417ac2..1c3f640fbcb8 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -1,6 +1,6 @@ //@needs-asm-support -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::arch::asm; diff --git a/tests/ui/entry_btree.fixed b/tests/ui/entry_btree.fixed index 1218202f02b9..8e8563ea3051 100644 --- a/tests/ui/entry_btree.fixed +++ b/tests/ui/entry_btree.fixed @@ -1,5 +1,4 @@ #![warn(clippy::map_entry)] -#![allow(dead_code)] use std::collections::BTreeMap; diff --git a/tests/ui/entry_btree.rs b/tests/ui/entry_btree.rs index b795e32158ad..96a5c1bf2643 100644 --- a/tests/ui/entry_btree.rs +++ b/tests/ui/entry_btree.rs @@ -1,5 +1,4 @@ #![warn(clippy::map_entry)] -#![allow(dead_code)] use std::collections::BTreeMap; diff --git a/tests/ui/entry_btree.stderr b/tests/ui/entry_btree.stderr index 671340b89532..c2196fb4782e 100644 --- a/tests/ui/entry_btree.stderr +++ b/tests/ui/entry_btree.stderr @@ -1,5 +1,5 @@ error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> tests/ui/entry_btree.rs:10:5 + --> tests/ui/entry_btree.rs:9:5 | LL | / if !m.contains_key(&k) { LL | | diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs index c4c055572086..26916e428f6c 100644 --- a/tests/ui/entry_unfixable.rs +++ b/tests/ui/entry_unfixable.rs @@ -14,40 +14,6 @@ macro_rules! insert { }; } -mod issue13306 { - use std::collections::HashMap; - - struct Env { - enclosing: Option>, - values: HashMap, - } - - impl Env { - fn assign(&mut self, name: String, value: usize) -> bool { - if !self.values.contains_key(&name) { - //~^ map_entry - self.values.insert(name, value); - true - } else if let Some(enclosing) = &mut self.enclosing { - enclosing.assign(name, value) - } else { - false - } - } - } -} - -fn issue9925(mut hm: HashMap) { - let key = "hello".to_string(); - if hm.contains_key(&key) { - //~^ map_entry - let bval = hm.get_mut(&key).unwrap(); - *bval = false; - } else { - hm.insert(key, true); - } -} - mod issue9470 { use std::collections::HashMap; use std::sync::Mutex; @@ -90,4 +56,38 @@ mod issue9470 { } } +fn issue9925(mut hm: HashMap) { + let key = "hello".to_string(); + if hm.contains_key(&key) { + //~^ map_entry + let bval = hm.get_mut(&key).unwrap(); + *bval = false; + } else { + hm.insert(key, true); + } +} + +mod issue13306 { + use std::collections::HashMap; + + struct Env { + enclosing: Option>, + values: HashMap, + } + + impl Env { + fn assign(&mut self, name: String, value: usize) -> bool { + if !self.values.contains_key(&name) { + //~^ map_entry + self.values.insert(name, value); + true + } else if let Some(enclosing) = &mut self.enclosing { + enclosing.assign(name, value) + } else { + false + } + } + } +} + fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr index f92f472512f6..55d16b0b76f6 100644 --- a/tests/ui/entry_unfixable.stderr +++ b/tests/ui/entry_unfixable.stderr @@ -1,12 +1,12 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:27:13 + --> tests/ui/entry_unfixable.rs:46:13 | -LL | / if !self.values.contains_key(&name) { +LL | / if self.globals.contains_key(&name) { LL | | -LL | | self.values.insert(name, value); -LL | | true -... | -LL | | false +LL | | self.globals.insert(name, value); +LL | | } else { +LL | | let interner = INTERNER.lock().unwrap(); +LL | | return Err(interner.resolve(name).unwrap().to_owned()); LL | | } | |_____________^ | @@ -15,7 +15,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::map_entry)]` error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:42:5 + --> tests/ui/entry_unfixable.rs:61:5 | LL | / if hm.contains_key(&key) { LL | | @@ -31,12 +31,12 @@ LL | | } error: usage of `contains_key` followed by `insert` on a `HashMap` --> tests/ui/entry_unfixable.rs:80:13 | -LL | / if self.globals.contains_key(&name) { +LL | / if !self.values.contains_key(&name) { LL | | -LL | | self.globals.insert(name, value); -LL | | } else { -LL | | let interner = INTERNER.lock().unwrap(); -LL | | return Err(interner.resolve(name).unwrap().to_owned()); +LL | | self.values.insert(name, value); +LL | | true +... | +LL | | false LL | | } | |_____________^ | diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed index 76e6998235c2..0135ad325b69 100644 --- a/tests/ui/entry_with_else.fixed +++ b/tests/ui/entry_with_else.fixed @@ -1,4 +1,4 @@ -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::collections::{BTreeMap, HashMap}; diff --git a/tests/ui/entry_with_else.rs b/tests/ui/entry_with_else.rs index 1669cdc0c7cf..74c2addee04c 100644 --- a/tests/ui/entry_with_else.rs +++ b/tests/ui/entry_with_else.rs @@ -1,4 +1,4 @@ -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::collections::{BTreeMap, HashMap}; From 283d07eea171445cc511488f1eafdebe42f4c500 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 24 Sep 2025 22:51:42 +0200 Subject: [PATCH 2/2] fix(map_entry): don't autofix when `{e.insert(_); None }` would be required --- clippy_lints/src/entry.rs | 2 ++ tests/ui/entry_unfixable.rs | 8 ++++++++ tests/ui/entry_unfixable.stderr | 25 +++++++++++++++++++++---- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index ea5dbd3a4156..a27afb03d415 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -682,6 +682,8 @@ impl<'tcx> InsertSearchResults<'tcx> { } else { write!(res, "{{ e.insert({value_str}); None }}") }; + // This might leave `None`'s type ambiguous, so reduce applicability + *app = Applicability::MaybeIncorrect; }), "Vacant(e)", ) diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs index 26916e428f6c..4ba572aa7d0f 100644 --- a/tests/ui/entry_unfixable.rs +++ b/tests/ui/entry_unfixable.rs @@ -1,3 +1,4 @@ +//@no-rustfix #![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] @@ -90,4 +91,11 @@ mod issue13306 { } } +fn issue15307(mut m: HashMap, k: K, v: V) { + if !m.contains_key(&k) { + //~^ map_entry + assert!(m.insert(k, v).is_none()); + } +} + fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr index 55d16b0b76f6..731917e82876 100644 --- a/tests/ui/entry_unfixable.stderr +++ b/tests/ui/entry_unfixable.stderr @@ -1,5 +1,5 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:46:13 + --> tests/ui/entry_unfixable.rs:47:13 | LL | / if self.globals.contains_key(&name) { LL | | @@ -15,7 +15,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::map_entry)]` error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:61:5 + --> tests/ui/entry_unfixable.rs:62:5 | LL | / if hm.contains_key(&key) { LL | | @@ -29,7 +29,7 @@ LL | | } = help: consider using the `Entry` API: https://doc.rust-lang.org/std/collections/struct.HashMap.html#entry-api error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:80:13 + --> tests/ui/entry_unfixable.rs:81:13 | LL | / if !self.values.contains_key(&name) { LL | | @@ -42,5 +42,22 @@ LL | | } | = help: consider using the `Entry` API: https://doc.rust-lang.org/std/collections/struct.HashMap.html#entry-api -error: aborting due to 3 previous errors +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_unfixable.rs:95:5 + | +LL | / if !m.contains_key(&k) { +LL | | +LL | | assert!(m.insert(k, v).is_none()); +LL | | } + | |_____^ + | +help: try + | +LL ~ if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL + +LL + assert!({ e.insert(v); None }.is_none()); +LL + } + | + +error: aborting due to 4 previous errors