Skip to content

Commit aaf6bcf

Browse files
camsteffencamsteffen
committed
# This is a combination of 6 commits.
# This is the 1st commit message: Split filter_map into manual_filter_map # The commit message rust-lang#2 will be skipped: # fixup! Split filter_map into manual_filter_map # The commit message rust-lang#3 will be skipped: # fixup! Split filter_map into manual_filter_map # The commit message rust-lang#4 will be skipped: # fixup! Split filter_map into manual_filter_map # The commit message rust-lang#5 will be skipped: # fixup! Split filter_map into manual_filter_map # The commit message rust-lang#6 will be skipped: # fixup! Split filter_map into manual_filter_map
1 parent d93889a commit aaf6bcf

File tree

8 files changed

+225
-18
lines changed

8 files changed

+225
-18
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2033,6 +2033,7 @@ Released 2018-09-13
20332033
[`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
20342034
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
20352035
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
2036+
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
20362037
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
20372038
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
20382039
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or

‎clippy_lints/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
740740
&methods::ITER_NTH,
741741
&methods::ITER_NTH_ZERO,
742742
&methods::ITER_SKIP_NEXT,
743+
&methods::MANUAL_FILTER_MAP,
743744
&methods::MANUAL_SATURATING_ARITHMETIC,
744745
&methods::MAP_COLLECT_RESULT_UNIT,
745746
&methods::MAP_FLATTEN,
@@ -1514,6 +1515,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15141515
LintId::of(&methods::ITER_NTH),
15151516
LintId::of(&methods::ITER_NTH_ZERO),
15161517
LintId::of(&methods::ITER_SKIP_NEXT),
1518+
LintId::of(&methods::MANUAL_FILTER_MAP),
15171519
LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
15181520
LintId::of(&methods::MAP_COLLECT_RESULT_UNIT),
15191521
LintId::of(&methods::NEW_RET_NO_SELF),
@@ -1807,6 +1809,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
18071809
LintId::of(&methods::CLONE_ON_COPY),
18081810
LintId::of(&methods::FILTER_NEXT),
18091811
LintId::of(&methods::FLAT_MAP_IDENTITY),
1812+
LintId::of(&methods::MANUAL_FILTER_MAP),
18101813
LintId::of(&methods::OPTION_AS_REF_DEREF),
18111814
LintId::of(&methods::SEARCH_IS_SOME),
18121815
LintId::of(&methods::SKIP_WHILE_NEXT),

‎clippy_lints/src/methods/mod.rs

Lines changed: 94 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ use if_chain::if_chain;
1414
use rustc_ast::ast;
1515
use rustc_errors::Applicability;
1616
use rustc_hir as hir;
17-
use rustc_hir::{TraitItem, TraitItemKind};
17+
use rustc_hir::def::Res;
18+
use rustc_hir::{Expr, ExprKind, PatKind, QPath, TraitItem, TraitItemKind, UnOp};
1819
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
1920
use rustc_middle::lint::in_external_macro;
2021
use rustc_middle::ty::{self, TraitRef, Ty, TyS};
@@ -449,6 +450,32 @@ declare_clippy_lint! {
449450
"using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call"
450451
}
451452

453+
declare_clippy_lint! {
454+
/// **What it does:** Checks for usage of `_.filter(_).map(_)` that can be written more simply
455+
/// as `filter_map(_)`.
456+
///
457+
/// **Why is this bad?** Redundant code in the `filter` and `map` operations is poor style and
458+
/// less performant.
459+
///
460+
/// **Known problems:** None.
461+
///
462+
/// **Example:**
463+
/// Bad:
464+
/// ```rust
465+
/// (0..10)
466+
/// .filter(|n| n.checked_add(1).is_some())
467+
/// .map(|n| n.checked_add(1).unwrap());
468+
/// ```
469+
///
470+
/// Good:
471+
/// ```rust
472+
/// (0..10).filter_map(|n| n.checked_add(1));
473+
/// ```
474+
pub MANUAL_FILTER_MAP,
475+
complexity,
476+
"using `_.filter(_).map(_)` in a way that can be written more simply as `filter_map(_)`"
477+
}
478+
452479
declare_clippy_lint! {
453480
/// **What it does:** Checks for usage of `_.filter_map(_).next()`.
454481
///
@@ -1442,6 +1469,7 @@ impl_lint_pass!(Methods => [
14421469
FILTER_NEXT,
14431470
SKIP_WHILE_NEXT,
14441471
FILTER_MAP,
1472+
MANUAL_FILTER_MAP,
14451473
FILTER_MAP_NEXT,
14461474
FLAT_MAP_IDENTITY,
14471475
FIND_MAP,
@@ -2958,14 +2986,72 @@ fn lint_skip_while_next<'tcx>(
29582986
fn lint_filter_map<'tcx>(
29592987
cx: &LateContext<'tcx>,
29602988
expr: &'tcx hir::Expr<'_>,
2961-
_filter_args: &'tcx [hir::Expr<'_>],
2962-
_map_args: &'tcx [hir::Expr<'_>],
2989+
filter_args: &'tcx [hir::Expr<'_>],
2990+
map_args: &'tcx [hir::Expr<'_>],
29632991
) {
2964-
// lint if caller of `.filter().map()` is an Iterator
2965-
if match_trait_method(cx, expr, &paths::ITERATOR) {
2966-
let msg = "called `filter(..).map(..)` on an `Iterator`";
2967-
let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
2968-
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
2992+
if_chain! {
2993+
if match_trait_method(cx, expr, &paths::ITERATOR);
2994+
if let [_, filter_arg] = filter_args;
2995+
2996+
// filter(|x| ...is_some())...
2997+
if let ExprKind::Closure(_, _, filter_body_id, ..) = filter_arg.kind;
2998+
let filter_body = cx.tcx.hir().body(filter_body_id);
2999+
if let [filter_param] = filter_body.params;
3000+
// optional ref pattern: `filter(|&x| ..)`
3001+
let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
3002+
(ref_pat, true)
3003+
} else {
3004+
(filter_param.pat, false)
3005+
};
3006+
// closure ends with is_some() or is_ok()
3007+
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
3008+
if let ExprKind::MethodCall(_, _, [filter_arg], filter_span) = filter_body.value.kind;
3009+
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(filter_body.value.hir_id);
3010+
if let Some(is_result) = if match_def_path(cx, def_id, &paths::OPTION_IS_SOME) {
3011+
Some(false)
3012+
} else if match_def_path(cx, def_id, &paths::RESULT_IS_OK) {
3013+
Some(true)
3014+
} else {
3015+
None
3016+
};
3017+
3018+
// ...map(|x| ...unwrap())
3019+
if let [_, map_arg] = map_args;
3020+
if let ExprKind::Closure(_, _, map_body_id, ..) = map_arg.kind;
3021+
let map_body = cx.tcx.hir().body(map_body_id);
3022+
if let [map_param] = map_body.params;
3023+
if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind;
3024+
// closure ends with expect() or unwrap()
3025+
if let ExprKind::MethodCall(seg, _, [map_arg, ..], map_span) = map_body.value.kind;
3026+
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
3027+
3028+
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
3029+
// in `filter(|x| ..)`, replace `*x` with `x`
3030+
let a_path = if_chain! {
3031+
if !is_filter_param_ref;
3032+
if let ExprKind::Unary(UnOp::UnDeref, expr_path) = a.kind;
3033+
then { expr_path } else { a }
3034+
};
3035+
// let the filter closure arg and the map closure arg be equal
3036+
if_chain! {
3037+
if let ExprKind::Path(QPath::Resolved(None, a_path)) = a_path.kind;
3038+
if let ExprKind::Path(QPath::Resolved(None, b_path)) = b.kind;
3039+
if a_path.res == Res::Local(filter_param_id);
3040+
if b_path.res == Res::Local(map_param_id);
3041+
if TyS::same_type(cx.typeck_results().expr_ty_adjusted(a), cx.typeck_results().expr_ty_adjusted(b));
3042+
then {
3043+
return true;
3044+
}
3045+
}
3046+
false
3047+
};
3048+
if SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg);
3049+
then {
3050+
let msg = "`filter(..).map(..)` can be simplified as `filter_map(..)`";
3051+
let to_opt = if is_result { ".ok()" } else { "" };
3052+
let sugg = format!(".filter_map(|{}| {}{})", map_param_ident, snippet(cx, map_arg.span, ".."), to_opt);
3053+
span_lint_and_sugg(cx, MANUAL_FILTER_MAP, span, msg, "try", sugg, Applicability::MachineApplicable);
3054+
}
29693055
}
29703056
}
29713057

‎clippy_lints/src/utils/paths.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ pub const MUTEX_GUARD: [&str; 4] = ["std", "sync", "mutex", "MutexGuard"];
8787
pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"];
8888
pub const OPS_MODULE: [&str; 2] = ["core", "ops"];
8989
pub const OPTION: [&str; 3] = ["core", "option", "Option"];
90+
pub const OPTION_IS_SOME: [&str; 4] = ["core", "option", "Option", "is_some"];
9091
pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"];
9192
pub const OPTION_SOME: [&str; 4] = ["core", "option", "Option", "Some"];
9293
pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
@@ -129,6 +130,7 @@ pub const REGEX_SET_NEW: [&str; 5] = ["regex", "re_set", "unicode", "RegexSet",
129130
pub const REPEAT: [&str; 3] = ["core", "iter", "repeat"];
130131
pub const RESULT: [&str; 3] = ["core", "result", "Result"];
131132
pub const RESULT_ERR: [&str; 4] = ["core", "result", "Result", "Err"];
133+
pub const RESULT_IS_OK: [&str; 4] = ["core", "result", "Result", "is_ok"];
132134
pub const RESULT_OK: [&str; 4] = ["core", "result", "Result", "Ok"];
133135
pub const RWLOCK_READ_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockReadGuard"];
134136
pub const RWLOCK_WRITE_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockWriteGuard"];

‎tests/ui/filter_methods.stderr

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,3 @@
1-
error: called `filter(..).map(..)` on an `Iterator`
2-
--> $DIR/filter_methods.rs:6:21
3-
|
4-
LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6-
|
7-
= note: `-D clippy::filter-map` implied by `-D warnings`
8-
= help: this is more succinctly expressed by calling `.filter_map(..)` instead
9-
101
error: called `filter(..).flat_map(..)` on an `Iterator`
112
--> $DIR/filter_methods.rs:8:21
123
|
@@ -17,6 +8,7 @@ LL | | .filter(|&x| x == 0)
178
LL | | .flat_map(|x| x.checked_mul(2))
189
| |_______________________________________^
1910
|
11+
= note: `-D clippy::filter-map` implied by `-D warnings`
2012
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
2113

2214
error: called `filter_map(..).flat_map(..)` on an `Iterator`
@@ -43,5 +35,5 @@ LL | | .map(|x| x.checked_mul(2))
4335
|
4436
= help: this is more succinctly expressed by only calling `.filter_map(..)` instead
4537

46-
error: aborting due to 4 previous errors
38+
error: aborting due to 3 previous errors
4739

‎tests/ui/manual_filter_map.fixed

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// run-rustfix
2+
#![allow(dead_code)]
3+
#![warn(clippy::manual_filter_map)]
4+
#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure
5+
6+
use std::iter;
7+
8+
fn main() {
9+
// is_some(), unwrap()
10+
let _ = iter::once(1).filter_map(|a| to_opt(a));
11+
12+
// ref pattern, expect()
13+
let _ = iter::once(1).filter_map(|a| to_opt(a));
14+
15+
// is_ok(), unwrap_or()
16+
let _ = iter::once(1).filter_map(|a| to_res(a).ok());
17+
}
18+
19+
fn no_lint() {
20+
// no shared code
21+
let _ = iter::once(1).filter(|n| *n > 1).map(|n| n + 1);
22+
23+
// very close but different since filter() provides a reference
24+
let _ = iter::once(1)
25+
.filter(|n| to_opt(n).is_some())
26+
.map(|a| to_opt(a).unwrap());
27+
28+
// similar but different
29+
let _ = iter::once(1)
30+
.filter(|n| to_opt(n).is_some())
31+
.map(|n| to_res(n).unwrap());
32+
let _ = iter::once(1)
33+
.filter(|n| to_opt(n).map(|n| n + 1).is_some())
34+
.map(|a| to_opt(a).unwrap());
35+
}
36+
37+
fn to_opt<T>(_: T) -> Option<T> {
38+
unimplemented!()
39+
}
40+
41+
fn to_res<T>(_: T) -> Result<T, ()> {
42+
unimplemented!()
43+
}

‎tests/ui/manual_filter_map.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// run-rustfix
2+
#![allow(dead_code)]
3+
#![warn(clippy::manual_filter_map)]
4+
#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure
5+
6+
use std::iter;
7+
8+
fn main() {
9+
// is_some(), unwrap()
10+
let _ = iter::once(1)
11+
.filter(|n| to_opt(*n).is_some())
12+
.map(|a| to_opt(a).unwrap());
13+
14+
// ref pattern, expect()
15+
let _ = iter::once(1)
16+
.filter(|&n| to_opt(n).is_some())
17+
.map(|a| to_opt(a).expect("hi"));
18+
19+
// is_ok(), unwrap_or()
20+
let _ = iter::once(1)
21+
.filter(|&n| to_res(n).is_ok())
22+
.map(|a| to_res(a).unwrap_or(1));
23+
}
24+
25+
fn no_lint() {
26+
// no shared code
27+
let _ = iter::once(1).filter(|n| *n > 1).map(|n| n + 1);
28+
29+
// very close but different since filter() provides a reference
30+
let _ = iter::once(1)
31+
.filter(|n| to_opt(n).is_some())
32+
.map(|a| to_opt(a).unwrap());
33+
34+
// similar but different
35+
let _ = iter::once(1)
36+
.filter(|n| to_opt(n).is_some())
37+
.map(|n| to_res(n).unwrap());
38+
let _ = iter::once(1)
39+
.filter(|n| to_opt(n).map(|n| n + 1).is_some())
40+
.map(|a| to_opt(a).unwrap());
41+
}
42+
43+
fn to_opt<T>(_: T) -> Option<T> {
44+
unimplemented!()
45+
}
46+
47+
fn to_res<T>(_: T) -> Result<T, ()> {
48+
unimplemented!()
49+
}

‎tests/ui/manual_filter_map.stderr

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
2+
--> $DIR/manual_filter_map.rs:10:26
3+
|
4+
LL | let _ = iter::once(1)
5+
| __________________________^
6+
LL | | .filter(|n| to_opt(*n).is_some())
7+
LL | | .map(|a| to_opt(a).unwrap());
8+
| |____________________________________^ help: try: `.filter_map(|a| to_opt(a))`
9+
|
10+
= note: `-D clippy::manual-filter-map` implied by `-D warnings`
11+
12+
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
13+
--> $DIR/manual_filter_map.rs:15:26
14+
|
15+
LL | let _ = iter::once(1)
16+
| __________________________^
17+
LL | | .filter(|&n| to_opt(n).is_some())
18+
LL | | .map(|a| to_opt(a).expect("hi"));
19+
| |________________________________________^ help: try: `.filter_map(|a| to_opt(a))`
20+
21+
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
22+
--> $DIR/manual_filter_map.rs:20:26
23+
|
24+
LL | let _ = iter::once(1)
25+
| __________________________^
26+
LL | | .filter(|&n| to_res(n).is_ok())
27+
LL | | .map(|a| to_res(a).unwrap_or(1));
28+
| |________________________________________^ help: try: `.filter_map(|a| to_res(a).ok())`
29+
30+
error: aborting due to 3 previous errors
31+

0 commit comments

Comments
 (0)