Skip to content

Commit

Permalink
Improve message for match_single_arms
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarcho
Jarcho committed Jan 22, 2022
1 parent 11b1fc7 commit 13fd7ac
Show file tree
Hide file tree
Showing 4 changed files with 250 additions and 236 deletions.
83 changes: 47 additions & 36 deletions clippy_lints/src/matches.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2483,6 +2483,7 @@ impl<'a> NormalizedPat<'a> {
}

/// Implementation of `MATCH_SAME_ARMS`.
#[allow(clippy::too_many_lines)]
fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
if let ExprKind::Match(_, arms, MatchSource::Normal) = expr.kind {
let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
Expand Down Expand Up @@ -2569,42 +2570,52 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
};

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,))
.help("...or consider changing the match arm bodies");
}
},
);
for (&(i, arm1), &(j, arm2)) in search_same(&indexed_arms, hash, eq) {
if matches!(arm2.pat.kind, PatKind::Wild) {
span_lint_and_then(
cx,
MATCH_SAME_ARMS,
arm1.span,
"this match arm has an identical body to the `_` wildcard arm",
|diag| {
diag.span_suggestion(
arm1.span,
"try removing the arm",
String::new(),
Applicability::MaybeIncorrect,
)
.help("or try changing either arm body")
.span_note(arm2.span, "`_` wildcard arm here");
},
);
} else {
let back_block = backwards_blocking_idxs[j];
let (keep_arm, move_arm) = if back_block < i || (back_block == 0 && forwards_blocking_idxs[i] <= j) {
(arm1, arm2)
} else {
(arm2, arm1)
};

span_lint_and_then(
cx,
MATCH_SAME_ARMS,
keep_arm.span,
"this match arm has an identical body to another arm",
|diag| {
let move_pat_snip = snippet(cx, move_arm.pat.span, "<pat2>");
let keep_pat_snip = snippet(cx, keep_arm.pat.span, "<pat1>");

diag.span_suggestion(
keep_arm.pat.span,
"try merging the arm patterns",
format!("{} | {}", keep_pat_snip, move_pat_snip),
Applicability::MaybeIncorrect,
)
.help("or try changing either arm body")
.span_note(move_arm.span, "other arm here");
},
);
}
}
}
}
Expand Down
165 changes: 79 additions & 86 deletions tests/ui/match_same_arms.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,128 +1,121 @@
error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:13:14
error: this match arm has an identical body to the `_` wildcard arm
--> $DIR/match_same_arms.rs:11:9
|
LL | _ => 0, //~ ERROR match arms have same body
| ^
LL | Abc::A => 0,
| ^^^^^^^^^^^ help: try removing the arm
|
= note: `-D clippy::match-same-arms` implied by `-D warnings`
note: same as this
--> $DIR/match_same_arms.rs:11:19
= help: or try changing either arm body
note: `_` wildcard arm here
--> $DIR/match_same_arms.rs:13:9
|
LL | Abc::A => 0,
| ^
note: `Abc::A` has the same arm body as the `_` wildcard, consider removing it
--> $DIR/match_same_arms.rs:11:19
|
LL | Abc::A => 0,
| ^
LL | _ => 0, //~ ERROR match arms have same body
| ^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:18:20
|
LL | (.., 3) => 42, //~ ERROR match arms have same body
| ^^
|
note: same as this
--> $DIR/match_same_arms.rs:17:23
|
LL | (1, .., 3) => 42,
| ^^
help: consider refactoring into `(1, .., 3) | (.., 3)`
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:17:9
|
LL | (1, .., 3) => 42,
| ^^^^^^^^^^
= help: ...or consider changing the match arm bodies
| ----------^^^^^^
| |
| help: try merging the arm patterns: `(1, .., 3) | (.., 3)`
|
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:18:9
|
LL | (.., 3) => 42, //~ ERROR match arms have same body
| ^^^^^^^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:24:15
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:24:9
|
LL | 51 => 1, //~ ERROR match arms have same body
| ^
| --^^^^^
| |
| help: try merging the arm patterns: `51 | 42`
|
note: same as this
--> $DIR/match_same_arms.rs:23:15
|
LL | 42 => 1,
| ^
help: consider refactoring into `42 | 51`
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:23:9
|
LL | 42 => 1,
| ^^
= help: ...or consider changing the match arm bodies
| ^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:26:15
|
LL | 52 => 2, //~ ERROR match arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:25:15
|
LL | 41 => 2,
| ^
help: consider refactoring into `41 | 52`
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:25:9
|
LL | 41 => 2,
| ^^
= help: ...or consider changing the match arm bodies
| --^^^^^
| |
| help: try merging the arm patterns: `41 | 52`
|
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:26:9
|
LL | 52 => 2, //~ ERROR match arms have same body
| ^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:32:14
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:32:9
|
LL | 2 => 2, //~ ERROR 2nd matched arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:31:14
| -^^^^^
| |
| help: try merging the arm patterns: `2 | 1`
|
LL | 1 => 2,
| ^
help: consider refactoring into `1 | 2`
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:31:9
|
LL | 1 => 2,
| ^
= help: ...or consider changing the match arm bodies
| ^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:33:14
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:33:9
|
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:31:14
| -^^^^^
| |
| help: try merging the arm patterns: `3 | 1`
|
LL | 1 => 2,
| ^
help: consider refactoring into `1 | 3`
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:31:9
|
LL | 1 => 2,
| ^
= help: ...or consider changing the match arm bodies
| ^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:50:55
error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:32:9
|
LL | CommandInfo::External { name, .. } => name.to_string(),
| ^^^^^^^^^^^^^^^^
LL | 2 => 2, //~ ERROR 2nd matched arms have same body
| -^^^^^
| |
| help: try merging the arm patterns: `2 | 3`
|
note: same as this
--> $DIR/match_same_arms.rs:49:54
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:33:9
|
LL | CommandInfo::BuiltIn { name, .. } => name.to_string(),
| ^^^^^^^^^^^^^^^^
help: consider refactoring into `CommandInfo::BuiltIn { name, .. } | CommandInfo::External { name, .. }`
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
| ^^^^^^

error: this match arm has an identical body to another arm
--> $DIR/match_same_arms.rs:50:17
|
LL | CommandInfo::External { name, .. } => name.to_string(),
| ----------------------------------^^^^^^^^^^^^^^^^^^^^
| |
| help: try merging the arm patterns: `CommandInfo::External { name, .. } | CommandInfo::BuiltIn { name, .. }`
|
= help: or try changing either arm body
note: other arm here
--> $DIR/match_same_arms.rs:49:17
|
LL | CommandInfo::BuiltIn { name, .. } => name.to_string(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: ...or consider changing the match arm bodies
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 7 previous errors
error: aborting due to 8 previous errors

17 changes: 17 additions & 0 deletions tests/ui/match_same_arms2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,27 @@ fn main() {
Z(u32),
}

// Don't lint. `Foo::X(0)` and `Foo::Z(_)` overlap with the arm in between.
let _ = match Foo::X(0) {
Foo::X(0) => 1,
Foo::X(_) | Foo::Y(_) | Foo::Z(0) => 2,
Foo::Z(_) => 1,
_ => 0,
};

// Suggest moving `Foo::Z(_)` up.
let _ = match Foo::X(0) {
Foo::X(0) => 1,
Foo::X(_) | Foo::Y(_) => 2,
Foo::Z(_) => 1,
_ => 0,
};

// Suggest moving `Foo::X(0)` down.
let _ = match Foo::X(0) {
Foo::X(0) => 1,
Foo::Y(_) | Foo::Z(0) => 2,
Foo::Z(_) => 1,
_ => 0,
};
}
Loading

0 comments on commit 13fd7ac

Please sign in to comment.