Auto merge of #7531 - Jarcho:manual_map_7413, r=camsteffen

Manual map 7413

fixes: #7413

This only fixes the specific problem from #7413, not the general case. The full fix requires interacting with the borrow checker to determine the lifetime of all the borrows made in the function. I'll open an issue about it later.

changelog: Don't suggest using `map` when the option is borrowed in the match, and also consumed in the arm.
changelog: Locals declared within the would-be closure will not prevent the closure from being suggested in `manual_map` and `map_entry`

12 files changed, 519 insertions, 60 deletions

diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs
index e1d0d65edb1..7fb8e427660 100644
--- a/clippy_lints/src/entry.rs
+++ b/clippy_lints/src/entry.rs
@@ -7,8 +7,9 @@ use clippy_utils::{
 };
 use rustc_errors::Applicability;
 use rustc_hir::{
+    hir_id::HirIdSet,
     intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
-    Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp,
+    Block, Expr, ExprKind, Guard, HirId, Pat, Stmt, StmtKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -336,6 +337,8 @@ struct InsertSearcher<'cx, 'tcx> {
     edits: Vec<Edit<'tcx>>,
     /// A stack of loops the visitor is currently in.
     loops: Vec<HirId>,
+    /// Local variables created in the expression. These don't need to be captured.
+    locals: HirIdSet,
 }
 impl<'tcx> InsertSearcher<'_, 'tcx> {
     /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but
@@ -383,13 +386,16 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
                 }
             },
             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);
+            StmtKind::Local(l) => {
+                self.visit_pat(l.pat);
+                if let Some(e) = l.init {
+                    self.allow_insert_closure &= !self.in_tail_pos;
+                    self.in_tail_pos = false;
+                    self.is_single_insert = false;
+                    self.visit_expr(e);
+                }
             },
-            _ => {
+            StmtKind::Item(_) => {
                 self.allow_insert_closure &= !self.in_tail_pos;
                 self.is_single_insert = false;
             },
@@ -471,6 +477,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
                     // Each branch may contain it's own insert expression.
                     let mut is_map_used = self.is_map_used;
                     for arm in arms {
+                        self.visit_pat(arm.pat);
                         if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard {
                             self.visit_non_tail_expr(guard);
                         }
@@ -496,7 +503,8 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
                 },
                 _ => {
                     self.allow_insert_closure &= !self.in_tail_pos;
-                    self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops);
+                    self.allow_insert_closure &=
+                        can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops, &self.locals);
                     // Sub expressions are no longer in the tail position.
                     self.is_single_insert = false;
                     self.in_tail_pos = false;
@@ -505,6 +513,12 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
             },
         }
     }
+
+    fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) {
+        p.each_binding_or_first(&mut |_, id, _, _| {
+            self.locals.insert(id);
+        });
+    }
 }
 
 struct InsertSearchResults<'tcx> {
@@ -630,6 +644,7 @@ fn find_insert_calls(
         in_tail_pos: true,
         is_single_insert: true,
         loops: Vec::new(),
+        locals: HirIdSet::default(),
     };
     s.visit_expr(expr);
     let allow_insert_closure = s.allow_insert_closure;
diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs
index 7dec1595e0d..3ea88f52a1c 100644
--- a/clippy_lints/src/manual_map.rs
+++ b/clippy_lints/src/manual_map.rs
@@ -4,12 +4,14 @@ use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 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_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id,
-    peel_hir_expr_refs,
+    peel_hir_expr_refs, peel_hir_expr_while, CaptureKind,
 };
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, OptionSome};
-use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind};
+use rustc_hir::{
+    def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind, Path, QPath,
+};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -111,10 +113,6 @@ impl LateLintPass<'_> for ManualMap {
                 return;
             }
 
-            if !can_move_expr_to_closure(cx, some_expr) {
-                return;
-            }
-
             // Determine which binding mode to use.
             let explicit_ref = some_pat.contains_explicit_ref_binding();
             let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability));
@@ -125,6 +123,32 @@ impl LateLintPass<'_> for ManualMap {
                 None => "",
             };
 
+            match can_move_expr_to_closure(cx, some_expr) {
+                Some(captures) => {
+                    // Check if captures the closure will need conflict with borrows made in the scrutinee.
+                    // TODO: check all the references made in the scrutinee expression. This will require interacting
+                    // with the borrow checker. Currently only `<local>[.<field>]*` is checked for.
+                    if let Some(binding_ref_mutability) = binding_ref {
+                        let e = peel_hir_expr_while(scrutinee, |e| match e.kind {
+                            ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e),
+                            _ => None,
+                        });
+                        if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind {
+                            match captures.get(l) {
+                                Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return,
+                                Some(CaptureKind::Ref(Mutability::Not))
+                                    if binding_ref_mutability == Mutability::Mut =>
+                                {
+                                    return;
+                                }
+                                Some(CaptureKind::Ref(Mutability::Not)) | None => (),
+                            }
+                        }
+                    }
+                },
+                None => return,
+            };
+
             let mut app = Applicability::MachineApplicable;
 
             // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 1d59d6bfea1..bd229402f41 100644
--- a/clippy_utils/src/lib.rs
+++ b/clippy_utils/src/lib.rs
@@ -62,23 +62,27 @@ use std::collections::hash_map::Entry;
 use std::hash::BuildHasherDefault;
 
 use if_chain::if_chain;
-use rustc_ast::ast::{self, Attribute, BorrowKind, LitKind};
+use rustc_ast::ast::{self, Attribute, LitKind};
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
+use rustc_hir::hir_id::{HirIdMap, HirIdSet};
 use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
 use rustc_hir::LangItem::{ResultErr, ResultOk};
 use rustc_hir::{
     def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
-    ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path,
-    PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp,
+    ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat,
+    PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
 use rustc_middle::hir::map::Map;
+use rustc_middle::hir::place::PlaceBase;
 use rustc_middle::ty as rustc_ty;
-use rustc_middle::ty::{layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
+use rustc_middle::ty::binding::BindingMode;
+use rustc_middle::ty::{layout::IntegerExt, BorrowKind, DefIdTree, Ty, TyCtxt, TypeAndMut, TypeFoldable, UpvarCapture};
 use rustc_semver::RustcVersion;
 use rustc_session::Session;
 use rustc_span::hygiene::{ExpnKind, MacroKind};
@@ -89,7 +93,7 @@ use rustc_span::{Span, DUMMY_SP};
 use rustc_target::abi::Integer;
 
 use crate::consts::{constant, Constant};
-use crate::ty::{can_partially_move_ty, is_recursively_primitive_type};
+use crate::ty::{can_partially_move_ty, is_copy, 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) {
@@ -626,11 +630,46 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -
 }
 
 /// 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 {
+/// Currently checks for:
+/// * Break/Continue outside the given loop HIR ids.
+/// * Yield/Return statments.
+/// * Inline assembly.
+/// * Usages of a field of a local where the type of the local can be partially moved.
+///
+/// For example, given the following function:
+///
+/// ```
+/// fn f<'a>(iter: &mut impl Iterator<Item = (usize, &'a mut String)>) {
+///     for item in iter {
+///         let s = item.1;
+///         if item.0 > 10 {
+///             continue;
+///         } else {
+///             s.clear();
+///         }
+///     }
+/// }
+/// ```
+///
+/// When called on the expression `item.0` this will return false unless the local `item` is in the
+/// `ignore_locals` set. The type `(usize, &mut String)` can have the second element moved, so it
+/// isn't always safe to move into a closure when only a single field is needed.
+///
+/// When called on the `continue` expression this will return false unless the outer loop expression
+/// is in the `loop_ids` set.
+///
+/// Note that this check is not recursive, so passing the `if` expression will always return true
+/// even though sub-expressions might return false.
+pub fn can_move_expr_to_closure_no_visit(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'_>,
+    loop_ids: &[HirId],
+    ignore_locals: &HirIdSet,
+) -> bool {
     match expr.kind {
         ExprKind::Break(Destination { target_id: Ok(id), .. }, _)
         | ExprKind::Continue(Destination { target_id: Ok(id), .. })
-            if jump_targets.contains(&id) =>
+            if loop_ids.contains(&id) =>
         {
             true
         },
@@ -642,25 +681,158 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp
         | 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)) =>
-        {
+        ExprKind::Field(
+            &Expr {
+                hir_id,
+                kind:
+                    ExprKind::Path(QPath::Resolved(
+                        _,
+                        Path {
+                            res: Res::Local(local_id),
+                            ..
+                        },
+                    )),
+                ..
+            },
+            _,
+        ) if !ignore_locals.contains(local_id) && can_partially_move_ty(cx, cx.typeck_results().node_type(hir_id)) => {
             // 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 {
+/// How a local is captured by a closure
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum CaptureKind {
+    Value,
+    Ref(Mutability),
+}
+impl std::ops::BitOr for CaptureKind {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        match (self, rhs) {
+            (CaptureKind::Value, _) | (_, CaptureKind::Value) => CaptureKind::Value,
+            (CaptureKind::Ref(Mutability::Mut), CaptureKind::Ref(_))
+            | (CaptureKind::Ref(_), CaptureKind::Ref(Mutability::Mut)) => CaptureKind::Ref(Mutability::Mut),
+            (CaptureKind::Ref(Mutability::Not), CaptureKind::Ref(Mutability::Not)) => CaptureKind::Ref(Mutability::Not),
+        }
+    }
+}
+impl std::ops::BitOrAssign for CaptureKind {
+    fn bitor_assign(&mut self, rhs: Self) {
+        *self = *self | rhs;
+    }
+}
+
+/// Given an expression referencing a local, determines how it would be captured in a closure.
+/// Note as this will walk up to parent expressions until the capture can be determined it should
+/// only be used while making a closure somewhere a value is consumed. e.g. a block, match arm, or
+/// function argument (other than a receiver).
+pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind {
+    fn pat_capture_kind(cx: &LateContext<'_>, pat: &Pat<'_>) -> CaptureKind {
+        let mut capture = CaptureKind::Ref(Mutability::Not);
+        pat.each_binding_or_first(&mut |_, id, span, _| match cx
+            .typeck_results()
+            .extract_binding_mode(cx.sess(), id, span)
+            .unwrap()
+        {
+            BindingMode::BindByValue(_) if !is_copy(cx, cx.typeck_results().node_type(id)) => {
+                capture = CaptureKind::Value;
+            },
+            BindingMode::BindByReference(Mutability::Mut) if capture != CaptureKind::Value => {
+                capture = CaptureKind::Ref(Mutability::Mut);
+            },
+            _ => (),
+        });
+        capture
+    }
+
+    debug_assert!(matches!(
+        e.kind,
+        ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. }))
+    ));
+
+    let map = cx.tcx.hir();
+    let mut child_id = e.hir_id;
+    let mut capture = CaptureKind::Value;
+    let mut capture_expr_ty = e;
+
+    for (parent_id, parent) in map.parent_iter(e.hir_id) {
+        if let [Adjustment {
+            kind: Adjust::Deref(_) | Adjust::Borrow(AutoBorrow::Ref(..)),
+            target,
+        }, ref adjust @ ..] = *cx
+            .typeck_results()
+            .adjustments()
+            .get(child_id)
+            .map_or(&[][..], |x| &**x)
+        {
+            if let rustc_ty::RawPtr(TypeAndMut { mutbl: mutability, .. }) | rustc_ty::Ref(_, _, mutability) =
+                *adjust.last().map_or(target, |a| a.target).kind()
+            {
+                return CaptureKind::Ref(mutability);
+            }
+        }
+
+        match parent {
+            Node::Expr(e) => match e.kind {
+                ExprKind::AddrOf(_, mutability, _) => return CaptureKind::Ref(mutability),
+                ExprKind::Index(..) | ExprKind::Unary(UnOp::Deref, _) => capture = CaptureKind::Ref(Mutability::Not),
+                ExprKind::Assign(lhs, ..) | ExprKind::Assign(_, lhs, _) if lhs.hir_id == child_id => {
+                    return CaptureKind::Ref(Mutability::Mut);
+                },
+                ExprKind::Field(..) => {
+                    if capture == CaptureKind::Value {
+                        capture_expr_ty = e;
+                    }
+                },
+                ExprKind::Match(_, arms, _) => {
+                    let mut mutability = Mutability::Not;
+                    for capture in arms.iter().map(|arm| pat_capture_kind(cx, arm.pat)) {
+                        match capture {
+                            CaptureKind::Value => break,
+                            CaptureKind::Ref(Mutability::Mut) => mutability = Mutability::Mut,
+                            CaptureKind::Ref(Mutability::Not) => (),
+                        }
+                    }
+                    return CaptureKind::Ref(mutability);
+                },
+                _ => break,
+            },
+            Node::Local(l) => match pat_capture_kind(cx, l.pat) {
+                CaptureKind::Value => break,
+                capture @ CaptureKind::Ref(_) => return capture,
+            },
+            _ => break,
+        }
+
+        child_id = parent_id;
+    }
+
+    if capture == CaptureKind::Value && is_copy(cx, cx.typeck_results().expr_ty(capture_expr_ty)) {
+        // Copy types are never automatically captured by value.
+        CaptureKind::Ref(Mutability::Not)
+    } else {
+        capture
+    }
+}
+
+/// Checks if the expression can be moved into a closure as is. This will return a list of captures
+/// if so, otherwise, `None`.
+pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<HirIdMap<CaptureKind>> {
     struct V<'cx, 'tcx> {
         cx: &'cx LateContext<'tcx>,
+        // Stack of potential break targets contained in the expression.
         loops: Vec<HirId>,
+        /// Local variables created in the expression. These don't need to be captured.
+        locals: HirIdSet,
+        /// Whether this expression can be turned into a closure.
         allow_closure: bool,
+        /// Locals which need to be captured, and whether they need to be by value, reference, or
+        /// mutable reference.
+        captures: HirIdMap<CaptureKind>,
     }
     impl Visitor<'tcx> for V<'_, 'tcx> {
         type Map = ErasedMap<'tcx>;
@@ -672,24 +844,67 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'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);
+
+            match e.kind {
+                ExprKind::Path(QPath::Resolved(None, &Path { res: Res::Local(l), .. })) => {
+                    if !self.locals.contains(&l) {
+                        let cap = capture_local_usage(self.cx, e);
+                        self.captures.entry(l).and_modify(|e| *e |= cap).or_insert(cap);
+                    }
+                },
+                ExprKind::Closure(..) => {
+                    let closure_id = self.cx.tcx.hir().local_def_id(e.hir_id).to_def_id();
+                    for capture in self.cx.typeck_results().closure_min_captures_flattened(closure_id) {
+                        let local_id = match capture.place.base {
+                            PlaceBase::Local(id) => id,
+                            PlaceBase::Upvar(var) => var.var_path.hir_id,
+                            _ => continue,
+                        };
+                        if !self.locals.contains(&local_id) {
+                            let capture = match capture.info.capture_kind {
+                                UpvarCapture::ByValue(_) => CaptureKind::Value,
+                                UpvarCapture::ByRef(borrow) => match borrow.kind {
+                                    BorrowKind::ImmBorrow => CaptureKind::Ref(Mutability::Not),
+                                    BorrowKind::UniqueImmBorrow | BorrowKind::MutBorrow => {
+                                        CaptureKind::Ref(Mutability::Mut)
+                                    },
+                                },
+                            };
+                            self.captures
+                                .entry(local_id)
+                                .and_modify(|e| *e |= capture)
+                                .or_insert(capture);
+                        }
+                    }
+                },
+                ExprKind::Loop(b, ..) => {
+                    self.loops.push(e.hir_id);
+                    self.visit_block(b);
+                    self.loops.pop();
+                },
+                _ => {
+                    self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals);
+                    walk_expr(self, e);
+                },
             }
         }
+
+        fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) {
+            p.each_binding_or_first(&mut |_, id, _, _| {
+                self.locals.insert(id);
+            });
+        }
     }
 
     let mut v = V {
         cx,
         allow_closure: true,
         loops: Vec::new(),
+        locals: HirIdSet::default(),
+        captures: HirIdMap::default(),
     };
     v.visit_expr(expr);
-    v.allow_closure
+    v.allow_closure.then(|| v.captures)
 }
 
 /// Returns the method names and argument list of nested method call expressions that make up
@@ -1715,7 +1930,7 @@ pub fn peel_hir_expr_while<'tcx>(
 pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
     let mut remaining = count;
     let e = peel_hir_expr_while(expr, |e| match e.kind {
-        ExprKind::AddrOf(BorrowKind::Ref, _, e) if remaining != 0 => {
+        ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) if remaining != 0 => {
             remaining -= 1;
             Some(e)
         },
@@ -1729,7 +1944,7 @@ pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>,
 pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) {
     let mut count = 0;
     let e = peel_hir_expr_while(expr, |e| match e.kind {
-        ExprKind::AddrOf(BorrowKind::Ref, _, e) => {
+        ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) => {
             count += 1;
             Some(e)
         },
diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed
index cfad3090ba3..8a36ec833d7 100644
--- a/tests/ui/entry.fixed
+++ b/tests/ui/entry.fixed
@@ -4,7 +4,7 @@
 #![warn(clippy::map_entry)]
 #![feature(asm)]
 
-use std::collections::{BTreeMap, HashMap};
+use std::collections::HashMap;
 use std::hash::Hash;
 
 macro_rules! m {
@@ -142,14 +142,13 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMa
     if !m.contains_key(&k) {
         insert!(m, k, v);
     }
-}
 
-fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
-    // insert then do something, use if let
-    if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
-        e.insert(v);
-        foo();
-    }
+    // or_insert_with. Partial move of a local declared in the closure is ok.
+    m.entry(k).or_insert_with(|| {
+        let x = (String::new(), String::new());
+        let _ = x.0;
+        v
+    });
 }
 
 fn main() {}
diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs
index fa9280b58de..d972a201ad7 100644
--- a/tests/ui/entry.rs
+++ b/tests/ui/entry.rs
@@ -4,7 +4,7 @@
 #![warn(clippy::map_entry)]
 #![feature(asm)]
 
-use std::collections::{BTreeMap, HashMap};
+use std::collections::HashMap;
 use std::hash::Hash;
 
 macro_rules! m {
@@ -146,13 +146,12 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMa
     if !m.contains_key(&k) {
         insert!(m, k, v);
     }
-}
 
-fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
-    // insert then do something, use if let
+    // or_insert_with. Partial move of a local declared in the closure is ok.
     if !m.contains_key(&k) {
+        let x = (String::new(), String::new());
+        let _ = x.0;
         m.insert(k, v);
-        foo();
     }
 }
 
diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr
index 8f2e383d675..1076500498d 100644
--- a/tests/ui/entry.stderr
+++ b/tests/ui/entry.stderr
@@ -165,21 +165,23 @@ LL | |         m.insert(m!(k), m!(v));
 LL | |     }
    | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));`
 
-error: usage of `contains_key` followed by `insert` on a `BTreeMap`
-  --> $DIR/entry.rs:153:5
+error: usage of `contains_key` followed by `insert` on a `HashMap`
+  --> $DIR/entry.rs:151:5
    |
 LL | /     if !m.contains_key(&k) {
+LL | |         let x = (String::new(), String::new());
+LL | |         let _ = x.0;
 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 +     }
+LL ~     m.entry(k).or_insert_with(|| {
+LL +         let x = (String::new(), String::new());
+LL +         let _ = x.0;
+LL +         v
+LL +     });
    |
 
 error: aborting due to 10 previous errors
diff --git a/tests/ui/entry_btree.fixed b/tests/ui/entry_btree.fixed
new file mode 100644
index 00000000000..94979104556
--- /dev/null
+++ b/tests/ui/entry_btree.fixed
@@ -0,0 +1,18 @@
+// run-rustfix
+
+#![warn(clippy::map_entry)]
+#![allow(dead_code)]
+
+use std::collections::BTreeMap;
+
+fn foo() {}
+
+fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V) {
+    // insert then do something, use if let
+    if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
+        e.insert(v);
+        foo();
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/entry_btree.rs b/tests/ui/entry_btree.rs
new file mode 100644
index 00000000000..080c1d959e8
--- /dev/null
+++ b/tests/ui/entry_btree.rs
@@ -0,0 +1,18 @@
+// run-rustfix
+
+#![warn(clippy::map_entry)]
+#![allow(dead_code)]
+
+use std::collections::BTreeMap;
+
+fn foo() {}
+
+fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V) {
+    // insert then do something, use if let
+    if !m.contains_key(&k) {
+        m.insert(k, v);
+        foo();
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/entry_btree.stderr b/tests/ui/entry_btree.stderr
new file mode 100644
index 00000000000..5c6fcdf1a28
--- /dev/null
+++ b/tests/ui/entry_btree.stderr
@@ -0,0 +1,20 @@
+error: usage of `contains_key` followed by `insert` on a `BTreeMap`
+  --> $DIR/entry_btree.rs:12:5
+   |
+LL | /     if !m.contains_key(&k) {
+LL | |         m.insert(k, v);
+LL | |         foo();
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::map-entry` implied by `-D warnings`
+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 previous error
+
diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed
new file mode 100644
index 00000000000..8cc12149403
--- /dev/null
+++ b/tests/ui/manual_map_option_2.fixed
@@ -0,0 +1,50 @@
+// run-rustfix
+
+#![warn(clippy::manual_map)]
+#![allow(clippy::toplevel_ref_arg)]
+
+fn main() {
+    // Lint. `y` is declared within the arm, so it isn't captured by the map closure
+    let _ = Some(0).map(|x| {
+            let y = (String::new(), String::new());
+            (x, y.0)
+        });
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map
+    // closure
+    let s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some((x.clone(), s)),
+        None => None,
+    };
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map
+    // closure
+    let s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some({
+            let clone = x.clone();
+            let s = || s;
+            (clone, s())
+        }),
+        None => None,
+    };
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable
+    // reference by the map closure
+    let mut s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some({
+            let clone = x.clone();
+            let ref mut s = s;
+            (clone, s)
+        }),
+        None => None,
+    };
+
+    // Lint. `s` is captured by reference, so no lifetime issues.
+    let s = Some(String::new());
+    let _ = s.as_ref().map(|x| {
+            if let Some(ref s) = s { (x.clone(), s) } else { panic!() }
+        });
+}
diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs
new file mode 100644
index 00000000000..0862b201ead
--- /dev/null
+++ b/tests/ui/manual_map_option_2.rs
@@ -0,0 +1,56 @@
+// run-rustfix
+
+#![warn(clippy::manual_map)]
+#![allow(clippy::toplevel_ref_arg)]
+
+fn main() {
+    // Lint. `y` is declared within the arm, so it isn't captured by the map closure
+    let _ = match Some(0) {
+        Some(x) => Some({
+            let y = (String::new(), String::new());
+            (x, y.0)
+        }),
+        None => None,
+    };
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map
+    // closure
+    let s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some((x.clone(), s)),
+        None => None,
+    };
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map
+    // closure
+    let s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some({
+            let clone = x.clone();
+            let s = || s;
+            (clone, s())
+        }),
+        None => None,
+    };
+
+    // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable
+    // reference by the map closure
+    let mut s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some({
+            let clone = x.clone();
+            let ref mut s = s;
+            (clone, s)
+        }),
+        None => None,
+    };
+
+    // Lint. `s` is captured by reference, so no lifetime issues.
+    let s = Some(String::new());
+    let _ = match &s {
+        Some(x) => Some({
+            if let Some(ref s) = s { (x.clone(), s) } else { panic!() }
+        }),
+        None => None,
+    };
+}
diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr
new file mode 100644
index 00000000000..711ff6c4a4b
--- /dev/null
+++ b/tests/ui/manual_map_option_2.stderr
@@ -0,0 +1,43 @@
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option_2.rs:8:13
+   |
+LL |       let _ = match Some(0) {
+   |  _____________^
+LL | |         Some(x) => Some({
+LL | |             let y = (String::new(), String::new());
+LL | |             (x, y.0)
+LL | |         }),
+LL | |         None => None,
+LL | |     };
+   | |_____^
+   |
+   = note: `-D clippy::manual-map` implied by `-D warnings`
+help: try this
+   |
+LL ~     let _ = Some(0).map(|x| {
+LL +             let y = (String::new(), String::new());
+LL +             (x, y.0)
+LL ~         });
+   |
+
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option_2.rs:50:13
+   |
+LL |       let _ = match &s {
+   |  _____________^
+LL | |         Some(x) => Some({
+LL | |             if let Some(ref s) = s { (x.clone(), s) } else { panic!() }
+LL | |         }),
+LL | |         None => None,
+LL | |     };
+   | |_____^
+   |
+help: try this
+   |
+LL ~     let _ = s.as_ref().map(|x| {
+LL +             if let Some(ref s) = s { (x.clone(), s) } else { panic!() }
+LL ~         });
+   |
+
+error: aborting due to 2 previous errors
+