squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Split filter_map into manual_filter_map

Browse source
This commit is contained in:
Cameron Steffen 2021-01-01 13:00:09 -06:00
parent 7a8660861e
commit c92bdc4dbb
7 changed files with 198 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -2035,6 +2035,7 @@ Released 2018-09-13
[`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or

3
clippy_lints/src/lib.rs
View file

@ -745,6 +745,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::ITER_NTH,
&methods::ITER_NTH_ZERO,
&methods::ITER_SKIP_NEXT,
&methods::MANUAL_FILTER_MAP,
&methods::MANUAL_SATURATING_ARITHMETIC,
&methods::MAP_COLLECT_RESULT_UNIT,
&methods::MAP_FLATTEN,
@ -1526,6 +1527,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::ITER_NTH),
LintId::of(&methods::ITER_NTH_ZERO),
LintId::of(&methods::ITER_SKIP_NEXT),
LintId::of(&methods::MANUAL_FILTER_MAP),
LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
LintId::of(&methods::MAP_COLLECT_RESULT_UNIT),
LintId::of(&methods::NEW_RET_NO_SELF),
@ -1823,6 +1825,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::FILTER_NEXT),
LintId::of(&methods::FLAT_MAP_IDENTITY),
LintId::of(&methods::INSPECT_FOR_EACH),
LintId::of(&methods::MANUAL_FILTER_MAP),
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::SEARCH_IS_SOME),
LintId::of(&methods::SKIP_WHILE_NEXT),

109
clippy_lints/src/methods/mod.rs
View file

@ -15,7 +15,8 @@ use if_chain::if_chain;
use rustc_ast::ast;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::{TraitItem, TraitItemKind};
use rustc_hir::def::Res;
use rustc_hir::{Expr, ExprKind, PatKind, QPath, TraitItem, TraitItemKind, UnOp};
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::{self, TraitRef, Ty, TyS};
@ -450,6 +451,32 @@ declare_clippy_lint! {
"using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call"
}
declare_clippy_lint! {
/// **What it does:** Checks for usage of `_.filter(_).map(_)` that can be written more simply
/// as `filter_map(_)`.
///
/// **Why is this bad?** Redundant code in the `filter` and `map` operations is poor style and
/// less performant.
///
/// **Known problems:** None.
///
/// **Example:**
/// Bad:
/// ```rust
/// (0_i32..10)
/// .filter(|n| n.checked_add(1).is_some())
/// .map(|n| n.checked_add(1).unwrap());
/// ```
///
/// Good:
/// ```rust
/// (0_i32..10).filter_map(|n| n.checked_add(1));
/// ```
pub MANUAL_FILTER_MAP,
complexity,
"using `_.filter(_).map(_)` in a way that can be written more simply as `filter_map(_)`"
}
declare_clippy_lint! {
/// **What it does:** Checks for usage of `_.filter_map(_).next()`.
///
@ -1473,6 +1500,7 @@ impl_lint_pass!(Methods => [
FILTER_NEXT,
SKIP_WHILE_NEXT,
FILTER_MAP,
MANUAL_FILTER_MAP,
FILTER_MAP_NEXT,
FLAT_MAP_IDENTITY,
FIND_MAP,
@ -1540,7 +1568,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
["next", "iter"] => lint_iter_next(cx, expr, arg_lists[1]),
["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
["map", "filter"] => lint_filter_map(cx, expr),
["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1], self.msrv.as_ref()),
["map", "find"] => lint_find_map(cx, expr, arg_lists[1], arg_lists[0]),
@ -2989,17 +3017,72 @@ fn lint_skip_while_next<'tcx>(
}
/// lint use of `filter().map()` for `Iterators`
fn lint_filter_map<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'_>,
_filter_args: &'tcx [hir::Expr<'_>],
_map_args: &'tcx [hir::Expr<'_>],
) {
// lint if caller of `.filter().map()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `filter(..).map(..)` on an `Iterator`";
let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
fn lint_filter_map<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if_chain! {
if let ExprKind::MethodCall(_, _, [map_recv, map_arg], map_span) = expr.kind;
if let ExprKind::MethodCall(_, _, [_, filter_arg], filter_span) = map_recv.kind;
if match_trait_method(cx, expr, &paths::ITERATOR);
// filter(|x| ...is_some())...
if let ExprKind::Closure(_, _, filter_body_id, ..) = filter_arg.kind;
let filter_body = cx.tcx.hir().body(filter_body_id);
if let [filter_param] = filter_body.params;
// optional ref pattern: `filter(|&x| ..)`
let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
(ref_pat, true)
} else {
(filter_param.pat, false)
};
// closure ends with is_some() or is_ok()
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
if let ExprKind::MethodCall(path, _, [filter_arg], _) = filter_body.value.kind;
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).ty_adt_def();
if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::option_type, opt_ty.did) {
Some(false)
} else if cx.tcx.is_diagnostic_item(sym::result_type, opt_ty.did) {
Some(true)
} else {
None
};
if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" };
// ...map(|x| ...unwrap())
if let ExprKind::Closure(_, _, map_body_id, ..) = map_arg.kind;
let map_body = cx.tcx.hir().body(map_body_id);
if let [map_param] = map_body.params;
if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind;
// closure ends with expect() or unwrap()
if let ExprKind::MethodCall(seg, _, [map_arg, ..], _) = map_body.value.kind;
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
// in `filter(|x| ..)`, replace `*x` with `x`
let a_path = if_chain! {
if !is_filter_param_ref;
if let ExprKind::Unary(UnOp::UnDeref, expr_path) = a.kind;
then { expr_path } else { a }
};
// let the filter closure arg and the map closure arg be equal
if_chain! {
if let ExprKind::Path(QPath::Resolved(None, a_path)) = a_path.kind;
if let ExprKind::Path(QPath::Resolved(None, b_path)) = b.kind;
if a_path.res == Res::Local(filter_param_id);
if b_path.res == Res::Local(map_param_id);
if TyS::same_type(cx.typeck_results().expr_ty_adjusted(a), cx.typeck_results().expr_ty_adjusted(b));
then {
return true;
}
}
false
};
if SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg);
then {
let span = filter_span.to(map_span);
let msg = "`filter(..).map(..)` can be simplified as `filter_map(..)`";
let to_opt = if is_result { ".ok()" } else { "" };
let sugg = format!("filter_map(|{}| {}{})", map_param_ident, snippet(cx, map_arg.span, ".."), to_opt);
span_lint_and_sugg(cx, MANUAL_FILTER_MAP, span, msg, "try", sugg, Applicability::MachineApplicable);
}
}
}

12
tests/ui/filter_methods.stderr
View file

@ -1,12 +1,3 @@
error: called `filter(..).map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:6:21
|
LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::filter-map` implied by `-D warnings`
= help: this is more succinctly expressed by calling `.filter_map(..)` instead
error: called `filter(..).flat_map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:8:21
|
@ -17,6 +8,7 @@ LL | | .filter(|&x| x == 0)
LL | | .flat_map(|x| x.checked_mul(2))
| |_______________________________________^
|
= note: `-D clippy::filter-map` implied by `-D warnings`
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
error: called `filter_map(..).flat_map(..)` on an `Iterator`
@ -43,5 +35,5 @@ LL | | .map(|x| x.checked_mul(2))
|
= help: this is more succinctly expressed by only calling `.filter_map(..)` instead
error: aborting due to 4 previous errors
error: aborting due to 3 previous errors

37
tests/ui/manual_filter_map.fixed Normal file
View file

@ -0,0 +1,37 @@
// run-rustfix
#![allow(dead_code)]
#![warn(clippy::manual_filter_map)]
#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure
fn main() {
// is_some(), unwrap()
let _ = (0..).filter_map(|a| to_opt(a));
// ref pattern, expect()
let _ = (0..).filter_map(|a| to_opt(a));
// is_ok(), unwrap_or()
let _ = (0..).filter_map(|a| to_res(a).ok());
}
fn no_lint() {
// no shared code
let _ = (0..).filter(|n| *n > 1).map(|n| n + 1);
// very close but different since filter() provides a reference
let _ = (0..).filter(|n| to_opt(n).is_some()).map(|a| to_opt(a).unwrap());
// similar but different
let _ = (0..).filter(|n| to_opt(n).is_some()).map(|n| to_res(n).unwrap());
let _ = (0..)
.filter(|n| to_opt(n).map(|n| n + 1).is_some())
.map(|a| to_opt(a).unwrap());
}
fn to_opt<T>(_: T) -> Option<T> {
unimplemented!()
}
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

37
tests/ui/manual_filter_map.rs Normal file
View file

@ -0,0 +1,37 @@
// run-rustfix
#![allow(dead_code)]
#![warn(clippy::manual_filter_map)]
#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure
fn main() {
// is_some(), unwrap()
let _ = (0..).filter(|n| to_opt(*n).is_some()).map(|a| to_opt(a).unwrap());
// ref pattern, expect()
let _ = (0..).filter(|&n| to_opt(n).is_some()).map(|a| to_opt(a).expect("hi"));
// is_ok(), unwrap_or()
let _ = (0..).filter(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1));
}
fn no_lint() {
// no shared code
let _ = (0..).filter(|n| *n > 1).map(|n| n + 1);
// very close but different since filter() provides a reference
let _ = (0..).filter(|n| to_opt(n).is_some()).map(|a| to_opt(a).unwrap());
// similar but different
let _ = (0..).filter(|n| to_opt(n).is_some()).map(|n| to_res(n).unwrap());
let _ = (0..)
.filter(|n| to_opt(n).map(|n| n + 1).is_some())
.map(|a| to_opt(a).unwrap());
}
fn to_opt<T>(_: T) -> Option<T> {
unimplemented!()
}
fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

22
tests/ui/manual_filter_map.stderr Normal file
View file

@ -0,0 +1,22 @@
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:8:19
|
LL | let _ = (0..).filter(|n| to_opt(*n).is_some()).map(|a| to_opt(a).unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter_map(|a| to_opt(a))`
|
= note: `-D clippy::manual-filter-map` implied by `-D warnings`
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:11:19
|
LL | let _ = (0..).filter(|&n| to_opt(n).is_some()).map(|a| to_opt(a).expect("hi"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter_map(|a| to_opt(a))`
error: `filter(..).map(..)` can be simplified as `filter_map(..)`
--> $DIR/manual_filter_map.rs:14:19
|
LL | let _ = (0..).filter(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter_map(|a| to_res(a).ok())`
error: aborting due to 3 previous errors