From d34f4eda58f60df54dc6d40e322f749472afee11 Mon Sep 17 00:00:00 2001 From: ceptontech <> Date: Tue, 23 Sep 2025 09:31:33 -0700 Subject: [PATCH 1/2] Add a check for some followed by filter The program will suggest using the `then_some` method. changelog: [`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/filter_some.rs | 61 +++++++++++++++++++++++++ clippy_lints/src/methods/mod.rs | 26 +++++++++++ tests/ui/filter_some.fixed | 8 ++++ tests/ui/filter_some.rs | 8 ++++ tests/ui/filter_some.stderr | 20 ++++++++ tests/ui/manual_filter.fixed | 2 +- tests/ui/manual_filter.rs | 2 +- tests/ui/option_filter_map.fixed | 2 +- tests/ui/option_filter_map.rs | 2 +- 11 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 clippy_lints/src/methods/filter_some.rs create mode 100644 tests/ui/filter_some.fixed create mode 100644 tests/ui/filter_some.rs create mode 100644 tests/ui/filter_some.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c773cf15756b..887b432f9e3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6135,6 +6135,7 @@ Released 2018-09-13 [`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 +[`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some [`find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#find_map [`flat_map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_identity [`flat_map_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_option diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5563b8094f01..e08c5a4e977f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -375,6 +375,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::FILTER_MAP_IDENTITY_INFO, crate::methods::FILTER_MAP_NEXT_INFO, crate::methods::FILTER_NEXT_INFO, + crate::methods::FILTER_SOME_INFO, crate::methods::FLAT_MAP_IDENTITY_INFO, crate::methods::FLAT_MAP_OPTION_INFO, crate::methods::FORMAT_COLLECT_INFO, diff --git a/clippy_lints/src/methods/filter_some.rs b/clippy_lints/src/methods/filter_some.rs new file mode 100644 index 000000000000..c67287e4d41d --- /dev/null +++ b/clippy_lints/src/methods/filter_some.rs @@ -0,0 +1,61 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::{indent_of, reindent_multiline, snippet}; +use clippy_utils::{is_res_lang_ctor, pat_is_wild, path_res}; +use rustc_ast::util::parser::ExprPrecedence; +use rustc_errors::Applicability; +use rustc_hir::LangItem::OptionSome; +use rustc_hir::{Body, Expr, ExprKind}; +use rustc_lint::LateContext; + +use super::FILTER_SOME; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + recv: &'tcx Expr<'tcx>, + arg: &'tcx Expr<'tcx>, +) { + let value = match recv.kind { + ExprKind::Call(f, [value]) if is_res_lang_ctor(cx, path_res(cx, f), OptionSome) => value, + _ => return, + }; + let condition = if let ExprKind::Closure(c) = arg.kind + && let Body { + params: [p], + value: condition, + } = cx.tcx.hir_body(c.body) + && pat_is_wild(cx, &p.pat.kind, arg) + { + Some(condition) + } else { + None + }; + span_lint_and_then(cx, FILTER_SOME, expr.span, "`filter` for `Some`", |diag| { + let help = "consider using `then_some` instead"; + if let Some(condition) = condition { + let parentheses = cx.precedence(condition) < ExprPrecedence::Unambiguous; + diag.span_suggestion( + expr.span, + help, + reindent_multiline( + &format!( + "{}{}{}.then_some({})", + if parentheses { "(" } else { "" }, + snippet(cx, condition.span, ".."), + if parentheses { ")" } else { "" }, + snippet(cx, value.span, "..") + ), + true, + indent_of(cx, expr.span), + ), + Applicability::MaybeIncorrect, + ); + diag.note( + "this change will alter the order in which the condition and \ + the value are evaluated", + ); + } else { + diag.help(help); + } + }); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8679689c8ad4..dbfe5c326e39 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -25,6 +25,7 @@ mod filter_map_bool_then; mod filter_map_identity; mod filter_map_next; mod filter_next; +mod filter_some; mod flat_map_identity; mod flat_map_option; mod format_collect; @@ -4639,6 +4640,29 @@ declare_clippy_lint! { "detects redundant calls to `Iterator::cloned`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `Some(_).filter(_)`. + /// + /// ### Why is this bad? + /// Readability, this can be written more concisely as `_.then_some(_)`. + /// + /// ### Example + /// ```no_run + /// let x = false; + /// Some(0).filter(|_| x); + /// ``` + /// Use instead: + /// ```no_run + /// let x = false; + /// x.then_some(0); + /// ``` + #[clippy::version = "1.91.0"] + pub FILTER_SOME, + complexity, + "using `Some(x).filter`, which is more succinctly expressed as `.then(x)`" +} + #[expect(clippy::struct_excessive_bools)] pub struct Methods { avoid_breaking_exported_api: bool, @@ -4820,6 +4844,7 @@ impl_lint_pass!(Methods => [ SWAP_WITH_TEMPORARY, IP_CONSTANT, REDUNDANT_ITER_CLONED, + FILTER_SOME, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -5191,6 +5216,7 @@ impl Methods { // use the sourcemap to get the span of the closure iter_filter::check(cx, expr, arg, span); } + filter_some::check(cx, expr, recv, arg); }, (sym::find, [arg]) => { if let Some((sym::cloned, recv2, [], _span2, _)) = method_call(recv) { diff --git a/tests/ui/filter_some.fixed b/tests/ui/filter_some.fixed new file mode 100644 index 000000000000..f90107e7a670 --- /dev/null +++ b/tests/ui/filter_some.fixed @@ -0,0 +1,8 @@ +#![warn(clippy::filter_some)] + +fn main() { + let _ = false.then_some(0); + //~^ filter_some + let _ = (1 == 0).then_some(0); + //~^ filter_some +} diff --git a/tests/ui/filter_some.rs b/tests/ui/filter_some.rs new file mode 100644 index 000000000000..0c8fc40b18c0 --- /dev/null +++ b/tests/ui/filter_some.rs @@ -0,0 +1,8 @@ +#![warn(clippy::filter_some)] + +fn main() { + let _ = Some(0).filter(|_| false); + //~^ filter_some + let _ = Some(0).filter(|_| 1 == 0); + //~^ filter_some +} diff --git a/tests/ui/filter_some.stderr b/tests/ui/filter_some.stderr new file mode 100644 index 000000000000..91ab5ca72a4f --- /dev/null +++ b/tests/ui/filter_some.stderr @@ -0,0 +1,20 @@ +error: `filter` for `Some` + --> tests/ui/filter_some.rs:4:13 + | +LL | let _ = Some(0).filter(|_| false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `false.then_some(0)` + | + = note: this change will alter the order in which the condition and the value are evaluated + = note: `-D clippy::filter-some` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::filter_some)]` + +error: `filter` for `Some` + --> tests/ui/filter_some.rs:6:13 + | +LL | let _ = Some(0).filter(|_| 1 == 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `(1 == 0).then_some(0)` + | + = note: this change will alter the order in which the condition and the value are evaluated + +error: aborting due to 2 previous errors + diff --git a/tests/ui/manual_filter.fixed b/tests/ui/manual_filter.fixed index a0fb0e32d601..0d520186beb0 100644 --- a/tests/ui/manual_filter.fixed +++ b/tests/ui/manual_filter.fixed @@ -1,5 +1,5 @@ #![warn(clippy::manual_filter)] -#![allow(unused_variables, dead_code, clippy::useless_vec)] +#![allow(unused_variables, dead_code, clippy::filter_some, clippy::useless_vec)] fn main() { Some(0).filter(|&x| x <= 0); diff --git a/tests/ui/manual_filter.rs b/tests/ui/manual_filter.rs index a9d0c35f8bb7..546c9d93ba2a 100644 --- a/tests/ui/manual_filter.rs +++ b/tests/ui/manual_filter.rs @@ -1,5 +1,5 @@ #![warn(clippy::manual_filter)] -#![allow(unused_variables, dead_code, clippy::useless_vec)] +#![allow(unused_variables, dead_code, clippy::filter_some, clippy::useless_vec)] fn main() { match Some(0) { diff --git a/tests/ui/option_filter_map.fixed b/tests/ui/option_filter_map.fixed index d390c41af53c..9fca7b3a03c1 100644 --- a/tests/ui/option_filter_map.fixed +++ b/tests/ui/option_filter_map.fixed @@ -1,5 +1,5 @@ #![warn(clippy::option_filter_map)] -#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)] +#![allow(clippy::filter_some, clippy::map_flatten, clippy::unnecessary_map_on_constructor)] fn main() { let _ = Some(Some(1)).flatten(); diff --git a/tests/ui/option_filter_map.rs b/tests/ui/option_filter_map.rs index 2d3b983a7670..21a81dcb8013 100644 --- a/tests/ui/option_filter_map.rs +++ b/tests/ui/option_filter_map.rs @@ -1,5 +1,5 @@ #![warn(clippy::option_filter_map)] -#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)] +#![allow(clippy::filter_some, clippy::map_flatten, clippy::unnecessary_map_on_constructor)] fn main() { let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap); From edd2d3b5708ee73eaaded033856725121c87ac24 Mon Sep 17 00:00:00 2001 From: ceptontech <> Date: Tue, 23 Sep 2025 16:01:02 -0700 Subject: [PATCH 2/2] Address comments The situation where the condition depends on the value is excluded form linting. Now the program always fixes automatically. Tests are added for code involving macros. The MSRV is checked. --- clippy_lints/src/methods/filter_some.rs | 58 +++++++++++++------------ clippy_lints/src/methods/mod.rs | 2 +- tests/ui/filter_some.fixed | 32 ++++++++++++++ tests/ui/filter_some.rs | 32 ++++++++++++++ tests/ui/filter_some.stderr | 53 ++++++++++++++++++++-- 5 files changed, 145 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/methods/filter_some.rs b/clippy_lints/src/methods/filter_some.rs index c67287e4d41d..cb66b748ef0a 100644 --- a/clippy_lints/src/methods/filter_some.rs +++ b/clippy_lints/src/methods/filter_some.rs @@ -1,5 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::{indent_of, reindent_multiline, snippet}; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::source::{indent_of, reindent_multiline, snippet, snippet_with_context}; use clippy_utils::{is_res_lang_ctor, pat_is_wild, path_res}; use rustc_ast::util::parser::ExprPrecedence; use rustc_errors::Applicability; @@ -14,7 +15,12 @@ pub(super) fn check<'tcx>( expr: &'tcx Expr<'tcx>, recv: &'tcx Expr<'tcx>, arg: &'tcx Expr<'tcx>, + msrv: Msrv, ) { + if !msrv.meets(cx, msrvs::BOOL_THEN_SOME) { + return; + } + let mut applicability = Applicability::MaybeIncorrect; let value = match recv.kind { ExprKind::Call(f, [value]) if is_res_lang_ctor(cx, path_res(cx, f), OptionSome) => value, _ => return, @@ -26,36 +32,32 @@ pub(super) fn check<'tcx>( } = cx.tcx.hir_body(c.body) && pat_is_wild(cx, &p.pat.kind, arg) { - Some(condition) + condition } else { - None + return; }; + let (condition_text, condition_is_macro) = + snippet_with_context(cx, condition.span, arg.span.ctxt(), "..", &mut applicability); + let parentheses = !condition_is_macro && cx.precedence(condition) < ExprPrecedence::Unambiguous; + let value_text = snippet(cx, value.span, ".."); span_lint_and_then(cx, FILTER_SOME, expr.span, "`filter` for `Some`", |diag| { - let help = "consider using `then_some` instead"; - if let Some(condition) = condition { - let parentheses = cx.precedence(condition) < ExprPrecedence::Unambiguous; - diag.span_suggestion( - expr.span, - help, - reindent_multiline( - &format!( - "{}{}{}.then_some({})", - if parentheses { "(" } else { "" }, - snippet(cx, condition.span, ".."), - if parentheses { ")" } else { "" }, - snippet(cx, value.span, "..") - ), - true, - indent_of(cx, expr.span), + diag.span_suggestion( + expr.span, + "consider using `then_some` instead", + reindent_multiline( + &format!( + "{}{condition_text}{}.then_some({value_text})", + if parentheses { "(" } else { "" }, + if parentheses { ")" } else { "" }, ), - Applicability::MaybeIncorrect, - ); - diag.note( - "this change will alter the order in which the condition and \ - the value are evaluated", - ); - } else { - diag.help(help); - } + true, + indent_of(cx, expr.span), + ), + applicability, + ); + diag.note( + "this change will alter the order in which the condition and the \ + value are evaluated", + ); }); } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index dbfe5c326e39..ee5e0df18f1e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -5216,7 +5216,7 @@ impl Methods { // use the sourcemap to get the span of the closure iter_filter::check(cx, expr, arg, span); } - filter_some::check(cx, expr, recv, arg); + filter_some::check(cx, expr, recv, arg, self.msrv); }, (sym::find, [arg]) => { if let Some((sym::cloned, recv2, [], _span2, _)) = method_call(recv) { diff --git a/tests/ui/filter_some.fixed b/tests/ui/filter_some.fixed index f90107e7a670..fbfdfd17781d 100644 --- a/tests/ui/filter_some.fixed +++ b/tests/ui/filter_some.fixed @@ -1,8 +1,40 @@ #![warn(clippy::filter_some)] +macro_rules! unchanged { + ($result:expr) => { + $result + }; +} + +macro_rules! condition { + ($condition:expr) => { + $condition || false + }; +} + fn main() { let _ = false.then_some(0); //~^ filter_some + // The condition contains an operator. The program should add parentheses. let _ = (1 == 0).then_some(0); //~^ filter_some + let _ = match 0 { + //~^ filter_some + 0 => false, + 1 => true, + _ => true, + }.then_some(0); + // The argument to filter requires the value in the option. The program + // can't figure out how to change it. It should leave it alone for now. + let _ = Some(0).filter(|x| *x == 0); + // The expression is a macro argument. The program should change the macro + // argument. It should not expand the macro. + let _ = unchanged!(false.then_some(0)); + //~^ filter_some + let _ = vec![false].is_empty().then_some(0); + //~^ filter_some + // The condition is a macro that expands to an expression containing an + // operator. The program should not add parentheses. + let _ = condition!(false).then_some(0); + //~^ filter_some } diff --git a/tests/ui/filter_some.rs b/tests/ui/filter_some.rs index 0c8fc40b18c0..cb5a5b0dd1ee 100644 --- a/tests/ui/filter_some.rs +++ b/tests/ui/filter_some.rs @@ -1,8 +1,40 @@ #![warn(clippy::filter_some)] +macro_rules! unchanged { + ($result:expr) => { + $result + }; +} + +macro_rules! condition { + ($condition:expr) => { + $condition || false + }; +} + fn main() { let _ = Some(0).filter(|_| false); //~^ filter_some + // The condition contains an operator. The program should add parentheses. let _ = Some(0).filter(|_| 1 == 0); //~^ filter_some + let _ = Some(0).filter(|_| match 0 { + //~^ filter_some + 0 => false, + 1 => true, + _ => true, + }); + // The argument to filter requires the value in the option. The program + // can't figure out how to change it. It should leave it alone for now. + let _ = Some(0).filter(|x| *x == 0); + // The expression is a macro argument. The program should change the macro + // argument. It should not expand the macro. + let _ = unchanged!(Some(0).filter(|_| false)); + //~^ filter_some + let _ = Some(0).filter(|_| vec![false].is_empty()); + //~^ filter_some + // The condition is a macro that expands to an expression containing an + // operator. The program should not add parentheses. + let _ = Some(0).filter(|_| condition!(false)); + //~^ filter_some } diff --git a/tests/ui/filter_some.stderr b/tests/ui/filter_some.stderr index 91ab5ca72a4f..62aa7e72429a 100644 --- a/tests/ui/filter_some.stderr +++ b/tests/ui/filter_some.stderr @@ -1,5 +1,5 @@ error: `filter` for `Some` - --> tests/ui/filter_some.rs:4:13 + --> tests/ui/filter_some.rs:16:13 | LL | let _ = Some(0).filter(|_| false); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `false.then_some(0)` @@ -9,12 +9,59 @@ LL | let _ = Some(0).filter(|_| false); = help: to override `-D warnings` add `#[allow(clippy::filter_some)]` error: `filter` for `Some` - --> tests/ui/filter_some.rs:6:13 + --> tests/ui/filter_some.rs:19:13 | LL | let _ = Some(0).filter(|_| 1 == 0); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `(1 == 0).then_some(0)` | = note: this change will alter the order in which the condition and the value are evaluated -error: aborting due to 2 previous errors +error: `filter` for `Some` + --> tests/ui/filter_some.rs:21:13 + | +LL | let _ = Some(0).filter(|_| match 0 { + | _____________^ +LL | | +LL | | 0 => false, +LL | | 1 => true, +LL | | _ => true, +LL | | }); + | |______^ + | + = note: this change will alter the order in which the condition and the value are evaluated +help: consider using `then_some` instead + | +LL ~ let _ = match 0 { +LL + +LL + 0 => false, +LL + 1 => true, +LL + _ => true, +LL ~ }.then_some(0); + | + +error: `filter` for `Some` + --> tests/ui/filter_some.rs:32:24 + | +LL | let _ = unchanged!(Some(0).filter(|_| false)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `false.then_some(0)` + | + = note: this change will alter the order in which the condition and the value are evaluated + +error: `filter` for `Some` + --> tests/ui/filter_some.rs:34:13 + | +LL | let _ = Some(0).filter(|_| vec![false].is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `vec![false].is_empty().then_some(0)` + | + = note: this change will alter the order in which the condition and the value are evaluated + +error: `filter` for `Some` + --> tests/ui/filter_some.rs:38:13 + | +LL | let _ = Some(0).filter(|_| condition!(false)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `then_some` instead: `condition!(false).then_some(0)` + | + = note: this change will alter the order in which the condition and the value are evaluated + +error: aborting due to 6 previous errors