Minor cleanup of map_entry and a few additional tests.

rust-lang · Apr 3, 2021 · 2458362 · 2458362
1 parent 09e3d9f
commit 2458362
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 85 deletions.
diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs
@@ -530,13 +530,17 @@ impl InsertSearchResults<'tcx> {
                 app,
             ));
             if is_expr_used_or_unified(cx.tcx, insertion.call) {
-                res.push_str("Some(e.insert(");
-                res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
-                res.push_str("))");
+                let _ = write!(
+                    res,
+                    "Some(e.insert({}))",
+                    snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0
+                );
             } else {
-                res.push_str("e.insert(");
-                res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
-                res.push(')');
+                let _ = write!(
+                    res,
+                    "e.insert({})",
+                    snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0
+                );
             }
             span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP);
         }
@@ -607,6 +611,7 @@ impl InsertSearchResults<'tcx> {
         res
     }
 }
+
 fn find_insert_calls(
     cx: &LateContext<'tcx>,
     contains_expr: &ContainsExpr<'tcx>,

diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed
@@ -2,6 +2,7 @@
 
 #![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
 #![warn(clippy::map_entry)]
+#![feature(asm)]
 
 use std::collections::{BTreeMap, HashMap};
 use std::hash::Hash;
@@ -10,11 +11,19 @@ macro_rules! m {
     ($e:expr) => {{ $e }};
 }
 
+macro_rules! insert {
+    ($map:expr, $key:expr, $val:expr) => {
+        $map.insert($key, $val)
+    };
+}
+
 fn foo() {}
 
-fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
+fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, k: K, k2: K, v: V, v2: V) {
+    // or_insert(v)
     m.entry(k).or_insert(v);
 
+    // semicolon on insert, use or_insert_with(..)
     m.entry(k).or_insert_with(|| {
         if true {
             v
@@ -23,6 +32,7 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     });
 
+    // semicolon on if, use or_insert_with(..)
     m.entry(k).or_insert_with(|| {
         if true {
             v
@@ -31,6 +41,7 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     });
 
+    // early return, use if let
     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
         if true {
             e.insert(v);
@@ -40,11 +51,13 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     }
 
+    // use or_insert_with(..)
     m.entry(k).or_insert_with(|| {
         foo();
         v
     });
 
+    // semicolon on insert and match, use or_insert_with(..)
     m.entry(k).or_insert_with(|| {
         match 0 {
             1 if true => {
@@ -56,18 +69,17 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     });
 
+    // one branch doesn't insert, use if let
     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
         match 0 {
-            0 => {},
-            1 => {
-                e.insert(v);
-            },
+            0 => foo(),
             _ => {
                 e.insert(v2);
             },
         };
     }
 
+    // use or_insert_with
     m.entry(k).or_insert_with(|| {
         foo();
         match 0 {
@@ -94,10 +106,46 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     });
 
+    // ok, insert in loop
+    if !m.contains_key(&k) {
+        for _ in 0..2 {
+            m.insert(k, v);
+        }
+    }
+
+    // macro_expansion test, use or_insert(..)
     m.entry(m!(k)).or_insert_with(|| m!(v));
+
+    // ok, map used before insertion
+    if !m.contains_key(&k) {
+        let _ = m.len();
+        m.insert(k, v);
+    }
+
+    // ok, inline asm
+    if !m.contains_key(&k) {
+        unsafe { asm!("nop") }
+        m.insert(k, v);
+    }
+
+    // ok, different keys.
+    if !m.contains_key(&k) {
+        m.insert(k2, v);
+    }
+
+    // ok, different maps
+    if !m.contains_key(&k) {
+        m2.insert(k, v);
+    }
+
+    // ok, insert in macro
+    if !m.contains_key(&k) {
+        insert!(m, k, v);
+    }
 }
 
 fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
+    // insert then do something, use if let
     if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
         e.insert(v);
         foo();

diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs
@@ -2,6 +2,7 @@
 
 #![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)]
 #![warn(clippy::map_entry)]
+#![feature(asm)]
 
 use std::collections::{BTreeMap, HashMap};
 use std::hash::Hash;
@@ -10,13 +11,21 @@ macro_rules! m {
     ($e:expr) => {{ $e }};
 }
 
+macro_rules! insert {
+    ($map:expr, $key:expr, $val:expr) => {
+        $map.insert($key, $val)
+    };
+}
+
 fn foo() {}
 
-fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {
+fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, k: K, k2: K, v: V, v2: V) {
+    // or_insert(v)
     if !m.contains_key(&k) {
         m.insert(k, v);
     }
 
+    // semicolon on insert, use or_insert_with(..)
     if !m.contains_key(&k) {
         if true {
             m.insert(k, v);
@@ -25,6 +34,7 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     }
 
+    // semicolon on if, use or_insert_with(..)
     if !m.contains_key(&k) {
         if true {
             m.insert(k, v)
@@ -33,6 +43,7 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         };
     }
 
+    // early return, use if let
     if !m.contains_key(&k) {
         if true {
             m.insert(k, v);
@@ -42,11 +53,13 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     }
 
+    // use or_insert_with(..)
     if !m.contains_key(&k) {
         foo();
         m.insert(k, v);
     }
 
+    // semicolon on insert and match, use or_insert_with(..)
     if !m.contains_key(&k) {
         match 0 {
             1 if true => {
@@ -58,18 +71,17 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         };
     }
 
+    // one branch doesn't insert, use if let
     if !m.contains_key(&k) {
         match 0 {
-            0 => {},
-            1 => {
-                m.insert(k, v);
-            },
+            0 => foo(),
             _ => {
                 m.insert(k, v2);
             },
         };
     }
 
+    // use or_insert_with
     if !m.contains_key(&k) {
         foo();
         match 0 {
@@ -96,12 +108,48 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     }
 
+    // ok, insert in loop
+    if !m.contains_key(&k) {
+        for _ in 0..2 {
+            m.insert(k, v);
+        }
+    }
+
+    // macro_expansion test, use or_insert(..)
     if !m.contains_key(&m!(k)) {
         m.insert(m!(k), m!(v));
     }
+
+    // ok, map used before insertion
+    if !m.contains_key(&k) {
+        let _ = m.len();
+        m.insert(k, v);
+    }
+
+    // ok, inline asm
+    if !m.contains_key(&k) {
+        unsafe { asm!("nop") }
+        m.insert(k, v);
+    }
+
+    // ok, different keys.
+    if !m.contains_key(&k) {
+        m.insert(k2, v);
+    }
+
+    // ok, different maps
+    if !m.contains_key(&k) {
+        m2.insert(k, v);
+    }
+
+    // ok, insert in macro
+    if !m.contains_key(&k) {
+        insert!(m, k, v);
+    }
 }
 
 fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
+    // insert then do something, use if let
     if !m.contains_key(&k) {
         m.insert(k, v);
         foo();

diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr
@@ -1,5 +1,5 @@
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:16:5
+  --> $DIR/entry.rs:24:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         m.insert(k, v);
@@ -9,7 +9,7 @@ LL | |     }
    = note: `-D clippy::map-entry` implied by `-D warnings`
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:20:5
+  --> $DIR/entry.rs:29:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         if true {
@@ -31,7 +31,7 @@ LL |         }
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:28:5
+  --> $DIR/entry.rs:38:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         if true {
@@ -53,7 +53,7 @@ LL |         }
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:36:5
+  --> $DIR/entry.rs:47:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         if true {
@@ -75,7 +75,7 @@ LL |             return;
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:45:5
+  --> $DIR/entry.rs:57:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         foo();
@@ -92,7 +92,7 @@ LL |     });
    |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:50:5
+  --> $DIR/entry.rs:63:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         match 0 {
@@ -114,12 +114,12 @@ LL |             _ => {
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:61:5
+  --> $DIR/entry.rs:75:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         match 0 {
-LL | |             0 => {},
-LL | |             1 => {
+LL | |             0 => foo(),
+LL | |             _ => {
 ...  |
 LL | |         };
 LL | |     }
@@ -129,14 +129,14 @@ help: try this
    |
 LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
 LL |         match 0 {
-LL |             0 => {},
-LL |             1 => {
-LL |                 e.insert(v);
+LL |             0 => foo(),
+LL |             _ => {
+LL |                 e.insert(v2);
 LL |             },
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:73:5
+  --> $DIR/entry.rs:85:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         foo();
@@ -158,15 +158,15 @@ LL |             },
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:99:5
+  --> $DIR/entry.rs:119:5
    |
 LL | /     if !m.contains_key(&m!(k)) {
 LL | |         m.insert(m!(k), m!(v));
 LL | |     }
    | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));`
 
 error: usage of `contains_key` followed by `insert` on a `BTreeMap`
-  --> $DIR/entry.rs:105:5
+  --> $DIR/entry.rs:153:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         m.insert(k, v);