`map_entry` improvements

Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.

diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs
index ca01d0a7f87581db2cef75ac7942cb9cb8fa62f0..3e1d70b2e4078ba9e53a5151c89e31c6cc2c2e88 100644 (file)
--- a/clippy_lints/src/entry.rs
+++ b/clippy_lints/src/entry.rs
@@ -1,7 +1,7 @@
 use clippy_utils::{
     diagnostics::span_lint_and_sugg,
     is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while,
-    source::{snippet_indent, snippet_with_applicability, snippet_with_context},
+    source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context},
     SpanlessEq,
 };
 use rustc_errors::Applicability;
@@ -75,7 +75,84 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         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 sugg = if !contains_expr.negated || else_expr.is_some() || then_search.insertions.is_empty() {
+        let sugg = if let Some(else_expr) = else_expr {
+            // if .. { .. } else { .. }
+            let else_search = match find_insert_calls(cx, &contains_expr, else_expr) {
+                Some(search) if !(then_search.insertions.is_empty() && search.insertions.is_empty()) => search,
+                _ => return,
+            };
+
+            if then_search.insertions.is_empty() || else_search.insertions.is_empty() {
+                // if .. { insert } else { .. } or if .. { .. } else { then } of
+                let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() {
+                    if contains_expr.negated {
+                        (
+                            then_search.snippet_vacant(cx, then_expr.span, &mut app),
+                            snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
+                            "Vacant(e)",
+                        )
+                    } else {
+                        (
+                            then_search.snippet_occupied(cx, then_expr.span, &mut app),
+                            snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app),
+                            "Occupied(mut e)",
+                        )
+                    }
+                } else if contains_expr.negated {
+                    (
+                        else_search.snippet_occupied(cx, else_expr.span, &mut app),
+                        snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
+                        "Occupied(mut e)",
+                    )
+                } else {
+                    (
+                        else_search.snippet_vacant(cx, else_expr.span, &mut app),
+                        snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app),
+                        "Vacant(e)",
+                    )
+                };
+                format!(
+                    "if let {}::{} = {}.entry({}) {} else {}",
+                    map_ty.entry_path(),
+                    entry_kind,
+                    map_str,
+                    key_str,
+                    then_str,
+                    else_str,
+                )
+            } else {
+                // if .. { insert } else { insert }
+                let (then_str, else_str, then_entry, else_entry) = if contains_expr.negated {
+                    (
+                        then_search.snippet_vacant(cx, then_expr.span, &mut app),
+                        else_search.snippet_occupied(cx, else_expr.span, &mut app),
+                        "Vacant(e)",
+                        "Occupied(mut e)",
+                    )
+                } else {
+                    (
+                        then_search.snippet_occupied(cx, then_expr.span, &mut app),
+                        else_search.snippet_vacant(cx, else_expr.span, &mut app),
+                        "Occupied(mut e)",
+                        "Vacant(e)",
+                    )
+                };
+                let indent_str = snippet_indent(cx, expr.span);
+                let indent_str = indent_str.as_deref().unwrap_or("");
+                format!(
+                    "match {}.entry({}) {{\n{indent}    {entry}::{} => {}\n\
+                        {indent}    {entry}::{} => {}\n{indent}}}",
+                    map_str,
+                    key_str,
+                    then_entry,
+                    reindent_multiline(then_str.into(), true, Some(4 + indent_str.len())),
+                    else_entry,
+                    reindent_multiline(else_str.into(), true, Some(4 + indent_str.len())),
+                    entry = map_ty.entry_path(),
+                    indent = indent_str,
+                )
+            }
+        } else if then_search.insertions.is_empty() || !contains_expr.negated {
             return;
         } else {
             // if .. { insert }

diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed
new file mode 100644 (file)
index 0000000..2332fa6
--- /dev/null
+++ b/tests/ui/entry_with_else.fixed
@@ -0,0 +1,73 @@
+// run-rustfix
+
+#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
+#![warn(clippy::map_entry)]
+
+use std::collections::{BTreeMap, HashMap};
+use std::hash::Hash;
+
+macro_rules! m {
+    ($e:expr) => {{ $e }};
+}
+
+fn foo() {}
+
+fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
+    match m.entry(k) {
+        std::collections::hash_map::Entry::Vacant(e) => {
+            e.insert(v);
+        }
+        std::collections::hash_map::Entry::Occupied(mut e) => {
+            e.insert(v2);
+        }
+    }
+
+    match m.entry(k) {
+        std::collections::hash_map::Entry::Occupied(mut e) => {
+            e.insert(v);
+        }
+        std::collections::hash_map::Entry::Vacant(e) => {
+            e.insert(v2);
+        }
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        e.insert(v);
+    } else {
+        foo();
+    }
+
+    if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
+        e.insert(v);
+    } else {
+        foo();
+    }
+
+    match m.entry(k) {
+        std::collections::hash_map::Entry::Vacant(e) => {
+            e.insert(v);
+        }
+        std::collections::hash_map::Entry::Occupied(mut e) => {
+            e.insert(v2);
+        }
+    }
+
+    match m.entry(k) {
+        std::collections::hash_map::Entry::Occupied(mut e) => {
+            if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
+        }
+        std::collections::hash_map::Entry::Vacant(e) => {
+            e.insert(v);
+            None
+        }
+    };
+
+    if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
+        foo();
+        Some(e.insert(v))
+    } else {
+        None
+    };
+}
+
+fn main() {}

diff --git a/tests/ui/entry_with_else.rs b/tests/ui/entry_with_else.rs
new file mode 100644 (file)
index 0000000..2ff0c03
--- /dev/null
+++ b/tests/ui/entry_with_else.rs
@@ -0,0 +1,60 @@
+// run-rustfix
+
+#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
+#![warn(clippy::map_entry)]
+
+use std::collections::{BTreeMap, HashMap};
+use std::hash::Hash;
+
+macro_rules! m {
+    ($e:expr) => {{ $e }};
+}
+
+fn foo() {}
+
+fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+    } else {
+        m.insert(k, v2);
+    }
+
+    if m.contains_key(&k) {
+        m.insert(k, v);
+    } else {
+        m.insert(k, v2);
+    }
+
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+    } else {
+        foo();
+    }
+
+    if !m.contains_key(&k) {
+        foo();
+    } else {
+        m.insert(k, v);
+    }
+
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+    } else {
+        m.insert(k, v2);
+    }
+
+    if m.contains_key(&k) {
+        if true { m.insert(k, v) } else { m.insert(k, v2) }
+    } else {
+        m.insert(k, v)
+    };
+
+    if m.contains_key(&k) {
+        foo();
+        m.insert(k, v)
+    } else {
+        None
+    };
+}
+
+fn main() {}

diff --git a/tests/ui/entry_with_else.stderr b/tests/ui/entry_with_else.stderr
new file mode 100644 (file)
index 0000000..6f62ff8
--- /dev/null
+++ b/tests/ui/entry_with_else.stderr
@@ -0,0 +1,142 @@
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:16:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     } else {
+LL | |         m.insert(k, v2);
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::map-entry` implied by `-D warnings`
+help: try this
+   |
+LL |     match m.entry(k) {
+LL |         std::collections::hash_map::Entry::Vacant(e) => {
+LL |             e.insert(v);
+LL |         }
+LL |         std::collections::hash_map::Entry::Occupied(mut e) => {
+LL |             e.insert(v2);
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:22:5
+   |
+LL | /     if m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     } else {
+LL | |         m.insert(k, v2);
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     match m.entry(k) {
+LL |         std::collections::hash_map::Entry::Occupied(mut e) => {
+LL |             e.insert(v);
+LL |         }
+LL |         std::collections::hash_map::Entry::Vacant(e) => {
+LL |             e.insert(v2);
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:28:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     } else {
+LL | |         foo();
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         e.insert(v);
+LL |     } else {
+LL |         foo();
+LL |     }
+   |
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:34:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |     } else {
+LL | |         m.insert(k, v);
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
+LL |         e.insert(v);
+LL |     } else {
+LL |         foo();
+LL |     }
+   |
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:40:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     } else {
+LL | |         m.insert(k, v2);
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     match m.entry(k) {
+LL |         std::collections::hash_map::Entry::Vacant(e) => {
+LL |             e.insert(v);
+LL |         }
+LL |         std::collections::hash_map::Entry::Occupied(mut e) => {
+LL |             e.insert(v2);
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:46:5
+   |
+LL | /     if m.contains_key(&k) {
+LL | |         if true { m.insert(k, v) } else { m.insert(k, v2) }
+LL | |     } else {
+LL | |         m.insert(k, v)
+LL | |     };
+   | |_____^
+   |
+help: try this
+   |
+LL |     match m.entry(k) {
+LL |         std::collections::hash_map::Entry::Occupied(mut e) => {
+LL |             if true { Some(e.insert(v)) } else { Some(e.insert(v2)) }
+LL |         }
+LL |         std::collections::hash_map::Entry::Vacant(e) => {
+LL |             e.insert(v);
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry_with_else.rs:52:5
+   |
+LL | /     if m.contains_key(&k) {
+LL | |         foo();
+LL | |         m.insert(k, v)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) {
+LL |         foo();
+LL |         Some(e.insert(v))
+LL |     } else {
+LL |         None
+LL |     };
+   |
+
+error: aborting due to 7 previous errors
+