mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #12745 - y21:collapsible_match_or_pat, r=llogiq

Browse Source 
Suggest collapsing nested or patterns if the MSRV allows it

Nested `or` patterns have been stable since 1.53, so we should be able to suggest `Some(1 | 2)` if the MSRV isn't set below that.

This change adds an msrv check and also moves it to `matches/mod.rs`, because it's also needed by `redundant_guards`.

changelog: [`collapsible_match`]: suggest collapsing nested or patterns if the MSRV allows it
This commit is contained in:
bors 2024-05-02 12:57:59 +00:00
commit 20b085d500
8 changed files with 134 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
clippy_lints/src/matches/collapsible_match.rs
View File

@ -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;
@ -11,12 +12,12 @@
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);
}
}
}
@ -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>(
@ -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)
@ -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)
@ -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
clippy_lints/src/matches/mod.rs
View File

@ -27,7 +27,7 @@
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;
@ -1040,7 +1040,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
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);
@ -1066,7 +1066,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
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);
@ -1083,7 +1083,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
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) {
@ -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
clippy_lints/src/matches/redundant_guards.rs
View File

@ -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, ..
},
..
},
],
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,
@ -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,

16
tests/ui/collapsible_match.rs
View File

@ -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>) {
@ -238,13 +239,22 @@ enum E<T> {
},
_ => 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 _ = || {

68
tests/ui/collapsible_match.stderr
View File

@ -1,5 +1,5 @@
error: this `match` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:13:20
--> tests/ui/collapsible_match.rs:14:20
|
LL | Ok(val) => match val {
| ____________________^
@ -10,7 +10,7 @@ LL | | },
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:13:12
--> tests/ui/collapsible_match.rs:14:12
|
LL | Ok(val) => match val {
| ^^^ replace this binding
@ -21,7 +21,7 @@ LL | Some(n) => foo(n),
= help: to override `-D warnings` add `#[allow(clippy::collapsible_match)]`
error: this `match` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:23:20
--> tests/ui/collapsible_match.rs:24:20
|
LL | Ok(val) => match val {
| ____________________^
@ -32,7 +32,7 @@ LL | | },
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:23:12
--> tests/ui/collapsible_match.rs:24:12
|
LL | Ok(val) => match val {
| ^^^ replace this binding
@ -41,7 +41,7 @@ LL | Some(n) => foo(n),
| ^^^^^^^ with this pattern
error: this `if let` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:33:9
--> tests/ui/collapsible_match.rs:34:9
|
LL | / if let Some(n) = val {
LL | |
@ -50,7 +50,7 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:32:15
--> tests/ui/collapsible_match.rs:33:15
|
LL | if let Ok(val) = res_opt {
| ^^^ replace this binding
@ -58,7 +58,7 @@ LL | if let Some(n) = val {
| ^^^^^^^ with this pattern
error: this `if let` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:41:9
--> tests/ui/collapsible_match.rs:42:9
|
LL | / if let Some(n) = val {
LL | |
@ -69,7 +69,7 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:40:15
--> tests/ui/collapsible_match.rs:41:15
|
LL | if let Ok(val) = res_opt {
| ^^^ replace this binding
@ -77,7 +77,7 @@ LL | if let Some(n) = val {
| ^^^^^^^ with this pattern
error: this `match` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:53:9
--> tests/ui/collapsible_match.rs:54:9
|
LL | / match val {
LL | |
@ -87,7 +87,7 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:52:15
--> tests/ui/collapsible_match.rs:53:15
|
LL | if let Ok(val) = res_opt {
| ^^^ replace this binding
@ -96,7 +96,7 @@ LL | Some(n) => foo(n),
| ^^^^^^^ with this pattern
error: this `if let` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:63:13
--> tests/ui/collapsible_match.rs:64:13
|
LL | / if let Some(n) = val {
LL | |
@ -105,7 +105,7 @@ LL | | }
| |_____________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:62:12
--> tests/ui/collapsible_match.rs:63:12
|
LL | Ok(val) => {
| ^^^ replace this binding
@ -113,7 +113,7 @@ LL | if let Some(n) = val {
| ^^^^^^^ with this pattern
error: this `match` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:73:9
--> tests/ui/collapsible_match.rs:74:9
|
LL | / match val {
LL | |
@ -123,7 +123,7 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:72:15
--> tests/ui/collapsible_match.rs:73:15
|
LL | if let Ok(val) = res_opt {
| ^^^ replace this binding
@ -132,7 +132,7 @@ LL | Some(n) => foo(n),
| ^^^^^^^ with this pattern
error: this `if let` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:85:13
--> tests/ui/collapsible_match.rs:86:13
|
LL | / if let Some(n) = val {
LL | |
@ -143,7 +143,7 @@ LL | | }
| |_____________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:84:12
--> tests/ui/collapsible_match.rs:85:12
|
LL | Ok(val) => {
| ^^^ replace this binding
@ -151,7 +151,7 @@ LL | if let Some(n) = val {
| ^^^^^^^ with this pattern
error: this `match` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:97:20
--> tests/ui/collapsible_match.rs:98:20
|
LL | Ok(val) => match val {
| ____________________^
@ -162,7 +162,7 @@ LL | | },
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:97:12
--> tests/ui/collapsible_match.rs:98:12
|
LL | Ok(val) => match val {
| ^^^ replace this binding
@ -171,7 +171,7 @@ LL | Some(n) => foo(n),
| ^^^^^^^ with this pattern
error: this `match` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:107:22
--> tests/ui/collapsible_match.rs:108:22
|
LL | Some(val) => match val {
| ______________________^
@ -182,7 +182,7 @@ LL | | },
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:107:14
--> tests/ui/collapsible_match.rs:108:14
|
LL | Some(val) => match val {
| ^^^ replace this binding
@ -190,8 +190,26 @@ LL |
LL | Some(n) => foo(n),
| ^^^^^^^ with this pattern
error: this `match` can be collapsed into the outer `match`
--> tests/ui/collapsible_match.rs:252:22
|
LL | Some(val) => match val {
| ______________________^
LL | | E::A(val) | E::B(val) => foo(val),
LL | | _ => return,
LL | | },
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:252:14
|
LL | Some(val) => match val {
| ^^^ replace this binding
LL | E::A(val) | E::B(val) => foo(val),
| ^^^^^^^^^^^^^^^^^^^^^ with this pattern
error: this `if let` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:273:9
--> tests/ui/collapsible_match.rs:283:9
|
LL | / if let Some(u) = a {
LL | |
@ -200,7 +218,7 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:272:27
--> tests/ui/collapsible_match.rs:282:27
|
LL | if let Issue9647::A { a, .. } = x {
| ^ replace this binding
@ -208,7 +226,7 @@ LL | if let Some(u) = a {
| ^^^^^^^ with this pattern, prefixed by a:
error: this `if let` can be collapsed into the outer `if let`
--> tests/ui/collapsible_match.rs:282:9
--> tests/ui/collapsible_match.rs:292:9
|
LL | / if let Some(u) = a {
LL | |
@ -217,12 +235,12 @@ LL | | }
| |_________^
|
help: the outer pattern can be modified to include the inner pattern
--> tests/ui/collapsible_match.rs:281:35
--> tests/ui/collapsible_match.rs:291:35
|
LL | if let Issue9647::A { a: Some(a), .. } = x {
| ^ replace this binding
LL | if let Some(u) = a {
| ^^^^^^^ with this pattern
error: aborting due to 12 previous errors
error: aborting due to 13 previous errors

12
tests/ui/redundant_guards.fixed
View File

@ -136,6 +136,18 @@ fn f(s: Option<std::ffi::OsString>) {
}
}
fn not_matches() {
match Some(42) {
// The pattern + guard is not equivalent to `Some(42)` because of the `panic!`
Some(v)
if match v {
42 => true,
_ => panic!(),
} => {},
_ => {},
}
}
struct S {
a: usize,
}

12
tests/ui/redundant_guards.rs
View File

@ -136,6 +136,18 @@ fn f(s: Option<std::ffi::OsString>) {
}
}
fn not_matches() {
match Some(42) {
// The pattern + guard is not equivalent to `Some(42)` because of the `panic!`
Some(v)
if match v {
42 => true,
_ => panic!(),
} => {},
_ => {},
}
}
struct S {
a: usize,
}

34
tests/ui/redundant_guards.stderr
View File

@ -132,7 +132,7 @@ LL + 1 => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:174:28
--> tests/ui/redundant_guards.rs:186:28
|
LL | Some(ref x) if x == &1 => {},
| ^^^^^^^
@ -144,7 +144,7 @@ LL + Some(1) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:175:28
--> tests/ui/redundant_guards.rs:187:28
|
LL | Some(ref x) if &1 == x => {},
| ^^^^^^^
@ -156,7 +156,7 @@ LL + Some(1) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:176:28
--> tests/ui/redundant_guards.rs:188:28
|
LL | Some(ref x) if let &2 = x => {},
| ^^^^^^^^^^
@ -168,7 +168,7 @@ LL + Some(2) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:177:28
--> tests/ui/redundant_guards.rs:189:28
|
LL | Some(ref x) if matches!(x, &3) => {},
| ^^^^^^^^^^^^^^^
@ -180,7 +180,7 @@ LL + Some(3) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:197:32
--> tests/ui/redundant_guards.rs:209:32
|
LL | B { ref c, .. } if c == &1 => {},
| ^^^^^^^
@ -192,7 +192,7 @@ LL + B { c: 1, .. } => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:198:32
--> tests/ui/redundant_guards.rs:210:32
|
LL | B { ref c, .. } if &1 == c => {},
| ^^^^^^^
@ -204,7 +204,7 @@ LL + B { c: 1, .. } => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:199:32
--> tests/ui/redundant_guards.rs:211:32
|
LL | B { ref c, .. } if let &1 = c => {},
| ^^^^^^^^^^
@ -216,7 +216,7 @@ LL + B { c: 1, .. } => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:200:32
--> tests/ui/redundant_guards.rs:212:32
|
LL | B { ref c, .. } if matches!(c, &1) => {},
| ^^^^^^^^^^^^^^^
@ -228,7 +228,7 @@ LL + B { c: 1, .. } => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:210:26
--> tests/ui/redundant_guards.rs:222:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
@ -240,7 +240,7 @@ LL + Some(Some("")) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:221:26
--> tests/ui/redundant_guards.rs:233:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
@ -252,7 +252,7 @@ LL + Some(Some([])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:226:26
--> tests/ui/redundant_guards.rs:238:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
@ -264,7 +264,7 @@ LL + Some(Some([])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:237:26
--> tests/ui/redundant_guards.rs:249:26
|
LL | Some(Some(x)) if x.starts_with(&[]) => {},
| ^^^^^^^^^^^^^^^^^^
@ -276,7 +276,7 @@ LL + Some(Some([..])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:242:26
--> tests/ui/redundant_guards.rs:254:26
|
LL | Some(Some(x)) if x.starts_with(&[1]) => {},
| ^^^^^^^^^^^^^^^^^^^
@ -288,7 +288,7 @@ LL + Some(Some([1, ..])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:247:26
--> tests/ui/redundant_guards.rs:259:26
|
LL | Some(Some(x)) if x.starts_with(&[1, 2]) => {},
| ^^^^^^^^^^^^^^^^^^^^^^
@ -300,7 +300,7 @@ LL + Some(Some([1, 2, ..])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:252:26
--> tests/ui/redundant_guards.rs:264:26
|
LL | Some(Some(x)) if x.ends_with(&[1, 2]) => {},
| ^^^^^^^^^^^^^^^^^^^^
@ -312,7 +312,7 @@ LL + Some(Some([.., 1, 2])) => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:274:18
--> tests/ui/redundant_guards.rs:286:18
|
LL | y if y.is_empty() => {},
| ^^^^^^^^^^^^
@ -324,7 +324,7 @@ LL + "" => {},
|
error: redundant guard
--> tests/ui/redundant_guards.rs:293:22
--> tests/ui/redundant_guards.rs:305:22
|
LL | y if y.is_empty() => {},
| ^^^^^^^^^^^^