`map_entry` improvements

Suggest using `or_insert_with` when possible

6 files changed, 322 insertions, 147 deletions

diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs
index 3e1d70b2e40..0decb22a491 100644
--- a/clippy_lints/src/entry.rs
+++ b/clippy_lints/src/entry.rs
@@ -1,4 +1,5 @@
 use clippy_utils::{
+    can_move_expr_to_closure_no_visit,
     diagnostics::span_lint_and_sugg,
     is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while,
     source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context},
@@ -7,7 +8,7 @@ use clippy_utils::{
 use rustc_errors::Applicability;
 use rustc_hir::{
     intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
-    Expr, ExprKind, Guard, Local, Stmt, StmtKind, UnOp,
+    Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -78,13 +79,13 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
         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,
+                Some(search) if !(then_search.edits.is_empty() && search.edits.is_empty()) => search,
                 _ => return,
             };
 
-            if then_search.insertions.is_empty() || else_search.insertions.is_empty() {
+            if then_search.edits.is_empty() || else_search.edits.is_empty() {
                 // if .. { insert } else { .. } or if .. { .. } else { then } of
-                let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() {
+                let (then_str, else_str, entry_kind) = if else_search.edits.is_empty() {
                     if contains_expr.negated {
                         (
                             then_search.snippet_vacant(cx, then_expr.span, &mut app),
@@ -152,37 +153,48 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass {
                     indent = indent_str,
                 )
             }
-        } else if then_search.insertions.is_empty() || !contains_expr.negated {
+        } else if then_search.edits.is_empty() {
+            // no insertions
             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,
+            if !then_search.allow_insert_closure {
+                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)",
                     )
-                },
-                _ => {
-                    let (body_str, entry_kind) = if contains_expr.negated {
-                        (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)")
+                };
+                format!(
+                    "if let {}::{} = {}.entry({}) {}",
+                    map_ty.entry_path(),
+                    entry_kind,
+                    map_str,
+                    key_str,
+                    body_str,
+                )
+            } else if let Some(insertion) = then_search.as_single_insertion() {
+                let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "..", &mut app).0;
+                if contains_expr.negated {
+                    if insertion.value.can_have_side_effects() {
+                        format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, value_str)
                     } else {
-                        (
-                            then_search.snippet_occupied(cx, then_expr.span, &mut app),
-                            "Occupied(mut e)",
-                        )
-                    };
-                    format!(
-                        "if let {}::{} = {}.entry({}) {}",
-                        map_ty.entry_path(),
-                        entry_kind,
-                        map_str,
-                        key_str,
-                        body_str,
-                    )
-                },
+                        format!("{}.entry({}).or_insert({});", map_str, key_str, value_str)
+                    }
+                } else {
+                    // Todo: if let Some(v) = map.get_mut(k)
+                    return;
+                }
+            } else {
+                let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app);
+                if contains_expr.negated {
+                    format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, block_str)
+                } else {
+                    // Todo: if let Some(v) = map.get_mut(k)
+                    return;
+                }
             }
         };
 
@@ -281,6 +293,19 @@ fn try_parse_insert(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<Inse
     }
 }
 
+/// An edit that will need to be made to move the expression to use the entry api
+#[derive(Clone, Copy)]
+enum Edit<'tcx> {
+    /// A semicolon that needs to be removed. Used to create a closure for `insert_with`.
+    RemoveSemi(Span),
+    /// An insertion into the map.
+    Insertion(Insertion<'tcx>),
+}
+impl Edit<'tcx> {
+    fn as_insertion(self) -> Option<Insertion<'tcx>> {
+        if let Self::Insertion(i) = self { Some(i) } else { None }
+    }
+}
 #[derive(Clone, Copy)]
 struct Insertion<'tcx> {
     call: &'tcx Expr<'tcx>,
@@ -303,12 +328,17 @@ struct InsertSearcher<'cx, 'i, 'tcx> {
     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 be safely moved into a closure.
+    allow_insert_closure: bool,
     /// Whether this expression can use the entry api.
     can_use_entry: bool,
+    /// Whether this expression is the final expression in this code path. This may be a statement.
+    in_tail_pos: bool,
     // A single insert expression has a slightly different suggestion.
     is_single_insert: bool,
     is_map_used: bool,
-    insertions: &'i mut Vec<Insertion<'tcx>>,
+    edits: &'i mut Vec<Edit<'tcx>>,
+    loops: Vec<HirId>,
 }
 impl<'tcx> InsertSearcher<'_, '_, 'tcx> {
     /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but
@@ -316,11 +346,22 @@ impl<'tcx> InsertSearcher<'_, '_, 'tcx> {
     /// given expression.
     fn visit_cond_arm(&mut self, e: &'tcx Expr<'_>) -> bool {
         let is_map_used = self.is_map_used;
+        let in_tail_pos = self.in_tail_pos;
         self.visit_expr(e);
         let res = self.is_map_used;
         self.is_map_used = is_map_used;
+        self.in_tail_pos = in_tail_pos;
         res
     }
+
+    /// Visits an expression which is not itself in a tail position, but other sibling expressions
+    /// may be. e.g. if conditions
+    fn visit_non_tail_expr(&mut self, e: &'tcx Expr<'_>) {
+        let in_tail_pos = self.in_tail_pos;
+        self.in_tail_pos = false;
+        self.visit_expr(e);
+        self.in_tail_pos = in_tail_pos;
+    }
 }
 impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
     type Map = ErasedMap<'tcx>;
@@ -330,17 +371,63 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
 
     fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) {
         match stmt.kind {
-            StmtKind::Semi(e) | StmtKind::Expr(e) => self.visit_expr(e),
+            StmtKind::Semi(e) => {
+                self.visit_expr(e);
+
+                if self.in_tail_pos && self.allow_insert_closure {
+                    // The spans are used to slice the top level expression into multiple parts. This requires that
+                    // they all come from the same part of the source code.
+                    if stmt.span.ctxt() == self.ctxt && e.span.ctxt() == self.ctxt {
+                        self.edits
+                            .push(Edit::RemoveSemi(stmt.span.trim_start(e.span).unwrap_or(DUMMY_SP)));
+                    } else {
+                        self.allow_insert_closure = false;
+                    }
+                }
+            },
+            StmtKind::Expr(e) => self.visit_expr(e),
             StmtKind::Local(Local { init: Some(e), .. }) => {
+                self.allow_insert_closure &= !self.in_tail_pos;
+                self.in_tail_pos = false;
                 self.is_single_insert = false;
                 self.visit_expr(e);
             },
             _ => {
+                self.allow_insert_closure &= !self.in_tail_pos;
                 self.is_single_insert = false;
             },
         }
     }
 
+    fn visit_block(&mut self, block: &'tcx Block<'_>) {
+        // If the block is in a tail position, then the last expression (possibly a statement) is in the
+        // tail position. The rest, however, are not.
+        match (block.stmts, block.expr) {
+            ([], None) => {
+                self.allow_insert_closure &= !self.in_tail_pos;
+            },
+            ([], Some(expr)) => self.visit_expr(expr),
+            (stmts, Some(expr)) => {
+                let in_tail_pos = self.in_tail_pos;
+                self.in_tail_pos = false;
+                for stmt in stmts {
+                    self.visit_stmt(stmt);
+                }
+                self.in_tail_pos = in_tail_pos;
+                self.visit_expr(expr);
+            },
+            ([stmts @ .., stmt], None) => {
+                let in_tail_pos = self.in_tail_pos;
+                self.in_tail_pos = false;
+                for stmt in stmts {
+                    self.visit_stmt(stmt);
+                }
+                self.in_tail_pos = in_tail_pos;
+                self.visit_stmt(stmt);
+            },
+        }
+    }
+
     fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
         if !self.can_use_entry {
             return;
@@ -357,15 +444,16 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
                     return;
                 }
 
-                self.insertions.push(Insertion {
+                self.edits.push(Edit::Insertion(Insertion {
                     call: expr,
                     value: insert_expr.value,
-                });
+                }));
                 self.is_map_used = true;
+                self.allow_insert_closure &= self.in_tail_pos;
 
                 // 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.visit_non_tail_expr(insert_expr.value);
                 self.is_single_insert = is_single_insert;
             },
             _ if SpanlessEq::new(self.cx).eq_expr(self.map, expr) => {
@@ -374,7 +462,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
             _ => match expr.kind {
                 ExprKind::If(cond_expr, then_expr, Some(else_expr)) => {
                     self.is_single_insert = false;
-                    self.visit_expr(cond_expr);
+                    self.visit_non_tail_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);
@@ -382,31 +470,38 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
                 },
                 ExprKind::Match(scrutinee_expr, arms, _) => {
                     self.is_single_insert = false;
-                    self.visit_expr(scrutinee_expr);
+                    self.visit_non_tail_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)
+                            self.visit_non_tail_expr(guard)
                         }
                         is_map_used |= self.visit_cond_arm(arm.body);
                     }
                     self.is_map_used = is_map_used;
                 },
                 ExprKind::Loop(block, ..) => {
+                    self.loops.push(expr.hir_id);
+                    self.allow_insert_closure &= !self.in_tail_pos;
                     // Don't allow insertions inside of a loop.
-                    let insertions_len = self.insertions.len();
+                    let edit_len = self.edits.len();
                     self.visit_block(block);
-                    if self.insertions.len() != insertions_len {
+                    if self.edits.len() != edit_len {
                         self.can_use_entry = false;
                     }
+                    self.loops.pop();
                 },
                 ExprKind::Block(block, _) => self.visit_block(block),
                 ExprKind::InlineAsm(_) | ExprKind::LlvmInlineAsm(_) => {
                     self.can_use_entry = false;
                 },
                 _ => {
+                    self.allow_insert_closure &= !self.in_tail_pos;
+                    self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops);
+                    // Sub expressions are no longer in the tail position.
                     self.is_single_insert = false;
+                    self.in_tail_pos = false;
                     walk_expr(self, expr);
                 },
             },
@@ -415,18 +510,19 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> {
 }
 
 struct InsertSearchResults<'tcx> {
-    insertions: Vec<Insertion<'tcx>>,
+    edits: Vec<Edit<'tcx>>,
+    allow_insert_closure: bool,
     is_single_insert: bool,
 }
 impl InsertSearchResults<'tcx> {
     fn as_single_insertion(&self) -> Option<Insertion<'tcx>> {
-        self.is_single_insert.then(|| self.insertions[0])
+        self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap())
     }
 
     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() {
+        for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) {
             res.push_str(&snippet_with_applicability(
                 cx,
                 span.until(insertion.call.span),
@@ -451,7 +547,7 @@ impl InsertSearchResults<'tcx> {
     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() {
+        for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) {
             res.push_str(&snippet_with_applicability(
                 cx,
                 span.until(insertion.call.span),
@@ -485,27 +581,57 @@ impl InsertSearchResults<'tcx> {
         res.push_str(&snippet_with_applicability(cx, span, "..", app));
         res
     }
+
+    fn snippet_closure(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String {
+        let ctxt = span.ctxt();
+        let mut res = String::new();
+        for edit in &self.edits {
+            match *edit {
+                Edit::Insertion(insertion) => {
+                    res.push_str(&snippet_with_applicability(
+                        cx,
+                        span.until(insertion.call.span),
+                        "..",
+                        app,
+                    ));
+                    res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0);
+                    span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP);
+                },
+                Edit::RemoveSemi(semi_span) => {
+                    res.push_str(&snippet_with_applicability(cx, span.until(semi_span), "..", app));
+                    span = span.trim_start(semi_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 edits = Vec::new();
     let mut s = InsertSearcher {
         cx,
         map: contains_expr.map,
         key: contains_expr.key,
         ctxt: expr.span.ctxt(),
-        insertions: &mut insertions,
+        edits: &mut edits,
         is_map_used: false,
+        allow_insert_closure: true,
         can_use_entry: true,
+        in_tail_pos: true,
         is_single_insert: true,
+        loops: Vec::new(),
     };
     s.visit_expr(expr);
+    let allow_insert_closure = s.allow_insert_closure;
     let is_single_insert = s.is_single_insert;
     s.can_use_entry.then(|| InsertSearchResults {
-        insertions,
+        edits,
+        allow_insert_closure,
         is_single_insert,
     })
 }
diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs
index 29be0739977..3f8b976fcac 100644
--- a/clippy_lints/src/manual_map.rs
+++ b/clippy_lints/src/manual_map.rs
@@ -1,15 +1,13 @@
 use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
-use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
-use clippy_utils::{in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, peel_hir_expr_refs};
+use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
+use clippy_utils::{can_move_expr_to_closure, in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, peel_hir_expr_refs};
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, OptionSome};
 use rustc_hir::{
-    def::Res,
-    intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
-    Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, Path, QPath,
+    Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, QPath,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
@@ -193,51 +191,6 @@ impl LateLintPass<'_> for ManualMap {
     }
 }
 
-// Checks if the expression can be moved into a closure as is.
-fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
-    struct V<'cx, 'tcx> {
-        cx: &'cx LateContext<'tcx>,
-        make_closure: bool,
-    }
-    impl Visitor<'tcx> for V<'_, 'tcx> {
-        type Map = ErasedMap<'tcx>;
-        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-            NestedVisitorMap::None
-        }
-
-        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
-            match e.kind {
-                ExprKind::Break(..)
-                | ExprKind::Continue(_)
-                | ExprKind::Ret(_)
-                | ExprKind::Yield(..)
-                | ExprKind::InlineAsm(_)
-                | ExprKind::LlvmInlineAsm(_) => {
-                    self.make_closure = false;
-                },
-                // Accessing a field of a local value can only be done if the type isn't
-                // partially moved.
-                ExprKind::Field(base_expr, _)
-                    if matches!(
-                        base_expr.kind,
-                        ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
-                    ) && can_partially_move_ty(self.cx, self.cx.typeck_results().expr_ty(base_expr)) =>
-                {
-                    // TODO: check if the local has been partially moved. Assume it has for now.
-                    self.make_closure = false;
-                    return;
-                }
-                _ => (),
-            };
-            walk_expr(self, e);
-        }
-    }
-
-    let mut v = V { cx, make_closure: true };
-    v.visit_expr(expr);
-    v.make_closure
-}
-
 // Checks whether the expression could be passed as a function, or whether a closure is needed.
 // Returns the function to be passed to `map` if it exists.
 fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 0abee49f40c..5d6c1337be6 100644
--- a/clippy_utils/src/lib.rs
+++ b/clippy_utils/src/lib.rs
@@ -60,7 +60,7 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
-use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
 use rustc_hir::LangItem::{ResultErr, ResultOk};
 use rustc_hir::{
     def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
@@ -82,7 +82,7 @@ use rustc_span::{Span, DUMMY_SP};
 use rustc_target::abi::Integer;
 
 use crate::consts::{constant, Constant};
-use crate::ty::is_recursively_primitive_type;
+use crate::ty::{can_partially_move_ty, is_recursively_primitive_type};
 
 pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option<Span>) -> Option<RustcVersion> {
     if let Ok(version) = RustcVersion::parse(msrv) {
@@ -539,6 +539,73 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
     None
 }
 
+/// Checks if the top level expression can be moved into a closure as is.
+pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool {
+    match expr.kind {
+        ExprKind::Break(Destination { target_id: Ok(id), .. }, _)
+        | ExprKind::Continue(Destination { target_id: Ok(id), .. })
+            if jump_targets.contains(&id) =>
+        {
+            true
+        },
+        ExprKind::Break(..)
+        | ExprKind::Continue(_)
+        | ExprKind::Ret(_)
+        | ExprKind::Yield(..)
+        | ExprKind::InlineAsm(_)
+        | ExprKind::LlvmInlineAsm(_) => false,
+        // Accessing a field of a local value can only be done if the type isn't
+        // partially moved.
+        ExprKind::Field(base_expr, _)
+            if matches!(
+                base_expr.kind,
+                ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
+            ) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) =>
+        {
+            // TODO: check if the local has been partially moved. Assume it has for now.
+            false
+        }
+        _ => true,
+    }
+}
+
+/// Checks if the expression can be moved into a closure as is.
+pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+    struct V<'cx, 'tcx> {
+        cx: &'cx LateContext<'tcx>,
+        loops: Vec<HirId>,
+        allow_closure: bool,
+    }
+    impl Visitor<'tcx> for V<'_, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if !self.allow_closure {
+                return;
+            }
+            if let ExprKind::Loop(b, ..) = e.kind {
+                self.loops.push(e.hir_id);
+                self.visit_block(b);
+                self.loops.pop();
+            } else {
+                self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops);
+                walk_expr(self, e);
+            }
+        }
+    }
+
+    let mut v = V {
+        cx,
+        allow_closure: true,
+        loops: Vec::new(),
+    };
+    v.visit_expr(expr);
+    v.allow_closure
+}
+
 /// Returns the method names and argument list of nested method call expressions that make up
 /// `expr`. method/span lists are sorted with the most recent call first.
 pub fn method_calls<'tcx>(
diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed
index 60371c9833c..524f2132797 100644
--- a/tests/ui/entry.fixed
+++ b/tests/ui/entry.fixed
@@ -15,13 +15,21 @@ 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) {
+    m.entry(k).or_insert_with(|| {
         if true {
-            e.insert(v);
+            v
         } else {
-            e.insert(v2);
+            v2
         }
-    }
+    });
+
+    m.entry(k).or_insert_with(|| {
+        if true {
+            v
+        } else {
+            v2
+        }
+    });
 
     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
         if true {
@@ -32,21 +40,21 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         }
     }
 
-    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+    m.entry(k).or_insert_with(|| {
         foo();
-        e.insert(v);
-    }
+        v
+    });
 
-    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+    m.entry(k).or_insert_with(|| {
         match 0 {
             1 if true => {
-                e.insert(v);
+                v
             },
             _ => {
-                e.insert(v2);
+                v2
             },
-        };
-    }
+        }
+    });
 
     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
         match 0 {
@@ -60,35 +68,33 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
         };
     }
 
-    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+    m.entry(k).or_insert_with(|| {
         foo();
         match 0 {
             0 if false => {
-                e.insert(v);
+                v
             },
             1 => {
                 foo();
-                e.insert(v);
+                v
             },
             2 | 3 => {
                 for _ in 0..2 {
                     foo();
                 }
                 if true {
-                    e.insert(v);
+                    v
                 } else {
-                    e.insert(v2);
-                };
+                    v2
+                }
             },
             _ => {
-                e.insert(v2);
+                v2
             },
         }
-    }
+    });
 
-    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) {
-        e.insert(m!(v));
-    }
+    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) {
diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs
index 4d3e241de78..ff4890eeeb6 100644
--- a/tests/ui/entry.rs
+++ b/tests/ui/entry.rs
@@ -27,6 +27,14 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K, v: V, v2:
 
     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);
diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr
index ab108ed6861..63b5f5a0b2c 100644
--- a/tests/ui/entry.stderr
+++ b/tests/ui/entry.stderr
@@ -22,11 +22,11 @@ LL | |     }
    |
 help: try this
    |
-LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |     m.entry(k).or_insert_with(|| {
 LL |         if true {
-LL |             e.insert(v);
+LL |             v
 LL |         } else {
-LL |             e.insert(v2);
+LL |             v2
 LL |         }
  ...
 
@@ -35,6 +35,28 @@ error: usage of `contains_key` followed by `insert` on a `HashMap`
    |
 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 |     m.entry(k).or_insert_with(|| {
+LL |         if true {
+LL |             v
+LL |         } else {
+LL |             v2
+LL |         }
+ ...
+
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:36:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         if true {
 LL | |             m.insert(k, v);
 LL | |         } else {
 ...  |
@@ -53,7 +75,7 @@ LL |             return;
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:37:5
+  --> $DIR/entry.rs:45:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         foo();
@@ -63,14 +85,14 @@ LL | |     }
    |
 help: try this
    |
-LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |     m.entry(k).or_insert_with(|| {
 LL |         foo();
-LL |         e.insert(v);
-LL |     }
+LL |         v
+LL |     });
    |
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:42:5
+  --> $DIR/entry.rs:50:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         match 0 {
@@ -83,16 +105,16 @@ LL | |     }
    |
 help: try this
    |
-LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |     m.entry(k).or_insert_with(|| {
 LL |         match 0 {
 LL |             1 if true => {
-LL |                 e.insert(v);
+LL |                 v
 LL |             },
 LL |             _ => {
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:53:5
+  --> $DIR/entry.rs:61:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         match 0 {
@@ -114,7 +136,7 @@ LL |             },
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:65:5
+  --> $DIR/entry.rs:73:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         foo();
@@ -127,31 +149,24 @@ LL | |     }
    |
 help: try this
    |
-LL |     if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+LL |     m.entry(k).or_insert_with(|| {
 LL |         foo();
 LL |         match 0 {
 LL |             0 if false => {
-LL |                 e.insert(v);
+LL |                 v
 LL |             },
  ...
 
 error: usage of `contains_key` followed by `insert` on a `HashMap`
-  --> $DIR/entry.rs:91:5
+  --> $DIR/entry.rs:99: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 |     }
-   |
+   | |_____^ 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:97:5
+  --> $DIR/entry.rs:105:5
    |
 LL | /     if !m.contains_key(&k) {
 LL | |         m.insert(k, v);
@@ -167,5 +182,5 @@ LL |         foo();
 LL |     }
    |
 
-error: aborting due to 9 previous errors
+error: aborting due to 10 previous errors