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.

Sign up for GitHub

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

Jump to bottom

Implement the manual_non_exhaustive lint #5550

Merged
merged 3 commits into from
May 2, 2020
Merged

Implement the manual_non_exhaustive lint #5550

merged 3 commits into from
May 2, 2020

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Apr 30, 2020

Some implementation notes:

  • Not providing automatic fixups because additional changes may be needed in other parts of the code, e.g. when constructing a struct.
  • Even though the attribute is valid on enum variants, it's not possible to use the manual implementation of the pattern because the visibility is always public, so the lint ignores enum variants.
  • Unit structs are also ignored, it's not possible to implement the pattern manually without fields.
  • The attribute is not accepted in unions, so those are ignored too.
  • Even though the original issue did not mention it, tuple structs are also linted because it's possible to apply the pattern manually.

changelog: Added the manual non-exhaustive implementation lint

Closes #2017

@ebroto
Copy link
Member Author

ebroto commented May 1, 2020

I've realized that we should keep linting if the attribute was added but the private field/marker variant was not removed. I'll add a follow-up commit tonight.

Also, the RFC proposes a clippy lint to check when matching non-exhaustive enums that all patterns are covered, to help downstream crates notice that new variants were added. Would it make sense to add it?

flip1995
flip1995 requested changes May 1, 2020
View reviewed changes
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.

I've realized that we should keep linting if the attribute was added but the private field/marker variant was not removed.

I should have read your comment before reviewing. Just removed my comment about this 😄

Would it make sense to add it?

This would be a good allow-by-default lint. So I would separate it from this lint and add it as a pedantic lint.

clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
@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 May 1, 2020
flip1995
flip1995 approved these changes May 1, 2020
View reviewed changes
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.

One small thing left.

clippy_lints/src/manual_non_exhaustive.rs Outdated Show resolved Hide resolved
@ebroto @flip1995
Use the only variant left instead of a wildcard
350c17d 
Co-authored-by: Philipp Krones <hello@philkrones.com>
@flip1995
Copy link
Member

flip1995 commented May 1, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented May 1, 2020

📌 Commit 350c17d has been approved by flip1995

@bors
Copy link
Collaborator

bors commented May 1, 2020

⌛ Testing commit 350c17d with merge d9894c1...

bors added a commit that referenced this pull request May 1, 2020
@bors
Auto merge of #5550 - ebroto:manual_non_exhaustive, r=flip1995
d9894c1 
Implement the manual_non_exhaustive lint

Some implementation notes:
* Not providing automatic fixups because additional changes may be needed in other parts of the code, e.g. when constructing a struct.
* Even though the attribute is valid on enum variants, it's not possible to use the manual implementation of the pattern because the visibility is always public, so the lint ignores enum variants.
* Unit structs are also ignored, it's not possible to implement the pattern manually without fields.
* The attribute is not accepted in unions, so those are ignored too.
* Even though the original issue did not mention it, tuple structs are also linted because it's possible to apply the pattern manually.

changelog: Added the manual non-exhaustive implementation lint

Closes #2017
@bors
Copy link
Collaborator

bors commented May 1, 2020

💥 Test timed out

@flip1995
Copy link
Member

flip1995 commented May 2, 2020

@bors retry

@bors
Copy link
Collaborator

bors commented May 2, 2020

⌛ Testing commit 350c17d with merge cc9088f...

@bors
Copy link
Collaborator

bors commented May 2, 2020

💔 Test failed - checks-action_test

@bors
Copy link
Collaborator

bors commented May 2, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing cc9088f to master...

@bors bors merged commit cc9088f into rust-lang:master May 2, 2020
@bors bors mentioned this pull request May 2, 2020
Merged
@ebroto ebroto deleted the manual_non_exhaustive branch May 2, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint potential uses of non_exhaustive
3 participants
@ebroto @flip1995 @bors