Improve map_entry lint

Fix false positives where the map is used before inserting into the map.
Fix false positives where two insertions happen.
Suggest using `if let Entry::Vacant(e) = _.entry(_)` when `or_insert` might be a semantic change

Author: Jarcho

clippy_lints/src/entry.rs

@@ -61,10 +61,10 @@ use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs,
-    GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef,
-    MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind,
-    TraitRef, TyKind, Variant, Visibility,
+    def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Destination, Expr, ExprKind, FnDecl, ForeignItem,
+    GenericArgs, GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local,
+    MacroDef, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StmtKind, StructField, TraitItem,
+    TraitItemKind, TraitRef, TyKind, Variant, Visibility,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -1254,6 +1254,82 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some())
 }
 
+pub fn get_expr_use_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::Break(
+                    Destination {
+                        target_id: Ok(dest), ..
+                    },
+                    _,
+                ) => {
+                    iter = map.parent_iter(dest);
+                    child_id = dest;
+                },
+                ExprKind::DropTemps(_) | ExprKind::Block(..) => child_id = expr.hir_id,
+                ExprKind::If(control_expr, ..) | ExprKind::Match(control_expr, ..)
+                    if control_expr.hir_id != child_id =>
+                {
+                    child_id = expr.hir_id
+                },
+                _ => break Some(Node::Expr(expr)),
+            },
+            Some((_, node)) => break Some(node),
+        }
+    }
+}
+
+pub fn is_expr_used(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+    !matches!(
+        get_expr_use_node(tcx, expr),
+        Some(Node::Stmt(Stmt {
+            kind: StmtKind::Expr(_) | StmtKind::Semi(_),
+            ..
+        }))
+    )
+}
+
+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 {
@@ -1413,28 +1489,43 @@ pub fn peel_hir_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) {
     peel(pat, 0)
 }
 
+/// Peels of expressions while the given closure returns `Some`.
+pub fn peel_hir_expr_while<'tcx>(
+    mut expr: &'tcx Expr<'tcx>,
+    mut f: impl FnMut(&'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>>,
+) -> &'tcx Expr<'tcx> {
+    while let Some(e) = f(expr) {
+        expr = e;
+    }
+    expr
+}
+
 /// Peels off up to the given number of references on the expression. Returns the underlying
 /// expression and the number of references removed.
 pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
-    fn f(expr: &'a Expr<'a>, count: usize, target: usize) -> (&'a Expr<'a>, usize) {
-        match expr.kind {
-            ExprKind::AddrOf(_, _, expr) if count != target => f(expr, count + 1, target),
-            _ => (expr, count),
-        }
-    }
-    f(expr, 0, count)
+    let mut remaining = count;
+    let e = peel_hir_expr_while(expr, |e| match e.kind {
+        ExprKind::AddrOf(BorrowKind::Ref, _, e) if remaining != 0 => {
+            remaining -= 1;
+            Some(e)
+        },
+        _ => None,
+    });
+    (e, count - remaining)
 }
 
 /// Peels off all references on the expression. Returns the underlying expression and the number of
 /// references removed.
 pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) {
-    fn f(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
-        match expr.kind {
-            ExprKind::AddrOf(BorrowKind::Ref, _, expr) => f(expr, count + 1),
-            _ => (expr, count),
-        }
-    }
-    f(expr, 0)
+    let mut count = 0;
+    let e = peel_hir_expr_while(expr, |e| match e.kind {
+        ExprKind::AddrOf(BorrowKind::Ref, _, e) => {
+            count += 1;
+            Some(e)
+        },
+        _ => None,
+    });
+    (e, count)
 }
 
 #[macro_export]

clippy_utils/src/lib.rs

@@ -13,7 +13,9 @@ pub(super) const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_
 pub const BINARY_HEAP: [&str; 4] = ["alloc", "collections", "binary_heap", "BinaryHeap"];
 pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"];
 pub const BTREEMAP: [&str; 5] = ["alloc", "collections", "btree", "map", "BTreeMap"];
+pub const BTREEMAP_CONTAINS_KEY: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "contains_key"];
 pub const BTREEMAP_ENTRY: [&str; 6] = ["alloc", "collections", "btree", "map", "entry", "Entry"];
+pub const BTREEMAP_INSERT: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "insert"];
 pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeSet"];
 pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
 pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
@@ -46,7 +48,9 @@ pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "From
 pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"];
 pub const HASH: [&str; 3] = ["core", "hash", "Hash"];
 pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"];
+pub const HASHMAP_CONTAINS_KEY: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "contains_key"];
 pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"];
+pub const HASHMAP_INSERT: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "insert"];
 pub const HASHSET: [&str; 5] = ["std", "collections", "hash", "set", "HashSet"];
 #[cfg(feature = "internal-lints")]
 pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"];

clippy_utils/src/paths.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.

clippy_utils/src/source.rs

@@ -0,0 +1,101 @@
+// 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 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);
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        if true {
+            e.insert(v);
+        } else {
+            e.insert(v2);
+        }
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        if true {
+            e.insert(v);
+        } else {
+            e.insert(v2);
+            return;
+        }
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        foo();
+        e.insert(v);
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        match 0 {
+            1 if true => {
+                e.insert(v);
+            },
+            _ => {
+                e.insert(v2);
+            },
+        };
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        match 0 {
+            0 => {},
+            1 => {
+                e.insert(v);
+            },
+            _ => {
+                e.insert(v2);
+            },
+        };
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        foo();
+        match 0 {
+            0 if false => {
+                e.insert(v);
+            },
+            1 => {
+                foo();
+                e.insert(v);
+            },
+            2 | 3 => {
+                for _ in 0..2 {
+                    foo();
+                }
+                if true {
+                    e.insert(v);
+                } else {
+                    e.insert(v2);
+                };
+            },
+            _ => {
+                e.insert(v2);
+            },
+        }
+    }
+
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) {
+        e.insert(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() {}

tests/ui/entry.fixed

@@ -0,0 +1,103 @@
+// 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 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);
+    }
+
+    if !m.contains_key(&k) {
+        if true {
+            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);
+            return;
+        }
+    }
+
+    if !m.contains_key(&k) {
+        foo();
+        m.insert(k, v);
+    }
+
+    if !m.contains_key(&k) {
+        match 0 {
+            1 if true => {
+                m.insert(k, v);
+            },
+            _ => {
+                m.insert(k, v2);
+            },
+        };
+    }
+
+    if !m.contains_key(&k) {
+        match 0 {
+            0 => {},
+            1 => {
+                m.insert(k, v);
+            },
+            _ => {
+                m.insert(k, v2);
+            },
+        };
+    }
+
+    if !m.contains_key(&k) {
+        foo();
+        match 0 {
+            0 if false => {
+                m.insert(k, v);
+            },
+            1 => {
+                foo();
+                m.insert(k, v);
+            },
+            2 | 3 => {
+                for _ in 0..2 {
+                    foo();
+                }
+                if true {
+                    m.insert(k, v);
+                } else {
+                    m.insert(k, v2);
+                };
+            },
+            _ => {
+                m.insert(k, v2);
+            },
+        }
+    }
+
+    if !m.contains_key(&m!(k)) {
+        m.insert(m!(k), m!(v));
+    }
+}
+
+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() {}