Remove some redundant checks in various matches lints

Jarcho · Jarcho · commit 9bfcbf4f027f · 2022-02-17T10:48:24.000-05:00
diff --git a/clippy_lints/src/matches/match_like_matches.rs b/clippy_lints/src/matches/match_like_matches.rs
@@ -3,49 +3,52 @@ 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::{BorrowKind, Expr, ExprKind, Guard, MatchSource, Pat};
+use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat};
 use rustc_lint::LateContext;
 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<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     if let Some(higher::IfLet {
         let_pat,
         let_expr,
         if_then,
         if_else: Some(if_else),
     }) = higher::IfLet::hir(cx, expr)
     {
-        return find_matches_sugg(
+        find_matches_sugg(
             cx,
             let_expr,
             IntoIterator::into_iter([(&[][..], Some(let_pat), if_then, None), (&[][..], None, if_else, None)]),
             expr,
             true,
         );
     }
+}
 
-    if let ExprKind::Match(scrut, arms, MatchSource::Normal) = expr.kind {
-        return find_matches_sugg(
-            cx,
-            scrut,
-            arms.iter().map(|arm| {
-                (
-                    cx.tcx.hir().attrs(arm.hir_id),
-                    Some(arm.pat),
-                    arm.body,
-                    arm.guard.as_ref(),
-                )
-            }),
-            expr,
-            false,
-        );
-    }
-
-    false
+pub(super) fn check_match<'tcx>(
+    cx: &LateContext<'tcx>,
+    e: &'tcx Expr<'_>,
+    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.as_ref(),
+            )
+        }),
+        e,
+        false,
+    )
 }
 
 /// Lint a `match` or `if let` for replacement by `matches!`
diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs
@@ -1,96 +1,94 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
 use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash};
-use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, MatchSource, Pat, PatKind};
+use rustc_hir::{Arm, Expr, HirId, HirIdMap, HirIdSet, Pat, PatKind};
 use rustc_lint::LateContext;
 use std::collections::hash_map::Entry;
 
 use super::MATCH_SAME_ARMS;
 
-pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
-    if let ExprKind::Match(_, arms, MatchSource::Normal) = expr.kind {
-        let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
-            let mut h = SpanlessHash::new(cx);
-            h.hash_expr(arm.body);
-            h.finish()
-        };
+pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
+    let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
+        let mut h = SpanlessHash::new(cx);
+        h.hash_expr(arm.body);
+        h.finish()
+    };
 
-        let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
-            let min_index = usize::min(lindex, rindex);
-            let max_index = usize::max(lindex, rindex);
+    let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
+        let min_index = usize::min(lindex, rindex);
+        let max_index = usize::max(lindex, rindex);
 
-            let mut local_map: HirIdMap<HirId> = HirIdMap::default();
-            let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
-                if_chain! {
-                    if let Some(a_id) = path_to_local(a);
-                    if let Some(b_id) = path_to_local(b);
-                    let entry = match local_map.entry(a_id) {
-                        Entry::Vacant(entry) => entry,
-                        // check if using the same bindings as before
-                        Entry::Occupied(entry) => return *entry.get() == b_id,
-                    };
-                    // the names technically don't have to match; this makes the lint more conservative
-                    if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
-                    if cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b);
-                    if pat_contains_local(lhs.pat, a_id);
-                    if pat_contains_local(rhs.pat, b_id);
-                    then {
-                        entry.insert(b_id);
-                        true
-                    } else {
-                        false
-                    }
+        let mut local_map: HirIdMap<HirId> = HirIdMap::default();
+        let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
+            if_chain! {
+                if let Some(a_id) = path_to_local(a);
+                if let Some(b_id) = path_to_local(b);
+                let entry = match local_map.entry(a_id) {
+                    Entry::Vacant(entry) => entry,
+                    // check if using the same bindings as before
+                    Entry::Occupied(entry) => return *entry.get() == b_id,
+                };
+                // the names technically don't have to match; this makes the lint more conservative
+                if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
+                if cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b);
+                if pat_contains_local(lhs.pat, a_id);
+                if pat_contains_local(rhs.pat, b_id);
+                then {
+                    entry.insert(b_id);
+                    true
+                } else {
+                    false
                 }
-            };
-            // Arms with a guard are ignored, those can’t always be merged together
-            // This is also the case for arms in-between each there is an arm with a guard
-            (min_index..=max_index).all(|index| arms[index].guard.is_none())
-                && SpanlessEq::new(cx)
-                    .expr_fallback(eq_fallback)
-                    .eq_expr(lhs.body, rhs.body)
-                // these checks could be removed to allow unused bindings
-                && bindings_eq(lhs.pat, local_map.keys().copied().collect())
-                && bindings_eq(rhs.pat, local_map.values().copied().collect())
+            }
         };
+        // Arms with a guard are ignored, those can’t always be merged together
+        // This is also the case for arms in-between each there is an arm with a guard
+        (min_index..=max_index).all(|index| arms[index].guard.is_none())
+            && SpanlessEq::new(cx)
+                .expr_fallback(eq_fallback)
+                .eq_expr(lhs.body, rhs.body)
+            // these checks could be removed to allow unused bindings
+            && bindings_eq(lhs.pat, local_map.keys().copied().collect())
+            && bindings_eq(rhs.pat, local_map.values().copied().collect())
+    };
 
-        let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
-        for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
-            span_lint_and_then(
-                cx,
-                MATCH_SAME_ARMS,
-                j.body.span,
-                "this `match` has identical arm bodies",
-                |diag| {
-                    diag.span_note(i.body.span, "same as this");
+    let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
+    for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
+        span_lint_and_then(
+            cx,
+            MATCH_SAME_ARMS,
+            j.body.span,
+            "this `match` has identical arm bodies",
+            |diag| {
+                diag.span_note(i.body.span, "same as this");
 
-                    // Note: this does not use `span_suggestion` on purpose:
-                    // there is no clean way
-                    // to remove the other arm. Building a span and suggest to replace it to ""
-                    // makes an even more confusing error message. Also in order not to make up a
-                    // span for the whole pattern, the suggestion is only shown when there is only
-                    // one pattern. The user should know about `|` if they are already using it…
+                // Note: this does not use `span_suggestion` on purpose:
+                // there is no clean way
+                // to remove the other arm. Building a span and suggest to replace it to ""
+                // makes an even more confusing error message. Also in order not to make up a
+                // span for the whole pattern, the suggestion is only shown when there is only
+                // one pattern. The user should know about `|` if they are already using it…
 
-                    let lhs = snippet(cx, i.pat.span, "<pat1>");
-                    let rhs = snippet(cx, j.pat.span, "<pat2>");
+                let lhs = snippet(cx, i.pat.span, "<pat1>");
+                let rhs = snippet(cx, j.pat.span, "<pat2>");
 
-                    if let PatKind::Wild = j.pat.kind {
-                        // if the last arm is _, then i could be integrated into _
-                        // note that i.pat cannot be _, because that would mean that we're
-                        // hiding all the subsequent arms, and rust won't compile
-                        diag.span_note(
-                            i.body.span,
-                            &format!(
-                                "`{}` has the same arm body as the `_` wildcard, consider removing it",
-                                lhs
-                            ),
-                        );
-                    } else {
-                        diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
-                            .help("...or consider changing the match arm bodies");
-                    }
-                },
-            );
-        }
+                if let PatKind::Wild = j.pat.kind {
+                    // if the last arm is _, then i could be integrated into _
+                    // note that i.pat cannot be _, because that would mean that we're
+                    // hiding all the subsequent arms, and rust won't compile
+                    diag.span_note(
+                        i.body.span,
+                        &format!(
+                            "`{}` has the same arm body as the `_` wildcard, consider removing it",
+                            lhs
+                        ),
+                    );
+                } else {
+                    diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
+                        .help("...or consider changing the match arm bodies");
+                }
+            },
+        );
     }
 }
 
diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs
@@ -604,33 +604,35 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
             return;
         }
 
-        redundant_pattern_match::check(cx, expr);
+        if let ExprKind::Match(ex, arms, source) = expr.kind {
+            if source == MatchSource::Normal {
+                if !(meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO)
+                    && match_like_matches::check_match(cx, expr, ex, arms))
+                {
+                    match_same_arms::check(cx, arms);
+                }
 
-        if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
-            if !match_like_matches::check(cx, expr) {
-                match_same_arms::check(cx, expr);
-            }
-        } else {
-            match_same_arms::check(cx, expr);
-        }
-
-        if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
-            single_match::check(cx, ex, arms, expr);
-            match_bool::check(cx, ex, arms, expr);
-            overlapping_arms::check(cx, ex, arms);
-            match_wild_err_arm::check(cx, ex, arms);
-            match_wild_enum::check(cx, ex, arms);
-            match_as_ref::check(cx, ex, arms, expr);
-            wild_in_or_pats::check(cx, arms);
+                redundant_pattern_match::check_match(cx, expr, ex, arms);
+                single_match::check(cx, ex, arms, expr);
+                match_bool::check(cx, ex, arms, expr);
+                overlapping_arms::check(cx, ex, arms);
+                match_wild_err_arm::check(cx, ex, arms);
+                match_wild_enum::check(cx, ex, arms);
+                match_as_ref::check(cx, ex, arms, expr);
+                wild_in_or_pats::check(cx, arms);
 
-            if self.infallible_destructuring_match_linted {
-                self.infallible_destructuring_match_linted = false;
-            } else {
-                match_single_binding::check(cx, ex, arms, expr);
+                if self.infallible_destructuring_match_linted {
+                    self.infallible_destructuring_match_linted = false;
+                } else {
+                    match_single_binding::check(cx, ex, arms, expr);
+                }
             }
-        }
-        if let ExprKind::Match(ex, arms, _) = expr.kind {
             match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), expr);
+        } else {
+            if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
+                match_like_matches::check(cx, expr);
+            }
+            redundant_pattern_match::check(cx, expr);
         }
     }
 
diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs
@@ -12,13 +12,13 @@ use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, PollPending};
 use rustc_hir::{
     intravisit::{walk_expr, Visitor},
-    Arm, Block, Expr, ExprKind, LangItem, MatchSource, Node, Pat, PatKind, QPath, UnOp,
+    Arm, Block, Expr, ExprKind, LangItem, Node, Pat, PatKind, QPath, UnOp,
 };
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
 use rustc_span::sym;
 
-pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     if let Some(higher::IfLet {
         if_else,
         let_pat,
@@ -27,11 +27,7 @@ pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     }) = higher::IfLet::hir(cx, expr)
     {
         find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some());
-    }
-    if let ExprKind::Match(op, arms, MatchSource::Normal) = &expr.kind {
-        find_sugg_for_match(cx, expr, op, arms);
-    }
-    if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
+    } else 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);
     }
 }
@@ -304,7 +300,7 @@ fn find_sugg_for_if_let<'tcx>(
     );
 }
 
-fn find_sugg_for_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
+pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
     if arms.len() == 2 {
         let node_pair = (&arms[0].pat.kind, &arms[1].pat.kind);
 
