Skip to content

Commit

Permalink
New lint [filter_map_bool_then]
Browse files Browse the repository at this point in the history
  • Loading branch information
Centri3 committed Jul 12, 2023
1 parent dd8e44c commit afdf612
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4808,6 +4808,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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,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,
Expand Down
39 changes: 39 additions & 0 deletions clippy_lints/src/methods/filter_map_bool_then.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
use clippy_utils::{
diagnostics::span_lint_and_sugg, is_from_proc_macro, match_def_path, paths::BOOL_THEN, peel_blocks,
source::snippet_opt,
};
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` and `map` instead",
format!("filter({decl_snippet} {filter}).map({decl_snippet} {map})"),
Applicability::MachineApplicable,
);
}
}
34 changes: 33 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -3378,6 +3379,35 @@ declare_clippy_lint! {
"calling `Stdin::read_line`, then trying to parse it without first trimming"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `bool::then` in `Iterator::filter_map`.
///
/// ### Why is this bad?
/// This can be more concisely written with `filter` then `map` instead.
///
/// ### 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,
Expand Down Expand Up @@ -3512,6 +3542,7 @@ impl_lint_pass!(Methods => [
UNNECESSARY_LITERAL_UNWRAP,
DRAIN_COLLECT,
MANUAL_TRY_FOLD,
FILTER_MAP_BOOL_THEN,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -3790,6 +3821,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]) => {
Expand Down Expand Up @@ -4204,7 +4236,7 @@ impl SelfKind {
};

let Some(trait_def_id) = cx.tcx.get_diagnostic_item(trait_sym) else {
return false
return false;
};
implements_trait(cx, ty, trait_def_id, &[parent_ty.into()])
}
Expand Down
2 changes: 2 additions & 0 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,5 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
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"];
#[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so
pub const BOOL_THEN: [&str; 4] = ["core", "bool", "<impl bool>", "then"];
33 changes: 33 additions & 0 deletions tests/ui/filter_map_bool_then.fixed
Original file line number Diff line number Diff line change
@@ -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));
}
}
33 changes: 33 additions & 0 deletions tests/ui/filter_map_bool_then.rs
Original file line number Diff line number Diff line change
@@ -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));
}
}
28 changes: 28 additions & 0 deletions tests/ui/filter_map_bool_then.stderr
Original file line number Diff line number Diff line change
@@ -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` and `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` and `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` and `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` and `map` instead: `filter(|i| (i.clone() % 2 == 0)).map(|i| i + 1)`

error: aborting due to 4 previous errors

0 comments on commit afdf612

Please sign in to comment.