Skip to content

Commit 6454825

Browse files
JarchoJarcho
committed
Split out match_same_arms 
1 parent e41a6fc commit 6454825

File tree

2 files changed

+119
-113
lines changed

2 files changed

+119
-113
lines changed

‎clippy_lints/src/matches/match_same_arms.rs‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::source::snippet;
3+
use clippy_utils::{path_to_local, search_same, SpanlessEq, SpanlessHash};
4+
use rustc_hir::{Arm, Expr, ExprKind, HirId, HirIdMap, HirIdSet, MatchSource, Pat, PatKind};
5+
use rustc_lint::LateContext;
6+
use rustc_middle::ty::TyS;
7+
use std::collections::hash_map::Entry;
8+
9+
use super::MATCH_SAME_ARMS;
10+
11+
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
12+
if let ExprKind::Match(_, arms, MatchSource::Normal) = expr.kind {
13+
let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
14+
let mut h = SpanlessHash::new(cx);
15+
h.hash_expr(arm.body);
16+
h.finish()
17+
};
18+
19+
let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
20+
let min_index = usize::min(lindex, rindex);
21+
let max_index = usize::max(lindex, rindex);
22+
23+
let mut local_map: HirIdMap<HirId> = HirIdMap::default();
24+
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
25+
if_chain! {
26+
if let Some(a_id) = path_to_local(a);
27+
if let Some(b_id) = path_to_local(b);
28+
let entry = match local_map.entry(a_id) {
29+
Entry::Vacant(entry) => entry,
30+
// check if using the same bindings as before
31+
Entry::Occupied(entry) => return *entry.get() == b_id,
32+
};
33+
// the names technically don't have to match; this makes the lint more conservative
34+
if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
35+
if TyS::same_type(cx.typeck_results().expr_ty(a), cx.typeck_results().expr_ty(b));
36+
if pat_contains_local(lhs.pat, a_id);
37+
if pat_contains_local(rhs.pat, b_id);
38+
then {
39+
entry.insert(b_id);
40+
true
41+
} else {
42+
false
43+
}
44+
}
45+
};
46+
// Arms with a guard are ignored, those can’t always be merged together
47+
// This is also the case for arms in-between each there is an arm with a guard
48+
(min_index..=max_index).all(|index| arms[index].guard.is_none())
49+
&& SpanlessEq::new(cx)
50+
.expr_fallback(eq_fallback)
51+
.eq_expr(lhs.body, rhs.body)
52+
// these checks could be removed to allow unused bindings
53+
&& bindings_eq(lhs.pat, local_map.keys().copied().collect())
54+
&& bindings_eq(rhs.pat, local_map.values().copied().collect())
55+
};
56+
57+
let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
58+
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
59+
span_lint_and_then(
60+
cx,
61+
MATCH_SAME_ARMS,
62+
j.body.span,
63+
"this `match` has identical arm bodies",
64+
|diag| {
65+
diag.span_note(i.body.span, "same as this");
66+
67+
// Note: this does not use `span_suggestion` on purpose:
68+
// there is no clean way
69+
// to remove the other arm. Building a span and suggest to replace it to ""
70+
// makes an even more confusing error message. Also in order not to make up a
71+
// span for the whole pattern, the suggestion is only shown when there is only
72+
// one pattern. The user should know about `|` if they are already using it…
73+
74+
let lhs = snippet(cx, i.pat.span, "<pat1>");
75+
let rhs = snippet(cx, j.pat.span, "<pat2>");
76+
77+
if let PatKind::Wild = j.pat.kind {
78+
// if the last arm is _, then i could be integrated into _
79+
// note that i.pat cannot be _, because that would mean that we're
80+
// hiding all the subsequent arms, and rust won't compile
81+
diag.span_note(
82+
i.body.span,
83+
&format!(
84+
"`{}` has the same arm body as the `_` wildcard, consider removing it",
85+
lhs
86+
),
87+
);
88+
} else {
89+
diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
90+
.help("...or consider changing the match arm bodies");
91+
}
92+
},
93+
);
94+
}
95+
}
96+
}
97+
98+
fn pat_contains_local(pat: &Pat<'_>, id: HirId) -> bool {
99+
let mut result = false;
100+
pat.walk_short(|p| {
101+
result |= matches!(p.kind, PatKind::Binding(_, binding_id, ..) if binding_id == id);
102+
!result
103+
});
104+
result
105+
}
106+
107+
/// Returns true if all the bindings in the `Pat` are in `ids` and vice versa
108+
fn bindings_eq(pat: &Pat<'_>, mut ids: HirIdSet) -> bool {
109+
let mut result = true;
110+
pat.each_binding_or_first(&mut |_, id, _, _| result &= ids.remove(&id));
111+
result && ids.is_empty()
112+
}

‎clippy_lints/src/matches/mod.rs‎

Lines changed: 7 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -3,37 +3,35 @@ use clippy_utils::diagnostics::{
33
multispan_sugg, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then,
44
};
55
use clippy_utils::macros::{is_panic, root_macro_call};
6+
use clippy_utils::paths;
67
use clippy_utils::peel_blocks_with_stmt;
78
use clippy_utils::source::{expr_block, indent_of, snippet, snippet_block, snippet_opt, snippet_with_applicability};
89
use clippy_utils::sugg::Sugg;
910
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, peel_mid_ty_refs};
1011
use clippy_utils::visitors::is_local_used;
1112
use clippy_utils::{
1213
get_parent_expr, is_lang_ctor, is_lint_allowed, is_refutable, is_unit_expr, is_wild, meets_msrv, msrvs,
13-
path_to_local, path_to_local_id, peel_blocks, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns,
14-
strip_pat_refs,
14+
path_to_local_id, peel_blocks, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, strip_pat_refs,
1515
};
16-
use clippy_utils::{paths, search_same, SpanlessEq, SpanlessHash};
1716
use core::iter::once;
1817
use if_chain::if_chain;
1918
use rustc_ast::ast::LitKind;
2019
use rustc_errors::Applicability;
2120
use rustc_hir::def::{CtorKind, DefKind, Res};
2221
use rustc_hir::LangItem::{OptionNone, OptionSome};
2322
use rustc_hir::{
24-
self as hir, Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, HirId, Local, MatchSource, Mutability,
25-
Node, Pat, PatKind, PathSegment, QPath, RangeEnd, TyKind,
23+
self as hir, Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Local, MatchSource, Mutability, Node, Pat,
24+
PatKind, PathSegment, QPath, RangeEnd, TyKind,
2625
};
27-
use rustc_hir::{HirIdMap, HirIdSet};
2826
use rustc_lint::{LateContext, LateLintPass};
2927
use rustc_middle::ty::{self, Ty, TyS, VariantDef};
3028
use rustc_semver::RustcVersion;
3129
use rustc_session::{declare_tool_lint, impl_lint_pass};
3230
use rustc_span::{sym, symbol::kw, Span};
3331
use std::cmp::{max, Ordering};
34-
use std::collections::hash_map::Entry;
3532

3633
mod match_like_matches;
34+
mod match_same_arms;
3735

3836
declare_clippy_lint! {
3937
/// ### What it does
@@ -624,10 +622,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
624622

625623
if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
626624
if !match_like_matches::check(cx, expr) {
627-
lint_match_arms(cx, expr);
625+
match_same_arms::check(cx, expr);
628626
}
629627
} else {
630-
lint_match_arms(cx, expr);
628+
match_same_arms::check(cx, expr);
631629
}
632630

633631
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
@@ -2185,107 +2183,3 @@ fn test_overlapping() {
21852183
],)
21862184
);
21872185
}
2188-
2189-
/// Implementation of `MATCH_SAME_ARMS`.
2190-
fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
2191-
if let ExprKind::Match(_, arms, MatchSource::Normal) = expr.kind {
2192-
let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
2193-
let mut h = SpanlessHash::new(cx);
2194-
h.hash_expr(arm.body);
2195-
h.finish()
2196-
};
2197-
2198-
let eq = |&(lindex, lhs): &(usize, &Arm<'_>), &(rindex, rhs): &(usize, &Arm<'_>)| -> bool {
2199-
let min_index = usize::min(lindex, rindex);
2200-
let max_index = usize::max(lindex, rindex);
2201-
2202-
let mut local_map: HirIdMap<HirId> = HirIdMap::default();
2203-
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
2204-
if_chain! {
2205-
if let Some(a_id) = path_to_local(a);
2206-
if let Some(b_id) = path_to_local(b);
2207-
let entry = match local_map.entry(a_id) {
2208-
Entry::Vacant(entry) => entry,
2209-
// check if using the same bindings as before
2210-
Entry::Occupied(entry) => return *entry.get() == b_id,
2211-
};
2212-
// the names technically don't have to match; this makes the lint more conservative
2213-
if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
2214-
if TyS::same_type(cx.typeck_results().expr_ty(a), cx.typeck_results().expr_ty(b));
2215-
if pat_contains_local(lhs.pat, a_id);
2216-
if pat_contains_local(rhs.pat, b_id);
2217-
then {
2218-
entry.insert(b_id);
2219-
true
2220-
} else {
2221-
false
2222-
}
2223-
}
2224-
};
2225-
// Arms with a guard are ignored, those can’t always be merged together
2226-
// This is also the case for arms in-between each there is an arm with a guard
2227-
(min_index..=max_index).all(|index| arms[index].guard.is_none())
2228-
&& SpanlessEq::new(cx)
2229-
.expr_fallback(eq_fallback)
2230-
.eq_expr(lhs.body, rhs.body)
2231-
// these checks could be removed to allow unused bindings
2232-
&& bindings_eq(lhs.pat, local_map.keys().copied().collect())
2233-
&& bindings_eq(rhs.pat, local_map.values().copied().collect())
2234-
};
2235-
2236-
let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
2237-
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
2238-
span_lint_and_then(
2239-
cx,
2240-
MATCH_SAME_ARMS,
2241-
j.body.span,
2242-
"this `match` has identical arm bodies",
2243-
|diag| {
2244-
diag.span_note(i.body.span, "same as this");
2245-
2246-
// Note: this does not use `span_suggestion` on purpose:
2247-
// there is no clean way
2248-
// to remove the other arm. Building a span and suggest to replace it to ""
2249-
// makes an even more confusing error message. Also in order not to make up a
2250-
// span for the whole pattern, the suggestion is only shown when there is only
2251-
// one pattern. The user should know about `|` if they are already using it…
2252-
2253-
let lhs = snippet(cx, i.pat.span, "<pat1>");
2254-
let rhs = snippet(cx, j.pat.span, "<pat2>");
2255-
2256-
if let PatKind::Wild = j.pat.kind {
2257-
// if the last arm is _, then i could be integrated into _
2258-
// note that i.pat cannot be _, because that would mean that we're
2259-
// hiding all the subsequent arms, and rust won't compile
2260-
diag.span_note(
2261-
i.body.span,
2262-
&format!(
2263-
"`{}` has the same arm body as the `_` wildcard, consider removing it",
2264-
lhs
2265-
),
2266-
);
2267-
} else {
2268-
diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs,))
2269-
.help("...or consider changing the match arm bodies");
2270-
}
2271-
},
2272-
);
2273-
}
2274-
}
2275-
}
2276-
2277-
fn pat_contains_local(pat: &Pat<'_>, id: HirId) -> bool {
2278-
let mut result = false;
2279-
pat.walk_short(|p| {
2280-
result |= matches!(p.kind, PatKind::Binding(_, binding_id, ..) if binding_id == id);
2281-
!result
2282-
});
2283-
result
2284-
}
2285-
2286-
/// Returns true if all the bindings in the `Pat` are in `ids` and vice versa
2287-
fn bindings_eq(pat: &Pat<'_>, mut ids: HirIdSet) -> bool {
2288-
let mut result = true;
2289-
pat.each_binding_or_first(&mut |_, id, _, _| result &= ids.remove(&id));
2290-
result && ids.is_empty()
2291-
}

0 commit comments

Comments
 (0)