diff options
| author | Jason Newcomb <jsnewcomb@pm.me> | 2022-02-17 10:48:24 -0500 |
|---|---|---|
| committer | Jason Newcomb <jsnewcomb@pm.me> | 2022-02-17 10:48:24 -0500 |
| commit | 9bfcbf4f027fea5096e6a721b1b73fbf057e14cf (patch) | |
| tree | cca3252db35445dfd8783a14baecd61aeb269a03 | |
| parent | a4cf91b9c8b393a83dc9690d2721346a1fe0569d (diff) | |
| download | rust-9bfcbf4f027fea5096e6a721b1b73fbf057e14cf.tar.gz rust-9bfcbf4f027fea5096e6a721b1b73fbf057e14cf.zip | |
Remove some redundant checks in various matches lints
| -rw-r--r-- | clippy_lints/src/matches/match_like_matches.rs | 45 | ||||
| -rw-r--r-- | clippy_lints/src/matches/match_same_arms.rs | 152 | ||||
| -rw-r--r-- | clippy_lints/src/matches/mod.rs | 48 | ||||
| -rw-r--r-- | clippy_lints/src/matches/redundant_pattern_match.rs | 12 |
4 files changed, 128 insertions, 129 deletions
diff --git a/clippy_lints/src/matches/match_like_matches.rs b/clippy_lints/src/matches/match_like_matches.rs index d605b6d73c0..2e1f7646eb4 100644 --- a/clippy_lints/src/matches/match_like_matches.rs +++ b/clippy_lints/src/matches/match_like_matches.rs @@ -3,7 +3,7 @@ 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; @@ -11,7 +11,7 @@ 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, @@ -19,7 +19,7 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool 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)]), @@ -27,25 +27,28 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool 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 index 271a3868595..d11dda57e6f 100644 --- 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 index b5ee4561f06..614a824010f 100644 --- 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 index 61c5fa0872f..c491b2775d8 100644 --- 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); |
