clean-up, add some comments

Author: ada4a

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<InsertExpr<'tcx>> {
-    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,15 +524,15 @@ 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;
                 },
                 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) => {

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;

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;

tests/ui/entry_btree.fixed

@@ -1,5 +1,4 @@
 #![warn(clippy::map_entry)]
-#![allow(dead_code)]
 
 use std::collections::BTreeMap;
 

tests/ui/entry_btree.rs

@@ -1,5 +1,4 @@
 #![warn(clippy::map_entry)]
-#![allow(dead_code)]
 
 use std::collections::BTreeMap;
 

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 | |

tests/ui/entry_unfixable.rs

@@ -14,40 +14,6 @@ macro_rules! insert {
     };
 }
 
-mod issue13306 {
-    use std::collections::HashMap;
-
-    struct Env {
-        enclosing: Option<Box<Env>>,
-        values: HashMap<String, usize>,
-    }
-
-    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<String, bool>) {
-    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<String, bool>) {
+    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<Box<Env>>,
+        values: HashMap<String, usize>,
+    }
+
+    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() {}

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 | |             }
    | |_____________^
    |

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};

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};