Skip to content

Suggest collapsing nested or patterns if the MSRV allows it #12745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 2, 2024
Merged

Suggest collapsing nested or patterns if the MSRV allows it #12745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
272413f
allow or patterns for a high enough MSRV in `collapsible_match`
y21 May 2, 2024
790fb93
check for `matches!` macro directly in `redundant_guards`
y21 May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions clippy_lints/src/matches/collapsible_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use clippy_config::msrvs::Msrv;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::higher::IfLetOrMatch;
use clippy_utils::source::snippet;
Expand All @@ -11,12 +12,12 @@ use rustc_hir::{Arm, Expr, HirId, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_span::Span;

use super::COLLAPSIBLE_MATCH;
use super::{pat_contains_disallowed_or, COLLAPSIBLE_MATCH};

pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], msrv: &Msrv) {
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, Some(els_arm.body));
check_arm(cx, true, arm.pat, arm.body, arm.guard, Some(els_arm.body), msrv);
}
}
}
Expand All @@ -26,8 +27,9 @@ pub(super) fn check_if_let<'tcx>(
pat: &'tcx Pat<'_>,
body: &'tcx Expr<'_>,
else_expr: Option<&'tcx Expr<'_>>,
msrv: &Msrv,
) {
check_arm(cx, false, pat, body, None, else_expr);
check_arm(cx, false, pat, body, None, else_expr, msrv);
}

fn check_arm<'tcx>(
Expand All @@ -37,6 +39,7 @@ fn check_arm<'tcx>(
outer_then_body: &'tcx Expr<'tcx>,
outer_guard: Option<&'tcx Expr<'tcx>>,
outer_else_body: Option<&'tcx Expr<'tcx>>,
msrv: &Msrv,
) {
let inner_expr = peel_blocks_with_stmt(outer_then_body);
if let Some(inner) = IfLetOrMatch::parse(cx, inner_expr)
Expand All @@ -57,7 +60,7 @@ fn check_arm<'tcx>(
// match expression must be a local binding
// match <local> { .. }
&& let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_scrutinee))
&& !pat_contains_or(inner_then_pat)
&& !pat_contains_disallowed_or(inner_then_pat, msrv)
// the binding must come from the pattern of the containing match arm
// ..<local>.. => match <local> { .. }
&& let (Some(binding_span), is_innermost_parent_pat_struct)
Expand Down Expand Up @@ -142,13 +145,3 @@ fn find_pat_binding_and_is_innermost_parent_pat_struct(pat: &Pat<'_>, hir_id: Hi
});
(span, is_innermost_parent_pat_struct)
}

fn pat_contains_or(pat: &Pat<'_>) -> bool {
let mut result = false;
pat.walk(|p| {
let is_or = matches!(p.kind, PatKind::Or(_));
result |= is_or;
!is_or
});
result
}
23 changes: 19 additions & 4 deletions clippy_lints/src/matches/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ mod wild_in_or_pats;
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::source::walk_span_to_context;
use clippy_utils::{higher, in_constant, is_direct_expn_of, is_span_match, span_contains_cfg};
use rustc_hir::{Arm, Expr, ExprKind, LetStmt, MatchSource, Pat};
use rustc_hir::{Arm, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::impl_lint_pass;
Expand Down Expand Up @@ -1040,7 +1040,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
significant_drop_in_scrutinee::check(cx, expr, ex, arms, source);
}

collapsible_match::check_match(cx, arms);
collapsible_match::check_match(cx, arms, &self.msrv);
if !from_expansion {
// These don't depend on a relationship between multiple arms
match_wild_err_arm::check(cx, ex, arms);
Expand All @@ -1066,7 +1066,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
needless_match::check_match(cx, ex, arms, expr);
match_on_vec_items::check(cx, ex);
match_str_case_mismatch::check(cx, ex, arms);
redundant_guards::check(cx, arms);
redundant_guards::check(cx, arms, &self.msrv);

if !in_constant(cx, expr.hir_id) {
manual_unwrap_or::check(cx, expr, ex, arms);
Expand All @@ -1083,7 +1083,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), 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);
collapsible_match::check_if_let(cx, if_let.let_pat, if_let.if_then, if_let.if_else, &self.msrv);
if !from_expansion {
if let Some(else_expr) = if_let.if_else {
if self.msrv.meets(msrvs::MATCHES_MACRO) {
Expand Down Expand Up @@ -1195,3 +1195,18 @@ fn contains_cfg_arm(cx: &LateContext<'_>, e: &Expr<'_>, scrutinee: &Expr<'_>, ar
Err(()) => true,
}
}

/// Checks if `pat` contains OR patterns that cannot be nested due to a too low MSRV.
fn pat_contains_disallowed_or(pat: &Pat<'_>, msrv: &Msrv) -> bool {
if msrv.meets(msrvs::OR_PATTERNS) {
return false;
}

let mut result = false;
pat.walk(|p| {
let is_or = matches!(p.kind, PatKind::Or(_));
result |= is_or;
!is_or
});
result
}
27 changes: 10 additions & 17 deletions clippy_lints/src/matches/redundant_guards.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,32 @@
use clippy_config::msrvs::Msrv;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::macros::matching_root_macro_call;
use clippy_utils::source::snippet;
use clippy_utils::visitors::{for_each_expr, is_local_used};
use clippy_utils::{in_constant, path_to_local};
use rustc_ast::{BorrowKind, LitKind};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, Pat, PatKind, UnOp};
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, PatKind, UnOp};
use rustc_lint::LateContext;
use rustc_span::symbol::Ident;
use rustc_span::{Span, Symbol};
use rustc_span::{sym, Span, Symbol};
use std::borrow::Cow;
use std::ops::ControlFlow;

use super::REDUNDANT_GUARDS;
use super::{pat_contains_disallowed_or, REDUNDANT_GUARDS};

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>], msrv: &Msrv) {
for outer_arm in arms {
let Some(guard) = outer_arm.guard else {
continue;
};

// `Some(x) if matches!(x, y)`
if let ExprKind::Match(
scrutinee,
[
arm,
Arm {
pat: Pat {
kind: PatKind::Wild, ..
},
..
},
Comment on lines -28 to -33
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't look right. It seems like the intention was to catch if matches!, but because of the way it was written it would still emit a warning on a manually written match that happens to have the same pattern structure but does something in the _ => ... arm that's semantically different. Changed it to look for the matches! macro and added a test case for a false positive that this fixes.

],
MatchSource::Normal,
) = guard.kind
if let ExprKind::Match(scrutinee, [arm, _], MatchSource::Normal) = guard.kind
&& matching_root_macro_call(cx, guard.span, sym::matches_macro).is_some()
&& let Some(binding) = get_pat_binding(cx, scrutinee, outer_arm)
&& !pat_contains_disallowed_or(arm.pat, msrv)
{
let pat_span = match (arm.pat.kind, binding.byref_ident) {
(PatKind::Ref(pat, _), Some(_)) => pat.span,
Expand All @@ -53,6 +45,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
// `Some(x) if let Some(2) = x`
else if let ExprKind::Let(let_expr) = guard.kind
&& let Some(binding) = get_pat_binding(cx, let_expr.init, outer_arm)
&& !pat_contains_disallowed_or(let_expr.pat, msrv)
{
let pat_span = match (let_expr.pat.kind, binding.byref_ident) {
(PatKind::Ref(pat, _), Some(_)) => pat.span,
Expand Down
16 changes: 13 additions & 3 deletions tests/ui/collapsible_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
clippy::needless_return,
clippy::no_effect,
clippy::single_match,
clippy::uninlined_format_args
clippy::uninlined_format_args,
clippy::let_unit_value
)]

fn lint_cases(opt_opt: Option<Option<u32>>, res_opt: Result<Option<u32>, String>) {
Expand Down Expand Up @@ -238,13 +239,22 @@ fn negative_cases(res_opt: Result<Option<u32>, String>, res_res: Result<Result<u
},
_ => return,
}
match make::<Option<E<u32>>>() {
#[clippy::msrv = "1.52.0"]
let _ = match make::<Option<E<u32>>>() {
Some(val) => match val {
E::A(val) | E::B(val) => foo(val),
_ => return,
},
_ => return,
}
};
#[clippy::msrv = "1.53.0"]
let _ = match make::<Option<E<u32>>>() {
Some(val) => match val {
E::A(val) | E::B(val) => foo(val),
_ => return,
},
_ => return,
};
if let Ok(val) = res_opt {
if let Some(n) = val {
let _ = || {
Expand Down
Loading