diff --git a/CHANGELOG.md b/CHANGELOG.md index 8feb1a148af..7f2de888d35 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 bde9c630c6c..b22ddfacf86 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 518d2e67ad1..9d078588746 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 aea3cc62afc..00000000000 --- 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 00000000000..95e97c4fd1f --- /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 00000000000..cd3c82e3b25 --- /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 00000000000..9e7f798df45 --- /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 +