Skip to content

Commit 4616e9c

Browse files
JarchoJarcho
committed
Fix manual_map: don't lint when partially moved values are used.
Fix `manual_map`: don't lint when `return`, `break`, and `continue` are used.
1 parent d5223be commit 4616e9c

File tree

5 files changed

+168
-22
lines changed

5 files changed

+168
-22
lines changed

‎clippy_lints/src/manual_map.rs

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,17 @@ use crate::{
22
map_unit_fn::OPTION_MAP_UNIT_FN,
33
matches::MATCH_AS_REF,
44
utils::{
5-
is_allowed, is_type_diagnostic_item, match_def_path, match_var, paths, peel_hir_expr_refs,
6-
peel_mid_ty_refs_is_mutable, snippet_with_applicability, span_lint_and_sugg,
5+
can_partially_move_ty, is_allowed, is_type_diagnostic_item, match_def_path, match_var, paths,
6+
peel_hir_expr_refs, peel_mid_ty_refs_is_mutable, snippet_with_applicability, span_lint_and_sugg,
77
},
88
};
99
use rustc_ast::util::parser::PREC_POSTFIX;
1010
use rustc_errors::Applicability;
11-
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, QPath};
11+
use rustc_hir::{
12+
def::Res,
13+
intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
14+
Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, Path, QPath,
15+
};
1216
use rustc_lint::{LateContext, LateLintPass, LintContext};
1317
use rustc_middle::lint::in_external_macro;
1418
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -99,6 +103,10 @@ impl LateLintPass<'_> for ManualMap {
99103
return;
100104
}
101105

106+
if !can_move_expr_to_closure(cx, some_expr) {
107+
return;
108+
}
109+
102110
// Determine which binding mode to use.
103111
let explicit_ref = some_pat.contains_explicit_ref_binding();
104112
let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability));
@@ -171,6 +179,46 @@ impl LateLintPass<'_> for ManualMap {
171179
}
172180
}
173181

182+
// Checks if the expression can be moved into a closure as is.
183+
fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
184+
struct V<'cx, 'tcx> {
185+
cx: &'cx LateContext<'tcx>,
186+
make_closure: bool,
187+
}
188+
impl Visitor<'tcx> for V<'_, 'tcx> {
189+
type Map = ErasedMap<'tcx>;
190+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
191+
NestedVisitorMap::None
192+
}
193+
194+
fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
195+
match e.kind {
196+
ExprKind::Break(..) | ExprKind::Continue(_) | ExprKind::Ret(_) => {
197+
self.make_closure = false;
198+
},
199+
// Accessing a field of a local value can only be done if the type isn't
200+
// partially moved.
201+
ExprKind::Field(base_expr, _)
202+
if matches!(
203+
base_expr.kind,
204+
ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
205+
) && can_partially_move_ty(self.cx, self.cx.typeck_results().expr_ty(base_expr)) =>
206+
{
207+
// TODO: check if the local has been partially moved. Assume it has for now.
208+
self.make_closure = false;
209+
return;
210+
}
211+
_ => (),
212+
};
213+
walk_expr(self, e);
214+
}
215+
}
216+
217+
let mut v = V { cx, make_closure: true };
218+
v.visit_expr(expr);
219+
v.make_closure
220+
}
221+
174222
// Checks whether the expression could be passed as a function, or whether a closure is needed.
175223
// Returns the function to be passed to `map` if it exists.
176224
fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {

‎clippy_utils/src/lib.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,18 @@ pub fn has_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
456456
}
457457
}
458458

459+
/// Checks whether a type can be partially moved.
460+
pub fn can_partially_move_ty(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
461+
if has_drop(cx, ty) || is_copy(cx, ty) {
462+
return false;
463+
}
464+
match ty.kind() {
465+
ty::Param(_) => false,
466+
ty::Adt(def, subs) => def.all_fields().any(|f| !is_copy(cx, f.ty(cx.tcx, subs))),
467+
_ => true,
468+
}
469+
}
470+
459471
/// Returns the method names and argument list of nested method call expressions that make up
460472
/// `expr`. method/span lists are sorted with the most recent call first.
461473
pub fn method_calls<'tcx>(

‎tests/ui/manual_map_option.fixed

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
// run-rustfix
22

33
#![warn(clippy::manual_map)]
4-
#![allow(clippy::no_effect, clippy::map_identity, clippy::unit_arg, clippy::match_ref_pats)]
4+
#![allow(
5+
clippy::no_effect,
6+
clippy::map_identity,
7+
clippy::unit_arg,
8+
clippy::match_ref_pats,
9+
dead_code
10+
)]
511

612
fn main() {
713
Some(0).map(|_| 2);
@@ -67,4 +73,41 @@ fn main() {
6773
Some(Some((x, 1))) => Some(x),
6874
_ => None,
6975
};
76+
77+
// #6795
78+
fn f1() -> Result<(), ()> {
79+
let _ = match Some(Ok(())) {
80+
Some(x) => Some(x?),
81+
None => None,
82+
};
83+
Ok(())
84+
}
85+
86+
for &x in Some(Some(true)).iter() {
87+
let _ = match x {
88+
Some(x) => Some(if x { continue } else { x }),
89+
None => None,
90+
};
91+
}
92+
93+
// #6797
94+
let x1 = (Some(String::new()), 0);
95+
let x2 = x1.0;
96+
match x2 {
97+
Some(x) => Some((x, x1.1)),
98+
None => None,
99+
};
100+
101+
struct S1 {
102+
x: Option<String>,
103+
y: u32,
104+
}
105+
impl S1 {
106+
fn f(self) -> Option<(String, u32)> {
107+
match self.x {
108+
Some(x) => Some((x, self.y)),
109+
None => None,
110+
}
111+
}
112+
}
70113
}

‎tests/ui/manual_map_option.rs

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
// run-rustfix
22

33
#![warn(clippy::manual_map)]
4-
#![allow(clippy::no_effect, clippy::map_identity, clippy::unit_arg, clippy::match_ref_pats)]
4+
#![allow(
5+
clippy::no_effect,
6+
clippy::map_identity,
7+
clippy::unit_arg,
8+
clippy::match_ref_pats,
9+
dead_code
10+
)]
511

612
fn main() {
713
match Some(0) {
@@ -119,4 +125,41 @@ fn main() {
119125
Some(Some((x, 1))) => Some(x),
120126
_ => None,
121127
};
128+
129+
// #6795
130+
fn f1() -> Result<(), ()> {
131+
let _ = match Some(Ok(())) {
132+
Some(x) => Some(x?),
133+
None => None,
134+
};
135+
Ok(())
136+
}
137+
138+
for &x in Some(Some(true)).iter() {
139+
let _ = match x {
140+
Some(x) => Some(if x { continue } else { x }),
141+
None => None,
142+
};
143+
}
144+
145+
// #6797
146+
let x1 = (Some(String::new()), 0);
147+
let x2 = x1.0;
148+
match x2 {
149+
Some(x) => Some((x, x1.1)),
150+
None => None,
151+
};
152+
153+
struct S1 {
154+
x: Option<String>,
155+
y: u32,
156+
}
157+
impl S1 {
158+
fn f(self) -> Option<(String, u32)> {
159+
match self.x {
160+
Some(x) => Some((x, self.y)),
161+
None => None,
162+
}
163+
}
164+
}
122165
}

‎tests/ui/manual_map_option.stderr

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: manual implementation of `Option::map`
2-
--> $DIR/manual_map_option.rs:7:5
2+
--> $DIR/manual_map_option.rs:13:5
33
|
44
LL | / match Some(0) {
55
LL | | Some(_) => Some(2),
@@ -10,7 +10,7 @@ LL | | };
1010
= note: `-D clippy::manual-map` implied by `-D warnings`
1111

1212
error: manual implementation of `Option::map`
13-
--> $DIR/manual_map_option.rs:12:5
13+
--> $DIR/manual_map_option.rs:18:5
1414
|
1515
LL | / match Some(0) {
1616
LL | | Some(x) => Some(x + 1),
@@ -19,7 +19,7 @@ LL | | };
1919
| |_____^ help: try this: `Some(0).map(|x| x + 1)`
2020

2121
error: manual implementation of `Option::map`
22-
--> $DIR/manual_map_option.rs:17:5
22+
--> $DIR/manual_map_option.rs:23:5
2323
|
2424
LL | / match Some("") {
2525
LL | | Some(x) => Some(x.is_empty()),
@@ -28,7 +28,7 @@ LL | | };
2828
| |_____^ help: try this: `Some("").map(|x| x.is_empty())`
2929

3030
error: manual implementation of `Option::map`
31-
--> $DIR/manual_map_option.rs:22:5
31+
--> $DIR/manual_map_option.rs:28:5
3232
|
3333
LL | / if let Some(x) = Some(0) {
3434
LL | | Some(!x)
@@ -38,7 +38,7 @@ LL | | };
3838
| |_____^ help: try this: `Some(0).map(|x| !x)`
3939

4040
error: manual implementation of `Option::map`
41-
--> $DIR/manual_map_option.rs:29:5
41+
--> $DIR/manual_map_option.rs:35:5
4242
|
4343
LL | / match Some(0) {
4444
LL | | Some(x) => { Some(std::convert::identity(x)) }
@@ -47,7 +47,7 @@ LL | | };
4747
| |_____^ help: try this: `Some(0).map(std::convert::identity)`
4848

4949
error: manual implementation of `Option::map`
50-
--> $DIR/manual_map_option.rs:34:5
50+
--> $DIR/manual_map_option.rs:40:5
5151
|
5252
LL | / match Some(&String::new()) {
5353
LL | | Some(x) => Some(str::len(x)),
@@ -56,7 +56,7 @@ LL | | };
5656
| |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))`
5757

5858
error: manual implementation of `Option::map`
59-
--> $DIR/manual_map_option.rs:44:5
59+
--> $DIR/manual_map_option.rs:50:5
6060
|
6161
LL | / match &Some([0, 1]) {
6262
LL | | Some(x) => Some(x[0]),
@@ -65,7 +65,7 @@ LL | | };
6565
| |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])`
6666

6767
error: manual implementation of `Option::map`
68-
--> $DIR/manual_map_option.rs:49:5
68+
--> $DIR/manual_map_option.rs:55:5
6969
|
7070
LL | / match &Some(0) {
7171
LL | | &Some(x) => Some(x * 2),
@@ -74,7 +74,7 @@ LL | | };
7474
| |_____^ help: try this: `Some(0).map(|x| x * 2)`
7575

7676
error: manual implementation of `Option::map`
77-
--> $DIR/manual_map_option.rs:54:5
77+
--> $DIR/manual_map_option.rs:60:5
7878
|
7979
LL | / match Some(String::new()) {
8080
LL | | Some(ref x) => Some(x.is_empty()),
@@ -83,7 +83,7 @@ LL | | };
8383
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
8484

8585
error: manual implementation of `Option::map`
86-
--> $DIR/manual_map_option.rs:59:5
86+
--> $DIR/manual_map_option.rs:65:5
8787
|
8888
LL | / match &&Some(String::new()) {
8989
LL | | Some(x) => Some(x.len()),
@@ -92,7 +92,7 @@ LL | | };
9292
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
9393

9494
error: manual implementation of `Option::map`
95-
--> $DIR/manual_map_option.rs:64:5
95+
--> $DIR/manual_map_option.rs:70:5
9696
|
9797
LL | / match &&Some(0) {
9898
LL | | &&Some(x) => Some(x + x),
@@ -101,7 +101,7 @@ LL | | };
101101
| |_____^ help: try this: `Some(0).map(|x| x + x)`
102102

103103
error: manual implementation of `Option::map`
104-
--> $DIR/manual_map_option.rs:77:9
104+
--> $DIR/manual_map_option.rs:83:9
105105
|
106106
LL | / match &mut Some(String::new()) {
107107
LL | | Some(x) => Some(x.push_str("")),
@@ -110,7 +110,7 @@ LL | | };
110110
| |_________^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))`
111111

112112
error: manual implementation of `Option::map`
113-
--> $DIR/manual_map_option.rs:83:5
113+
--> $DIR/manual_map_option.rs:89:5
114114
|
115115
LL | / match &mut Some(String::new()) {
116116
LL | | Some(ref x) => Some(x.len()),
@@ -119,7 +119,7 @@ LL | | };
119119
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
120120

121121
error: manual implementation of `Option::map`
122-
--> $DIR/manual_map_option.rs:88:5
122+
--> $DIR/manual_map_option.rs:94:5
123123
|
124124
LL | / match &mut &Some(String::new()) {
125125
LL | | Some(x) => Some(x.is_empty()),
@@ -128,7 +128,7 @@ LL | | };
128128
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
129129

130130
error: manual implementation of `Option::map`
131-
--> $DIR/manual_map_option.rs:93:5
131+
--> $DIR/manual_map_option.rs:99:5
132132
|
133133
LL | / match Some((0, 1, 2)) {
134134
LL | | Some((x, y, z)) => Some(x + y + z),
@@ -137,7 +137,7 @@ LL | | };
137137
| |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)`
138138

139139
error: manual implementation of `Option::map`
140-
--> $DIR/manual_map_option.rs:98:5
140+
--> $DIR/manual_map_option.rs:104:5
141141
|
142142
LL | / match Some([1, 2, 3]) {
143143
LL | | Some([first, ..]) => Some(first),
@@ -146,7 +146,7 @@ LL | | };
146146
| |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)`
147147

148148
error: manual implementation of `Option::map`
149-
--> $DIR/manual_map_option.rs:103:5
149+
--> $DIR/manual_map_option.rs:109:5
150150
|
151151
LL | / match &Some((String::new(), "test")) {
152152
LL | | Some((x, y)) => Some((y, x)),

0 commit comments

Comments
 (0)