Improve manual_map and map_entry

Locals which can be partially moved created within the to-be-created closure shouldn't block the use of a closure

Author: Jarcho

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_utils/src/lib.rs

@@ -67,6 +67,7 @@ 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::HirIdSet;
 use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
 use rustc_hir::LangItem::{ResultErr, ResultOk};
 use rustc_hir::{
@@ -626,7 +627,12 @@ 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 {
+pub fn can_move_expr_to_closure_no_visit(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'_>,
+    jump_targets: &[HirId],
+    ignore_locals: &HirIdSet,
+) -> bool {
     match expr.kind {
         ExprKind::Break(Destination { target_id: Ok(id), .. }, _)
         | ExprKind::Continue(Destination { target_id: Ok(id), .. })
@@ -642,15 +648,24 @@ 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,
     }
 }
@@ -659,7 +674,11 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp
 pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
     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,
     }
     impl Visitor<'tcx> for V<'_, 'tcx> {
@@ -677,16 +696,23 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) ->
                 self.visit_block(b);
                 self.loops.pop();
             } else {
-                self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops);
+                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(),
     };
     v.visit_expr(expr);
     v.allow_closure

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,10 @@
+// run-rustfix
+
+#![warn(clippy::manual_map)]
+
+fn main() {
+    let _ = Some(0).map(|x| {
+            let y = (String::new(), String::new());
+            (x, y.0)
+        });
+}

tests/ui/manual_map_option_2.rs

@@ -0,0 +1,13 @@
+// run-rustfix
+
+#![warn(clippy::manual_map)]
+
+fn main() {
+    let _ = match Some(0) {
+        Some(x) => Some({
+            let y = (String::new(), String::new());
+            (x, y.0)
+        }),
+        None => None,
+    };
+}