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

clippy_lints/src/entry.rs

@@ -1,17 +1,18 @@
-use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
-use clippy_utils::ty::{is_type_diagnostic_item, match_type};
-use clippy_utils::SpanlessEq;
-use clippy_utils::{get_item_name, paths};
-use if_chain::if_chain;
+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},
+    SpanlessEq,
+};
 use rustc_errors::Applicability;
-use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
-use rustc_hir::{BorrowKind, Expr, ExprKind, UnOp};
+use rustc_hir::{
+    intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
+    Expr, ExprKind, Guard, Local, Stmt, StmtKind, UnOp,
+};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::source_map::Span;
-use rustc_span::sym;
+use rustc_span::{Span, SyntaxContext, DUMMY_SP};
+use std::fmt::Write;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for uses of `contains_key` + `insert` on `HashMap`
@@ -19,15 +20,14 @@ declare_clippy_lint! {
     ///
     /// **Why is this bad?** Using `entry` is more efficient.
     ///
-    /// **Known problems:** Some false negatives, eg.:
+    /// **Known problems:** The suggestion may have type inference errors in some cases. e.g.
     /// ```rust
-    /// # use std::collections::HashMap;
-    /// # let mut map = HashMap::new();
-    /// # let v = 1;
-    /// # let k = 1;
-    /// if !map.contains_key(&k) {
-    ///     map.insert(k.clone(), v);
-    /// }
+    /// let mut map = std::collections::HashMap::new();
+    /// let _ = if !map.contains_key(&0) {
+    ///     map.insert(0, 0)
+    /// } else {
+    ///     None
+    /// };
     /// ```
     ///
     /// **Example:**
@@ -56,132 +56,379 @@ declare_clippy_lint! {
 declare_lint_pass!(HashMapPass => [MAP_ENTRY]);
 
 impl<'tcx> LateLintPass<'tcx> for HashMapPass {
+    #[allow(clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if let ExprKind::If(check, then_block, ref else_block) = expr.kind {
-            if let ExprKind::Unary(UnOp::Not, check) = check.kind {
-                if let Some((ty, map, key)) = check_cond(cx, check) {
-                    // in case of `if !m.contains_key(&k) { m.insert(k, v); }`
-                    // we can give a better error message
-                    let sole_expr = {
-                        else_block.is_none()
-                            && if let ExprKind::Block(then_block, _) = then_block.kind {
-                                (then_block.expr.is_some() as usize) + then_block.stmts.len() == 1
-                            } else {
-                                true
-                            }
-                        // XXXManishearth we can also check for if/else blocks containing `None`.
-                    };
+        let (cond_expr, then_expr, else_expr) = match expr.kind {
+            ExprKind::If(c, t, e) => (c, t, e),
+            _ => return,
+        };
+        let (map_ty, contains_expr) = match try_parse_contains(cx, cond_expr) {
+            Some(x) => x,
+            None => return,
+        };
 
-                    let mut visitor = InsertVisitor {
-                        cx,
-                        span: expr.span,
-                        ty,
-                        map,
-                        key,
-                        sole_expr,
-                    };
+        let then_search = match find_insert_calls(cx, &contains_expr, then_expr) {
+            Some(x) => x,
+            None => return,
+        };
 
-                    walk_expr(&mut visitor, then_block);
-                }
-            } else if let Some(else_block) = *else_block {
-                if let Some((ty, map, key)) = check_cond(cx, check) {
-                    let mut visitor = InsertVisitor {
-                        cx,
-                        span: expr.span,
-                        ty,
-                        map,
-                        key,
-                        sole_expr: false,
+        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() {
+            return;
+        } else {
+            // if .. { insert }
+            match then_search.as_single_insertion() {
+                Some(insertion) if !insertion.value.can_have_side_effects() => {
+                    format!(
+                        "{}.entry({}).or_insert({});",
+                        map_str,
+                        key_str,
+                        snippet_with_context(cx, insertion.value.span, insertion.call.span.ctxt(), "..", &mut app).0,
+                    )
+                },
+                _ => {
+                    let (body_str, entry_kind) = if contains_expr.negated {
+                        (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)")
+                    } else {
+                        (
+                            then_search.snippet_occupied(cx, then_expr.span, &mut app),
+                            "Occupied(mut e)",
+                        )
                     };
-
-                    walk_expr(&mut visitor, else_block);
-                }
+                    format!(
+                        "if let {}::{} = {}.entry({}) {}",
+                        map_ty.entry_path(),
+                        entry_kind,
+                        map_str,
+                        key_str,
+                        body_str,
+                    )
+                },
             }
+        };
+
+        span_lint_and_sugg(
+            cx,
+            MAP_ENTRY,
+            expr.span,
+            &format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()),
+            "try this",
+            sugg,
+            app,
+        );
+    }
+}
+
+#[derive(Clone, Copy)]
+enum MapType {
+    Hash,
+    BTree,
+}
+impl MapType {
+    fn name(self) -> &'static str {
+        match self {
+            Self::Hash => "HashMap",
+            Self::BTree => "BTreeMap",
+        }
+    }
+    fn entry_path(self) -> &'staic str {
+        match self {
+            Self::Hash => "std::collections::hash_map::Entry",
+            Self::BTree => "std::collections::btree_map::Entry",
         }
     }
 }
 
-fn check_cond<'a>(cx: &LateContext<'_>, check: &'a Expr<'a>) -> Option<(&'static str, &'a Expr<'a>, &'a Expr<'a>)> {
-    if_chain! {
-        if let ExprKind::MethodCall(path, _, params, _) = check.kind;
-        if params.len() >= 2;
-        if path.ident.name == sym!(contains_key);
-        if let ExprKind::AddrOf(BorrowKind::Ref, _, key) = params[1].kind;
-        then {
-            let map = &params[0];
-            let obj_ty = cx.typeck_results().expr_ty(map).peel_refs();
-
-            return if match_type(cx, obj_ty, &paths::BTREEMAP) {
-                Some(("BTreeMap", map, key))
-            }
-            else if is_type_diagnostic_item(cx, obj_ty, sym::hashmap_type) {
-                Some(("HashMap", map, key))
-            }
-            else {
-                None
+struct ContainsExpr<'tcx> {
+    negated: bool,
+    map: &'tcx Expr<'tcx>,
+    key: &'tcx Expr<'tcx>,
+    call_ctxt: SyntaxContext,
+}
+fn try_parse_contains(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(MapType, ContainsExpr<'tcx>)> {
+    let mut negated = false;
+    let expr = peel_hir_expr_while(expr, |e| match e.kind {
+        ExprKind::Unary(UnOp::Not, e) => {
+            negated = !negated;
+            Some(e)
+        },
+        _ => None,
+    });
+    match expr.kind {
+        ExprKind::MethodCall(
+            _,
+            _,
+            [map, Expr {
+                kind: ExprKind::AddrOf(_, _, key),
+                span: key_span,
+                ..
+            }],
+            _,
+        ) if key_span.ctxt() == expr.span.ctxt() => {
+            let id = cx.typeck_results().type_dependent_def_id(expr.hir_id)?;
+            let expr = ContainsExpr {
+                negated,
+                map,
+                key,
+                call_ctxt: expr.span.ctxt(),
             };
-        }
-    }
-
-    None
-}
-
-struct InsertVisitor<'a, 'tcx, 'b> {
-    cx: &'a LateContext<'tcx>,
-    span: Span,
-    ty: &'static str,
-    map: &'b Expr<'b>,
-    key: &'b Expr<'b>,
-    sole_expr: bool,
-}
-
-impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> {
-    type Map = Map<'tcx>;
-
-    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-        if_chain! {
-            if let ExprKind::MethodCall(path, _, params, _) = expr.kind;
-            if params.len() == 3;
-            if path.ident.name == sym!(insert);
-            if get_item_name(self.cx, self.map) == get_item_name(self.cx, &params[0]);
-            if SpanlessEq::new(self.cx).eq_expr(self.key, &params[1]);
-            if snippet_opt(self.cx, self.map.span) == snippet_opt(self.cx, params[0].span);
-            then {
-                span_lint_and_then(self.cx, MAP_ENTRY, self.span,
-                                   &format!("usage of `contains_key` followed by `insert` on a `{}`", self.ty), |diag| {
-                    if self.sole_expr {
-                        let mut app = Applicability::MachineApplicable;
-                        let help = format!("{}.entry({}).or_insert({});",
-                                           snippet_with_applicability(self.cx, self.map.span, "map", &mut app),
-                                           snippet_with_applicability(self.cx, params[1].span, "..", &mut app),
-                                           snippet_with_applicability(self.cx, params[2].span, "..", &mut app));
-
-                        diag.span_suggestion(
-                            self.span,
-                            "consider using",
-                            help,
-                            Applicability::MachineApplicable, // snippet
-                        );
-                    }
-                    else {
-                        let help = format!("consider using `{}.entry({})`",
-                                           snippet(self.cx, self.map.span, "map"),
-                                           snippet(self.cx, params[1].span, ".."));
-
-                        diag.span_label(
-                            self.span,
-                            &help,
-                        );
-                    }
-                });
+            if match_def_path(cx, id, &paths::BTREEMAP_CONTAINS_KEY) {
+                Some((MapType::BTree, expr))
+            } else if match_def_path(cx, id, &paths::HASHMAP_CONTAINS_KEY) {
+                Some((MapType::Hash, expr))
+            } else {
+                None
             }
-        }
-
-        if !self.sole_expr {
-            walk_expr(self, expr);
-        }
+        },
+        _ => None,
     }
+}
+
+struct InsertExpr<'tcx> {
+    map: &'tcx Expr<'tcx>,
+    key: &'tcx Expr<'tcx>,
+    value: &'tcx Expr<'tcx>,
+}
+fn try_parse_insert(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 match_def_path(cx, id, &paths::BTREEMAP_INSERT) || match_def_path(cx, id, &paths::HASHMAP_INSERT) {
+            Some(InsertExpr { map, key, value })
+        } else {
+            None
+        }
+    } else {
+        None
+    }
+}
+
+#[derive(Clone, Copy)]
+struct Insertion<'tcx> {
+    call: &'tcx Expr<'tcx>,
+    value: &'tcx Expr<'tcx>,
+}
+
+// This visitor needs to do a multiple things:
+// * Find all usages of the map. Only insertions into the map which share the same key are
+//   permitted. All others will prevent the lint.
+// * Determine if the final statement executed is an insertion. This is needed to use `insert_with`.
+// * Determine if there's any sub-expression that can't be placed in a closure.
+// * Determine if there's only a single insert statement. This is needed to give better suggestions.
+
+#[allow(clippy::struct_excessive_bools)]
+struct InsertSearcher<'cx, 'i, 'tcx> {
+    cx: &'cx LateContext<'tcx>,
+    /// The map expression used in the contains call.
+    map: &'tcx Expr<'tcx>,
+    /// The key expression used in the contains call.
+    key: &'tcx Expr<'tcx>,
+    /// The context of the top level block. All insert calls must be in the same context.
+    ctxt: SyntaxContext,
+    /// Whether this expression can use the entry api.
+    can_use_entry: bool,
+    // A single insert expression has a slightly different suggestion.
+    is_single_insert: bool,
+    is_map_used: bool,
+    insertions: &'i mut Vec<Insertion<'tcx>>,
+}
+impl<'tcx> InsertSearcher<'_, '_, 'tcx> {
+    /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but
+    /// only if they are on separate code paths. This will return whether the map was used in the
+    /// given expression.
+    fn visit_cond_arm(&mut self, e: &'tcx Expr<'_>) -> bool {
+        let is_map_used = self.is_map_used;
+        self.visit_expr(e);
+        let res = self.is_map_used;
+        self.is_map_used = is_map_used;
+        res
+    }
+}
+impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
+    type Map = ErasedMap<'tcx>;
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
         NestedVisitorMap::None
     }
+
+    fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) {
+        match stmt.kind {
+            StmtKind::Semi(e) | StmtKind::Expr(e) => self.visit_expr(e),
+            StmtKind::Local(Local { init: Some(e), .. }) => {
+                self.is_single_insert = false;
+                self.visit_expr(e);
+            },
+            _ => {
+                self.is_single_insert = false;
+            },
+        }
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        if !self.can_use_entry {
+            return;
+        }
+
+        match try_parse_insert(self.cx, expr) {
+            Some(insert_expr) if SpanlessEq::new(self.cx).eq_expr(self.map, insert_expr.map) => {
+                // Multiple inserts, inserts with a different key, and inserts from a macro can't use the entry api.
+                if self.is_map_used
+                    || !SpanlessEq::new(self.cx).eq_expr(self.key, insert_expr.key)
+                    || expr.span.ctxt() != self.ctxt
+                {
+                    self.can_use_entry = false;
+                    return;
+                }
+
+                self.insertions.push(Insertion {
+                    call: expr,
+                    value: insert_expr.value,
+                });
+                self.is_map_used = true;
+
+                // The value doesn't affect whether there is only a single insert expression.
+                let is_single_insert = self.is_single_insert;
+                self.visit_expr(insert_expr.value);
+                self.is_single_insert = is_single_insert;
+            },
+            _ if SpanlessEq::new(self.cx).eq_expr(self.map, expr) => {
+                self.is_map_used = true;
+            },
+            _ => match expr.kind {
+                ExprKind::If(cond_expr, then_expr, Some(else_expr)) => {
+                    self.is_single_insert = false;
+                    self.visit_expr(cond_expr);
+                    // Each branch may contain it's 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_expr(scrutinee_expr);
+                    // Each branch may contain it's own insert expression.
+                    let mut is_map_used = self.is_map_used;
+                    for arm in arms {
+                        if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard {
+                            self.visit_expr(guard)
+                        }
+                        is_map_used |= self.visit_cond_arm(arm.body);
+                    }
+                    self.is_map_used = is_map_used;
+                },
+                ExprKind::Loop(block, ..) => {
+                    // Don't allow insertions inside of a loop.
+                    let insertions_len = self.insertions.len();
+                    self.visit_block(block);
+                    if self.insertions.len() != insertions_len {
+                        self.can_use_entry = false;
+                    }
+                },
+                ExprKind::Block(block, _) => self.visit_block(block),
+                ExprKind::InlineAsm(_) | ExprKind::LlvmInlineAsm(_) => {
+                    self.can_use_entry = false;
+                },
+                _ => {
+                    self.is_single_insert = false;
+                    walk_expr(self, expr);
+                },
+            },
+        }
+    }
+}
+
+struct InsertSearchResults<'tcx> {
+    insertions: Vec<Insertion<'tcx>>,
+    is_single_insert: bool,
+}
+impl InsertSearchResults<'tcx> {
+    fn as_single_insertion(&self) -> Option<Insertion<'tcx>> {
+        self.is_single_insert.then(|| self.insertions[0])
+    }
+
+    fn snippet_occupied(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
+        let ctxt = span.ctxt();
+        let mut res = String::new();
+        for insertion in self.insertions.iter() {
+            res.push_str(&snippet_with_applicability(
+                cx,
+                span.until(insertion.call.span),
+                "..",
+                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("))");
+            } else {
+                res.push_str("e.insert(");
+                res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
+                res.push(')');
+            }
+            span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP);
+        }
+        res.push_str(&snippet_with_applicability(cx, span, "..", app));
+        res
+    }
+
+    fn snippet_vacant(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
+        let ctxt = span.ctxt();
+        let mut res = String::new();
+        for insertion in self.insertions.iter() {
+            res.push_str(&snippet_with_applicability(
+                cx,
+                span.until(insertion.call.span),
+                "..",
+                app,
+            ));
+            if is_expr_used_or_unified(cx.tcx, insertion.call) {
+                if is_expr_final_block_expr(cx.tcx, insertion.call) {
+                    let _ = 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(""),
+                    );
+                } else {
+                    let _ = write!(
+                        res,
+                        "{{ e.insert({}); None }}",
+                        snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0,
+                    );
+                }
+            } else {
+                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);
+        }
+        res.push_str(&snippet_with_applicability(cx, span, "..", app));
+        res
+    }
+}
+fn find_insert_calls(
+    cx: &LateContext<'tcx>,
+    contains_expr: &ContainsExpr<'tcx>,
+    expr: &'tcx Expr<'_>,
+) -> Option<InsertSearchResults<'tcx>> {
+    let mut insertions = Vec::new();
+    let mut s = InsertSearcher {
+        cx,
+        map: contains_expr.map,
+        key: contains_expr.key,
+        ctxt: expr.span.ctxt(),
+        insertions: &mut insertions,
+        is_map_used: false,
+        can_use_entry: true,
+        is_single_insert: true,
+    };
+    s.visit_expr(expr);
+    let is_single_insert = s.is_single_insert;
+    s.can_use_entry.then(|| InsertSearchResults {
+        insertions,
+        is_single_insert,
+    })
 }

clippy_utils/src/lib.rs

@@ -63,9 +63,9 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::LangItem::{ResultErr, ResultOk};
 use rustc_hir::{
-    def, Arm, BindingAnnotation, Block, Body, Constness, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, ImplItem,
-    ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath,
-    TraitItem, TraitItemKind, TraitRef, TyKind,
+    def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
+    ImplItem, ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath,
+    Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -1245,6 +1245,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 {
@@ -1414,28 +1490,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/paths.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"];
@@ -45,7 +47,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/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.

tests/ui/entry.fixed

@@ -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.rs

@@ -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() {}

tests/ui/entry.stderr

@@ -0,0 +1,171 @@
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:16:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |     }
+   | |_____^ help: try this: `m.entry(k).or_insert(v);`
+   |
+   = 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
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         if true {
+LL | |             m.insert(k, v);
+LL | |         } else {
+LL | |             m.insert(k, v2);
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         if true {
+LL |             e.insert(v);
+LL |         } else {
+LL |             e.insert(v2);
+LL |         }
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:28:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         if true {
+LL | |             m.insert(k, v);
+LL | |         } else {
+...  |
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         if true {
+LL |             e.insert(v);
+LL |         } else {
+LL |             e.insert(v2);
+LL |             return;
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:37:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |         m.insert(k, v);
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         foo();
+LL |         e.insert(v);
+LL |     }
+   |
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:42:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         match 0 {
+LL | |             1 if true => {
+LL | |                 m.insert(k, v);
+...  |
+LL | |         };
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         match 0 {
+LL |             1 if true => {
+LL |                 e.insert(v);
+LL |             },
+LL |             _ => {
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:53:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         match 0 {
+LL | |             0 => {},
+LL | |             1 => {
+...  |
+LL | |         };
+LL | |     }
+   | |_____^
+   |
+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 |             },
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:65:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         foo();
+LL | |         match 0 {
+LL | |             0 if false => {
+...  |
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |         foo();
+LL |         match 0 {
+LL |             0 if false => {
+LL |                 e.insert(v);
+LL |             },
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:91:5
+   |
+LL | /     if !m.contains_key(&m!(k)) {
+LL | |         m.insert(m!(k), m!(v));
+LL | |     }
+   | |_____^
+   |
+help: try this
+   |
+LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) {
+LL |         e.insert(m!(v));
+LL |     }
+   |
+
+error: usage of `contains_key` followed by `insert` on a `BTreeMap`
+  --> $DIR/entry.rs:97: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 9 previous errors
+

tests/ui/entry_fixable.fixed

@@ -1,15 +0,0 @@
-// run-rustfix
-
-#![allow(unused, clippy::needless_pass_by_value)]
-#![warn(clippy::map_entry)]
-
-use std::collections::{BTreeMap, HashMap};
-use std::hash::Hash;
-
-fn foo() {}
-
-fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    m.entry(k).or_insert(v);
-}
-
-fn main() {}

tests/ui/entry_fixable.rs

@@ -1,17 +0,0 @@
-// run-rustfix
-
-#![allow(unused, clippy::needless_pass_by_value)]
-#![warn(clippy::map_entry)]
-
-use std::collections::{BTreeMap, HashMap};
-use std::hash::Hash;
-
-fn foo() {}
-
-fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
-    if !m.contains_key(&k) {
-        m.insert(k, v);
-    }
-}
-
-fn main() {}

tests/ui/entry_fixable.stderr

@@ -1,12 +0,0 @@
-error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry_fixable.rs:12:5
-   |
-LL | /     if !m.contains_key(&k) {
-LL | |         m.insert(k, v);
-LL | |     }
-   | |_____^ help: consider using: `m.entry(k).or_insert(v);`
-   |
-   = note: `-D clippy::map-entry` implied by `-D warnings`
-
-error: aborting due to previous error
-

tests/ui/entry_unfixable.rs

@@ -4,51 +4,15 @@
 use std::collections::{BTreeMap, HashMap};
 use std::hash::Hash;
 
+macro_rules! m {
+    ($map:expr, $key:expr, $value:expr) => {
+        $map.insert($key, $value)
+    };
+    ($e:expr) => {{ $e }};
+}
+
 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) {
@@ -70,4 +34,17 @@ fn insert_from_different_map2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, n: &mut Ha
     }
 }
 
+fn insert_in_macro<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
+    if !m.contains_key(&k) {
+        m!(m, k, v);
+    }
+}
+
+fn use_map_then_insert<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
+    if !m.contains_key(&k) {
+        let _ = m.len();
+        m.insert(k, v);
+    }
+}
+
 fn main() {}

tests/ui/entry_unfixable.stderr

@@ -1,57 +0,0 @@
-error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry_unfixable.rs:10:5
-   |
-LL | /     if !m.contains_key(&k) {
-LL | |         m.insert(k, v)
-LL | |     } else {
-LL | |         None
-LL | |     };
-   | |_____^ consider using `m.entry(k)`
-   |
-   = note: `-D clippy::map-entry` implied by `-D warnings`
-
-error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry_unfixable.rs:18:5
-   |
-LL | /     if m.contains_key(&k) {
-LL | |         None
-LL | |     } else {
-LL | |         m.insert(k, v)
-LL | |     };
-   | |_____^ consider using `m.entry(k)`
-
-error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry_unfixable.rs:26:5
-   |
-LL | /     if !m.contains_key(&k) {
-LL | |         foo();
-LL | |         m.insert(k, v)
-LL | |     } else {
-LL | |         None
-LL | |     };
-   | |_____^ consider using `m.entry(k)`
-
-error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry_unfixable.rs:35:5
-   |
-LL | /     if m.contains_key(&k) {
-LL | |         None
-LL | |     } else {
-LL | |         foo();
-LL | |         m.insert(k, v)
-LL | |     };
-   | |_____^ consider using `m.entry(k)`
-
-error: usage of `contains_key` followed by `insert` on a `BTreeMap`
-  --> $DIR/entry_unfixable.rs:44:5
-   |
-LL | /     if !m.contains_key(&k) {
-LL | |         foo();
-LL | |         m.insert(k, v)
-LL | |     } else {
-LL | |         None
-LL | |     };
-   | |_____^ consider using `m.entry(k)`
-
-error: aborting due to 5 previous errors
-