Skip to content

Commit 902726c

Browse files
author
Vincent Dal Maso
committed
Fix match_same_arms to fail late
Changes: - Add a function search_same_list which return a list of matched expressions - Change the match_same_arms implementation behaviour. It will lint each same arms found.
1 parent f49d878 commit 902726c

File tree

1 file changed

+74
-37
lines changed

1 file changed

+74
-37
lines changed

clippy_lints/src/copies.rs

Lines changed: 74 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -185,44 +185,48 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
185185
};
186186

187187
let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect();
188-
if let Some((&(_, i), &(_, j))) = search_same(&indexed_arms, hash, eq) {
189-
span_lint_and_then(
190-
cx,
191-
MATCH_SAME_ARMS,
192-
j.body.span,
193-
"this `match` has identical arm bodies",
194-
|db| {
195-
db.span_note(i.body.span, "same as this");
196-
197-
// Note: this does not use `span_suggestion` on purpose:
198-
// there is no clean way
199-
// to remove the other arm. Building a span and suggest to replace it to ""
200-
// makes an even more confusing error message. Also in order not to make up a
201-
// span for the whole pattern, the suggestion is only shown when there is only
202-
// one pattern. The user should know about `|` if they are already using it…
203-
204-
if i.pats.len() == 1 && j.pats.len() == 1 {
205-
let lhs = snippet(cx, i.pats[0].span, "<pat1>");
206-
let rhs = snippet(cx, j.pats[0].span, "<pat2>");
207-
208-
if let PatKind::Wild = j.pats[0].node {
209-
// if the last arm is _, then i could be integrated into _
210-
// note that i.pats[0] cannot be _, because that would mean that we're
211-
// hiding all the subsequent arms, and rust won't compile
212-
db.span_note(
213-
i.body.span,
214-
&format!(
215-
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
216-
lhs
217-
),
218-
);
219-
} else {
220-
db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
188+
search_same_list(&indexed_arms, hash, eq).map(|item| {
189+
for match_expr in item {
190+
let (&(_, i), &(_, j)) = match_expr;
191+
192+
span_lint_and_then(
193+
cx,
194+
MATCH_SAME_ARMS,
195+
j.body.span,
196+
"this `match` has identical arm bodies",
197+
|db| {
198+
db.span_note(i.body.span, "same as this");
199+
200+
// Note: this does not use `span_suggestion` on purpose:
201+
// there is no clean way
202+
// to remove the other arm. Building a span and suggest to replace it to ""
203+
// makes an even more confusing error message. Also in order not to make up a
204+
// span for the whole pattern, the suggestion is only shown when there is only
205+
// one pattern. The user should know about `|` if they are already using it…
206+
207+
if i.pats.len() == 1 && j.pats.len() == 1 {
208+
let lhs = snippet(cx, i.pats[0].span, "<pat1>");
209+
let rhs = snippet(cx, j.pats[0].span, "<pat2>");
210+
211+
if let PatKind::Wild = j.pats[0].node {
212+
// if the last arm is _, then i could be integrated into _
213+
// note that i.pats[0] cannot be _, because that would mean that we're
214+
// hiding all the subsequent arms, and rust won't compile
215+
db.span_note(
216+
i.body.span,
217+
&format!(
218+
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
219+
lhs
220+
),
221+
);
222+
} else {
223+
db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
224+
}
221225
}
222-
}
223-
},
224-
);
225-
}
226+
},
227+
);
228+
}
229+
});
226230
}
227231
}
228232

@@ -360,3 +364,36 @@ where
360364

361365
None
362366
}
367+
368+
fn search_same_list<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Option<Vec<(&T, &T)>>
369+
where
370+
Hash: Fn(&T) -> u64,
371+
Eq: Fn(&T, &T) -> bool,
372+
{
373+
let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
374+
375+
let mut map: FxHashMap<_, Vec<&_>> =
376+
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
377+
378+
for expr in exprs {
379+
match map.entry(hash(expr)) {
380+
Entry::Occupied(mut o) => {
381+
for o in o.get() {
382+
if eq(o, expr) {
383+
match_expr_list.push((o, expr));
384+
}
385+
}
386+
o.get_mut().push(expr);
387+
},
388+
Entry::Vacant(v) => {
389+
v.insert(vec![expr]);
390+
},
391+
}
392+
}
393+
394+
if match_expr_list.is_empty() {
395+
None
396+
} else {
397+
Some(match_expr_list)
398+
}
399+
}

0 commit comments

Comments
 (0)