From a60c143fe0cec1125e894f3d2d5008cc317fdacf Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Wed, 3 Feb 2021 23:24:06 +0900 Subject: [PATCH 1/2] Add new lint `filter_map_identity` This commit adds a new lint named `filter_map_identity`. This lint is the same as `flat_map_identity` except that it checks for `filter_map`. Closes #6643 --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + .../src/methods/filter_map_identity.rs | 56 +++++++++++++++++++ clippy_lints/src/methods/mod.rs | 30 +++++++++- tests/ui/filter_map_identity.fixed | 16 ++++++ tests/ui/filter_map_identity.rs | 16 ++++++ tests/ui/filter_map_identity.stderr | 22 ++++++++ 7 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/methods/filter_map_identity.rs create mode 100644 tests/ui/filter_map_identity.fixed create mode 100644 tests/ui/filter_map_identity.rs create mode 100644 tests/ui/filter_map_identity.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 45321751ba7e..f32f5529751b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1955,6 +1955,7 @@ Released 2018-09-13 [`field_reassign_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default [`filetype_is_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#filetype_is_file [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map +[`filter_map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_identity [`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next [`find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#find_map diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7f5c4f56f9e0..760b490dc129 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -742,6 +742,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::EXPECT_USED, &methods::FILETYPE_IS_FILE, &methods::FILTER_MAP, + &methods::FILTER_MAP_IDENTITY, &methods::FILTER_MAP_NEXT, &methods::FILTER_NEXT, &methods::FLAT_MAP_IDENTITY, @@ -1531,6 +1532,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::CLONE_DOUBLE_REF), LintId::of(&methods::CLONE_ON_COPY), LintId::of(&methods::EXPECT_FUN_CALL), + LintId::of(&methods::FILTER_MAP_IDENTITY), LintId::of(&methods::FILTER_NEXT), LintId::of(&methods::FLAT_MAP_IDENTITY), LintId::of(&methods::FROM_ITER_INSTEAD_OF_COLLECT), @@ -1838,6 +1840,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::WILDCARD_IN_OR_PATTERNS), LintId::of(&methods::BIND_INSTEAD_OF_MAP), LintId::of(&methods::CLONE_ON_COPY), + LintId::of(&methods::FILTER_MAP_IDENTITY), LintId::of(&methods::FILTER_NEXT), LintId::of(&methods::FLAT_MAP_IDENTITY), LintId::of(&methods::INSPECT_FOR_EACH), diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs new file mode 100644 index 000000000000..d04e4be87ac2 --- /dev/null +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -0,0 +1,56 @@ +use crate::utils::{match_qpath, match_trait_method, paths, span_lint_and_sugg}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_span::source_map::Span; + +use super::FILTER_MAP_IDENTITY; + +pub(super) fn check( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + filter_map_args: &[hir::Expr<'_>], + filter_map_span: Span, +) { + if match_trait_method(cx, expr, &paths::ITERATOR) { + let arg_node = &filter_map_args[1].kind; + + let apply_lint = |message: &str| { + span_lint_and_sugg( + cx, + FILTER_MAP_IDENTITY, + filter_map_span.with_hi(expr.span.hi()), + message, + "try", + "flatten()".to_string(), + Applicability::MachineApplicable, + ); + }; + + if_chain! { + if let hir::ExprKind::Closure(_, _, body_id, _, _) = arg_node; + let body = cx.tcx.hir().body(*body_id); + + if let hir::PatKind::Binding(_, _, binding_ident, _) = body.params[0].pat.kind; + if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = body.value.kind; + + if path.segments.len() == 1; + if path.segments[0].ident.name == binding_ident.name; + + then { + apply_lint("called `filter_map(|x| x)` on an `Iterator`"); + } + } + + if_chain! { + if let hir::ExprKind::Path(ref qpath) = arg_node; + + if match_qpath(qpath, &paths::STD_CONVERT_IDENTITY); + + then { + apply_lint("called `filter_map(std::convert::identity)` on an `Iterator`"); + } + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a17c5996293e..2f68bc0121aa 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1,4 +1,5 @@ mod bind_instead_of_map; +mod filter_map_identity; mod inefficient_to_string; mod inspect_for_each; mod manual_saturating_arithmetic; @@ -1467,6 +1468,29 @@ declare_clippy_lint! { "using `.inspect().for_each()`, which can be replaced with `.for_each()`" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `filter_map(|x| x)`. + /// + /// **Why is this bad?** Readability, this can be written more concisely by using `flatten`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// # let iter = vec![Some(1)].into_iter(); + /// iter.filter_map(|x| x); + /// ``` + /// Use instead: + /// ```rust + /// # let iter = vec![Some(1)].into_iter(); + /// iter.flatten(); + /// ``` + pub FILTER_MAP_IDENTITY, + complexity, + "call to `filter_map` where `flatten` is sufficient" +} + pub struct Methods { msrv: Option, } @@ -1504,6 +1528,7 @@ impl_lint_pass!(Methods => [ FILTER_NEXT, SKIP_WHILE_NEXT, FILTER_MAP, + FILTER_MAP_IDENTITY, MANUAL_FILTER_MAP, MANUAL_FIND_MAP, FILTER_MAP_NEXT, @@ -1597,7 +1622,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods { ["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]), ["as_mut"] => lint_asref(cx, expr, "as_mut", arg_lists[0]), ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]), - ["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]), + ["filter_map", ..] => { + unnecessary_filter_map::lint(cx, expr, arg_lists[0]); + filter_map_identity::check(cx, expr, arg_lists[0], method_spans[0]); + }, ["count", "map"] => lint_suspicious_map(cx, expr), ["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr), ["unwrap_or", arith @ ("checked_add" | "checked_sub" | "checked_mul")] => { diff --git a/tests/ui/filter_map_identity.fixed b/tests/ui/filter_map_identity.fixed new file mode 100644 index 000000000000..23ce28d8e9be --- /dev/null +++ b/tests/ui/filter_map_identity.fixed @@ -0,0 +1,16 @@ +// run-rustfix + +#![allow(unused_imports)] +#![warn(clippy::filter_map_identity)] + +fn main() { + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.flatten(); + + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.flatten(); + + use std::convert::identity; + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.flatten(); +} diff --git a/tests/ui/filter_map_identity.rs b/tests/ui/filter_map_identity.rs new file mode 100644 index 000000000000..e698df13eea4 --- /dev/null +++ b/tests/ui/filter_map_identity.rs @@ -0,0 +1,16 @@ +// run-rustfix + +#![allow(unused_imports)] +#![warn(clippy::filter_map_identity)] + +fn main() { + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.filter_map(|x| x); + + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.filter_map(std::convert::identity); + + use std::convert::identity; + let iterator = vec![Some(1), None, Some(2)].into_iter(); + let _ = iterator.filter_map(identity); +} diff --git a/tests/ui/filter_map_identity.stderr b/tests/ui/filter_map_identity.stderr new file mode 100644 index 000000000000..596a6320608c --- /dev/null +++ b/tests/ui/filter_map_identity.stderr @@ -0,0 +1,22 @@ +error: called `filter_map(|x| x)` on an `Iterator` + --> $DIR/filter_map_identity.rs:8:22 + | +LL | let _ = iterator.filter_map(|x| x); + | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` + | + = note: `-D clippy::filter-map-identity` implied by `-D warnings` + +error: called `filter_map(std::convert::identity)` on an `Iterator` + --> $DIR/filter_map_identity.rs:11:22 + | +LL | let _ = iterator.filter_map(std::convert::identity); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: called `filter_map(std::convert::identity)` on an `Iterator` + --> $DIR/filter_map_identity.rs:15:22 + | +LL | let _ = iterator.filter_map(identity); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: aborting due to 3 previous errors + From fbe436b1d4369603a6f89cbb8fb382ef5fe210f7 Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Sat, 6 Feb 2021 18:06:58 +0900 Subject: [PATCH 2/2] Use flatten instead of filter_map --- clippy_lints/src/loops.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 663c2df23e22..ac4005d0cfde 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -739,7 +739,7 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult { let stmts = block.stmts.iter().map(stmt_to_expr); let expr = once(block.expr.as_deref()); - let mut iter = stmts.chain(expr).filter_map(|e| e); + let mut iter = stmts.chain(expr).flatten(); never_loop_expr_seq(&mut iter, main_loop_id) }