diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 51b3babef7f..b1da36a0d40 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -38,7 +38,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(casts::UNNECESSARY_CAST), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(collapsible_if::COLLAPSIBLE_IF), - LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), LintId::of(copies::IFS_SAME_COND), LintId::of(copies::IF_SAME_THEN_ELSE), @@ -143,6 +142,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(match_str_case_mismatch::MATCH_STR_CASE_MISMATCH), + LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 29bfc660d29..58e18b710ea 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -93,7 +93,6 @@ store.register_lints(&[ cognitive_complexity::COGNITIVE_COMPLEXITY, collapsible_if::COLLAPSIBLE_ELSE_IF, collapsible_if::COLLAPSIBLE_IF, - collapsible_match::COLLAPSIBLE_MATCH, comparison_chain::COMPARISON_CHAIN, copies::BRANCHES_SHARING_CODE, copies::IFS_SAME_COND, @@ -263,6 +262,7 @@ store.register_lints(&[ match_on_vec_items::MATCH_ON_VEC_ITEMS, match_result_ok::MATCH_RESULT_OK, match_str_case_mismatch::MATCH_STR_CASE_MISMATCH, + matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, matches::MATCH_AS_REF, matches::MATCH_BOOL, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index ea2e1082458..07e90b8b656 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -12,7 +12,6 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(collapsible_if::COLLAPSIBLE_IF), - LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(dereference::NEEDLESS_BORROW), @@ -50,6 +49,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(map_clone::MAP_CLONE), LintId::of(match_result_ok::MATCH_RESULT_OK), + LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6c3d7594f5c..6e16d470ac8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -192,7 +192,6 @@ mod casts; mod checked_conversions; mod cognitive_complexity; mod collapsible_if; -mod collapsible_match; mod comparison_chain; mod copies; mod copy_iterator; @@ -568,7 +567,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(len_zero::LenZero)); store.register_late_pass(|| Box::new(attrs::Attributes)); store.register_late_pass(|| Box::new(blocks_in_if_conditions::BlocksInIfConditions)); - store.register_late_pass(|| Box::new(collapsible_match::CollapsibleMatch)); store.register_late_pass(|| Box::new(unicode::Unicode)); store.register_late_pass(|| Box::new(uninit_vec::UninitVec)); store.register_late_pass(|| Box::new(unit_hash::UnitHash)); diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/matches/collapsible_match.rs similarity index 71% rename from clippy_lints/src/collapsible_match.rs rename to clippy_lints/src/matches/collapsible_match.rs index ec55009f347..07021f1bcad 100644 --- a/clippy_lints/src/collapsible_match.rs +++ b/clippy_lints/src/matches/collapsible_match.rs @@ -6,68 +6,28 @@ use if_chain::if_chain; use rustc_errors::MultiSpan; use rustc_hir::LangItem::OptionNone; use rustc_hir::{Arm, Expr, Guard, HirId, Let, Pat, PatKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_lint::LateContext; use rustc_span::Span; -declare_clippy_lint! { - /// ### What it does - /// Finds nested `match` or `if let` expressions where the patterns may be "collapsed" together - /// without adding any branches. - /// - /// Note that this lint is not intended to find _all_ cases where nested match patterns can be merged, but only - /// cases where merging would most likely make the code more readable. - /// - /// ### Why is this bad? - /// It is unnecessarily verbose and complex. - /// - /// ### Example - /// ```rust - /// fn func(opt: Option>) { - /// let n = match opt { - /// Some(n) => match n { - /// Ok(n) => n, - /// _ => return, - /// } - /// None => return, - /// }; - /// } - /// ``` - /// Use instead: - /// ```rust - /// fn func(opt: Option>) { - /// let n = match opt { - /// Some(Ok(n)) => n, - /// _ => return, - /// }; - /// } - /// ``` - #[clippy::version = "1.50.0"] - pub COLLAPSIBLE_MATCH, - style, - "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." -} +use super::COLLAPSIBLE_MATCH; -declare_lint_pass!(CollapsibleMatch => [COLLAPSIBLE_MATCH]); - -impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - match IfLetOrMatch::parse(cx, expr) { - Some(IfLetOrMatch::Match(_, arms, _)) => { - if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) { - for arm in arms { - check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); - } - } - }, - Some(IfLetOrMatch::IfLet(_, pat, body, els)) => { - check_arm(cx, false, pat, body, None, els); - }, - None => {}, +pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { + if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) { + for arm in arms { + check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); } } } +pub(super) fn check_if_let<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + body: &'tcx Expr<'_>, + else_expr: Option<&'tcx Expr<'_>>, +) { + check_arm(cx, false, pat, body, None, else_expr); +} + fn check_arm<'tcx>( cx: &LateContext<'tcx>, outer_is_match: bool, diff --git a/clippy_lints/src/matches/match_like_matches.rs b/clippy_lints/src/matches/match_like_matches.rs index 2e1f7646eb4..a68eec842ab 100644 --- a/clippy_lints/src/matches/match_like_matches.rs +++ b/clippy_lints/src/matches/match_like_matches.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_wild; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{higher, is_wild}; use rustc_ast::{Attribute, LitKind}; use rustc_errors::Applicability; use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat}; @@ -11,22 +11,24 @@ use rustc_span::source_map::Spanned; use super::MATCH_LIKE_MATCHES_MACRO; /// Lint a `match` or `if let .. { .. } else { .. }` expr that could be replaced by `matches!` -pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(higher::IfLet { - let_pat, +pub(crate) 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<'_>, +) { + find_matches_sugg( + cx, let_expr, - if_then, - if_else: Some(if_else), - }) = higher::IfLet::hir(cx, expr) - { - find_matches_sugg( - cx, - let_expr, - IntoIterator::into_iter([(&[][..], Some(let_pat), if_then, None), (&[][..], None, if_else, None)]), - expr, - true, - ); - } + IntoIterator::into_iter([ + (&[][..], Some(let_pat), then_expr, None), + (&[][..], None, else_expr, None), + ]), + expr, + true, + ); } pub(super) fn check_match<'tcx>( diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 3d8391bce2b..4a77fb8c31b 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -1,12 +1,14 @@ use clippy_utils::source::{snippet_opt, span_starts_with, walk_span_to_context}; -use clippy_utils::{meets_msrv, msrvs}; +use clippy_utils::{higher, meets_msrv, msrvs}; use rustc_hir::{Arm, Expr, ExprKind, Local, MatchSource, Pat}; use rustc_lexer::{tokenize, TokenKind}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{Span, SpanData, SyntaxContext}; +mod collapsible_match; mod infallible_destructuring_match; mod match_as_ref; mod match_bool; @@ -610,6 +612,44 @@ declare_clippy_lint! { "`match` or match-like `if let` that are unnecessary" } +declare_clippy_lint! { + /// ### What it does + /// Finds nested `match` or `if let` expressions where the patterns may be "collapsed" together + /// without adding any branches. + /// + /// Note that this lint is not intended to find _all_ cases where nested match patterns can be merged, but only + /// cases where merging would most likely make the code more readable. + /// + /// ### Why is this bad? + /// It is unnecessarily verbose and complex. + /// + /// ### Example + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(n) => match n { + /// Ok(n) => n, + /// _ => return, + /// } + /// None => return, + /// }; + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(Ok(n)) => n, + /// _ => return, + /// }; + /// } + /// ``` + #[clippy::version = "1.50.0"] + pub COLLAPSIBLE_MATCH, + style, + "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -644,19 +684,29 @@ impl_lint_pass!(Matches => [ MATCH_LIKE_MATCHES_MACRO, MATCH_SAME_ARMS, NEEDLESS_MATCH, + COLLAPSIBLE_MATCH, ]); impl<'tcx> LateLintPass<'tcx> for Matches { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if expr.span.from_expansion() { + if in_external_macro(cx.sess(), expr.span) { return; } + let from_expansion = expr.span.from_expansion(); if let ExprKind::Match(ex, arms, source) = expr.kind { if !span_starts_with(cx, expr.span, "match") { return; } - if !contains_cfg_arm(cx, expr, ex, arms) { + + collapsible_match::check_match(cx, arms); + if !from_expansion { + // These don't depend on a relationship between multiple arms + match_wild_err_arm::check(cx, ex, arms); + wild_in_or_pats::check(cx, arms); + } + + if !from_expansion && !contains_cfg_arm(cx, expr, ex, arms) { if source == MatchSource::Normal { if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO) && match_like_matches::check_match(cx, expr, ex, arms)) @@ -680,16 +730,32 @@ impl<'tcx> LateLintPass<'tcx> for Matches { } match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), expr); } - - // These don't depend on a relationship between multiple arms - match_wild_err_arm::check(cx, ex, arms); - wild_in_or_pats::check(cx, arms); - } else { - if meets_msrv(self.msrv, msrvs::MATCHES_MACRO) { - match_like_matches::check(cx, expr); + } else if let Some(if_let) = higher::IfLet::hir(cx, expr) { + collapsible_match::check_if_let(cx, if_let.let_pat, if_let.if_then, if_let.if_else); + if !from_expansion { + if let Some(else_expr) = if_let.if_else { + if meets_msrv(self.msrv, msrvs::MATCHES_MACRO) { + match_like_matches::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( + cx, + expr, + if_let.let_pat, + if_let.let_expr, + if_let.if_else.is_some(), + ); + needless_match::check_if_let(cx, expr, &if_let); } + } else if !from_expansion { redundant_pattern_match::check(cx, expr); - needless_match::check(cx, expr); } } diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index f920ad4651f..fa19cddd35e 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -47,20 +47,18 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], /// some_enum /// } /// ``` -pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { - if let Some(ref if_let) = higher::IfLet::hir(cx, ex) { - if !is_else_clause(cx.tcx, ex) && expr_ty_matches_p_ty(cx, if_let.let_expr, ex) && check_if_let(cx, if_let) { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - NEEDLESS_MATCH, - ex.span, - "this if-let expression is unnecessary", - "replace it with", - snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(), - applicability, - ); - } +pub(crate) fn check_if_let<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'_>, if_let: &higher::IfLet<'tcx>) { + if !is_else_clause(cx.tcx, ex) && expr_ty_matches_p_ty(cx, if_let.let_expr, ex) && check_if_let_inner(cx, if_let) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + NEEDLESS_MATCH, + ex.span, + "this if-let expression is unnecessary", + "replace it with", + snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(), + applicability, + ); } } @@ -77,7 +75,7 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>]) true } -fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { +fn check_if_let_inner(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { if let Some(if_else) = if_let.if_else { if !pat_same_as_expr(if_let.let_pat, peel_blocks_with_stmt(if_let.if_then)) { return false; @@ -85,7 +83,7 @@ fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { // Recursively check for each `else if let` phrase, if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) { - return check_if_let(cx, nested_if_let); + return check_if_let_inner(cx, nested_if_let); } if matches!(if_else.kind, ExprKind::Block(..)) { diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs index 1a8b9d15f37..095cd43ea13 100644 --- a/clippy_lints/src/matches/redundant_pattern_match.rs +++ b/clippy_lints/src/matches/redundant_pattern_match.rs @@ -18,19 +18,21 @@ use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty}; use rustc_span::sym; pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(higher::IfLet { - if_else, - let_pat, - let_expr, - .. - }) = higher::IfLet::hir(cx, expr) - { - find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some()); - } else if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) { + if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) { find_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false); } } +pub(super) fn check_if_let<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + pat: &'tcx Pat<'_>, + scrutinee: &'tcx Expr<'_>, + has_else: bool, +) { + find_sugg_for_if_let(cx, expr, pat, scrutinee, "if", has_else); +} + // Extract the generic arguments out of a type fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option> { if_chain! {