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`

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;

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

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)
         },

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

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();
     }
 }
 

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

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

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

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
+

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!() }
+        });
+}

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,
+    };
+}

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
+