Fix match_same_arms to fail late

[Urriel]

Changes:

• Add a function search_same_list which return a list of matched expressions
• Change the match_same_arms implementation behavior. It will lint each same arms found.

fixes #4096

changelog: none

[Urriel]

Hello, I would like some feedback, especially for the tests. I have some tests failing and did not figure out yet how to fix them.

[flip1995]

Hey, thanks for the contribution!

Have you looked at https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#testing? It is described there how to update reference files (*.stderr files) and how to add tests.
If you have and tests are still failing, please push the tests+stderr files, so I can take a look at them.

[Urriel]

Thank you for the tips I did not pay attention to the doc folder. You guys did a good job making contributing trivial.

My last commit should add the right test and fix the test errors.

[flip1995]

You guys did a good job making contributing trivial.

Happy to hear that!

With your changes the first error doesn't get printed anymore. That's a regression we want to avoid. Could you look into that?

[pJunger]

Is the implementation for search_same still needed?
Or maybe more important: Shouldn't the if check also fail late?

[Urriel]

I have questions about the "unwrap" line 387. I am not sure its the right way to do it.
Also at the moment, we can see the expression found twice.

[Urriel]

@pJunger Thank you for your insight it helps a lot.

[pJunger]

You're welcome!

[pJunger]

Looks good to me.

[Urriel]

Great !
Hope this little PR helps the cause.

[flip1995]

Thanks for the review @pJunger !

Now that search_same is removed you could rename the search_same_list function to search_same, since it does the same as before, just better. Thoughts?

Also (not really related to the issue, but while you're at it... 😄), could you change these span_notes:

rust-clippy/clippy_lints/src/copies.rs

Lines 212 to 220 in cbae9ed

	db.span_note(
	i.body.span,
	&format!(
	"`{}` has the same arm body as the `_` wildcard, consider removing it`",
	lhs
	),
	);
	} else {
	db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));

to span_help and use the span of the pats instead of the body. The lint message should then look something like this:

 ...
+help: consider refactoring into `41 | 52`
-note: consider refactoring into `41 | 52`
   --> $DIR/match_same_arms.rs:115:15
    |
 LL |         41 => 2,
+   |         ^^
-   |               ^

And last but not least: Could you add a test where more than 2 match arms are the same:

match x {
    1 => 2,
    2 => 2,
    3 => 2, // 3rd time the same arm
    4 => 3,
}

[flip1995]

Thanks! You have to run tests/ui/update-all-references.sh again, since tests/ui/matches.rs fails now.

[pJunger]

Maybe also add a function to the if_same_then_else block with multiple branches?

if true {
    let foo = "";
    return Ok(&foo[0..]);
} else if false {
    let foo = "bar";
    return Ok(&foo[0..]);
} else if true {
    let foo = "";
    return Ok(&foo[0..]);
} else {
    let foo = "";
    return Ok(&foo[0..]);
}

[Urriel]

I hope this fixes everything. I forgot to do a full uitest before commiting.

[flip1995]

LGTM now.

2 Things left to do:

• run cargo fmt --all
• squash some of the commits

and this is good to go.

[Urriel]

I try to install rustfmt, but I get this errors:

'cargo-fmt' is not installed for the toolchain 'master'
toolchain 'master' does not support components

[flip1995]

Yeah, you can't really install components on the master toolchain.

For Clippy we use nightly rustfmt anyway (for this reason). So install rustfmt for nightly and then run cargo +nightly fmt --all

[Urriel]

That should cover it.

[flip1995]

Thanks!

Every PR is currently blocked on #4121, but I'll merge this once this bug is fixed.

[flip1995]

@bors r+

[bors]

📌 Commit 40f3665 has been approved by flip1995

[bors]

⌛ Testing commit 40f3665 with merge eb0a288...