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 unneeded-wildcard-pattern lint #4537

Merged
merged 7 commits into from Sep 23, 2019
Merged

Add unneeded-wildcard-pattern lint #4537

merged 7 commits into from Sep 23, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 12, 2019

changelog: Add unneeded-wildcard-pattern lint

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.

Implementation LGTM.

But I'm concerned, that _, ../.., _ and .. isn't quite the same. While the _, .. means, there is at least one more variant in this struct/tuple, .. means, there are 0 or more variants in this struct/tuple:

struct TS(u32, u32);

fn main() {
    let a = TS(1, 2);
    match a {
        TS(_, _, ..) => (),
    };
}

compiles just fine.

If we'd remove a field from the struct, this code would error, while the .. variant, would still compile. I don't think, this could really lead to an error, since the fields aren't used anyway, but I would add the difference between _, .. and .. at least as a note to the lint documentation (not necessarily to the Known Problems section).

clippy_lints/src/misc_early.rs Outdated Show resolved Hide resolved
clippy_lints/src/misc_early.rs Outdated Show resolved Hide resolved
clippy_lints/src/misc_early.rs Outdated Show resolved Hide resolved
clippy_lints/src/misc_early.rs Outdated Show resolved Hide resolved
Fix grammar errors and use `Pat::is_rest` instead of own function.
@ghost
Copy link
Author

ghost commented Sep 13, 2019

I made all the changes suggested apart from adding a note between the difference between .. and .., _. I still thinking about how best to do that.

If anyone wants to fork this branch and finish that off, I'm happy to pull the changes. (Maintainers can, of course, just push to this branch directly.)

if let (0, _, _, ..) = t {};
if let (0, .., _, _) = t {};
if let (_, 0, ..) = t {};
if let (.., 0, _) = t {};
Copy link
Member

Choose a reason for hiding this comment

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

if let (_, .., 2, 3) = t {} doesn't get linted, does it? Can you add a test for it? This could actually lead to refactoring errors, because with the _, .. it means, there is at least one element before the 2.

Copy link
Author

Choose a reason for hiding this comment

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

It absolutely will. This is the purpose of this lint.
What errors could this lead to and how is this different from the case (0, .., _, _)(0, ..)?

Copy link
Member

Choose a reason for hiding this comment

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

I can't remember what I had in mind. And I also cannot come up with something. Can you add a test for it though?

Co-Authored-By: Philipp Krones <hello@philkrones.com>
@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 18, 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.

Only a test for the _, .., 0 case is missing.

@bors
Copy link
Contributor

bors commented Sep 20, 2019

☔ The latest upstream changes (presumably #4511) made this pull request unmergeable. Please resolve the merge conflicts.

@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 23, 2019
@phansch
Copy link
Member

phansch commented Sep 23, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2019

📌 Commit ca6d36b has been approved by phansch

@bors
Copy link
Contributor

bors commented Sep 23, 2019

⌛ Testing commit ca6d36b with merge c23b375...

bors added a commit that referenced this pull request Sep 23, 2019
Add `unneeded-wildcard-pattern` lint

changelog: Add `unneeded-wildcard-pattern` lint
@bors
Copy link
Contributor

bors commented Sep 23, 2019

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

@bors bors merged commit ca6d36b into rust-lang:master Sep 23, 2019
@ghost ghost deleted the unneeded_wildcard_pattern branch September 24, 2019 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants