From e89ad4ba71816cbe6fb1f90743d9229fc824b1c0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 13:50:53 -0600 Subject: [PATCH 1/6] Fix comment --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 623554ea2d15..c8b69f4fbaea 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3059,7 +3059,7 @@ fn lint_filter_map_map<'tcx>( _filter_args: &'tcx [hir::Expr<'_>], _map_args: &'tcx [hir::Expr<'_>], ) { - // lint if caller of `.filter().map()` is an Iterator + // lint if caller of `.filter_map().map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { let msg = "called `filter_map(..).map(..)` on an `Iterator`"; let hint = "this is more succinctly expressed by only calling `.filter_map(..)` instead"; From 7a8660861ecf11474e03823387a50eeaa2c18a57 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 1 Jan 2021 13:00:44 -0600 Subject: [PATCH 2/6] Add expr_fallback to SpanlessEq --- clippy_lints/src/utils/hir_utils.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index 10120a8805db..1c7398f17f2e 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -24,6 +24,7 @@ pub struct SpanlessEq<'a, 'tcx> { cx: &'a LateContext<'tcx>, maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, allow_side_effects: bool, + expr_fallback: Option, &Expr<'_>) -> bool + 'a>>, } impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { @@ -32,6 +33,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { cx, maybe_typeck_results: cx.maybe_typeck_results(), allow_side_effects: true, + expr_fallback: None, } } @@ -43,6 +45,13 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } } + pub fn expr_fallback(self, expr_fallback: impl Fn(&Expr<'_>, &Expr<'_>) -> bool + 'a) -> Self { + Self { + expr_fallback: Some(Box::new(expr_fallback)), + ..self + } + } + /// Checks whether two statements are the same. pub fn eq_stmt(&mut self, left: &Stmt<'_>, right: &Stmt<'_>) -> bool { match (&left.kind, &right.kind) { @@ -81,7 +90,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } } - match (&reduce_exprkind(&left.kind), &reduce_exprkind(&right.kind)) { + let is_eq = match (&reduce_exprkind(&left.kind), &reduce_exprkind(&right.kind)) { (&ExprKind::AddrOf(lb, l_mut, ref le), &ExprKind::AddrOf(rb, r_mut, ref re)) => { lb == rb && l_mut == r_mut && self.eq_expr(le, re) }, @@ -158,7 +167,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { (&ExprKind::Array(l), &ExprKind::Array(r)) => self.eq_exprs(l, r), (&ExprKind::DropTemps(ref le), &ExprKind::DropTemps(ref re)) => self.eq_expr(le, re), _ => false, - } + }; + is_eq || self.expr_fallback.as_ref().map_or(false, |f| f(left, right)) } fn eq_exprs(&mut self, left: &[Expr<'_>], right: &[Expr<'_>]) -> bool { From c92bdc4dbbd777f6933f7990f87066147a629c8d Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 1 Jan 2021 13:00:09 -0600 Subject: [PATCH 3/6] Split filter_map into manual_filter_map --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/methods/mod.rs | 109 ++++++++++++++++++++++++++---- tests/ui/filter_methods.stderr | 12 +--- tests/ui/manual_filter_map.fixed | 37 ++++++++++ tests/ui/manual_filter_map.rs | 37 ++++++++++ tests/ui/manual_filter_map.stderr | 22 ++++++ 7 files changed, 198 insertions(+), 23 deletions(-) create mode 100644 tests/ui/manual_filter_map.fixed create mode 100644 tests/ui/manual_filter_map.rs create mode 100644 tests/ui/manual_filter_map.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 04f042b2debf..8feb1a148afa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 53764bb73903..bde9c630c6c7 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c8b69f4fbaea..518d2e67ad16 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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); + } } } diff --git a/tests/ui/filter_methods.stderr b/tests/ui/filter_methods.stderr index 119226813793..c7b4f28be3a4 100644 --- a/tests/ui/filter_methods.stderr +++ b/tests/ui/filter_methods.stderr @@ -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 diff --git a/tests/ui/manual_filter_map.fixed b/tests/ui/manual_filter_map.fixed new file mode 100644 index 000000000000..fc8f58f8ea5c --- /dev/null +++ b/tests/ui/manual_filter_map.fixed @@ -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) -> Option { + unimplemented!() +} + +fn to_res(_: T) -> Result { + unimplemented!() +} diff --git a/tests/ui/manual_filter_map.rs b/tests/ui/manual_filter_map.rs new file mode 100644 index 000000000000..3af4bbee3bf8 --- /dev/null +++ b/tests/ui/manual_filter_map.rs @@ -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) -> Option { + unimplemented!() +} + +fn to_res(_: T) -> Result { + unimplemented!() +} diff --git a/tests/ui/manual_filter_map.stderr b/tests/ui/manual_filter_map.stderr new file mode 100644 index 000000000000..4d4e2d5c12fe --- /dev/null +++ b/tests/ui/manual_filter_map.stderr @@ -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 + From a752d31e0a8cd8c7d8549daf421a8b791d1325a4 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 14 Jan 2021 16:36:36 -0600 Subject: [PATCH 4/6] Replace find_map with manual_find_map --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/methods/mod.rs | 64 +++++++++++++++++++++------------ tests/ui/find_map.stderr | 26 -------------- tests/ui/manual_find_map.fixed | 37 +++++++++++++++++++ tests/ui/manual_find_map.rs | 37 +++++++++++++++++++ tests/ui/manual_find_map.stderr | 22 ++++++++++++ 7 files changed, 141 insertions(+), 49 deletions(-) delete mode 100644 tests/ui/find_map.stderr create mode 100644 tests/ui/manual_find_map.fixed create mode 100644 tests/ui/manual_find_map.rs create mode 100644 tests/ui/manual_find_map.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 8feb1a148afa..7f2de888d35f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2036,6 +2036,7 @@ Released 2018-09-13 [`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_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bde9c630c6c7..b22ddfacf86a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -746,6 +746,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::ITER_NTH_ZERO, &methods::ITER_SKIP_NEXT, &methods::MANUAL_FILTER_MAP, + &methods::MANUAL_FIND_MAP, &methods::MANUAL_SATURATING_ARITHMETIC, &methods::MAP_COLLECT_RESULT_UNIT, &methods::MAP_FLATTEN, @@ -1528,6 +1529,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::ITER_NTH_ZERO), LintId::of(&methods::ITER_SKIP_NEXT), LintId::of(&methods::MANUAL_FILTER_MAP), + LintId::of(&methods::MANUAL_FIND_MAP), LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC), LintId::of(&methods::MAP_COLLECT_RESULT_UNIT), LintId::of(&methods::NEW_RET_NO_SELF), @@ -1826,6 +1828,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::FLAT_MAP_IDENTITY), LintId::of(&methods::INSPECT_FOR_EACH), LintId::of(&methods::MANUAL_FILTER_MAP), + LintId::of(&methods::MANUAL_FIND_MAP), LintId::of(&methods::OPTION_AS_REF_DEREF), LintId::of(&methods::SEARCH_IS_SOME), LintId::of(&methods::SKIP_WHILE_NEXT), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 518d2e67ad16..9d0785887467 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -477,6 +477,32 @@ declare_clippy_lint! { "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 `_.find(_).map(_)` that can be written more simply + /// as `find_map(_)`. + /// + /// **Why is this bad?** Redundant code in the `find` and `map` operations is poor style and + /// less performant. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Bad: + /// ```rust + /// (0_i32..10) + /// .find(|n| n.checked_add(1).is_some()) + /// .map(|n| n.checked_add(1).unwrap()); + /// ``` + /// + /// Good: + /// ```rust + /// (0_i32..10).find_map(|n| n.checked_add(1)); + /// ``` + pub MANUAL_FIND_MAP, + complexity, + "using `_.find(_).map(_)` in a way that can be written more simply as `find_map(_)`" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `_.filter_map(_).next()`. /// @@ -1501,6 +1527,7 @@ impl_lint_pass!(Methods => [ SKIP_WHILE_NEXT, FILTER_MAP, MANUAL_FILTER_MAP, + MANUAL_FIND_MAP, FILTER_MAP_NEXT, FLAT_MAP_IDENTITY, FIND_MAP, @@ -1568,10 +1595,10 @@ 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), + ["map", "filter"] => lint_filter_map(cx, expr, false), ["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]), + ["map", "find"] => lint_filter_map(cx, expr, true), ["flat_map", "filter"] => lint_filter_flat_map(cx, expr, arg_lists[1], arg_lists[0]), ["flat_map", "filter_map"] => lint_filter_map_flat_map(cx, expr, arg_lists[1], arg_lists[0]), ["flat_map", ..] => lint_flat_map_identity(cx, expr, arg_lists[0], method_spans[0]), @@ -3016,12 +3043,12 @@ 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<'_>) { +/// lint use of `filter().map()` or `find().map()` for `Iterators` +fn lint_filter_map<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_find: bool) { 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); + if match_trait_method(cx, map_recv, &paths::ITERATOR); // filter(|x| ...is_some())... if let ExprKind::Closure(_, _, filter_body_id, ..) = filter_arg.kind; @@ -3078,10 +3105,16 @@ fn lint_filter_map<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { 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 (filter_name, lint) = if is_find { + ("find", MANUAL_FIND_MAP) + } else { + ("filter", MANUAL_FILTER_MAP) + }; + let msg = format!("`{}(..).map(..)` can be simplified as `{0}_map(..)`", filter_name); 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); + let sugg = format!("{}_map(|{}| {}{})", filter_name, map_param_ident, + snippet(cx, map_arg.span, ".."), to_opt); + span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable); } } } @@ -3120,21 +3153,6 @@ fn lint_filter_map_next<'tcx>( } } -/// lint use of `find().map()` for `Iterators` -fn lint_find_map<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx hir::Expr<'_>, - _find_args: &'tcx [hir::Expr<'_>], - map_args: &'tcx [hir::Expr<'_>], -) { - // lint if caller of `.filter().map()` is an Iterator - if match_trait_method(cx, &map_args[0], &paths::ITERATOR) { - let msg = "called `find(..).map(..)` on an `Iterator`"; - let hint = "this is more succinctly expressed by calling `.find_map(..)` instead"; - span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint); - } -} - /// lint use of `filter_map().map()` for `Iterators` fn lint_filter_map_map<'tcx>( cx: &LateContext<'tcx>, diff --git a/tests/ui/find_map.stderr b/tests/ui/find_map.stderr deleted file mode 100644 index aea3cc62afcc..000000000000 --- a/tests/ui/find_map.stderr +++ /dev/null @@ -1,26 +0,0 @@ -error: called `find(..).map(..)` on an `Iterator` - --> $DIR/find_map.rs:20:26 - | -LL | let _: Option = a.iter().find(|s| s.parse::().is_ok()).map(|s| s.parse().unwrap()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::find-map` implied by `-D warnings` - = help: this is more succinctly expressed by calling `.find_map(..)` instead - -error: called `find(..).map(..)` on an `Iterator` - --> $DIR/find_map.rs:23:29 - | -LL | let _: Option = desserts_of_the_week - | _____________________________^ -LL | | .iter() -LL | | .find(|dessert| match *dessert { -LL | | Dessert::Cake(_) => true, -... | -LL | | _ => unreachable!(), -LL | | }); - | |__________^ - | - = help: this is more succinctly expressed by calling `.find_map(..)` instead - -error: aborting due to 2 previous errors - diff --git a/tests/ui/manual_find_map.fixed b/tests/ui/manual_find_map.fixed new file mode 100644 index 000000000000..95e97c4fd1ff --- /dev/null +++ b/tests/ui/manual_find_map.fixed @@ -0,0 +1,37 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::manual_find_map)] +#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure + +fn main() { + // is_some(), unwrap() + let _ = (0..).find_map(|a| to_opt(a)); + + // ref pattern, expect() + let _ = (0..).find_map(|a| to_opt(a)); + + // is_ok(), unwrap_or() + let _ = (0..).find_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..).find(|n| to_opt(n).is_some()).map(|a| to_opt(a).unwrap()); + + // similar but different + let _ = (0..).find(|n| to_opt(n).is_some()).map(|n| to_res(n).unwrap()); + let _ = (0..) + .find(|n| to_opt(n).map(|n| n + 1).is_some()) + .map(|a| to_opt(a).unwrap()); +} + +fn to_opt(_: T) -> Option { + unimplemented!() +} + +fn to_res(_: T) -> Result { + unimplemented!() +} diff --git a/tests/ui/manual_find_map.rs b/tests/ui/manual_find_map.rs new file mode 100644 index 000000000000..cd3c82e3b25a --- /dev/null +++ b/tests/ui/manual_find_map.rs @@ -0,0 +1,37 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::manual_find_map)] +#![allow(clippy::redundant_closure)] // FIXME suggestion may have redundant closure + +fn main() { + // is_some(), unwrap() + let _ = (0..).find(|n| to_opt(*n).is_some()).map(|a| to_opt(a).unwrap()); + + // ref pattern, expect() + let _ = (0..).find(|&n| to_opt(n).is_some()).map(|a| to_opt(a).expect("hi")); + + // is_ok(), unwrap_or() + let _ = (0..).find(|&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..).find(|n| to_opt(n).is_some()).map(|a| to_opt(a).unwrap()); + + // similar but different + let _ = (0..).find(|n| to_opt(n).is_some()).map(|n| to_res(n).unwrap()); + let _ = (0..) + .find(|n| to_opt(n).map(|n| n + 1).is_some()) + .map(|a| to_opt(a).unwrap()); +} + +fn to_opt(_: T) -> Option { + unimplemented!() +} + +fn to_res(_: T) -> Result { + unimplemented!() +} diff --git a/tests/ui/manual_find_map.stderr b/tests/ui/manual_find_map.stderr new file mode 100644 index 000000000000..9e7f798df457 --- /dev/null +++ b/tests/ui/manual_find_map.stderr @@ -0,0 +1,22 @@ +error: `find(..).map(..)` can be simplified as `find_map(..)` + --> $DIR/manual_find_map.rs:8:19 + | +LL | let _ = (0..).find(|n| to_opt(*n).is_some()).map(|a| to_opt(a).unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `find_map(|a| to_opt(a))` + | + = note: `-D clippy::manual-find-map` implied by `-D warnings` + +error: `find(..).map(..)` can be simplified as `find_map(..)` + --> $DIR/manual_find_map.rs:11:19 + | +LL | let _ = (0..).find(|&n| to_opt(n).is_some()).map(|a| to_opt(a).expect("hi")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `find_map(|a| to_opt(a))` + +error: `find(..).map(..)` can be simplified as `find_map(..)` + --> $DIR/manual_find_map.rs:14:19 + | +LL | let _ = (0..).find(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `find_map(|a| to_res(a).ok())` + +error: aborting due to 3 previous errors + From a22915bf4893e16d64100365d902e52211374a0c Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 14 Dec 2020 14:03:11 -0600 Subject: [PATCH 5/6] Remove unneeded allow's --- clippy_dev/src/ra_setup.rs | 2 -- clippy_lints/src/loops.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/clippy_dev/src/ra_setup.rs b/clippy_dev/src/ra_setup.rs index 5f5048e79e78..a8a6a2cb1bd6 100644 --- a/clippy_dev/src/ra_setup.rs +++ b/clippy_dev/src/ra_setup.rs @@ -1,5 +1,3 @@ -#![allow(clippy::filter_map)] - use std::fs; use std::fs::File; use std::io::prelude::*; diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 1c5ab2874b04..c7e1088e3530 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1069,7 +1069,6 @@ fn get_assignments<'a: 'c, 'tcx: 'c, 'c>( ) -> impl Iterator, &'tcx Expr<'tcx>)>> + 'c { // As the `filter` and `map` below do different things, I think putting together // just increases complexity. (cc #3188 and #4193) - #[allow(clippy::filter_map)] stmts .iter() .filter_map(move |stmt| match stmt.kind { From 82bab19a0105ff86176b79a5eae7a9ea7d300cb9 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 14 Jan 2021 16:47:22 -0600 Subject: [PATCH 6/6] Deprecate find_map lint --- clippy_lints/src/deprecated_lints.rs | 5 +++++ clippy_lints/src/lib.rs | 6 ++++-- clippy_lints/src/methods/mod.rs | 23 ----------------------- 3 files changed, 9 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index bec0c9f93a0d..ea386237f20a 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/clippy_lints/src/deprecated_lints.rs @@ -166,3 +166,8 @@ declare_deprecated_lint! { pub PANIC_PARAMS, "this lint has been uplifted to rustc and is now called `panic_fmt`" } + +declare_deprecated_lint! { + pub FIND_MAP, + "this lint is replaced by `manual_find_map`, a more specific lint" +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b22ddfacf86a..7097812e088e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -505,6 +505,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: "clippy::panic_params", "this lint has been uplifted to rustc and is now called `panic_fmt`", ); + store.register_removed( + "clippy::find_map", + "this lint is replaced by `manual_find_map`, a more specific lint", + ); // end deprecated lints, do not remove this comment, it’s used in `update_lints` // begin register lints, do not remove this comment, it’s used in `update_lints` @@ -732,7 +736,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::FILTER_MAP, &methods::FILTER_MAP_NEXT, &methods::FILTER_NEXT, - &methods::FIND_MAP, &methods::FLAT_MAP_IDENTITY, &methods::FROM_ITER_INSTEAD_OF_COLLECT, &methods::GET_UNWRAP, @@ -1333,7 +1336,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::SINGLE_MATCH_ELSE), LintId::of(&methods::FILTER_MAP), LintId::of(&methods::FILTER_MAP_NEXT), - LintId::of(&methods::FIND_MAP), LintId::of(&methods::INEFFICIENT_TO_STRING), LintId::of(&methods::MAP_FLATTEN), LintId::of(&methods::MAP_UNWRAP_OR), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9d0785887467..7a459a440cae 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -547,28 +547,6 @@ declare_clippy_lint! { "call to `flat_map` where `flatten` is sufficient" } -declare_clippy_lint! { - /// **What it does:** Checks for usage of `_.find(_).map(_)`. - /// - /// **Why is this bad?** Readability, this can be written more concisely as - /// `_.find_map(_)`. - /// - /// **Known problems:** Often requires a condition + Option/Iterator creation - /// inside the closure. - /// - /// **Example:** - /// ```rust - /// (0..3).find(|x| *x == 2).map(|x| x * 2); - /// ``` - /// Can be written as - /// ```rust - /// (0..3).find_map(|x| if x == 2 { Some(x * 2) } else { None }); - /// ``` - pub FIND_MAP, - pedantic, - "using a combination of `find` and `map` can usually be written as a single method call" -} - declare_clippy_lint! { /// **What it does:** Checks for an iterator or string search (such as `find()`, /// `position()`, or `rposition()`) followed by a call to `is_some()`. @@ -1530,7 +1508,6 @@ impl_lint_pass!(Methods => [ MANUAL_FIND_MAP, FILTER_MAP_NEXT, FLAT_MAP_IDENTITY, - FIND_MAP, MAP_FLATTEN, ITERATOR_STEP_BY_ZERO, ITER_NEXT_SLICE,