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

Only lint manual_non_exhaustive for exported types #13345

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

Alexendoo
Copy link
Member

For types that are not exported the attribute doesn't make a difference, but the manual pattern can still be used to achieve module level non exhaustiveness

Fixes #10301
Fixes #12106

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2024

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 4, 2024
@y21
Copy link
Member

y21 commented Sep 5, 2024

As for the second linked issue, isn't that still an issue even with this change of restricting the lint to only warn exported types?

As in, someone could have a type that happens to be exported but would still only want it constructed through a specific constructor and not anywhere else in the same crate, which isn't something that #[non_exhaustive] helps with.

But then again, the lint requires that there's a public field and exactly one private field that is the unit type, so I don't see why someone would want that behavior. The field clearly doesn't have invariants that need to be upheld in the constructor if it's public, because it can just be reassigned a new value anywhere.

the manual pattern can still be used to achieve module level non exhaustiveness

Out of curiosity, are there use cases for wanting to restrict exhaustiveness to specific modules within the same crate?

(To be clear, this PR makes sense to me, just mentioning this because the description mentions closing that one, but not sure it fixes it)

@Alexendoo
Copy link
Member Author

#12106 mentions that it's about a private module, but yeah the PR doesn't handle the case of an exported struct with a module private constructor

@y21
Copy link
Member

y21 commented Sep 5, 2024

Ah oops sorry, I missed the word "private" while reading through them. In that case, looks all good to me :)

@bors r+

@bors
Copy link
Contributor

bors commented Sep 5, 2024

📌 Commit f7f5505 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 5, 2024

⌛ Testing commit f7f5505 with merge c41be9e...

@bors
Copy link
Contributor

bors commented Sep 5, 2024

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

@bors bors merged commit c41be9e into rust-lang:master Sep 5, 2024
8 checks passed
@Alexendoo Alexendoo deleted the manual-non-exhaustive-visibility branch September 5, 2024 22:37
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
4 participants