Skip to content

Commit 9e362ba

Browse files
AlexendooAlexendoo
committed
Lint simple expressions in manual_filter_map, manual_find_map
1 parent 0f6e50f commit 9e362ba

File tree

7 files changed

+437
-53
lines changed

7 files changed

+437
-53
lines changed

‎clippy_lints/src/methods/filter_map.rs‎

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use rustc_hir as hir;
88
use rustc_hir::def::Res;
99
use rustc_hir::{Expr, ExprKind, PatKind, PathSegment, QPath, UnOp};
1010
use rustc_lint::LateContext;
11+
use rustc_middle::ty::adjustment::Adjust;
1112
use rustc_span::source_map::Span;
1213
use rustc_span::symbol::{sym, Symbol};
1314
use std::borrow::Cow;
@@ -58,15 +59,15 @@ fn lint_filter_some_map_unwrap(
5859
map_arg: &hir::Expr<'_>,
5960
target_span: Span,
6061
methods_span: Span,
61-
) {
62+
) -> bool {
6263
let iterator = is_trait_method(cx, expr, sym::Iterator);
6364
let option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(filter_recv), sym::Option);
6465
if (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg) {
6566
let msg = "`filter` for `Some` followed by `unwrap`";
6667
let help = "consider using `flatten` instead";
6768
let sugg = format!(
6869
"{}",
69-
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, target_span),)
70+
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, target_span)),
7071
);
7172
span_lint_and_sugg(
7273
cx,
@@ -77,6 +78,10 @@ fn lint_filter_some_map_unwrap(
7778
sugg,
7879
Applicability::MachineApplicable,
7980
);
81+
82+
true
83+
} else {
84+
false
8085
}
8186
}
8287

@@ -93,16 +98,17 @@ pub(super) fn check<'tcx>(
9398
map_span: Span,
9499
is_find: bool,
95100
) {
96-
lint_filter_some_map_unwrap(
97-
cx,
98-
expr,
99-
filter_recv,
100-
filter_arg,
101-
map_arg,
102-
map_span,
103-
filter_span.with_hi(expr.span.hi()),
104-
);
105101
if_chain! {
102+
if !lint_filter_some_map_unwrap(
103+
cx,
104+
expr,
105+
filter_recv,
106+
filter_arg,
107+
map_arg,
108+
map_span,
109+
filter_span.with_hi(expr.span.hi()),
110+
);
111+
106112
if is_trait_method(cx, map_recv, sym::Iterator);
107113

108114
// filter(|x| ...is_some())...
@@ -118,7 +124,7 @@ pub(super) fn check<'tcx>(
118124
// closure ends with is_some() or is_ok()
119125
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
120126
if let ExprKind::MethodCall(path, [filter_arg], _) = filter_body.value.kind;
121-
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).ty_adt_def();
127+
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).peel_refs().ty_adt_def();
122128
if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::Option, opt_ty.did()) {
123129
Some(false)
124130
} else if cx.tcx.is_diagnostic_item(sym::Result, opt_ty.did()) {
@@ -137,6 +143,19 @@ pub(super) fn check<'tcx>(
137143
if let ExprKind::MethodCall(seg, [map_arg, ..], _) = map_body.value.kind;
138144
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
139145

146+
// .filter(..).map(|y| f(y).copied().unwrap())
147+
// ~~~~
148+
let map_arg_peeled = match map_arg.kind {
149+
ExprKind::MethodCall(method, [original_arg], _) if acceptable_methods(method) => {
150+
original_arg
151+
},
152+
_ => map_arg,
153+
};
154+
155+
// .filter(|x| x.is_some()).map(|y| y[.acceptable_method()].unwrap())
156+
let simple_equal = path_to_local_id(filter_arg, filter_param_id)
157+
&& path_to_local_id(map_arg_peeled, map_param_id);
158+
140159
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
141160
// in `filter(|x| ..)`, replace `*x` with `x`
142161
let a_path = if_chain! {
@@ -145,36 +164,35 @@ pub(super) fn check<'tcx>(
145164
then { expr_path } else { a }
146165
};
147166
// let the filter closure arg and the map closure arg be equal
148-
if_chain! {
149-
if path_to_local_id(a_path, filter_param_id);
150-
if path_to_local_id(b, map_param_id);
151-
if cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b);
152-
then {
153-
return true;
154-
}
155-
}
156-
false
157-
};
158-
159-
if match map_arg.kind {
160-
ExprKind::MethodCall(method, [original_arg], _) => {
161-
acceptable_methods(method)
162-
&& SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, original_arg)
163-
},
164-
_ => SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg)
167+
path_to_local_id(a_path, filter_param_id)
168+
&& path_to_local_id(b, map_param_id)
169+
&& cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
165170
};
166171

172+
if simple_equal || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg_peeled);
167173
then {
168174
let span = filter_span.with_hi(expr.span.hi());
169175
let (filter_name, lint) = if is_find {
170176
("find", MANUAL_FIND_MAP)
171177
} else {
172178
("filter", MANUAL_FILTER_MAP)
173179
};
174-
let msg = format!("`{}(..).map(..)` can be simplified as `{0}_map(..)`", filter_name);
175-
let to_opt = if is_result { ".ok()" } else { "" };
176-
let sugg = format!("{}_map(|{}| {}{})", filter_name, map_param_ident,
177-
snippet(cx, map_arg.span, ".."), to_opt);
180+
let msg = format!("`{filter_name}(..).map(..)` can be simplified as `{filter_name}_map(..)`");
181+
let (to_opt, deref) = if is_result {
182+
(".ok()", String::new())
183+
} else {
184+
let derefs = cx.typeck_results()
185+
.expr_adjustments(map_arg)
186+
.iter()
187+
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
188+
.count();
189+
190+
("", "*".repeat(derefs))
191+
};
192+
let sugg = format!(
193+
"{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
194+
snippet(cx, map_arg.span, ".."),
195+
);
178196
span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable);
179197
}
180198
}

‎tests/ui/manual_filter_map.fixed‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,32 @@ fn main() {
1212

1313
// is_ok(), unwrap_or()
1414
let _ = (0..).filter_map(|a| to_res(a).ok());
15+
16+
let _ = (1..5)
17+
.filter_map(|y| *to_ref(to_opt(y)));
18+
let _ = (1..5)
19+
.filter_map(|y| *to_ref(to_opt(y)));
20+
21+
let _ = (1..5)
22+
.filter_map(|y| to_ref(to_res(y)).ok());
23+
let _ = (1..5)
24+
.filter_map(|y| to_ref(to_res(y)).ok());
25+
}
26+
27+
#[rustfmt::skip]
28+
fn simple_equal() {
29+
iter::<Option<&u8>>().find_map(|x| x.cloned());
30+
iter::<&Option<&u8>>().find_map(|x| x.cloned());
31+
iter::<&Option<String>>().find_map(|x| x.as_deref());
32+
iter::<Option<&String>>().find_map(|y| to_ref(y).cloned());
33+
34+
iter::<Result<u8, ()>>().find_map(|x| x.ok());
35+
iter::<&Result<u8, ()>>().find_map(|x| x.ok());
36+
iter::<&&Result<u8, ()>>().find_map(|x| x.ok());
37+
iter::<Result<&u8, ()>>().find_map(|x| x.cloned().ok());
38+
iter::<&Result<&u8, ()>>().find_map(|x| x.cloned().ok());
39+
iter::<&Result<String, ()>>().find_map(|x| x.as_deref().ok());
40+
iter::<Result<&String, ()>>().find_map(|y| to_ref(y).cloned().ok());
1541
}
1642

1743
fn no_lint() {
@@ -28,6 +54,10 @@ fn no_lint() {
2854
.map(|a| to_opt(a).unwrap());
2955
}
3056

57+
fn iter<T>() -> impl Iterator<Item = T> {
58+
std::iter::empty()
59+
}
60+
3161
fn to_opt<T>(_: T) -> Option<T> {
3262
unimplemented!()
3363
}
@@ -36,6 +66,10 @@ fn to_res<T>(_: T) -> Result<T, ()> {
3666
unimplemented!()
3767
}
3868

69+
fn to_ref<'a, T>(_: T) -> &'a T {
70+
unimplemented!()
71+
}
72+
3973
struct Issue8920<'a> {
4074
option_field: Option<String>,
4175
result_field: Result<String, ()>,

‎tests/ui/manual_filter_map.rs‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,36 @@ fn main() {
1212

1313
// is_ok(), unwrap_or()
1414
let _ = (0..).filter(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1));
15+
16+
let _ = (1..5)
17+
.filter(|&x| to_ref(to_opt(x)).is_some())
18+
.map(|y| to_ref(to_opt(y)).unwrap());
19+
let _ = (1..5)
20+
.filter(|x| to_ref(to_opt(*x)).is_some())
21+
.map(|y| to_ref(to_opt(y)).unwrap());
22+
23+
let _ = (1..5)
24+
.filter(|&x| to_ref(to_res(x)).is_ok())
25+
.map(|y| to_ref(to_res(y)).unwrap());
26+
let _ = (1..5)
27+
.filter(|x| to_ref(to_res(*x)).is_ok())
28+
.map(|y| to_ref(to_res(y)).unwrap());
29+
}
30+
31+
#[rustfmt::skip]
32+
fn simple_equal() {
33+
iter::<Option<&u8>>().find(|x| x.is_some()).map(|x| x.cloned().unwrap());
34+
iter::<&Option<&u8>>().find(|x| x.is_some()).map(|x| x.cloned().unwrap());
35+
iter::<&Option<String>>().find(|x| x.is_some()).map(|x| x.as_deref().unwrap());
36+
iter::<Option<&String>>().find(|&x| to_ref(x).is_some()).map(|y| to_ref(y).cloned().unwrap());
37+
38+
iter::<Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
39+
iter::<&Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
40+
iter::<&&Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
41+
iter::<Result<&u8, ()>>().find(|x| x.is_ok()).map(|x| x.cloned().unwrap());
42+
iter::<&Result<&u8, ()>>().find(|x| x.is_ok()).map(|x| x.cloned().unwrap());
43+
iter::<&Result<String, ()>>().find(|x| x.is_ok()).map(|x| x.as_deref().unwrap());
44+
iter::<Result<&String, ()>>().find(|&x| to_ref(x).is_ok()).map(|y| to_ref(y).cloned().unwrap());
1545
}
1646

1747
fn no_lint() {
@@ -28,6 +58,10 @@ fn no_lint() {
2858
.map(|a| to_opt(a).unwrap());
2959
}
3060

61+
fn iter<T>() -> impl Iterator<Item = T> {
62+
std::iter::empty()
63+
}
64+
3165
fn to_opt<T>(_: T) -> Option<T> {
3266
unimplemented!()
3367
}
@@ -36,6 +70,10 @@ fn to_res<T>(_: T) -> Result<T, ()> {
3670
unimplemented!()
3771
}
3872

73+
fn to_ref<'a, T>(_: T) -> &'a T {
74+
unimplemented!()
75+
}
76+
3977
struct Issue8920<'a> {
4078
option_field: Option<String>,
4179
result_field: Result<String, ()>,

0 commit comments

Comments
 (0)