Skip to content

Commit 54feac1

Browse files
borsbors
committed
Auto merge of #8958 - Alexendoo:simple_filter_map, r=giraffate
Lint simple expressions in `manual_filter_map`, `manual_find_map` changelog: Lint simple expressions in [`manual_filter_map`], [`manual_find_map`] The current comparison rules out `.find(|a| a.is_some()).map(|b| b.unwrap())` because `a` being a reference can effect more complicated expressions, this adds a simple check for that case and adds the necessary derefs There's some overlap with `option_filter_map` so `lint_filter_some_map_unwrap` now returns a `bool` to indicate it linted
2 parents 2058b92 + 307b8cd commit 54feac1

File tree

7 files changed

+440
-73
lines changed

7 files changed

+440
-73
lines changed

‎clippy_lints/src/methods/filter_map.rs‎

Lines changed: 54 additions & 53 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;
@@ -49,35 +50,18 @@ fn is_option_filter_map<'tcx>(cx: &LateContext<'tcx>, filter_arg: &hir::Expr<'_>
4950
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
5051
}
5152

52-
/// lint use of `filter().map()` for `Iterators`
53-
fn lint_filter_some_map_unwrap(
53+
/// is `filter(|x| x.is_some()).map(|x| x.unwrap())`
54+
fn is_filter_some_map_unwrap(
5455
cx: &LateContext<'_>,
5556
expr: &hir::Expr<'_>,
5657
filter_recv: &hir::Expr<'_>,
5758
filter_arg: &hir::Expr<'_>,
5859
map_arg: &hir::Expr<'_>,
59-
target_span: Span,
60-
methods_span: Span,
61-
) {
60+
) -> bool {
6261
let iterator = is_trait_method(cx, expr, sym::Iterator);
6362
let option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(filter_recv), sym::Option);
64-
if (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg) {
65-
let msg = "`filter` for `Some` followed by `unwrap`";
66-
let help = "consider using `flatten` instead";
67-
let sugg = format!(
68-
"{}",
69-
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, target_span),)
70-
);
71-
span_lint_and_sugg(
72-
cx,
73-
OPTION_FILTER_MAP,
74-
methods_span,
75-
msg,
76-
help,
77-
sugg,
78-
Applicability::MachineApplicable,
79-
);
80-
}
63+
64+
(iterator || option) && is_option_filter_map(cx, filter_arg, map_arg)
8165
}
8266

8367
/// lint use of `filter().map()` or `find().map()` for `Iterators`
@@ -93,15 +77,20 @@ pub(super) fn check<'tcx>(
9377
map_span: Span,
9478
is_find: bool,
9579
) {
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-
);
80+
if is_filter_some_map_unwrap(cx, expr, filter_recv, filter_arg, map_arg) {
81+
span_lint_and_sugg(
82+
cx,
83+
OPTION_FILTER_MAP,
84+
filter_span.with_hi(expr.span.hi()),
85+
"`filter` for `Some` followed by `unwrap`",
86+
"consider using `flatten` instead",
87+
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(),
88+
Applicability::MachineApplicable,
89+
);
90+
91+
return;
92+
}
93+
10594
if_chain! {
10695
if is_trait_method(cx, map_recv, sym::Iterator);
10796

@@ -118,7 +107,7 @@ pub(super) fn check<'tcx>(
118107
// closure ends with is_some() or is_ok()
119108
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
120109
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();
110+
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).peel_refs().ty_adt_def();
122111
if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::Option, opt_ty.did()) {
123112
Some(false)
124113
} else if cx.tcx.is_diagnostic_item(sym::Result, opt_ty.did()) {
@@ -137,6 +126,19 @@ pub(super) fn check<'tcx>(
137126
if let ExprKind::MethodCall(seg, [map_arg, ..], _) = map_body.value.kind;
138127
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
139128

129+
// .filter(..).map(|y| f(y).copied().unwrap())
130+
// ~~~~
131+
let map_arg_peeled = match map_arg.kind {
132+
ExprKind::MethodCall(method, [original_arg], _) if acceptable_methods(method) => {
133+
original_arg
134+
},
135+
_ => map_arg,
136+
};
137+
138+
// .filter(|x| x.is_some()).map(|y| y[.acceptable_method()].unwrap())
139+
let simple_equal = path_to_local_id(filter_arg, filter_param_id)
140+
&& path_to_local_id(map_arg_peeled, map_param_id);
141+
140142
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
141143
// in `filter(|x| ..)`, replace `*x` with `x`
142144
let a_path = if_chain! {
@@ -145,36 +147,35 @@ pub(super) fn check<'tcx>(
145147
then { expr_path } else { a }
146148
};
147149
// 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)
150+
path_to_local_id(a_path, filter_param_id)
151+
&& path_to_local_id(b, map_param_id)
152+
&& cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
165153
};
166154

155+
if simple_equal || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg_peeled);
167156
then {
168157
let span = filter_span.with_hi(expr.span.hi());
169158
let (filter_name, lint) = if is_find {
170159
("find", MANUAL_FIND_MAP)
171160
} else {
172161
("filter", MANUAL_FILTER_MAP)
173162
};
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);
163+
let msg = format!("`{filter_name}(..).map(..)` can be simplified as `{filter_name}_map(..)`");
164+
let (to_opt, deref) = if is_result {
165+
(".ok()", String::new())
166+
} else {
167+
let derefs = cx.typeck_results()
168+
.expr_adjustments(map_arg)
169+
.iter()
170+
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
171+
.count();
172+
173+
("", "*".repeat(derefs))
174+
};
175+
let sugg = format!(
176+
"{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
177+
snippet(cx, map_arg.span, ".."),
178+
);
178179
span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable);
179180
}
180181
}

‎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)