From 978b1daf99d8326718684381704902fdaaf71b18 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Thu, 6 Jul 2023 15:56:52 -0500 Subject: [PATCH] New lint [`filter_map_bool_then`] --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/methods/filter_map_bool_then.rs | 39 +++++++++++++++++++ clippy_lints/src/methods/mod.rs | 34 ++++++++++++++++ clippy_utils/src/paths.rs | 2 + tests/ui/filter_map_bool_then.fixed | 33 ++++++++++++++++ tests/ui/filter_map_bool_then.rs | 33 ++++++++++++++++ tests/ui/filter_map_bool_then.stderr | 28 +++++++++++++ 8 files changed, 171 insertions(+) create mode 100644 clippy_lints/src/methods/filter_map_bool_then.rs create mode 100644 tests/ui/filter_map_bool_then.fixed create mode 100644 tests/ui/filter_map_bool_then.rs create mode 100644 tests/ui/filter_map_bool_then.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 02d12d60e..251f32d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4844,6 +4844,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_bool_then`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_bool_then [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 9b99a2554..e08447143 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -337,6 +337,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::EXPECT_USED_INFO, crate::methods::EXTEND_WITH_DRAIN_INFO, crate::methods::FILETYPE_IS_FILE_INFO, + crate::methods::FILTER_MAP_BOOL_THEN_INFO, crate::methods::FILTER_MAP_IDENTITY_INFO, crate::methods::FILTER_MAP_NEXT_INFO, crate::methods::FILTER_NEXT_INFO, diff --git a/clippy_lints/src/methods/filter_map_bool_then.rs b/clippy_lints/src/methods/filter_map_bool_then.rs new file mode 100644 index 000000000..e5d8be21f --- /dev/null +++ b/clippy_lints/src/methods/filter_map_bool_then.rs @@ -0,0 +1,39 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::paths::BOOL_THEN; +use clippy_utils::source::snippet_opt; +use clippy_utils::{is_from_proc_macro, match_def_path, peel_blocks}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_span::Span; + +use super::FILTER_MAP_BOOL_THEN; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, arg: &Expr<'_>, call_span: Span) { + if !in_external_macro(cx.sess(), expr.span) + && let ExprKind::Closure(closure) = arg.kind + && let body = peel_blocks(cx.tcx.hir().body(closure.body).value) + && let ExprKind::MethodCall(_, recv, [then_arg], _) = body.kind + && let ExprKind::Closure(then_closure) = then_arg.kind + && let then_body = peel_blocks(cx.tcx.hir().body(then_closure.body).value) + && let Some(def_id) = cx.typeck_results().type_dependent_def_id(body.hir_id) + && match_def_path(cx, def_id, &BOOL_THEN) + && !is_from_proc_macro(cx, expr) + && let Some(decl_snippet) = closure.fn_arg_span.and_then(|s| snippet_opt(cx, s)) + // NOTE: This will include the `()` (parenthesis) around it. Maybe there's some utils method + // to remove them? `unused_parens` will already take care of this but it may be nice. + && let Some(filter) = snippet_opt(cx, recv.span) + && let Some(map) = snippet_opt(cx, then_body.span) + { + span_lint_and_sugg( + cx, + FILTER_MAP_BOOL_THEN, + call_span, + "usage of `bool::then` in `filter_map`", + "use `filter` then `map` instead", + format!("filter({decl_snippet} {filter}).map({decl_snippet} {map})"), + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 105828967..e77445001 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -21,6 +21,7 @@ mod expect_used; mod extend_with_drain; mod filetype_is_file; mod filter_map; +mod filter_map_bool_then; mod filter_map_identity; mod filter_map_next; mod filter_next; @@ -3475,6 +3476,37 @@ declare_clippy_lint! { "disallows `.skip(0)`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `bool::then` in `Iterator::filter_map`. + /// + /// ### Why is this bad? + /// This can be written with `filter` then `map` instead, which would reduce nesting and + /// separates the filtering from the transformation phase. This comes with no cost to + /// performance and is just cleaner. + /// + /// ### Limitations + /// Does not lint `bool::then_some`, as it eagerly evaluates its arguments rather than lazily. + /// This can create differing behavior, so better safe than sorry. + /// + /// ### Example + /// ```rust + /// # fn really_expensive_fn(i: i32) -> i32 { i } + /// # let v = vec![]; + /// _ = v.into_iter().filter_map(|i| (i % 2 == 0).then(|| really_expensive_fn(i))); + /// ``` + /// Use instead: + /// ```rust + /// # fn really_expensive_fn(i: i32) -> i32 { i } + /// # let v = vec![]; + /// _ = v.into_iter().filter(|i| i % 2 == 0).map(|i| really_expensive_fn(i)); + /// ``` + #[clippy::version = "1.72.0"] + pub FILTER_MAP_BOOL_THEN, + style, + "checks for usage of `bool::then` in `Iterator::filter_map`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3612,6 +3644,7 @@ impl_lint_pass!(Methods => [ FORMAT_COLLECT, STRING_LIT_CHARS_ANY, ITER_SKIP_ZERO, + FILTER_MAP_BOOL_THEN, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3889,6 +3922,7 @@ impl Methods { }, ("filter_map", [arg]) => { unnecessary_filter_map::check(cx, expr, arg, name); + filter_map_bool_then::check(cx, expr, arg, call_span); filter_map_identity::check(cx, expr, arg, span); }, ("find_map", [arg]) => { diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 8d96d3cfe..914ea85ac 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -163,3 +163,5 @@ pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"]; pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"]; pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"]; pub const ORD_CMP: [&str; 4] = ["core", "cmp", "Ord", "cmp"]; +#[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so +pub const BOOL_THEN: [&str; 4] = ["core", "bool", "", "then"]; diff --git a/tests/ui/filter_map_bool_then.fixed b/tests/ui/filter_map_bool_then.fixed new file mode 100644 index 000000000..0db0b0a8a --- /dev/null +++ b/tests/ui/filter_map_bool_then.fixed @@ -0,0 +1,33 @@ +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::clone_on_copy, clippy::unnecessary_lazy_evaluations, unused)] +#![warn(clippy::filter_map_bool_then)] + +#[macro_use] +extern crate proc_macros; + +fn main() { + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter(|i| (i % 2 == 0)).map(|i| i + 1); + v.clone() + .into_iter() + .filter(|i| (i % 2 == 0)).map(|i| i + 1); + v.clone() + .into_iter() + .filter(|&i| i != 1000) + .filter(|i| (i % 2 == 0)).map(|i| i + 1); + v.iter() + .copied() + .filter(|&i| i != 1000) + .filter(|i| (i.clone() % 2 == 0)).map(|i| i + 1); + // Do not lint + external! { + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + } + with_span! { + span + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + } +} diff --git a/tests/ui/filter_map_bool_then.rs b/tests/ui/filter_map_bool_then.rs new file mode 100644 index 000000000..764f9c4a8 --- /dev/null +++ b/tests/ui/filter_map_bool_then.rs @@ -0,0 +1,33 @@ +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::clone_on_copy, clippy::unnecessary_lazy_evaluations, unused)] +#![warn(clippy::filter_map_bool_then)] + +#[macro_use] +extern crate proc_macros; + +fn main() { + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + v.clone() + .into_iter() + .filter_map(|i| -> Option<_> { (i % 2 == 0).then(|| i + 1) }); + v.clone() + .into_iter() + .filter(|&i| i != 1000) + .filter_map(|i| (i % 2 == 0).then(|| i + 1)); + v.iter() + .copied() + .filter(|&i| i != 1000) + .filter_map(|i| (i.clone() % 2 == 0).then(|| i + 1)); + // Do not lint + external! { + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + } + with_span! { + span + let v = vec![1, 2, 3, 4, 5, 6]; + v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + } +} diff --git a/tests/ui/filter_map_bool_then.stderr b/tests/ui/filter_map_bool_then.stderr new file mode 100644 index 000000000..79bc7ba02 --- /dev/null +++ b/tests/ui/filter_map_bool_then.stderr @@ -0,0 +1,28 @@ +error: usage of `bool::then` in `filter_map` + --> $DIR/filter_map_bool_then.rs:11:27 + | +LL | v.clone().into_iter().filter_map(|i| (i % 2 == 0).then(|| i + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|i| (i % 2 == 0)).map(|i| i + 1)` + | + = note: `-D clippy::filter-map-bool-then` implied by `-D warnings` + +error: usage of `bool::then` in `filter_map` + --> $DIR/filter_map_bool_then.rs:14:10 + | +LL | .filter_map(|i| -> Option<_> { (i % 2 == 0).then(|| i + 1) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|i| (i % 2 == 0)).map(|i| i + 1)` + +error: usage of `bool::then` in `filter_map` + --> $DIR/filter_map_bool_then.rs:18:10 + | +LL | .filter_map(|i| (i % 2 == 0).then(|| i + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|i| (i % 2 == 0)).map(|i| i + 1)` + +error: usage of `bool::then` in `filter_map` + --> $DIR/filter_map_bool_then.rs:22:10 + | +LL | .filter_map(|i| (i.clone() % 2 == 0).then(|| i + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `filter` then `map` instead: `filter(|i| (i.clone() % 2 == 0)).map(|i| i + 1)` + +error: aborting due to 4 previous errors +