mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-15 17:28:07 +00:00
Auto merge of #6216 - alex-700:improve-match-like-matches-lint, r=ebroto
Improve match like matches lint fixes #6186 changelog: improve MATCH_LIKE_MATCHES_MACRO lint
This commit is contained in:
commit
de83f09be8
11 changed files with 441 additions and 244 deletions
|
@ -1,13 +1,8 @@
|
|||
use crate::utils::{eq_expr_value, in_macro, SpanlessEq, SpanlessHash};
|
||||
use crate::utils::{get_parent_expr, higher, if_sequence, snippet, span_lint_and_note, span_lint_and_then};
|
||||
use rustc_data_structures::fx::FxHashMap;
|
||||
use rustc_hir::{Arm, Block, Expr, ExprKind, MatchSource, Pat, PatKind};
|
||||
use crate::utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
|
||||
use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_note};
|
||||
use rustc_hir::{Block, Expr};
|
||||
use rustc_lint::{LateContext, LateLintPass};
|
||||
use rustc_middle::ty::{Ty, TyS};
|
||||
use rustc_session::{declare_lint_pass, declare_tool_lint};
|
||||
use rustc_span::symbol::Symbol;
|
||||
use std::collections::hash_map::Entry;
|
||||
use std::hash::BuildHasherDefault;
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// **What it does:** Checks for consecutive `if`s with the same condition.
|
||||
|
@ -108,48 +103,7 @@ declare_clippy_lint! {
|
|||
"`if` with the same `then` and `else` blocks"
|
||||
}
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// **What it does:** Checks for `match` with identical arm bodies.
|
||||
///
|
||||
/// **Why is this bad?** This is probably a copy & paste error. If arm bodies
|
||||
/// are the same on purpose, you can factor them
|
||||
/// [using `|`](https://doc.rust-lang.org/book/patterns.html#multiple-patterns).
|
||||
///
|
||||
/// **Known problems:** False positive possible with order dependent `match`
|
||||
/// (see issue
|
||||
/// [#860](https://github.com/rust-lang/rust-clippy/issues/860)).
|
||||
///
|
||||
/// **Example:**
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar => bar(),
|
||||
/// Quz => quz(),
|
||||
/// Baz => bar(), // <= oops
|
||||
/// }
|
||||
/// ```
|
||||
///
|
||||
/// This should probably be
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar => bar(),
|
||||
/// Quz => quz(),
|
||||
/// Baz => baz(), // <= fixed
|
||||
/// }
|
||||
/// ```
|
||||
///
|
||||
/// or if the original code was not a typo:
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar | Baz => bar(), // <= shows the intent better
|
||||
/// Quz => quz(),
|
||||
/// }
|
||||
/// ```
|
||||
pub MATCH_SAME_ARMS,
|
||||
pedantic,
|
||||
"`match` with identical arm bodies"
|
||||
}
|
||||
|
||||
declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
|
||||
declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE]);
|
||||
|
||||
impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
|
||||
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
|
||||
|
@ -167,7 +121,6 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
|
|||
lint_same_then_else(cx, &blocks);
|
||||
lint_same_cond(cx, &conds);
|
||||
lint_same_fns_in_if_cond(cx, &conds);
|
||||
lint_match_arms(cx, expr);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -243,122 +196,6 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) {
|
|||
}
|
||||
}
|
||||
|
||||
/// Implementation of `MATCH_SAME_ARMS`.
|
||||
fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
|
||||
fn same_bindings<'tcx>(lhs: &FxHashMap<Symbol, Ty<'tcx>>, rhs: &FxHashMap<Symbol, Ty<'tcx>>) -> bool {
|
||||
lhs.len() == rhs.len()
|
||||
&& lhs
|
||||
.iter()
|
||||
.all(|(name, l_ty)| rhs.get(name).map_or(false, |r_ty| TyS::same_type(l_ty, r_ty)))
|
||||
}
|
||||
|
||||
if let ExprKind::Match(_, ref arms, MatchSource::Normal) = expr.kind {
|
||||
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);
|
||||
|
||||
// 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).eq_expr(&lhs.body, &rhs.body) &&
|
||||
// all patterns should have the same bindings
|
||||
same_bindings(&bindings(cx, &lhs.pat), &bindings(cx, &rhs.pat))
|
||||
};
|
||||
|
||||
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…
|
||||
|
||||
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));
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the list of bindings in a pattern.
|
||||
fn bindings<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>) -> FxHashMap<Symbol, Ty<'tcx>> {
|
||||
fn bindings_impl<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>, map: &mut FxHashMap<Symbol, Ty<'tcx>>) {
|
||||
match pat.kind {
|
||||
PatKind::Box(ref pat) | PatKind::Ref(ref pat, _) => bindings_impl(cx, pat, map),
|
||||
PatKind::TupleStruct(_, pats, _) => {
|
||||
for pat in pats {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Binding(.., ident, ref as_pat) => {
|
||||
if let Entry::Vacant(v) = map.entry(ident.name) {
|
||||
v.insert(cx.typeck_results().pat_ty(pat));
|
||||
}
|
||||
if let Some(ref as_pat) = *as_pat {
|
||||
bindings_impl(cx, as_pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Or(fields) | PatKind::Tuple(fields, _) => {
|
||||
for pat in fields {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Struct(_, fields, _) => {
|
||||
for pat in fields {
|
||||
bindings_impl(cx, &pat.pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Slice(lhs, ref mid, rhs) => {
|
||||
for pat in lhs {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
if let Some(ref mid) = *mid {
|
||||
bindings_impl(cx, mid, map);
|
||||
}
|
||||
for pat in rhs {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Lit(..) | PatKind::Range(..) | PatKind::Wild | PatKind::Path(..) => (),
|
||||
}
|
||||
}
|
||||
|
||||
let mut result = FxHashMap::default();
|
||||
bindings_impl(cx, pat, &mut result);
|
||||
result
|
||||
}
|
||||
|
||||
fn search_same_sequenced<T, Eq>(exprs: &[T], eq: Eq) -> Option<(&T, &T)>
|
||||
where
|
||||
Eq: Fn(&T, &T) -> bool,
|
||||
|
@ -370,47 +207,3 @@ where
|
|||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a T)>
|
||||
where
|
||||
Eq: Fn(&T, &T) -> bool,
|
||||
{
|
||||
if exprs.len() == 2 && eq(&exprs[0], &exprs[1]) {
|
||||
Some((&exprs[0], &exprs[1]))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
|
||||
where
|
||||
Hash: Fn(&T) -> u64,
|
||||
Eq: Fn(&T, &T) -> bool,
|
||||
{
|
||||
if let Some(expr) = search_common_cases(&exprs, &eq) {
|
||||
return vec![expr];
|
||||
}
|
||||
|
||||
let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
|
||||
|
||||
let mut map: FxHashMap<_, Vec<&_>> =
|
||||
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
|
||||
|
||||
for expr in exprs {
|
||||
match map.entry(hash(expr)) {
|
||||
Entry::Occupied(mut o) => {
|
||||
for o in o.get() {
|
||||
if eq(o, expr) {
|
||||
match_expr_list.push((o, expr));
|
||||
}
|
||||
}
|
||||
o.get_mut().push(expr);
|
||||
},
|
||||
Entry::Vacant(v) => {
|
||||
v.insert(vec![expr]);
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
match_expr_list
|
||||
}
|
||||
|
|
|
@ -528,7 +528,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
&comparison_chain::COMPARISON_CHAIN,
|
||||
&copies::IFS_SAME_COND,
|
||||
&copies::IF_SAME_THEN_ELSE,
|
||||
&copies::MATCH_SAME_ARMS,
|
||||
&copies::SAME_FUNCTIONS_IN_IF_CONDITION,
|
||||
©_iterator::COPY_ITERATOR,
|
||||
&create_dir::CREATE_DIR,
|
||||
|
@ -659,6 +658,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
&matches::MATCH_LIKE_MATCHES_MACRO,
|
||||
&matches::MATCH_OVERLAPPING_ARM,
|
||||
&matches::MATCH_REF_PATS,
|
||||
&matches::MATCH_SAME_ARMS,
|
||||
&matches::MATCH_SINGLE_BINDING,
|
||||
&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
|
||||
&matches::MATCH_WILD_ERR_ARM,
|
||||
|
@ -1204,7 +1204,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
LintId::of(&attrs::INLINE_ALWAYS),
|
||||
LintId::of(&bit_mask::VERBOSE_BIT_MASK),
|
||||
LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
|
||||
LintId::of(&copies::MATCH_SAME_ARMS),
|
||||
LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),
|
||||
LintId::of(©_iterator::COPY_ITERATOR),
|
||||
LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS),
|
||||
|
@ -1234,6 +1233,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
|
||||
LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
|
||||
LintId::of(&matches::MATCH_BOOL),
|
||||
LintId::of(&matches::MATCH_SAME_ARMS),
|
||||
LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),
|
||||
LintId::of(&matches::MATCH_WILD_ERR_ARM),
|
||||
LintId::of(&matches::SINGLE_MATCH_ELSE),
|
||||
|
|
|
@ -1,5 +1,4 @@
|
|||
use crate::consts::{constant, miri_to_const, Constant};
|
||||
use crate::utils::paths;
|
||||
use crate::utils::sugg::Sugg;
|
||||
use crate::utils::usage::is_unused;
|
||||
use crate::utils::{
|
||||
|
@ -8,8 +7,10 @@ use crate::utils::{
|
|||
snippet_block, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
|
||||
span_lint_and_then,
|
||||
};
|
||||
use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash};
|
||||
use if_chain::if_chain;
|
||||
use rustc_ast::ast::LitKind;
|
||||
use rustc_data_structures::fx::FxHashMap;
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir::def::CtorKind;
|
||||
use rustc_hir::{
|
||||
|
@ -18,10 +19,12 @@ use rustc_hir::{
|
|||
};
|
||||
use rustc_lint::{LateContext, LateLintPass, LintContext};
|
||||
use rustc_middle::lint::in_external_macro;
|
||||
use rustc_middle::ty::{self, Ty};
|
||||
use rustc_middle::ty::{self, Ty, TyS};
|
||||
use rustc_session::{declare_tool_lint, impl_lint_pass};
|
||||
use rustc_span::source_map::{Span, Spanned};
|
||||
use rustc_span::Symbol;
|
||||
use std::cmp::Ordering;
|
||||
use std::collections::hash_map::Entry;
|
||||
use std::collections::Bound;
|
||||
|
||||
declare_clippy_lint! {
|
||||
|
@ -475,6 +478,47 @@ declare_clippy_lint! {
|
|||
"a match that could be written with the matches! macro"
|
||||
}
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// **What it does:** Checks for `match` with identical arm bodies.
|
||||
///
|
||||
/// **Why is this bad?** This is probably a copy & paste error. If arm bodies
|
||||
/// are the same on purpose, you can factor them
|
||||
/// [using `|`](https://doc.rust-lang.org/book/patterns.html#multiple-patterns).
|
||||
///
|
||||
/// **Known problems:** False positive possible with order dependent `match`
|
||||
/// (see issue
|
||||
/// [#860](https://github.com/rust-lang/rust-clippy/issues/860)).
|
||||
///
|
||||
/// **Example:**
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar => bar(),
|
||||
/// Quz => quz(),
|
||||
/// Baz => bar(), // <= oops
|
||||
/// }
|
||||
/// ```
|
||||
///
|
||||
/// This should probably be
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar => bar(),
|
||||
/// Quz => quz(),
|
||||
/// Baz => baz(), // <= fixed
|
||||
/// }
|
||||
/// ```
|
||||
///
|
||||
/// or if the original code was not a typo:
|
||||
/// ```rust,ignore
|
||||
/// match foo {
|
||||
/// Bar | Baz => bar(), // <= shows the intent better
|
||||
/// Quz => quz(),
|
||||
/// }
|
||||
/// ```
|
||||
pub MATCH_SAME_ARMS,
|
||||
pedantic,
|
||||
"`match` with identical arm bodies"
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
pub struct Matches {
|
||||
infallible_destructuring_match_linted: bool,
|
||||
|
@ -495,7 +539,8 @@ impl_lint_pass!(Matches => [
|
|||
INFALLIBLE_DESTRUCTURING_MATCH,
|
||||
REST_PAT_IN_FULLY_BOUND_STRUCTS,
|
||||
REDUNDANT_PATTERN_MATCHING,
|
||||
MATCH_LIKE_MATCHES_MACRO
|
||||
MATCH_LIKE_MATCHES_MACRO,
|
||||
MATCH_SAME_ARMS,
|
||||
]);
|
||||
|
||||
impl<'tcx> LateLintPass<'tcx> for Matches {
|
||||
|
@ -505,7 +550,9 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
|
|||
}
|
||||
|
||||
redundant_pattern_match::check(cx, expr);
|
||||
check_match_like_matches(cx, expr);
|
||||
if !check_match_like_matches(cx, expr) {
|
||||
lint_match_arms(cx, expr);
|
||||
}
|
||||
|
||||
if let ExprKind::Match(ref ex, ref arms, MatchSource::Normal) = expr.kind {
|
||||
check_single_match(cx, ex, arms, expr);
|
||||
|
@ -1063,32 +1110,47 @@ fn check_wild_in_or_pats(cx: &LateContext<'_>, arms: &[Arm<'_>]) {
|
|||
}
|
||||
|
||||
/// Lint a `match` or `if let .. { .. } else { .. }` expr that could be replaced by `matches!`
|
||||
fn check_match_like_matches<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
|
||||
fn check_match_like_matches<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
|
||||
if let ExprKind::Match(ex, arms, ref match_source) = &expr.kind {
|
||||
match match_source {
|
||||
MatchSource::Normal => find_matches_sugg(cx, ex, arms, expr, false),
|
||||
MatchSource::IfLetDesugar { .. } => find_matches_sugg(cx, ex, arms, expr, true),
|
||||
_ => return,
|
||||
_ => false,
|
||||
}
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
/// Lint a `match` or desugared `if let` for replacement by `matches!`
|
||||
fn find_matches_sugg(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>, desugared: bool) {
|
||||
fn find_matches_sugg(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>, desugared: bool) -> bool {
|
||||
if_chain! {
|
||||
if arms.len() == 2;
|
||||
if arms.len() >= 2;
|
||||
if cx.typeck_results().expr_ty(expr).is_bool();
|
||||
if is_wild(&arms[1].pat);
|
||||
if let Some(first) = find_bool_lit(&arms[0].body.kind, desugared);
|
||||
if let Some(second) = find_bool_lit(&arms[1].body.kind, desugared);
|
||||
if first != second;
|
||||
if let Some((b1_arm, b0_arms)) = arms.split_last();
|
||||
if let Some(b0) = find_bool_lit(&b0_arms[0].body.kind, desugared);
|
||||
if let Some(b1) = find_bool_lit(&b1_arm.body.kind, desugared);
|
||||
if is_wild(&b1_arm.pat);
|
||||
if b0 != b1;
|
||||
let if_guard = &b0_arms[0].guard;
|
||||
if if_guard.is_none() || b0_arms.len() == 1;
|
||||
if b0_arms[1..].iter()
|
||||
.all(|arm| {
|
||||
find_bool_lit(&arm.body.kind, desugared).map_or(false, |b| b == b0) &&
|
||||
arm.guard.is_none()
|
||||
});
|
||||
then {
|
||||
let mut applicability = Applicability::MachineApplicable;
|
||||
|
||||
let pat_and_guard = if let Some(Guard::If(g)) = arms[0].guard {
|
||||
format!("{} if {}", snippet_with_applicability(cx, arms[0].pat.span, "..", &mut applicability), snippet_with_applicability(cx, g.span, "..", &mut applicability))
|
||||
let pat = {
|
||||
use itertools::Itertools as _;
|
||||
b0_arms.iter()
|
||||
.map(|arm| snippet_with_applicability(cx, arm.pat.span, "..", &mut applicability))
|
||||
.join(" | ")
|
||||
};
|
||||
let pat_and_guard = if let Some(Guard::If(g)) = if_guard {
|
||||
format!("{} if {}", pat, snippet_with_applicability(cx, g.span, "..", &mut applicability))
|
||||
} else {
|
||||
format!("{}", snippet_with_applicability(cx, arms[0].pat.span, "..", &mut applicability))
|
||||
pat
|
||||
};
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
|
@ -1098,12 +1160,15 @@ fn find_matches_sugg(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr
|
|||
"try this",
|
||||
format!(
|
||||
"{}matches!({}, {})",
|
||||
if first { "" } else { "!" },
|
||||
if b0 { "" } else { "!" },
|
||||
snippet_with_applicability(cx, ex.span, "..", &mut applicability),
|
||||
pat_and_guard,
|
||||
),
|
||||
applicability,
|
||||
)
|
||||
);
|
||||
true
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -1657,3 +1722,119 @@ fn test_overlapping() {
|
|||
],)
|
||||
);
|
||||
}
|
||||
|
||||
/// Implementation of `MATCH_SAME_ARMS`.
|
||||
fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
|
||||
fn same_bindings<'tcx>(lhs: &FxHashMap<Symbol, Ty<'tcx>>, rhs: &FxHashMap<Symbol, Ty<'tcx>>) -> bool {
|
||||
lhs.len() == rhs.len()
|
||||
&& lhs
|
||||
.iter()
|
||||
.all(|(name, l_ty)| rhs.get(name).map_or(false, |r_ty| TyS::same_type(l_ty, r_ty)))
|
||||
}
|
||||
|
||||
if let ExprKind::Match(_, ref arms, MatchSource::Normal) = expr.kind {
|
||||
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);
|
||||
|
||||
// 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).eq_expr(&lhs.body, &rhs.body) &&
|
||||
// all patterns should have the same bindings
|
||||
same_bindings(&bindings(cx, &lhs.pat), &bindings(cx, &rhs.pat))
|
||||
};
|
||||
|
||||
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…
|
||||
|
||||
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));
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the list of bindings in a pattern.
|
||||
fn bindings<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>) -> FxHashMap<Symbol, Ty<'tcx>> {
|
||||
fn bindings_impl<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>, map: &mut FxHashMap<Symbol, Ty<'tcx>>) {
|
||||
match pat.kind {
|
||||
PatKind::Box(ref pat) | PatKind::Ref(ref pat, _) => bindings_impl(cx, pat, map),
|
||||
PatKind::TupleStruct(_, pats, _) => {
|
||||
for pat in pats {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Binding(.., ident, ref as_pat) => {
|
||||
if let Entry::Vacant(v) = map.entry(ident.name) {
|
||||
v.insert(cx.typeck_results().pat_ty(pat));
|
||||
}
|
||||
if let Some(ref as_pat) = *as_pat {
|
||||
bindings_impl(cx, as_pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Or(fields) | PatKind::Tuple(fields, _) => {
|
||||
for pat in fields {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Struct(_, fields, _) => {
|
||||
for pat in fields {
|
||||
bindings_impl(cx, &pat.pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Slice(lhs, ref mid, rhs) => {
|
||||
for pat in lhs {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
if let Some(ref mid) = *mid {
|
||||
bindings_impl(cx, mid, map);
|
||||
}
|
||||
for pat in rhs {
|
||||
bindings_impl(cx, pat, map);
|
||||
}
|
||||
},
|
||||
PatKind::Lit(..) | PatKind::Range(..) | PatKind::Wild | PatKind::Path(..) => (),
|
||||
}
|
||||
}
|
||||
|
||||
let mut result = FxHashMap::default();
|
||||
bindings_impl(cx, pat, &mut result);
|
||||
result
|
||||
}
|
||||
|
|
|
@ -261,14 +261,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
|
|||
pub fn eq_path_segment(&mut self, left: &PathSegment<'_>, right: &PathSegment<'_>) -> bool {
|
||||
// The == of idents doesn't work with different contexts,
|
||||
// we have to be explicit about hygiene
|
||||
if left.ident.as_str() != right.ident.as_str() {
|
||||
return false;
|
||||
}
|
||||
match (&left.args, &right.args) {
|
||||
(&None, &None) => true,
|
||||
(&Some(ref l), &Some(ref r)) => self.eq_path_parameters(l, r),
|
||||
_ => false,
|
||||
}
|
||||
left.ident.as_str() == right.ident.as_str()
|
||||
&& both(&left.args, &right.args, |l, r| self.eq_path_parameters(l, r))
|
||||
}
|
||||
|
||||
pub fn eq_ty(&mut self, left: &Ty<'_>, right: &Ty<'_>) -> bool {
|
||||
|
|
|
@ -27,11 +27,14 @@ pub use self::diagnostics::*;
|
|||
pub use self::hir_utils::{both, eq_expr_value, over, SpanlessEq, SpanlessHash};
|
||||
|
||||
use std::borrow::Cow;
|
||||
use std::collections::hash_map::Entry;
|
||||
use std::hash::BuildHasherDefault;
|
||||
use std::mem;
|
||||
|
||||
use if_chain::if_chain;
|
||||
use rustc_ast::ast::{self, Attribute, LitKind};
|
||||
use rustc_attr as attr;
|
||||
use rustc_data_structures::fx::FxHashMap;
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir as hir;
|
||||
use rustc_hir::def::{DefKind, Res};
|
||||
|
@ -1465,6 +1468,41 @@ pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<S
|
|||
None
|
||||
}
|
||||
|
||||
/// returns list of all pairs (a, b) from `exprs` such that `eq(a, b)`
|
||||
/// `hash` must be comformed with `eq`
|
||||
pub fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
|
||||
where
|
||||
Hash: Fn(&T) -> u64,
|
||||
Eq: Fn(&T, &T) -> bool,
|
||||
{
|
||||
if exprs.len() == 2 && eq(&exprs[0], &exprs[1]) {
|
||||
return vec![(&exprs[0], &exprs[1])];
|
||||
}
|
||||
|
||||
let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
|
||||
|
||||
let mut map: FxHashMap<_, Vec<&_>> =
|
||||
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
|
||||
|
||||
for expr in exprs {
|
||||
match map.entry(hash(expr)) {
|
||||
Entry::Occupied(mut o) => {
|
||||
for o in o.get() {
|
||||
if eq(o, expr) {
|
||||
match_expr_list.push((o, expr));
|
||||
}
|
||||
}
|
||||
o.get_mut().push(expr);
|
||||
},
|
||||
Entry::Vacant(v) => {
|
||||
v.insert(vec![expr]);
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
match_expr_list
|
||||
}
|
||||
|
||||
#[macro_export]
|
||||
macro_rules! unwrap_cargo_metadata {
|
||||
($cx: ident, $lint: ident, $deps: expr) => {{
|
||||
|
|
|
@ -1304,7 +1304,7 @@ vec![
|
|||
group: "pedantic",
|
||||
desc: "`match` with identical arm bodies",
|
||||
deprecation: None,
|
||||
module: "copies",
|
||||
module: "matches",
|
||||
},
|
||||
Lint {
|
||||
name: "match_single_binding",
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
// run-rustfix
|
||||
|
||||
#![warn(clippy::match_like_matches_macro)]
|
||||
#![allow(unreachable_patterns)]
|
||||
#![allow(unreachable_patterns, dead_code)]
|
||||
|
||||
fn main() {
|
||||
let x = Some(5);
|
||||
|
@ -33,4 +33,70 @@ fn main() {
|
|||
_ => true,
|
||||
None => false,
|
||||
};
|
||||
|
||||
enum E {
|
||||
A(u32),
|
||||
B(i32),
|
||||
C,
|
||||
D,
|
||||
};
|
||||
let x = E::A(2);
|
||||
{
|
||||
// lint
|
||||
let _ans = matches!(x, E::A(_) | E::B(_));
|
||||
}
|
||||
{
|
||||
// lint
|
||||
let _ans = !matches!(x, E::B(_) | E::C);
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(_) => false,
|
||||
E::C => true,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => true,
|
||||
E::B(_) => false,
|
||||
E::C => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(a) if a < 10 => false,
|
||||
E::B(a) if a < 10 => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(a) if a < 10 => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(a) => a == 10,
|
||||
E::B(_) => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(_) => true,
|
||||
_ => false,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
// run-rustfix
|
||||
|
||||
#![warn(clippy::match_like_matches_macro)]
|
||||
#![allow(unreachable_patterns)]
|
||||
#![allow(unreachable_patterns, dead_code)]
|
||||
|
||||
fn main() {
|
||||
let x = Some(5);
|
||||
|
@ -45,4 +45,78 @@ fn main() {
|
|||
_ => true,
|
||||
None => false,
|
||||
};
|
||||
|
||||
enum E {
|
||||
A(u32),
|
||||
B(i32),
|
||||
C,
|
||||
D,
|
||||
};
|
||||
let x = E::A(2);
|
||||
{
|
||||
// lint
|
||||
let _ans = match x {
|
||||
E::A(_) => true,
|
||||
E::B(_) => true,
|
||||
_ => false,
|
||||
};
|
||||
}
|
||||
{
|
||||
// lint
|
||||
let _ans = match x {
|
||||
E::B(_) => false,
|
||||
E::C => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(_) => false,
|
||||
E::C => true,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => true,
|
||||
E::B(_) => false,
|
||||
E::C => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(a) if a < 10 => false,
|
||||
E::B(a) if a < 10 => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(a) if a < 10 => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(a) => a == 10,
|
||||
E::B(_) => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
{
|
||||
// no lint
|
||||
let _ans = match x {
|
||||
E::A(_) => false,
|
||||
E::B(_) => true,
|
||||
_ => false,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
|
|
@ -48,5 +48,27 @@ error: if let .. else expression looks like `matches!` macro
|
|||
LL | let _zzz = if let Some(5) = x { true } else { false };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `matches!(x, Some(5))`
|
||||
|
||||
error: aborting due to 5 previous errors
|
||||
error: match expression looks like `matches!` macro
|
||||
--> $DIR/match_expr_like_matches_macro.rs:58:20
|
||||
|
|
||||
LL | let _ans = match x {
|
||||
| ____________________^
|
||||
LL | | E::A(_) => true,
|
||||
LL | | E::B(_) => true,
|
||||
LL | | _ => false,
|
||||
LL | | };
|
||||
| |_________^ help: try this: `matches!(x, E::A(_) | E::B(_))`
|
||||
|
||||
error: match expression looks like `matches!` macro
|
||||
--> $DIR/match_expr_like_matches_macro.rs:66:20
|
||||
|
|
||||
LL | let _ans = match x {
|
||||
| ____________________^
|
||||
LL | | E::B(_) => false,
|
||||
LL | | E::C => false,
|
||||
LL | | _ => true,
|
||||
LL | | };
|
||||
| |_________^ help: try this: `!matches!(x, E::B(_) | E::C)`
|
||||
|
||||
error: aborting due to 7 previous errors
|
||||
|
||||
|
|
|
@ -119,6 +119,22 @@ fn match_same_arms() {
|
|||
unreachable!();
|
||||
},
|
||||
}
|
||||
|
||||
match_expr_like_matches_macro_priority();
|
||||
}
|
||||
|
||||
fn match_expr_like_matches_macro_priority() {
|
||||
enum E {
|
||||
A,
|
||||
B,
|
||||
C,
|
||||
}
|
||||
let x = E::A;
|
||||
let _ans = match x {
|
||||
E::A => false,
|
||||
E::B => false,
|
||||
_ => true,
|
||||
};
|
||||
}
|
||||
|
||||
fn main() {}
|
||||
|
|
|
@ -141,5 +141,18 @@ LL | Ok(3) => println!("ok"),
|
|||
| ^^^^^
|
||||
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
|
||||
|
||||
error: aborting due to 7 previous errors
|
||||
error: match expression looks like `matches!` macro
|
||||
--> $DIR/match_same_arms2.rs:133:16
|
||||
|
|
||||
LL | let _ans = match x {
|
||||
| ________________^
|
||||
LL | | E::A => false,
|
||||
LL | | E::B => false,
|
||||
LL | | _ => true,
|
||||
LL | | };
|
||||
| |_____^ help: try this: `!matches!(x, E::A | E::B)`
|
||||
|
|
||||
= note: `-D clippy::match-like-matches-macro` implied by `-D warnings`
|
||||
|
||||
error: aborting due to 8 previous errors
|
||||
|
||||
|
|
Loading…
Reference in a new issue