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..cb66b748ef0a --- /dev/null +++ b/clippy_lints/src/methods/filter_some.rs @@ -0,0 +1,63 @@ +use clippy_utils::diagnostics::span_lint_and_then; +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; +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>, + 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, + }; + 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) + { + condition + } else { + 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| { + 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 { "" }, + ), + 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 8679689c8ad4..ee5e0df18f1e 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, 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 new file mode 100644 index 000000000000..fbfdfd17781d --- /dev/null +++ b/tests/ui/filter_some.fixed @@ -0,0 +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 new file mode 100644 index 000000000000..cb5a5b0dd1ee --- /dev/null +++ b/tests/ui/filter_some.rs @@ -0,0 +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 new file mode 100644 index 000000000000..62aa7e72429a --- /dev/null +++ b/tests/ui/filter_some.stderr @@ -0,0 +1,67 @@ +error: `filter` for `Some` + --> tests/ui/filter_some.rs:16: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: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: `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 + 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);