From 0958f9486ba0cb8ecd0093ff3100af45d19529f3 Mon Sep 17 00:00:00 2001 From: kraktus Date: Mon, 3 Oct 2022 13:57:42 +0200 Subject: [PATCH 1/3] 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 9f12a673596..cab2808ee3b 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 9ad2a88eb26..21d686fcdf5 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 a58d066fa6b..e3849e5a626 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 307ec40f40b..70defe3bcbd 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 00000000000..9931f1268ab --- /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 76f5e1c941c..2b6a07c5d74 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 e6b183fc05f..cf1bd7a12ad 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 ef836e84829..e88542b77a6 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 39540e4b048..d24342076b6 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 00000000000..19a4d9319d9 --- /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 00000000000..3553291b87d --- /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 00000000000..aa9f90f752b --- /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 00000000000..53dea922930 --- /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 + From b89ac0cefcc19b99a5af902ced053344a8054568 Mon Sep 17 00:00:00 2001 From: kraktus Date: Mon, 3 Oct 2022 14:11:54 +0200 Subject: [PATCH 2/3] refactor `manual_filter` Move common functions to `manual_utils.rs`, better arm matching, use clippy utils `contains_unsafe_block` --- clippy_lints/src/matches/manual_filter.rs | 76 ++---- clippy_lints/src/matches/manual_map.rs | 287 +--------------------- clippy_lints/src/matches/manual_utils.rs | 278 +++++++++++++++++++++ clippy_lints/src/matches/mod.rs | 1 + clippy_utils/src/sugg.rs | 2 +- 5 files changed, 311 insertions(+), 333 deletions(-) create mode 100644 clippy_lints/src/matches/manual_utils.rs diff --git a/clippy_lints/src/matches/manual_filter.rs b/clippy_lints/src/matches/manual_filter.rs index 9931f1268ab..66ba1f6f9c5 100644 --- a/clippy_lints/src/matches/manual_filter.rs +++ b/clippy_lints/src/matches/manual_filter.rs @@ -1,39 +1,20 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::visitors::contains_unsafe_block; 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_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind}; use rustc_lint::LateContext; use rustc_span::{sym, SyntaxContext}; -use super::manual_map::{check_with, SomeExpr}; +use super::manual_utils::{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 {}` +// Function called on the of `[&+]Some((ref | ref mut) x) => ` +// Need to check if it's of the form `=if {} else {}` // AND that only one `then/else_expr` resolves to `Some(x)` while the other resolves to `None` -// return `cond` if +// return the `cond` expression if so. fn get_cond_expr<'tcx>( cx: &LateContext<'tcx>, pat: &Pat<'_>, @@ -45,15 +26,13 @@ fn get_cond_expr<'tcx>( 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)); + = (is_some_expr(cx, target, ctxt, then_expr), + is_some_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_unsafe_block: contains_unsafe_block(cx, expr), needs_negated: !then_visitor // if the `then_expr` resolves to `None`, need to negate the cond }) } @@ -63,7 +42,7 @@ fn get_cond_expr<'tcx>( 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` + // checked by `contains_unsafe_block` if let ExprKind::Block(block, None) = expr.kind { if block.stmts.is_empty() { return block.expr; @@ -76,23 +55,18 @@ 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: +// 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) { +// Returns true if resolves to `Some(x)`, `false` otherwise +fn is_some_expr<'tcx>(cx: &LateContext<'_>, target: HirId, ctxt: SyntaxContext, expr: &'tcx Expr<'_>) -> bool { + if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(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() + return ctxt == expr.span.ctxt() && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) && path_to_local_id(arg, target); } @@ -119,15 +93,13 @@ pub(super) fn check_match<'tcx>( 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) + if is_type_diagnostic_item(cx, ty, sym::Option) + && let [first_arm, second_arm] = arms + && first_arm.guard.is_none() + && second_arm.guard.is_none() + { + check(cx, expr, scrutinee, first_arm.pat, first_arm.body, Some(second_arm.pat), second_arm.body); } - } } pub(super) fn check_if_let<'tcx>( diff --git a/clippy_lints/src/matches/manual_map.rs b/clippy_lints/src/matches/manual_map.rs index 2b6a07c5d74..aaba239677f 100644 --- a/clippy_lints/src/matches/manual_map.rs +++ b/clippy_lints/src/matches/manual_map.rs @@ -1,21 +1,13 @@ +use super::manual_utils::{check_with, SomeExpr}; 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_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, sugg::Sugg, CaptureKind, -}; -use rustc_ast::util::parser::PREC_POSTFIX; -use rustc_errors::Applicability; -use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{ - def::Res, Arm, BindingAnnotation, Block, BlockCheckMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, - QPath, UnsafeSource, -}; + +use clippy_utils::{is_res_lang_ctor, path_res}; + +use rustc_hir::LangItem::OptionSome; +use rustc_hir::{Arm, Block, BlockCheckMode, Expr, ExprKind, Pat, UnsafeSource}; use rustc_lint::LateContext; -use rustc_span::{sym, SyntaxContext}; +use rustc_span::SyntaxContext; pub(super) fn check_match<'tcx>( cx: &LateContext<'tcx>, @@ -83,266 +75,6 @@ fn check<'tcx>( } } -#[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 None; - } - - let expr_ctxt = expr.span.ctxt(); - let (some_expr, some_pat, pat_ref_count, is_wild_none) = match ( - try_parse_pattern(cx, then_pat, expr_ctxt), - else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)), - ) { - (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { - (else_body, pattern, ref_count, true) - }, - (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { - (else_body, pattern, ref_count, false) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => { - (then_body, pattern, ref_count, true) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => { - (then_body, pattern, ref_count, false) - }, - _ => return None, - }; - - // Top level or patterns aren't allowed in closures. - if matches!(some_pat.kind, PatKind::Or(_)) { - return None; - } - - let some_expr = match get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) { - Some(expr) => expr, - 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 None; - } - - // `map` won't perform any adjustments. - if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { - return None; - } - - // Determine which binding mode to use. - let explicit_ref = some_pat.contains_explicit_ref_binding(); - let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then_some(ty_mutability)); - - let as_ref_str = match binding_ref { - Some(Mutability::Mut) => ".as_mut()", - Some(Mutability::Not) => ".as_ref()", - None => "", - }; - - match can_move_expr_to_closure(cx, some_expr.expr) { - Some(captures) => { - // Check if captures the closure will need conflict with borrows made in the scrutinee. - // TODO: check all the references made in the scrutinee expression. This will require interacting - // with the borrow checker. Currently only `[.]*` is checked for. - if let Some(binding_ref_mutability) = binding_ref { - let e = peel_hir_expr_while(scrutinee, |e| match e.kind { - ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), - _ => None, - }); - 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 None, - Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { - return None; - }, - Some(CaptureKind::Ref(Mutability::Not)) | None => (), - } - } - } - }, - None => return None, - }; - - let mut app = Applicability::MachineApplicable; - - // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or - // it's being passed by value. - let scrutinee = peel_hir_expr_refs(scrutinee).0; - let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app); - let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX { - format!("({scrutinee_str})") - } else { - 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; - if let Some(func) = can_pass_as_func(cx, id, some_expr.expr); - if func.span.ctxt() == some_expr.expr.span.ctxt(); - then { - snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() - } else { - if path_to_local_id(some_expr.expr, id) - && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) - && binding_ref.is_some() - { - return None; - } - - // `ref` and `ref mut` annotations were handled earlier. - let annotation = if matches!(annotation, BindingAnnotation::MUT) { - "mut " - } else { - "" - }; - - if some_expr.needs_unsafe_block { - format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}") - } else { - 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; - if some_expr.needs_unsafe_block { - format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}") - } else { - format!("|{pat_snip}| {closure_expr_snip}") - } - } else { - // Refutable bindings and mixed reference annotations can't be handled by `map`. - return None; - }; - - // 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. -// Returns the function to be passed to `map` if it exists. -fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { - match expr.kind { - ExprKind::Call(func, [arg]) - if path_to_local_id(arg, binding) - && cx.typeck_results().expr_adjustments(arg).is_empty() - && !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) => - { - Some(func) - }, - _ => None, - } -} - -#[derive(Debug)] -pub(super) enum OptionPat<'a> { - Wild, - None, - Some { - // The pattern contained in the `Some` tuple. - pattern: &'a Pat<'a>, - // The number of references before the `Some` tuple. - // e.g. `&&Some(_)` has a ref count of 2. - ref_count: usize, - }, -} - -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. -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<'_>, - ref_count: usize, - ctxt: SyntaxContext, - ) -> Option> { - match pat.kind { - PatKind::Wild => Some(OptionPat::Wild), - PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt), - PatKind::Path(ref qpath) if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionNone) => { - Some(OptionPat::None) - }, - PatKind::TupleStruct(ref qpath, [pattern], _) - if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionSome) && pat.span.ctxt() == ctxt => - { - Some(OptionPat::Some { pattern, ref_count }) - }, - _ => None, - } - } - f(cx, pat, 0, ctxt) -} - // Checks for an expression wrapped by the `Some` constructor. Returns the contained expression. fn get_some_expr<'tcx>( cx: &LateContext<'tcx>, @@ -382,8 +114,3 @@ fn get_some_expr<'tcx>( } get_some_expr_internal(cx, expr, false, ctxt) } - -// Checks for the `None` value. -fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone) -} diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs new file mode 100644 index 00000000000..792908aa7df --- /dev/null +++ b/clippy_lints/src/matches/manual_utils.rs @@ -0,0 +1,278 @@ +use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF}; +use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; +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, sugg::Sugg, CaptureKind, +}; +use rustc_ast::util::parser::PREC_POSTFIX; +use rustc_errors::Applicability; +use rustc_hir::LangItem::{OptionNone, OptionSome}; +use rustc_hir::{def::Res, BindingAnnotation, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath}; +use rustc_lint::LateContext; +use rustc_span::{sym, SyntaxContext}; + +#[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 None; + } + + let expr_ctxt = expr.span.ctxt(); + let (some_expr, some_pat, pat_ref_count, is_wild_none) = match ( + try_parse_pattern(cx, then_pat, expr_ctxt), + else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)), + ) { + (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { + (else_body, pattern, ref_count, true) + }, + (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { + (else_body, pattern, ref_count, false) + }, + (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => { + (then_body, pattern, ref_count, true) + }, + (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => { + (then_body, pattern, ref_count, false) + }, + _ => return None, + }; + + // Top level or patterns aren't allowed in closures. + if matches!(some_pat.kind, PatKind::Or(_)) { + return None; + } + + let some_expr = match get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) { + Some(expr) => expr, + 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 None; + } + + // `map` won't perform any adjustments. + if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { + return None; + } + + // Determine which binding mode to use. + let explicit_ref = some_pat.contains_explicit_ref_binding(); + let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then_some(ty_mutability)); + + let as_ref_str = match binding_ref { + Some(Mutability::Mut) => ".as_mut()", + Some(Mutability::Not) => ".as_ref()", + None => "", + }; + + match can_move_expr_to_closure(cx, some_expr.expr) { + Some(captures) => { + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if let Some(binding_ref_mutability) = binding_ref { + let e = peel_hir_expr_while(scrutinee, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + 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 None, + Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { + return None; + }, + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } + }, + None => return None, + }; + + let mut app = Applicability::MachineApplicable; + + // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or + // it's being passed by value. + let scrutinee = peel_hir_expr_refs(scrutinee).0; + let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app); + let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX { + format!("({scrutinee_str})") + } else { + 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; + if let Some(func) = can_pass_as_func(cx, id, some_expr.expr); + if func.span.ctxt() == some_expr.expr.span.ctxt(); + then { + snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() + } else { + if path_to_local_id(some_expr.expr, id) + && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) + && binding_ref.is_some() + { + return None; + } + + // `ref` and `ref mut` annotations were handled earlier. + let annotation = if matches!(annotation, BindingAnnotation::MUT) { + "mut " + } else { + "" + }; + + if some_expr.needs_unsafe_block { + format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}") + } else { + 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; + if some_expr.needs_unsafe_block { + format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}") + } else { + format!("|{pat_snip}| {closure_expr_snip}") + } + } else { + // Refutable bindings and mixed reference annotations can't be handled by `map`. + return None; + }; + + // 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. +// Returns the function to be passed to `map` if it exists. +fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { + match expr.kind { + ExprKind::Call(func, [arg]) + if path_to_local_id(arg, binding) + && cx.typeck_results().expr_adjustments(arg).is_empty() + && !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) => + { + Some(func) + }, + _ => None, + } +} + +#[derive(Debug)] +pub(super) enum OptionPat<'a> { + Wild, + None, + Some { + // The pattern contained in the `Some` tuple. + pattern: &'a Pat<'a>, + // The number of references before the `Some` tuple. + // e.g. `&&Some(_)` has a ref count of 2. + ref_count: usize, + }, +} + +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. +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<'_>, + ref_count: usize, + ctxt: SyntaxContext, + ) -> Option> { + match pat.kind { + PatKind::Wild => Some(OptionPat::Wild), + PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt), + PatKind::Path(ref qpath) if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionNone) => { + Some(OptionPat::None) + }, + PatKind::TupleStruct(ref qpath, [pattern], _) + if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionSome) && pat.span.ctxt() == ctxt => + { + Some(OptionPat::Some { pattern, ref_count }) + }, + _ => None, + } + } + f(cx, pat, 0, ctxt) +} + +// Checks for the `None` value. +fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone) +} diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index cf1bd7a12ad..c472d628098 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -3,6 +3,7 @@ mod infallible_destructuring_match; mod manual_filter; mod manual_map; mod manual_unwrap_or; +mod manual_utils; mod match_as_ref; mod match_bool; mod match_like_matches; diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index e88542b77a6..f25bced0c2b 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -112,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_context(cx, expr.span, ctxt, default, applicability).0; + let (snip, _) = snippet_with_context(cx, expr.span, ctxt, default, applicability); Sugg::NonParen(snip) } } From 830fdf2b56d2a2f0f8e8135e05ec30b08e54ad3a Mon Sep 17 00:00:00 2001 From: kraktus Date: Sun, 2 Oct 2022 17:45:44 +0200 Subject: [PATCH 3/3] update rust version introduction --- clippy_lints/src/matches/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index c472d628098..7d8171ead89 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -922,7 +922,7 @@ declare_clippy_lint! { /// ```rust /// Some(0).filter(|&x| x % 2 == 0); /// ``` - #[clippy::version = "1.65.0"] + #[clippy::version = "1.66.0"] pub MANUAL_FILTER, complexity, "reimplentation of `filter`"