Skip to content

Commit 4838c78

Browse files
JarchoJarcho
committed
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
1 parent 7c5487d commit 4838c78

File tree

11 files changed

+181
-37
lines changed

11 files changed

+181
-37
lines changed

‎clippy_lints/src/entry.rs

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ use clippy_utils::{
77
};
88
use rustc_errors::Applicability;
99
use rustc_hir::{
10+
hir_id::HirIdSet,
1011
intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
11-
Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp,
12+
Block, Expr, ExprKind, Guard, HirId, Pat, Stmt, StmtKind, UnOp,
1213
};
1314
use rustc_lint::{LateContext, LateLintPass};
1415
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -336,6 +337,8 @@ struct InsertSearcher<'cx, 'tcx> {
336337
edits: Vec<Edit<'tcx>>,
337338
/// A stack of loops the visitor is currently in.
338339
loops: Vec<HirId>,
340+
/// Local variables created in the expression. These don't need to be captured.
341+
locals: HirIdSet,
339342
}
340343
impl<'tcx> InsertSearcher<'_, 'tcx> {
341344
/// 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> {
383386
}
384387
},
385388
StmtKind::Expr(e) => self.visit_expr(e),
386-
StmtKind::Local(Local { init: Some(e), .. }) => {
387-
self.allow_insert_closure &= !self.in_tail_pos;
388-
self.in_tail_pos = false;
389-
self.is_single_insert = false;
390-
self.visit_expr(e);
389+
StmtKind::Local(l) => {
390+
self.visit_pat(l.pat);
391+
if let Some(e) = l.init {
392+
self.allow_insert_closure &= !self.in_tail_pos;
393+
self.in_tail_pos = false;
394+
self.is_single_insert = false;
395+
self.visit_expr(e);
396+
}
391397
},
392-
_ => {
398+
StmtKind::Item(_) => {
393399
self.allow_insert_closure &= !self.in_tail_pos;
394400
self.is_single_insert = false;
395401
},
@@ -471,6 +477,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
471477
// Each branch may contain it's own insert expression.
472478
let mut is_map_used = self.is_map_used;
473479
for arm in arms {
480+
self.visit_pat(arm.pat);
474481
if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard {
475482
self.visit_non_tail_expr(guard);
476483
}
@@ -496,7 +503,8 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
496503
},
497504
_ => {
498505
self.allow_insert_closure &= !self.in_tail_pos;
499-
self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops);
506+
self.allow_insert_closure &=
507+
can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops, &self.locals);
500508
// Sub expressions are no longer in the tail position.
501509
self.is_single_insert = false;
502510
self.in_tail_pos = false;
@@ -505,6 +513,12 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
505513
},
506514
}
507515
}
516+
517+
fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) {
518+
p.each_binding_or_first(&mut |_, id, _, _| {
519+
self.locals.insert(id);
520+
});
521+
}
508522
}
509523

510524
struct InsertSearchResults<'tcx> {
@@ -630,6 +644,7 @@ fn find_insert_calls(
630644
in_tail_pos: true,
631645
is_single_insert: true,
632646
loops: Vec::new(),
647+
locals: HirIdSet::default(),
633648
};
634649
s.visit_expr(expr);
635650
let allow_insert_closure = s.allow_insert_closure;

‎clippy_utils/src/lib.rs

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ use rustc_data_structures::unhash::UnhashMap;
6767
use rustc_hir as hir;
6868
use rustc_hir::def::{DefKind, Res};
6969
use rustc_hir::def_id::DefId;
70+
use rustc_hir::hir_id::HirIdSet;
7071
use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor};
7172
use rustc_hir::LangItem::{ResultErr, ResultOk};
7273
use rustc_hir::{
@@ -626,7 +627,12 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -
626627
}
627628

628629
/// Checks if the top level expression can be moved into a closure as is.
629-
pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool {
630+
pub fn can_move_expr_to_closure_no_visit(
631+
cx: &LateContext<'tcx>,
632+
expr: &'tcx Expr<'_>,
633+
jump_targets: &[HirId],
634+
ignore_locals: &HirIdSet,
635+
) -> bool {
630636
match expr.kind {
631637
ExprKind::Break(Destination { target_id: Ok(id), .. }, _)
632638
| 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
642648
| ExprKind::LlvmInlineAsm(_) => false,
643649
// Accessing a field of a local value can only be done if the type isn't
644650
// partially moved.
645-
ExprKind::Field(base_expr, _)
646-
if matches!(
647-
base_expr.kind,
648-
ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
649-
) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) =>
650-
{
651+
ExprKind::Field(
652+
&Expr {
653+
hir_id,
654+
kind:
655+
ExprKind::Path(QPath::Resolved(
656+
_,
657+
Path {
658+
res: Res::Local(local_id),
659+
..
660+
},
661+
)),
662+
..
663+
},
664+
_,
665+
) if !ignore_locals.contains(local_id) && can_partially_move_ty(cx, cx.typeck_results().node_type(hir_id)) => {
651666
// TODO: check if the local has been partially moved. Assume it has for now.
652667
false
653-
}
668+
},
654669
_ => true,
655670
}
656671
}
@@ -659,7 +674,11 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp
659674
pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
660675
struct V<'cx, 'tcx> {
661676
cx: &'cx LateContext<'tcx>,
677+
// Stack of potential break targets contained in the expression.
662678
loops: Vec<HirId>,
679+
/// Local variables created in the expression. These don't need to be captured.
680+
locals: HirIdSet,
681+
/// Whether this expression can be turned into a closure.
663682
allow_closure: bool,
664683
}
665684
impl Visitor<'tcx> for V<'_, 'tcx> {
@@ -677,16 +696,23 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) ->
677696
self.visit_block(b);
678697
self.loops.pop();
679698
} else {
680-
self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops);
699+
self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals);
681700
walk_expr(self, e);
682701
}
683702
}
703+
704+
fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) {
705+
p.each_binding_or_first(&mut |_, id, _, _| {
706+
self.locals.insert(id);
707+
});
708+
}
684709
}
685710

686711
let mut v = V {
687712
cx,
688713
allow_closure: true,
689714
loops: Vec::new(),
715+
locals: HirIdSet::default(),
690716
};
691717
v.visit_expr(expr);
692718
v.allow_closure

‎tests/ui/entry.fixed

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
#![warn(clippy::map_entry)]
55
#![feature(asm)]
66

7-
use std::collections::{BTreeMap, HashMap};
7+
use std::collections::HashMap;
88
use std::hash::Hash;
99

1010
macro_rules! m {
@@ -142,14 +142,13 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMa
142142
if !m.contains_key(&k) {
143143
insert!(m, k, v);
144144
}
145-
}
146145

147-
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
148-
// insert then do something, use if let
149-
if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
150-
e.insert(v);
151-
foo();
152-
}
146+
// or_insert_with. Partial move of a local declared in the closure is ok.
147+
m.entry(k).or_insert_with(|| {
148+
let x = (String::new(), String::new());
149+
let _ = x.0;
150+
v
151+
});
153152
}
154153

155154
fn main() {}

‎tests/ui/entry.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
#![warn(clippy::map_entry)]
55
#![feature(asm)]
66

7-
use std::collections::{BTreeMap, HashMap};
7+
use std::collections::HashMap;
88
use std::hash::Hash;
99

1010
macro_rules! m {
@@ -146,13 +146,12 @@ fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMa
146146
if !m.contains_key(&k) {
147147
insert!(m, k, v);
148148
}
149-
}
150149

151-
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) {
152-
// insert then do something, use if let
150+
// or_insert_with. Partial move of a local declared in the closure is ok.
153151
if !m.contains_key(&k) {
152+
let x = (String::new(), String::new());
153+
let _ = x.0;
154154
m.insert(k, v);
155-
foo();
156155
}
157156
}
158157

‎tests/ui/entry.stderr

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,21 +165,23 @@ LL | | m.insert(m!(k), m!(v));
165165
LL | | }
166166
| |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));`
167167

168-
error: usage of `contains_key` followed by `insert` on a `BTreeMap`
169-
--> $DIR/entry.rs:153:5
168+
error: usage of `contains_key` followed by `insert` on a `HashMap`
169+
--> $DIR/entry.rs:151:5
170170
|
171171
LL | / if !m.contains_key(&k) {
172+
LL | | let x = (String::new(), String::new());
173+
LL | | let _ = x.0;
172174
LL | | m.insert(k, v);
173-
LL | | foo();
174175
LL | | }
175176
| |_____^
176177
|
177178
help: try this
178179
|
179-
LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
180-
LL + e.insert(v);
181-
LL + foo();
182-
LL + }
180+
LL ~ m.entry(k).or_insert_with(|| {
181+
LL + let x = (String::new(), String::new());
182+
LL + let _ = x.0;
183+
LL + v
184+
LL + });
183185
|
184186

185187
error: aborting due to 10 previous errors

‎tests/ui/entry_btree.fixed

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::map_entry)]
4+
#![allow(dead_code)]
5+
6+
use std::collections::BTreeMap;
7+
8+
fn foo() {}
9+
10+
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V) {
11+
// insert then do something, use if let
12+
if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
13+
e.insert(v);
14+
foo();
15+
}
16+
}
17+
18+
fn main() {}

‎tests/ui/entry_btree.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::map_entry)]
4+
#![allow(dead_code)]
5+
6+
use std::collections::BTreeMap;
7+
8+
fn foo() {}
9+
10+
fn btree_map<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V) {
11+
// insert then do something, use if let
12+
if !m.contains_key(&k) {
13+
m.insert(k, v);
14+
foo();
15+
}
16+
}
17+
18+
fn main() {}

‎tests/ui/entry_btree.stderr

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
error: usage of `contains_key` followed by `insert` on a `BTreeMap`
2+
--> $DIR/entry_btree.rs:12:5
3+
|
4+
LL | / if !m.contains_key(&k) {
5+
LL | | m.insert(k, v);
6+
LL | | foo();
7+
LL | | }
8+
| |_____^
9+
|
10+
= note: `-D clippy::map-entry` implied by `-D warnings`
11+
help: try this
12+
|
13+
LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) {
14+
LL + e.insert(v);
15+
LL + foo();
16+
LL + }
17+
|
18+
19+
error: aborting due to previous error
20+

‎tests/ui/manual_map_option_2.fixed

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::manual_map)]
4+
5+
fn main() {
6+
let _ = Some(0).map(|x| {
7+
let y = (String::new(), String::new());
8+
(x, y.0)
9+
});
10+
}

‎tests/ui/manual_map_option_2.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::manual_map)]
4+
5+
fn main() {
6+
let _ = match Some(0) {
7+
Some(x) => Some({
8+
let y = (String::new(), String::new());
9+
(x, y.0)
10+
}),
11+
None => None,
12+
};
13+
}

‎tests/ui/manual_map_option_2.stderr

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
error: manual implementation of `Option::map`
2+
--> $DIR/manual_map_option_2.rs:6:13
3+
|
4+
LL | let _ = match Some(0) {
5+
| _____________^
6+
LL | | Some(x) => Some({
7+
LL | | let y = (String::new(), String::new());
8+
LL | | (x, y.0)
9+
LL | | }),
10+
LL | | None => None,
11+
LL | | };
12+
| |_____^
13+
|
14+
= note: `-D clippy::manual-map` implied by `-D warnings`
15+
help: try this
16+
|
17+
LL | let _ = Some(0).map(|x| {
18+
LL | let y = (String::new(), String::new());
19+
LL | (x, y.0)
20+
LL | });
21+
|
22+
23+
error: aborting due to previous error
24+

0 commit comments

Comments
 (0)