fix(match_like_matches_macro): FP with guards containing if let

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::diagnostics::span_lint_and_then;
 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, BinOpKind, 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,47 @@ 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_then(
+            cx,
+            MATCH_LIKE_MATCHES_MACRO,
+            expr.span,
+            "if let .. else expression looks like `matches!` macro",
+            |diag| {
+                diag.span_suggestion_verbose(
+                    expr.span,
+                    "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 +70,83 @@ 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<Item = (&'a [Attribute], Option<&'a Pat<'b>>, &'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)|+)
+            // ```
+            //
+            // But if the guard _is_ present, it may not be an `if-let` guard, as `matches!` doesn't
+            // support these (currently?)
+            (middle_arms.is_empty() && first_arm.guard.is_none_or(|g| !contains_if_let(g)))
+
+            // - (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 +156,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,40 +171,52 @@ 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;
         }
-        span_lint_and_sugg(
+        span_lint_and_then(
             cx,
             MATCH_LIKE_MATCHES_MACRO,
-            expr.span,
-            format!(
-                "{} expression looks like `matches!` macro",
-                if is_if_let { "if let .. else" } else { "match" }
-            ),
-            "try",
-            format!(
-                "{}matches!({}, {pat_and_guard})",
-                if b0 { "" } else { "!" },
-                snippet_with_applicability(cx, ex_new.span, "..", &mut applicability),
-            ),
-            applicability,
+            e.span,
+            "match expression looks like `matches!` macro",
+            |diag| {
+                diag.span_suggestion_verbose(
+                    e.span,
+                    "try",
+                    format!(
+                        "{}matches!({}, {pat_and_guard})",
+                        if b0 { "" } else { "!" },
+                        snippet_with_applicability(cx, ex_new.span, "..", &mut applicability),
+                    ),
+                    applicability,
+                );
+            },
         );
         true
     } else {
         false
     }
 }
 
+fn contains_if_let(guard: &Expr<'_>) -> bool {
+    match guard.kind {
+        ExprKind::Binary(and, e1, e2) if matches!(and.node, BinOpKind::And) => {
+            contains_if_let(e1) || contains_if_let(e2)
+        },
+        ExprKind::Let(_) => true,
+        _ => false,
+    }
+}
+
 /// Extract a `bool` or `{ bool }`
-fn find_bool_lit(ex: &ExprKind<'_>) -> Option<bool> {
-    match ex {
+fn find_bool_lit(ex: &Expr<'_>) -> Option<bool> {
+    match ex.kind {
         ExprKind::Lit(Spanned {
             node: LitKind::Bool(b), ..
-        }) => Some(*b),
+        }) => Some(b),
         ExprKind::Block(
             rustc_hir::Block {
                 stmts: [],
@@ -168,8 +238,9 @@ fn find_bool_lit(ex: &ExprKind<'_>) -> Option<bool> {
     }
 }
 
-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