Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wildcard_match_arm lint #3652

Merged
merged 12 commits into from
Jan 29, 2019
Merged

Add wildcard_match_arm lint #3652

merged 12 commits into from
Jan 29, 2019

Conversation

Aehmlo
Copy link
Contributor

@Aehmlo Aehmlo commented Jan 10, 2019

This lint prevents using a wildcard in a match arm. Implemented as a restriction currently, because this is pretty much an edge case. See #3649 for more information.

Didn't add any tests because I wasn't sure how, but if someone wants to point me in the right direction, I'd be happy to!

@phansch phansch closed this Jan 11, 2019
@phansch phansch reopened this Jan 11, 2019
@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 11, 2019
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

It indeed would be great if you could add tests. You can look in the tests/ui/ folder. Just add a new file there, where you put patterns in where the lint should trigger and some where it shouldn't. One test case where it shouldn't trigger is for example an _ if some_cond => (), arm IMO. Make sure to add #![warn(clippy::wildcard_match_arm)] to the top of the file.

After you added this file run cargo test (you can restrict this to just your test, see CONTRIBUTING.md for this). Now your test should fail, because you need to generate a *.stderr file. You can do this by running the tests/ui/update-references.sh script. The exact command will be printed with the output of cargo test, so you can just c&p this.

clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 14, 2019
@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 15, 2019

Update: I've made some changes (including the above), but it'll be a few days until I can finish implementing the tests. I'll push everything once that's done.

@Aehmlo Aehmlo closed this Jan 25, 2019
@Aehmlo Aehmlo reopened this Jan 25, 2019
@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 25, 2019

Wrong button! I think this should be good to go now.

@Aehmlo Aehmlo changed the title Add match_wild lint. Add wildcard_match_arm lint Jan 25, 2019
@flip1995
Copy link
Member

Thanks!

Only a rustfmt run is missing, then this will be good to go.
https://travis-ci.com/rust-lang/rust-clippy/jobs/173071805#L1387-L1415

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 27, 2019

Ran rustfmt and pushed, but it seems that there are some other nightly-related breakages that popped up.

@flip1995
Copy link
Member

Thanks! That should be fixed by now.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 29, 2019

📌 Commit fefe55e has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jan 29, 2019

⌛ Testing commit fefe55e with merge a70d648...

bors added a commit that referenced this pull request Jan 29, 2019
Add wildcard_match_arm lint

This lint prevents using a wildcard in a match arm. Implemented as a restriction currently, because this is pretty much an edge case. See #3649 for more information.

Didn't add any tests because I wasn't sure how, but if someone wants to point me in the right direction, I'd be happy to!
@bors
Copy link
Contributor

bors commented Jan 29, 2019

💔 Test failed - checks-travis

@flip1995
Copy link
Member

flip1995 commented Jan 29, 2019

It seems that a formatting of the tests is missing. This should be the last chore.

https://travis-ci.com/rust-lang/rust-clippy/jobs/173709864#L1383-L1447

A cargo fmt --all inside the crate root should format everything

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 29, 2019

Neither cargo fmt --all nor cargo +nightly fmt --all was working to format the tests, so I just ran rustfmt on the file directly. Regardless, it looks like the build is now passing!

@phansch
Copy link
Member

phansch commented Jan 29, 2019

@bors retry

@orium
Copy link
Member

orium commented Jan 29, 2019

I think it would be better just to emit a lint in cases of an enum. So

match 3 {
    4 => println!(),
    _ => println!(),
};

would not emit a warning.

Note also that it would need to check for matching in an enum on any nesting level. I.e.

match Some(Color::Red) {
    Some(Color::Blue) => println!(),
    Some(_) => println!(),
    None => println!(),
};

would emit a lint.

@phansch
Copy link
Member

phansch commented Jan 29, 2019

@bors r- pending decision on linting non-enum matches

@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 29, 2019

I agree re: linting only on enums. I've changed the behavior and updated the lint name accordingly. I'm not familiar enough with compiler internals to implement the nested check yet, so I added a note of this limitation to the known issues. I may try to resolve this in the future if I have time, or someone else can, but I won't be able to do it in the near future.

Thanks for the insightful feedback, @orium!

@phansch
Copy link
Member

phansch commented Jan 29, 2019

@bors r+ LGTM now, thanks!

@bors
Copy link
Contributor

bors commented Jan 29, 2019

📌 Commit 97c7d28 has been approved by phansch

@bors
Copy link
Contributor

bors commented Jan 29, 2019

⌛ Testing commit 97c7d28 with merge c254711...

bors added a commit that referenced this pull request Jan 29, 2019
Add wildcard_match_arm lint

This lint prevents using a wildcard in a match arm. Implemented as a restriction currently, because this is pretty much an edge case. See #3649 for more information.

Didn't add any tests because I wasn't sure how, but if someone wants to point me in the right direction, I'd be happy to!
This lint prevents using a wildcard in a match.
@Aehmlo
Copy link
Contributor Author

Aehmlo commented Jan 29, 2019

Updated lint count; should actually work next time.

@matthiaskrgr
Copy link
Member

@bors retry

@matthiaskrgr
Copy link
Member

@bors ping

@bors
Copy link
Contributor

bors commented Jan 29, 2019

😪 I'm awake I'm awake

@matthiaskrgr
Copy link
Member

@bors r=phansch

@bors
Copy link
Contributor

bors commented Jan 29, 2019

📌 Commit 7fa50fb has been approved by phansch

@bors
Copy link
Contributor

bors commented Jan 29, 2019

⌛ Testing commit 7fa50fb with merge 6ce78d1...

bors added a commit that referenced this pull request Jan 29, 2019
Add wildcard_match_arm lint

This lint prevents using a wildcard in a match arm. Implemented as a restriction currently, because this is pretty much an edge case. See #3649 for more information.

Didn't add any tests because I wasn't sure how, but if someone wants to point me in the right direction, I'd be happy to!
@bors
Copy link
Contributor

bors commented Jan 29, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 6ce78d1 to master...

@bors bors merged commit 7fa50fb into rust-lang:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants