Handle returning a value in map_entry

Jarcho · Jarcho · commit c6feb9b80d8b · 2021-03-21T09:40:14.000-04:00
Allow the usage of the map after any insertions in `map_entry`
diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -1363,6 +1363,40 @@ pub fn is_expr_used(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
     )
 }
 
+pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
+    let map = tcx.hir();
+    let mut child_id = expr.hir_id;
+    let mut iter = map.parent_iter(child_id);
+    loop {
+        match iter.next() {
+            None => break None,
+            Some((id, Node::Block(_))) => child_id = id,
+            Some((id, Node::Arm(arm))) if arm.body.hir_id == child_id => child_id = id,
+            Some((_, Node::Expr(expr))) => match expr.kind {
+                ExprKind::Match(_, [arm], _) if arm.hir_id == child_id => child_id = expr.hir_id,
+                ExprKind::Block(..) | ExprKind::DropTemps(_) => child_id = expr.hir_id,
+                ExprKind::If(_, then_expr, None) if then_expr.hir_id == child_id => break None,
+                _ => break Some(Node::Expr(expr)),
+            },
+            Some((_, node)) => break Some(node),
+        }
+    }
+}
+
+pub fn is_expr_used_or_unified(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+    !matches!(
+        get_expr_use_or_unification_node(tcx, expr),
+        None | Some(Node::Stmt(Stmt {
+            kind: StmtKind::Expr(_) | StmtKind::Semi(_),
+            ..
+        }))
+    )
+}
+
+pub fn is_expr_final_block_expr(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+    matches!(get_parent_node(tcx, expr.hir_id), Some(Node::Block(..)))
+}
+
 pub fn is_no_std_crate(cx: &LateContext<'_>) -> bool {
     cx.tcx.hir().attrs(hir::CRATE_HIR_ID).iter().any(|attr| {
         if let ast::AttrKind::Normal(ref attr, _) = attr.kind {
diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -66,6 +66,15 @@ pub fn indent_of<T: LintContext>(cx: &T, span: Span) -> Option<usize> {
     snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace()))
 }
 
+/// Gets a snippet of the indentation of the line of a span
+pub fn snippet_indent<T: LintContext>(cx: &T, span: Span) -> Option<String> {
+    snippet_opt(cx, line_span(cx, span)).map(|mut s| {
+        let len = s.len() - s.trim_start().len();
+        s.truncate(len);
+        s
+    })
+}
+
 // If the snippet is empty, it's an attribute that was inserted during macro
 // expansion and we want to ignore those, because they could come from external
 // sources that the user has no control over.
diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed
@@ -12,7 +12,7 @@ macro_rules! m {
 
 fn foo() {}
 
-fn insert_if_absent0<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>, k: K, v: V, v2: V) {
     m.entry(k).or_insert(v);
 
     m.entry(k).or_insert_with(|| {
@@ -97,4 +97,11 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,
     m.entry(m!(k)).or_insert_with(|| m!(v));
 }
 
+fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
+    if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
+        e.insert(v);
+        foo();
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs
@@ -12,7 +12,7 @@ macro_rules! m {
 
 fn foo() {}
 
-fn insert_if_absent0<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>, k: K, v: V, v2: V) {
     if !m.contains_key(&k) {
         m.insert(k, v);
     }
@@ -101,4 +101,11 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,
     }
 }
 
+fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+        foo();
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr
@@ -165,5 +165,22 @@ LL | |         m.insert(m!(k), m!(v));
 LL | |     }
    | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));`
 
-error: aborting due to 9 previous errors
+error: usage of `contains_key` followed by `insert` on a `BTreeMap`
+  --> $DIR/entry.rs:105:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |         foo();
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
+LL |         e.insert(v);
+LL |         foo();
+LL |     }
+   |
+
+error: aborting due to 10 previous errors
 
diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs
@@ -13,49 +13,6 @@ macro_rules! m {
 
 fn foo() {}
 
-fn insert_if_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        m.insert(k, v)
-    } else {
-        None
-    };
-}
-
-fn insert_if_present2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if m.contains_key(&k) {
-        None
-    } else {
-        m.insert(k, v)
-    };
-}
-
-fn insert_if_absent3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        foo();
-        m.insert(k, v)
-    } else {
-        None
-    };
-}
-
-fn insert_if_present3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if m.contains_key(&k) {
-        None
-    } else {
-        foo();
-        m.insert(k, v)
-    };
-}
-
-fn insert_in_btreemap<K: Ord, V>(m: &mut BTreeMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        foo();
-        m.insert(k, v)
-    } else {
-        None
-    };
-}
-
 // should not trigger
 fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) {
     if !m.contains_key(&k) {
diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed
@@ -31,23 +31,43 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,
         }
     }
 
+    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(_) => {
-            foo();
+        std::collections::hash_map::Entry::Occupied(mut e) => {
+            e.insert(v2);
         }
     }
 
     match m.entry(k) {
-        std::collections::hash_map::Entry::Vacant(_) => {
-            foo();
-        }
         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
@@ -36,6 +36,25 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,
     } 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
@@ -51,13 +51,12 @@ 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(_) => {
-LL |             foo();
- ...
+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
@@ -66,18 +65,78 @@ 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(_) => {
-LL |             foo();
+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: aborting due to 4 previous errors
+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
 

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ macro_rules! m {`
`12`	`12`
`13`	`13`	`fn foo() {}`
`14`	`14`
`15`		`-fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {`
	`15`	`+fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2: V) {`
`16`	`16`	`if !m.contains_key(&k) {`
`17`	`17`	`m.insert(k, v);`
`18`	`18`	`}`
`@@ -101,4 +101,11 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,`
`101`	`101`	`}`
`102`	`102`	`}`
`103`	`103`
	`104`	`+fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {`
	`105`	`+ if !m.contains_key(&k) {`
	`106`	`+ m.insert(k, v);`
	`107`	`+ foo();`
	`108`	`+ }`
	`109`	`+}`
	`110`	`+`
`104`	`111`	`fn main() {}`