From 0958f9486ba0cb8ecd0093ff3100af45d19529f3 Mon Sep 17 00:00:00 2001 From: kraktus Date: Mon, 3 Oct 2022 13:57:42 +0200 Subject: [PATCH] Add `manual_filter` lint for `Option` Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback. --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_complexity.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/matches/manual_filter.rs | 181 +++++++++++++++ clippy_lints/src/matches/manual_map.rs | 213 +++++++++++------ clippy_lints/src/matches/mod.rs | 39 ++++ clippy_utils/src/sugg.rs | 6 +- src/docs.rs | 1 + src/docs/manual_filter.txt | 21 ++ tests/ui/manual_filter.fixed | 119 ++++++++++ tests/ui/manual_filter.rs | 243 ++++++++++++++++++++ tests/ui/manual_filter.stderr | 191 +++++++++++++++ 13 files changed, 951 insertions(+), 67 deletions(-) create mode 100644 clippy_lints/src/matches/manual_filter.rs create mode 100644 src/docs/manual_filter.txt create mode 100644 tests/ui/manual_filter.fixed create mode 100644 tests/ui/manual_filter.rs create mode 100644 tests/ui/manual_filter.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f12a6735..cab2808ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3986,6 +3986,7 @@ Released 2018-09-13 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits [`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp +[`manual_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map [`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 9ad2a88eb..21d686fcd 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -135,6 +135,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), + LintId::of(matches::MANUAL_FILTER), LintId::of(matches::MANUAL_MAP), LintId::of(matches::MANUAL_UNWRAP_OR), LintId::of(matches::MATCH_AS_REF), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index a58d066fa..e3849e5a6 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -27,6 +27,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(manual_strip::MANUAL_STRIP), LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), + LintId::of(matches::MANUAL_FILTER), LintId::of(matches::MANUAL_UNWRAP_OR), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_SINGLE_BINDING), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 307ec40f4..70defe3bc 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -257,6 +257,7 @@ store.register_lints(&[ match_result_ok::MATCH_RESULT_OK, matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, + matches::MANUAL_FILTER, matches::MANUAL_MAP, matches::MANUAL_UNWRAP_OR, matches::MATCH_AS_REF, diff --git a/clippy_lints/src/matches/manual_filter.rs b/clippy_lints/src/matches/manual_filter.rs new file mode 100644 index 000000000..9931f1268 --- /dev/null +++ b/clippy_lints/src/matches/manual_filter.rs @@ -0,0 +1,181 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{is_res_lang_ctor, path_res, path_to_local_id}; +use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::LangItem::OptionSome; +use rustc_hir::{Arm, Block, BlockCheckMode, Expr, ExprKind, HirId, Pat, PatKind, UnsafeSource}; +use rustc_lint::LateContext; +use rustc_span::{sym, SyntaxContext}; + +use super::manual_map::{check_with, SomeExpr}; +use super::MANUAL_FILTER; + +#[derive(Default)] +struct NeedsUnsafeBlock(pub bool); + +impl<'tcx> Visitor<'tcx> for NeedsUnsafeBlock { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + match expr.kind { + ExprKind::Block( + Block { + rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), + .. + }, + _, + ) => { + self.0 = true; + }, + _ => walk_expr(self, expr), + } + } +} + +// Function called on the `expr` of `[&+]Some((ref | ref mut) x) => ` +// Need to check if it's of the `if {} else {}` +// AND that only one `then/else_expr` resolves to `Some(x)` while the other resolves to `None` +// return `cond` if +fn get_cond_expr<'tcx>( + cx: &LateContext<'tcx>, + pat: &Pat<'_>, + expr: &'tcx Expr<'_>, + ctxt: SyntaxContext, +) -> Option> { + if_chain! { + if let Some(block_expr) = peels_blocks_incl_unsafe_opt(expr); + if let ExprKind::If(cond, then_expr, Some(else_expr)) = block_expr.kind; + if let PatKind::Binding(_,target, ..) = pat.kind; + if let (then_visitor, else_visitor) + = (handle_if_or_else_expr(cx, target, ctxt, then_expr), + handle_if_or_else_expr(cx, target, ctxt, else_expr)); + if then_visitor != else_visitor; // check that one expr resolves to `Some(x)`, the other to `None` + then { + let mut needs_unsafe_block = NeedsUnsafeBlock::default(); + needs_unsafe_block.visit_expr(expr); + return Some(SomeExpr { + expr: peels_blocks_incl_unsafe(cond.peel_drop_temps()), + needs_unsafe_block: needs_unsafe_block.0, + needs_negated: !then_visitor // if the `then_expr` resolves to `None`, need to negate the cond + }) + } + }; + None +} + +fn peels_blocks_incl_unsafe_opt<'a>(expr: &'a Expr<'a>) -> Option<&'a Expr<'a>> { + // we don't want to use `peel_blocks` here because we don't care if the block is unsafe, it's + // checked by `NeedsUnsafeBlock` + if let ExprKind::Block(block, None) = expr.kind { + if block.stmts.is_empty() { + return block.expr; + } + }; + None +} + +fn peels_blocks_incl_unsafe<'a>(expr: &'a Expr<'a>) -> &'a Expr<'a> { + peels_blocks_incl_unsafe_opt(expr).unwrap_or(expr) +} + +// function called for each expression: +// Some(x) => if { +// +// } else { +// +// } +// Returns true if resolves to `Some(x)`, `false` otherwise +fn handle_if_or_else_expr<'tcx>( + cx: &LateContext<'_>, + target: HirId, + ctxt: SyntaxContext, + if_or_else_expr: &'tcx Expr<'_>, +) -> bool { + if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(if_or_else_expr) { + // there can be not statements in the block as they would be removed when switching to `.filter` + if let ExprKind::Call(callee, [arg]) = inner_expr.kind { + return ctxt == if_or_else_expr.span.ctxt() + && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) + && path_to_local_id(arg, target); + } + }; + false +} + +// given the closure: `|| ` +// returns `|&| ` +fn add_ampersand_if_copy(body_str: String, has_copy_trait: bool) -> String { + if has_copy_trait { + let mut with_ampersand = body_str; + with_ampersand.insert(1, '&'); + with_ampersand + } else { + body_str + } +} + +pub(super) fn check_match<'tcx>( + cx: &LateContext<'tcx>, + scrutinee: &'tcx Expr<'_>, + arms: &'tcx [Arm<'_>], + expr: &'tcx Expr<'_>, +) { + let ty = cx.typeck_results().expr_ty(expr); + if_chain! { + if is_type_diagnostic_item(cx, ty, sym::Option); + if arms.len() == 2; + if arms[0].guard.is_none(); + if arms[1].guard.is_none(); + then { + check(cx, expr, scrutinee, arms[0].pat, arms[0].body, Some(arms[1].pat), arms[1].body) + } + } +} + +pub(super) fn check_if_let<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + let_pat: &'tcx Pat<'_>, + let_expr: &'tcx Expr<'_>, + then_expr: &'tcx Expr<'_>, + else_expr: &'tcx Expr<'_>, +) { + check(cx, expr, let_expr, let_pat, then_expr, None, else_expr); +} + +fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + scrutinee: &'tcx Expr<'_>, + then_pat: &'tcx Pat<'_>, + then_body: &'tcx Expr<'_>, + else_pat: Option<&'tcx Pat<'_>>, + else_body: &'tcx Expr<'_>, +) { + if let Some(sugg_info) = check_with( + cx, + expr, + scrutinee, + then_pat, + then_body, + else_pat, + else_body, + get_cond_expr, + ) { + let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy); + span_lint_and_sugg( + cx, + MANUAL_FILTER, + expr.span, + "manual implementation of `Option::filter`", + "try this", + if sugg_info.needs_brackets { + format!( + "{{ {}{}.filter({body_str}) }}", + sugg_info.scrutinee_str, sugg_info.as_ref_str + ) + } else { + format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str) + }, + sugg_info.app, + ); + } +} diff --git a/clippy_lints/src/matches/manual_map.rs b/clippy_lints/src/matches/manual_map.rs index 76f5e1c94..2b6a07c5d 100644 --- a/clippy_lints/src/matches/manual_map.rs +++ b/clippy_lints/src/matches/manual_map.rs @@ -1,10 +1,11 @@ +use super::MANUAL_MAP; use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; -use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function}; +use clippy_utils::ty::{is_copy, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function}; use clippy_utils::{ can_move_expr_to_closure, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local_id, - peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, CaptureKind, + peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, sugg::Sugg, CaptureKind, }; use rustc_ast::util::parser::PREC_POSTFIX; use rustc_errors::Applicability; @@ -16,8 +17,6 @@ use rustc_hir::{ use rustc_lint::LateContext; use rustc_span::{sym, SyntaxContext}; -use super::MANUAL_MAP; - pub(super) fn check_match<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, @@ -43,7 +42,6 @@ pub(super) fn check_if_let<'tcx>( check(cx, expr, let_expr, let_pat, then_expr, None, else_expr); } -#[expect(clippy::too_many_lines)] fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, @@ -53,12 +51,59 @@ fn check<'tcx>( else_pat: Option<&'tcx Pat<'_>>, else_body: &'tcx Expr<'_>, ) { + if let Some(sugg_info) = check_with( + cx, + expr, + scrutinee, + then_pat, + then_body, + else_pat, + else_body, + get_some_expr, + ) { + span_lint_and_sugg( + cx, + MANUAL_MAP, + expr.span, + "manual implementation of `Option::map`", + "try this", + if sugg_info.needs_brackets { + format!( + "{{ {}{}.map({}) }}", + sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str + ) + } else { + format!( + "{}{}.map({})", + sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str + ) + }, + sugg_info.app, + ); + } +} + +#[expect(clippy::too_many_arguments)] +#[expect(clippy::too_many_lines)] +pub(super) fn check_with<'tcx, F>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + scrutinee: &'tcx Expr<'_>, + then_pat: &'tcx Pat<'_>, + then_body: &'tcx Expr<'_>, + else_pat: Option<&'tcx Pat<'_>>, + else_body: &'tcx Expr<'_>, + get_some_expr_fn: F, +) -> Option> +where + F: Fn(&LateContext<'tcx>, &'tcx Pat<'_>, &'tcx Expr<'_>, SyntaxContext) -> Option>, +{ let (scrutinee_ty, ty_ref_count, ty_mutability) = peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee)); if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::Option) && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Option)) { - return; + return None; } let expr_ctxt = expr.span.ctxt(); @@ -78,29 +123,29 @@ fn check<'tcx>( (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => { (then_body, pattern, ref_count, false) }, - _ => return, + _ => return None, }; // Top level or patterns aren't allowed in closures. if matches!(some_pat.kind, PatKind::Or(_)) { - return; + return None; } - let some_expr = match get_some_expr(cx, some_expr, false, expr_ctxt) { + let some_expr = match get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) { Some(expr) => expr, - None => return, + None => return None, }; // These two lints will go back and forth with each other. if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) { - return; + return None; } // `map` won't perform any adjustments. if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { - return; + return None; } // Determine which binding mode to use. @@ -125,16 +170,16 @@ fn check<'tcx>( }); if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind { match captures.get(l) { - Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return, + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return None, Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { - return; + return None; }, Some(CaptureKind::Ref(Mutability::Not)) | None => (), } } } }, - None => return, + None => return None, }; let mut app = Applicability::MachineApplicable; @@ -149,6 +194,7 @@ fn check<'tcx>( scrutinee_str.into() }; + let closure_expr_snip = some_expr.to_snippet_with_context(cx, expr_ctxt, &mut app); let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { if_chain! { if !some_expr.needs_unsafe_block; @@ -161,7 +207,7 @@ fn check<'tcx>( && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) && binding_ref.is_some() { - return; + return None; } // `ref` and `ref mut` annotations were handled earlier. @@ -170,41 +216,47 @@ fn check<'tcx>( } else { "" }; - let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; + if some_expr.needs_unsafe_block { - format!("|{annotation}{some_binding}| unsafe {{ {expr_snip} }}") + format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}") } else { - format!("|{annotation}{some_binding}| {expr_snip}") + format!("|{annotation}{some_binding}| {closure_expr_snip}") } } } } else if !is_wild_none && explicit_ref.is_none() { // TODO: handle explicit reference annotations. let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0; - let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; if some_expr.needs_unsafe_block { - format!("|{pat_snip}| unsafe {{ {expr_snip} }}") + format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}") } else { - format!("|{pat_snip}| {expr_snip}") + format!("|{pat_snip}| {closure_expr_snip}") } } else { // Refutable bindings and mixed reference annotations can't be handled by `map`. - return; + return None; }; - span_lint_and_sugg( - cx, - MANUAL_MAP, - expr.span, - "manual implementation of `Option::map`", - "try this", - if else_pat.is_none() && is_else_clause(cx.tcx, expr) { - format!("{{ {scrutinee_str}{as_ref_str}.map({body_str}) }}") - } else { - format!("{scrutinee_str}{as_ref_str}.map({body_str})") - }, + // relies on the fact that Option: Copy where T: copy + let scrutinee_impl_copy = is_copy(cx, scrutinee_ty); + + Some(SuggInfo { + needs_brackets: else_pat.is_none() && is_else_clause(cx.tcx, expr), + scrutinee_impl_copy, + scrutinee_str, + as_ref_str, + body_str, app, - ); + }) +} + +pub struct SuggInfo<'a> { + pub needs_brackets: bool, + pub scrutinee_impl_copy: bool, + pub scrutinee_str: String, + pub as_ref_str: &'a str, + pub body_str: String, + pub app: Applicability, } // Checks whether the expression could be passed as a function, or whether a closure is needed. @@ -222,7 +274,8 @@ fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Ex } } -enum OptionPat<'a> { +#[derive(Debug)] +pub(super) enum OptionPat<'a> { Wild, None, Some { @@ -234,14 +287,39 @@ enum OptionPat<'a> { }, } -struct SomeExpr<'tcx> { - expr: &'tcx Expr<'tcx>, - needs_unsafe_block: bool, +pub(super) struct SomeExpr<'tcx> { + pub expr: &'tcx Expr<'tcx>, + pub needs_unsafe_block: bool, + pub needs_negated: bool, // for `manual_filter` lint +} + +impl<'tcx> SomeExpr<'tcx> { + pub fn new_no_negated(expr: &'tcx Expr<'tcx>, needs_unsafe_block: bool) -> Self { + Self { + expr, + needs_unsafe_block, + needs_negated: false, + } + } + + pub fn to_snippet_with_context( + &self, + cx: &LateContext<'tcx>, + ctxt: SyntaxContext, + app: &mut Applicability, + ) -> Sugg<'tcx> { + let sugg = Sugg::hir_with_context(cx, self.expr, ctxt, "..", app); + if self.needs_negated { !sugg } else { sugg } + } } // Try to parse into a recognized `Option` pattern. // i.e. `_`, `None`, `Some(..)`, or a reference to any of those. -fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option> { +pub(super) fn try_parse_pattern<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + ctxt: SyntaxContext, +) -> Option> { fn f<'tcx>( cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, @@ -268,36 +346,41 @@ fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: Syn // Checks for an expression wrapped by the `Some` constructor. Returns the contained expression. fn get_some_expr<'tcx>( cx: &LateContext<'tcx>, + _: &'tcx Pat<'_>, expr: &'tcx Expr<'_>, - needs_unsafe_block: bool, ctxt: SyntaxContext, ) -> Option> { - // TODO: Allow more complex expressions. - match expr.kind { - ExprKind::Call(callee, [arg]) - if ctxt == expr.span.ctxt() && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) => - { - Some(SomeExpr { - expr: arg, - needs_unsafe_block, - }) - }, - ExprKind::Block( - Block { - stmts: [], - expr: Some(expr), - rules, - .. + fn get_some_expr_internal<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + needs_unsafe_block: bool, + ctxt: SyntaxContext, + ) -> Option> { + // TODO: Allow more complex expressions. + match expr.kind { + ExprKind::Call(callee, [arg]) + if ctxt == expr.span.ctxt() && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) => + { + Some(SomeExpr::new_no_negated(arg, needs_unsafe_block)) }, - _, - ) => get_some_expr( - cx, - expr, - needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), - ctxt, - ), - _ => None, + ExprKind::Block( + Block { + stmts: [], + expr: Some(expr), + rules, + .. + }, + _, + ) => get_some_expr_internal( + cx, + expr, + needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), + ctxt, + ), + _ => None, + } } + get_some_expr_internal(cx, expr, false, ctxt) } // Checks for the `None` value. diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index e6b183fc0..cf1bd7a12 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -1,5 +1,6 @@ mod collapsible_match; mod infallible_destructuring_match; +mod manual_filter; mod manual_map; mod manual_unwrap_or; mod match_as_ref; @@ -898,6 +899,34 @@ declare_clippy_lint! { "reimplementation of `map`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usages of `match` which could be implemented using `filter` + /// + /// ### Why is this bad? + /// Using the `filter` method is clearer and more concise. + /// + /// ### Example + /// ```rust + /// match Some(0) { + /// Some(x) => if x % 2 == 0 { + /// Some(x) + /// } else { + /// None + /// }, + /// None => None, + /// }; + /// ``` + /// Use instead: + /// ```rust + /// Some(0).filter(|&x| x % 2 == 0); + /// ``` + #[clippy::version = "1.65.0"] + pub MANUAL_FILTER, + complexity, + "reimplentation of `filter`" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -939,6 +968,7 @@ impl_lint_pass!(Matches => [ SIGNIFICANT_DROP_IN_SCRUTINEE, TRY_ERR, MANUAL_MAP, + MANUAL_FILTER, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -988,6 +1018,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { if !in_constant(cx, expr.hir_id) { manual_unwrap_or::check(cx, expr, ex, arms); manual_map::check_match(cx, expr, ex, arms); + manual_filter::check_match(cx, ex, arms, expr); } if self.infallible_destructuring_match_linted { @@ -1014,6 +1045,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches { } if !in_constant(cx, expr.hir_id) { manual_map::check_if_let(cx, expr, if_let.let_pat, if_let.let_expr, if_let.if_then, else_expr); + manual_filter::check_if_let( + cx, + expr, + if_let.let_pat, + if_let.let_expr, + if_let.if_then, + else_expr, + ); } } redundant_pattern_match::check_if_let( diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index ef836e848..e88542b77 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -1,7 +1,9 @@ //! Contains utility functions to generate suggestions. #![deny(clippy::missing_docs_in_private_items)] -use crate::source::{snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite}; +use crate::source::{ + snippet, snippet_opt, snippet_with_applicability, snippet_with_context, snippet_with_macro_callsite, +}; use crate::ty::expr_sig; use crate::{get_parent_expr_for_hir, higher}; use rustc_ast::util::parser::AssocOp; @@ -110,7 +112,7 @@ impl<'a> Sugg<'a> { if expr.span.ctxt() == ctxt { Self::hir_from_snippet(expr, |span| snippet(cx, span, default)) } else { - let snip = snippet_with_applicability(cx, expr.span, default, applicability); + let snip = snippet_with_context(cx, expr.span, ctxt, default, applicability).0; Sugg::NonParen(snip) } } diff --git a/src/docs.rs b/src/docs.rs index 39540e4b0..d24342076 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -256,6 +256,7 @@ docs! { "manual_async_fn", "manual_bits", "manual_clamp", + "manual_filter", "manual_filter_map", "manual_find", "manual_find_map", diff --git a/src/docs/manual_filter.txt b/src/docs/manual_filter.txt new file mode 100644 index 000000000..19a4d9319 --- /dev/null +++ b/src/docs/manual_filter.txt @@ -0,0 +1,21 @@ +### What it does +Checks for usages of `match` which could be implemented using `filter` + +### Why is this bad? +Using the `filter` method is clearer and more concise. + +### Example +``` +match Some(0) { + Some(x) => if x % 2 == 0 { + Some(x) + } else { + None + }, + None => None, +}; +``` +Use instead: +``` +Some(0).filter(|&x| x % 2 == 0); +``` \ No newline at end of file diff --git a/tests/ui/manual_filter.fixed b/tests/ui/manual_filter.fixed new file mode 100644 index 000000000..3553291b8 --- /dev/null +++ b/tests/ui/manual_filter.fixed @@ -0,0 +1,119 @@ +// run-rustfix + +#![warn(clippy::manual_filter)] +#![allow(unused_variables, dead_code)] + +fn main() { + Some(0).filter(|&x| x <= 0); + + Some(1).filter(|&x| x <= 0); + + Some(2).filter(|&x| x <= 0); + + Some(3).filter(|&x| x > 0); + + let y = Some(4); + y.filter(|&x| x <= 0); + + Some(5).filter(|&x| x > 0); + + Some(6).as_ref().filter(|&x| x > &0); + + let external_cond = true; + Some(String::new()).filter(|x| external_cond); + + Some(7).filter(|&x| external_cond); + + Some(8).filter(|&x| x != 0); + + Some(9).filter(|&x| x > 10 && x < 100); + + const fn f1() { + // Don't lint, `.filter` is not const + match Some(10) { + Some(x) => { + if x > 10 && x < 100 { + Some(x) + } else { + None + } + }, + None => None, + }; + } + + #[allow(clippy::blocks_in_if_conditions)] + Some(11).filter(|&x| { + println!("foo"); + x > 10 && x < 100 + }); + + match Some(12) { + // Don't Lint, statement is lost by `.filter` + Some(x) => { + if x > 10 && x < 100 { + println!("foo"); + Some(x) + } else { + None + } + }, + None => None, + }; + + match Some(13) { + // Don't Lint, because of `None => Some(1)` + Some(x) => { + if x > 10 && x < 100 { + println!("foo"); + Some(x) + } else { + None + } + }, + None => Some(1), + }; + + unsafe fn f(x: u32) -> bool { + true + } + let _ = Some(14).filter(|&x| unsafe { f(x) }); + let _ = Some(15).filter(|&x| unsafe { f(x) }); + + #[allow(clippy::redundant_pattern_matching)] + if let Some(_) = Some(16) { + Some(16) + } else { Some(16).filter(|&x| x % 2 == 0) }; + + match Some((17, 17)) { + // Not linted for now could be + Some((x, y)) => { + if y != x { + Some((x, y)) + } else { + None + } + }, + None => None, + }; + + struct NamedTuple { + pub x: u8, + pub y: (i32, u32), + } + + match Some(NamedTuple { + // Not linted for now could be + x: 17, + y: (18, 19), + }) { + Some(NamedTuple { x, y }) => { + if y.1 != x as u32 { + Some(NamedTuple { x, y }) + } else { + None + } + }, + None => None, + }; +} diff --git a/tests/ui/manual_filter.rs b/tests/ui/manual_filter.rs new file mode 100644 index 000000000..aa9f90f75 --- /dev/null +++ b/tests/ui/manual_filter.rs @@ -0,0 +1,243 @@ +// run-rustfix + +#![warn(clippy::manual_filter)] +#![allow(unused_variables, dead_code)] + +fn main() { + match Some(0) { + None => None, + Some(x) => { + if x > 0 { + None + } else { + Some(x) + } + }, + }; + + match Some(1) { + Some(x) => { + if x > 0 { + None + } else { + Some(x) + } + }, + None => None, + }; + + match Some(2) { + Some(x) => { + if x > 0 { + None + } else { + Some(x) + } + }, + _ => None, + }; + + match Some(3) { + Some(x) => { + if x > 0 { + Some(x) + } else { + None + } + }, + None => None, + }; + + let y = Some(4); + match y { + // Some(4) + None => None, + Some(x) => { + if x > 0 { + None + } else { + Some(x) + } + }, + }; + + match Some(5) { + Some(x) => { + if x > 0 { + Some(x) + } else { + None + } + }, + _ => None, + }; + + match Some(6) { + Some(ref x) => { + if x > &0 { + Some(x) + } else { + None + } + }, + _ => None, + }; + + let external_cond = true; + match Some(String::new()) { + Some(x) => { + if external_cond { + Some(x) + } else { + None + } + }, + _ => None, + }; + + if let Some(x) = Some(7) { + if external_cond { Some(x) } else { None } + } else { + None + }; + + match &Some(8) { + &Some(x) => { + if x != 0 { + Some(x) + } else { + None + } + }, + _ => None, + }; + + match Some(9) { + Some(x) => { + if x > 10 && x < 100 { + Some(x) + } else { + None + } + }, + None => None, + }; + + const fn f1() { + // Don't lint, `.filter` is not const + match Some(10) { + Some(x) => { + if x > 10 && x < 100 { + Some(x) + } else { + None + } + }, + None => None, + }; + } + + #[allow(clippy::blocks_in_if_conditions)] + match Some(11) { + // Lint, statement is preserved by `.filter` + Some(x) => { + if { + println!("foo"); + x > 10 && x < 100 + } { + Some(x) + } else { + None + } + }, + None => None, + }; + + match Some(12) { + // Don't Lint, statement is lost by `.filter` + Some(x) => { + if x > 10 && x < 100 { + println!("foo"); + Some(x) + } else { + None + } + }, + None => None, + }; + + match Some(13) { + // Don't Lint, because of `None => Some(1)` + Some(x) => { + if x > 10 && x < 100 { + println!("foo"); + Some(x) + } else { + None + } + }, + None => Some(1), + }; + + unsafe fn f(x: u32) -> bool { + true + } + let _ = match Some(14) { + Some(x) => { + if unsafe { f(x) } { + Some(x) + } else { + None + } + }, + None => None, + }; + let _ = match Some(15) { + Some(x) => unsafe { + if f(x) { Some(x) } else { None } + }, + None => None, + }; + + #[allow(clippy::redundant_pattern_matching)] + if let Some(_) = Some(16) { + Some(16) + } else if let Some(x) = Some(16) { + // Lint starting from here + if x % 2 == 0 { Some(x) } else { None } + } else { + None + }; + + match Some((17, 17)) { + // Not linted for now could be + Some((x, y)) => { + if y != x { + Some((x, y)) + } else { + None + } + }, + None => None, + }; + + struct NamedTuple { + pub x: u8, + pub y: (i32, u32), + } + + match Some(NamedTuple { + // Not linted for now could be + x: 17, + y: (18, 19), + }) { + Some(NamedTuple { x, y }) => { + if y.1 != x as u32 { + Some(NamedTuple { x, y }) + } else { + None + } + }, + None => None, + }; +} diff --git a/tests/ui/manual_filter.stderr b/tests/ui/manual_filter.stderr new file mode 100644 index 000000000..53dea9229 --- /dev/null +++ b/tests/ui/manual_filter.stderr @@ -0,0 +1,191 @@ +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:7:5 + | +LL | / match Some(0) { +LL | | None => None, +LL | | Some(x) => { +LL | | if x > 0 { +... | +LL | | }, +LL | | }; + | |_____^ help: try this: `Some(0).filter(|&x| x <= 0)` + | + = note: `-D clippy::manual-filter` implied by `-D warnings` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:18:5 + | +LL | / match Some(1) { +LL | | Some(x) => { +LL | | if x > 0 { +LL | | None +... | +LL | | None => None, +LL | | }; + | |_____^ help: try this: `Some(1).filter(|&x| x <= 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:29:5 + | +LL | / match Some(2) { +LL | | Some(x) => { +LL | | if x > 0 { +LL | | None +... | +LL | | _ => None, +LL | | }; + | |_____^ help: try this: `Some(2).filter(|&x| x <= 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:40:5 + | +LL | / match Some(3) { +LL | | Some(x) => { +LL | | if x > 0 { +LL | | Some(x) +... | +LL | | None => None, +LL | | }; + | |_____^ help: try this: `Some(3).filter(|&x| x > 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:52:5 + | +LL | / match y { +LL | | // Some(4) +LL | | None => None, +LL | | Some(x) => { +... | +LL | | }, +LL | | }; + | |_____^ help: try this: `y.filter(|&x| x <= 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:64:5 + | +LL | / match Some(5) { +LL | | Some(x) => { +LL | | if x > 0 { +LL | | Some(x) +... | +LL | | _ => None, +LL | | }; + | |_____^ help: try this: `Some(5).filter(|&x| x > 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:75:5 + | +LL | / match Some(6) { +LL | | Some(ref x) => { +LL | | if x > &0 { +LL | | Some(x) +... | +LL | | _ => None, +LL | | }; + | |_____^ help: try this: `Some(6).as_ref().filter(|&x| x > &0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:87:5 + | +LL | / match Some(String::new()) { +LL | | Some(x) => { +LL | | if external_cond { +LL | | Some(x) +... | +LL | | _ => None, +LL | | }; + | |_____^ help: try this: `Some(String::new()).filter(|x| external_cond)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:98:5 + | +LL | / if let Some(x) = Some(7) { +LL | | if external_cond { Some(x) } else { None } +LL | | } else { +LL | | None +LL | | }; + | |_____^ help: try this: `Some(7).filter(|&x| external_cond)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:104:5 + | +LL | / match &Some(8) { +LL | | &Some(x) => { +LL | | if x != 0 { +LL | | Some(x) +... | +LL | | _ => None, +LL | | }; + | |_____^ help: try this: `Some(8).filter(|&x| x != 0)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:115:5 + | +LL | / match Some(9) { +LL | | Some(x) => { +LL | | if x > 10 && x < 100 { +LL | | Some(x) +... | +LL | | None => None, +LL | | }; + | |_____^ help: try this: `Some(9).filter(|&x| x > 10 && x < 100)` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:141:5 + | +LL | / match Some(11) { +LL | | // Lint, statement is preserved by `.filter` +LL | | Some(x) => { +LL | | if { +... | +LL | | None => None, +LL | | }; + | |_____^ + | +help: try this + | +LL ~ Some(11).filter(|&x| { +LL + println!("foo"); +LL + x > 10 && x < 100 +LL ~ }); + | + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:185:13 + | +LL | let _ = match Some(14) { + | _____________^ +LL | | Some(x) => { +LL | | if unsafe { f(x) } { +LL | | Some(x) +... | +LL | | None => None, +LL | | }; + | |_____^ help: try this: `Some(14).filter(|&x| unsafe { f(x) })` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:195:13 + | +LL | let _ = match Some(15) { + | _____________^ +LL | | Some(x) => unsafe { +LL | | if f(x) { Some(x) } else { None } +LL | | }, +LL | | None => None, +LL | | }; + | |_____^ help: try this: `Some(15).filter(|&x| unsafe { f(x) })` + +error: manual implementation of `Option::filter` + --> $DIR/manual_filter.rs:205:12 + | +LL | } else if let Some(x) = Some(16) { + | ____________^ +LL | | // Lint starting from here +LL | | if x % 2 == 0 { Some(x) } else { None } +LL | | } else { +LL | | None +LL | | }; + | |_____^ help: try this: `{ Some(16).filter(|&x| x % 2 == 0) }` + +error: aborting due to 15 previous errors +