diff --git a/clippy_lints/src/matches/match_like_matches.rs b/clippy_lints/src/matches/match_like_matches.rs index 5816da5695eb..b5f631e8fea3 100644 --- a/clippy_lints/src/matches/match_like_matches.rs +++ b/clippy_lints/src/matches/match_like_matches.rs @@ -1,17 +1,18 @@ +//! Lint a `match` or `if let .. { .. } else { .. }` expr that could be replaced by `matches!` + use super::REDUNDANT_PATTERN_MATCHING; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::{is_lint_allowed, is_wild, span_contains_comment}; use rustc_ast::LitKind; use rustc_errors::Applicability; -use rustc_hir::{Arm, Attribute, BorrowKind, Expr, ExprKind, Pat, PatKind, QPath}; +use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Pat, PatKind, QPath}; use rustc_lint::{LateContext, LintContext}; use rustc_middle::ty; 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_if_let<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, @@ -20,16 +21,42 @@ pub(crate) fn check_if_let<'tcx>( then_expr: &'tcx Expr<'_>, else_expr: &'tcx Expr<'_>, ) { - find_matches_sugg( - cx, - let_expr, - IntoIterator::into_iter([ - (&[][..], Some(let_pat), then_expr, None), - (&[][..], None, else_expr, None), - ]), - expr, - true, - ); + if !span_contains_comment(cx.sess().source_map(), expr.span) + && cx.typeck_results().expr_ty(expr).is_bool() + && let Some(b0) = find_bool_lit(then_expr) + && let Some(b1) = find_bool_lit(else_expr) + && b0 != b1 + { + if !is_lint_allowed(cx, REDUNDANT_PATTERN_MATCHING, let_pat.hir_id) && is_some_wild(let_pat.kind) { + return; + } + + // The suggestion may be incorrect, because some arms can have `cfg` attributes + // evaluated into `false` and so such arms will be stripped before. + let mut applicability = Applicability::MaybeIncorrect; + let pat = snippet_with_applicability(cx, let_pat.span, "..", &mut applicability); + + // strip potential borrows (#6503), but only if the type is a reference + let mut ex_new = let_expr; + if let ExprKind::AddrOf(BorrowKind::Ref, .., ex_inner) = let_expr.kind + && let ty::Ref(..) = cx.typeck_results().expr_ty(ex_inner).kind() + { + ex_new = ex_inner; + } + span_lint_and_sugg( + cx, + MATCH_LIKE_MATCHES_MACRO, + expr.span, + "if let .. else expression looks like `matches!` macro", + "try", + format!( + "{}matches!({}, {pat})", + if b0 { "" } else { "!" }, + snippet_with_applicability(cx, ex_new.span, "..", &mut applicability), + ), + applicability, + ); + } } pub(super) fn check_match<'tcx>( @@ -38,55 +65,80 @@ pub(super) fn check_match<'tcx>( scrutinee: &'tcx Expr<'_>, arms: &'tcx [Arm<'tcx>], ) -> bool { - find_matches_sugg( - cx, - scrutinee, - arms.iter() - .map(|arm| (cx.tcx.hir_attrs(arm.hir_id), Some(arm.pat), arm.body, arm.guard)), - e, - false, - ) -} - -/// Lint a `match` or `if let` for replacement by `matches!` -fn find_matches_sugg<'a, 'b, I>( - cx: &LateContext<'_>, - ex: &Expr<'_>, - mut iter: I, - expr: &Expr<'_>, - is_if_let: bool, -) -> bool -where - 'b: 'a, - I: Clone - + DoubleEndedIterator - + ExactSizeIterator - + Iterator>, &'a Expr<'b>, Option<&'a Expr<'b>>)>, -{ - if !span_contains_comment(cx.sess().source_map(), expr.span) - && iter.len() >= 2 - && cx.typeck_results().expr_ty(expr).is_bool() - && let Some((_, last_pat_opt, last_expr, _)) = iter.next_back() - && let iter_without_last = iter.clone() - && let Some((first_attrs, _, first_expr, first_guard)) = iter.next() - && let Some(b0) = find_bool_lit(&first_expr.kind) - && let Some(b1) = find_bool_lit(&last_expr.kind) + if let Some((last_arm, arms_without_last)) = arms.split_last() + && let Some((first_arm, middle_arms)) = arms_without_last.split_first() + && !span_contains_comment(cx.sess().source_map(), e.span) + && cx.typeck_results().expr_ty(e).is_bool() + && let Some(b0) = find_bool_lit(first_arm.body) + && let Some(b1) = find_bool_lit(last_arm.body) && b0 != b1 - && (first_guard.is_none() || iter.len() == 0) - && first_attrs.is_empty() - && iter.all(|arm| find_bool_lit(&arm.2.kind).is_some_and(|b| b == b0) && arm.3.is_none() && arm.0.is_empty()) + // We handle two cases: + && ( + // - There are no middle arms, i.e., 2 arms in total + // + // In that case, the first arm may or may not have a guard, because this: + // ```rs + // match e { + // Either::Left $(if $guard)|+ => true, // or `false`, but then we'll need `!matches!(..)` + // _ => false, + // } + // ``` + // can always become this: + // ```rs + // matches!(e, Either::Left $(if $guard)|+) + // ``` + middle_arms.is_empty() + + // - (added in #6216) There are middle arms + // + // In that case, neither they nor the first arm may have guards + // -- otherwise, they couldn't be combined into an or-pattern in `matches!` + // + // This: + // ```rs + // match e { + // Either3::First => true, + // Either3::Second => true, + // _ /* matches `Either3::Third` */ => false, + // } + // ``` + // can become this: + // ```rs + // matches!(e, Either3::First | Either3::Second) + // ``` + // + // But this: + // ```rs + // match e { + // Either3::First if X => true, + // Either3::Second => true, + // _ => false, + // } + // ``` + // cannot be transformed. + // + // We set an additional constraint of all of them needing to return the same bool, + // so we don't lint things like: + // ```rs + // match e { + // Either3::First => true, + // Either3::Second => false, + // _ => false, + // } + // ``` + // This is not *strictly* necessary, but it simplifies the logic a bit + || arms_without_last.iter().all(|arm| { + cx.tcx.hir_attrs(arm.hir_id).is_empty() && arm.guard.is_none() && find_bool_lit(arm.body) == Some(b0) + }) + ) { - if let Some(last_pat) = last_pat_opt - && !is_wild(last_pat) - { + if !is_wild(last_arm.pat) { return false; } - for arm in iter_without_last.clone() { - if let Some(pat) = arm.1 - && !is_lint_allowed(cx, REDUNDANT_PATTERN_MATCHING, pat.hir_id) - && is_some(pat.kind) - { + for arm in arms_without_last { + let pat = arm.pat; + if !is_lint_allowed(cx, REDUNDANT_PATTERN_MATCHING, pat.hir_id) && is_some_wild(pat.kind) { return false; } } @@ -96,14 +148,12 @@ where let mut applicability = Applicability::MaybeIncorrect; let pat = { use itertools::Itertools as _; - iter_without_last - .filter_map(|arm| { - let pat_span = arm.1?.span; - Some(snippet_with_applicability(cx, pat_span, "..", &mut applicability)) - }) + arms_without_last + .iter() + .map(|arm| snippet_with_applicability(cx, arm.pat.span, "..", &mut applicability)) .join(" | ") }; - let pat_and_guard = if let Some(g) = first_guard { + let pat_and_guard = if let Some(g) = first_arm.guard { format!( "{pat} if {}", snippet_with_applicability(cx, g.span, "..", &mut applicability) @@ -113,8 +163,8 @@ where }; // strip potential borrows (#6503), but only if the type is a reference - let mut ex_new = ex; - if let ExprKind::AddrOf(BorrowKind::Ref, .., ex_inner) = ex.kind + let mut ex_new = scrutinee; + if let ExprKind::AddrOf(BorrowKind::Ref, .., ex_inner) = scrutinee.kind && let ty::Ref(..) = cx.typeck_results().expr_ty(ex_inner).kind() { ex_new = ex_inner; @@ -122,11 +172,8 @@ where span_lint_and_sugg( cx, MATCH_LIKE_MATCHES_MACRO, - expr.span, - format!( - "{} expression looks like `matches!` macro", - if is_if_let { "if let .. else" } else { "match" } - ), + e.span, + "match expression looks like `matches!` macro", "try", format!( "{}matches!({}, {pat_and_guard})", @@ -142,11 +189,11 @@ where } /// Extract a `bool` or `{ bool }` -fn find_bool_lit(ex: &ExprKind<'_>) -> Option { - match ex { +fn find_bool_lit(ex: &Expr<'_>) -> Option { + match ex.kind { ExprKind::Lit(Spanned { node: LitKind::Bool(b), .. - }) => Some(*b), + }) => Some(b), ExprKind::Block( rustc_hir::Block { stmts: [], @@ -168,8 +215,9 @@ fn find_bool_lit(ex: &ExprKind<'_>) -> Option { } } -fn is_some(path_kind: PatKind<'_>) -> bool { - match path_kind { +/// Checks whether a pattern is `Some(_)` +fn is_some_wild(pat_kind: PatKind<'_>) -> bool { + match pat_kind { PatKind::TupleStruct(QPath::Resolved(_, path), [first, ..], _) if is_wild(first) => { let name = path.segments[0].ident; name.name == rustc_span::sym::Some diff --git a/tests/ui/match_expr_like_matches_macro.fixed b/tests/ui/match_like_matches_macro.fixed similarity index 97% rename from tests/ui/match_expr_like_matches_macro.fixed rename to tests/ui/match_like_matches_macro.fixed index 8530ab16bfd7..a1c95e8a94f1 100644 --- a/tests/ui/match_expr_like_matches_macro.fixed +++ b/tests/ui/match_like_matches_macro.fixed @@ -1,7 +1,6 @@ #![warn(clippy::match_like_matches_macro)] #![allow( unreachable_patterns, - dead_code, clippy::equatable_if_let, clippy::needless_borrowed_reference, clippy::redundant_guards @@ -14,11 +13,11 @@ fn main() { let _y = matches!(x, Some(0)); //~^^^^ match_like_matches_macro - // Lint + // No lint: covered by `redundant_pattern_matching` let _w = x.is_some(); //~^^^^ redundant_pattern_matching - // Turn into is_none + // No lint: covered by `redundant_pattern_matching` let _z = x.is_none(); //~^^^^ redundant_pattern_matching diff --git a/tests/ui/match_expr_like_matches_macro.rs b/tests/ui/match_like_matches_macro.rs similarity index 97% rename from tests/ui/match_expr_like_matches_macro.rs rename to tests/ui/match_like_matches_macro.rs index 81017936889e..eb419ba5bf8d 100644 --- a/tests/ui/match_expr_like_matches_macro.rs +++ b/tests/ui/match_like_matches_macro.rs @@ -1,7 +1,6 @@ #![warn(clippy::match_like_matches_macro)] #![allow( unreachable_patterns, - dead_code, clippy::equatable_if_let, clippy::needless_borrowed_reference, clippy::redundant_guards @@ -17,14 +16,14 @@ fn main() { }; //~^^^^ match_like_matches_macro - // Lint + // No lint: covered by `redundant_pattern_matching` let _w = match x { Some(_) => true, _ => false, }; //~^^^^ redundant_pattern_matching - // Turn into is_none + // No lint: covered by `redundant_pattern_matching` let _z = match x { Some(_) => false, None => true, diff --git a/tests/ui/match_expr_like_matches_macro.stderr b/tests/ui/match_like_matches_macro.stderr similarity index 84% rename from tests/ui/match_expr_like_matches_macro.stderr rename to tests/ui/match_like_matches_macro.stderr index 8fceb05bc6e8..ae277ce4dca6 100644 --- a/tests/ui/match_expr_like_matches_macro.stderr +++ b/tests/ui/match_like_matches_macro.stderr @@ -1,5 +1,5 @@ error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:14:14 + --> tests/ui/match_like_matches_macro.rs:13:14 | LL | let _y = match x { | ______________^ @@ -12,7 +12,7 @@ LL | | }; = help: to override `-D warnings` add `#[allow(clippy::match_like_matches_macro)]` error: redundant pattern matching, consider using `is_some()` - --> tests/ui/match_expr_like_matches_macro.rs:21:14 + --> tests/ui/match_like_matches_macro.rs:20:14 | LL | let _w = match x { | ______________^ @@ -25,7 +25,7 @@ LL | | }; = help: to override `-D warnings` add `#[allow(clippy::redundant_pattern_matching)]` error: redundant pattern matching, consider using `is_none()` - --> tests/ui/match_expr_like_matches_macro.rs:28:14 + --> tests/ui/match_like_matches_macro.rs:27:14 | LL | let _z = match x { | ______________^ @@ -35,7 +35,7 @@ LL | | }; | |_____^ help: try: `x.is_none()` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:35:15 + --> tests/ui/match_like_matches_macro.rs:34:15 | LL | let _zz = match x { | _______________^ @@ -45,13 +45,13 @@ LL | | }; | |_____^ help: try: `!matches!(x, Some(r) if r == 0)` error: if let .. else expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:42:16 + --> tests/ui/match_like_matches_macro.rs:41:16 | LL | let _zzz = if let Some(5) = x { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(x, Some(5))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:67:20 + --> tests/ui/match_like_matches_macro.rs:66:20 | LL | let _ans = match x { | ____________________^ @@ -62,7 +62,7 @@ LL | | }; | |_________^ help: try: `matches!(x, E::A(_) | E::B(_))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:78:20 + --> tests/ui/match_like_matches_macro.rs:77:20 | LL | let _ans = match x { | ____________________^ @@ -74,7 +74,7 @@ LL | | }; | |_________^ help: try: `matches!(x, E::A(_) | E::B(_))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:89:20 + --> tests/ui/match_like_matches_macro.rs:88:20 | LL | let _ans = match x { | ____________________^ @@ -85,7 +85,7 @@ LL | | }; | |_________^ help: try: `!matches!(x, E::B(_) | E::C)` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:150:18 + --> tests/ui/match_like_matches_macro.rs:149:18 | LL | let _z = match &z { | __________________^ @@ -95,7 +95,7 @@ LL | | }; | |_________^ help: try: `matches!(z, Some(3))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:160:18 + --> tests/ui/match_like_matches_macro.rs:159:18 | LL | let _z = match &z { | __________________^ @@ -105,7 +105,7 @@ LL | | }; | |_________^ help: try: `matches!(&z, Some(3))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:178:21 + --> tests/ui/match_like_matches_macro.rs:177:21 | LL | let _ = match &z { | _____________________^ @@ -115,7 +115,7 @@ LL | | }; | |_____________^ help: try: `matches!(&z, AnEnum::X)` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:193:20 + --> tests/ui/match_like_matches_macro.rs:192:20 | LL | let _res = match &val { | ____________________^ @@ -125,7 +125,7 @@ LL | | }; | |_________^ help: try: `matches!(&val, &Some(ref _a))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:206:20 + --> tests/ui/match_like_matches_macro.rs:205:20 | LL | let _res = match &val { | ____________________^ @@ -135,7 +135,7 @@ LL | | }; | |_________^ help: try: `matches!(&val, &Some(ref _a))` error: match expression looks like `matches!` macro - --> tests/ui/match_expr_like_matches_macro.rs:265:14 + --> tests/ui/match_like_matches_macro.rs:264:14 | LL | let _y = match Some(5) { | ______________^