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

Fix exposing fields marked unstable or doc hidden #90358

Merged
merged 9 commits into from
Mar 13, 2022
Merged

Fix exposing fields marked unstable or doc hidden #90358

merged 9 commits into from
Mar 13, 2022

Conversation

DevinR528
Copy link
Contributor

Closes #89837

Work towards #89554

Filter fields that are marked doc(hidden) or are unstable with that feature turned off. This brings structs and enums into alignment behavior-wise when emitting warning/errors about pattern exhaustiveness/reachability.

cc @Nadrieril

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@apiraino apiraino added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2021
@DevinR528
Copy link
Contributor Author

DevinR528 commented Dec 17, 2021 •
edited
Loading

Now that final exams are over I can finish this up!

@m-ou-se I collected some more feedback on converting doc(hidden) field/variants and it was mixed (zulip which was started from the above review here). So my plan is to remove all the changes made in this PR about doc(hidden) and if needed open a new PR reverting back to doc(hidden)s previous/default behavior. (Sorry I didn't want to add to the number of pings I imagine you get but I wanted to let you know).

So as long as leaving it as is (or rather as it was) sounds good to everyone I'll start by removing the commits from this PR and see if anything else has to be changed/removed so doc(hidden) items are treated as they were.

@JohnCSimon
Copy link
Member

Ping from triage:
@DevinR528 what is the status of this PR?

When it's ready for review send a message containing
@rustbot ready to switch to S-waiting-on-review

thanks.

@DevinR528
Copy link
Contributor Author

@JohnCSimon I am waiting for a slightly more official go-ahead to remove the special treatment of doc(hidden) from @m-ou-se mostly. Or more discussion needs to be had?. There was a stream on zulip I started but I can't for the life of me find it.

@Nadrieril
Copy link
Member

Sounds like you're waiting for a review; let's update the tags
@rustbot ready

@rustbot rustbot 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 (such as code changes or more information) from the author. labels Jan 31, 2022
@camelid
Copy link
Member

camelid commented Feb 3, 2022

There was a stream on zulip I started but I can't for the life of me find it.

@DevinR528 is this what you were looking for? https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Should.20error.20msgs.20list.20doc.28hidden.29.20fields

@DevinR528
Copy link
Contributor Author

That's the one thanks @camelid !

@m-ou-se
Copy link
Member

m-ou-se commented Feb 4, 2022

I still think we should not expose doc(hidden) fields and variants through suggestions and diagnostics. This attribute is currently the only way users of stable Rust have to mark something as 'not part of the public API' for things that need to be pub, e.g. for macro reasons. In the standard library we have #[unstable], but that attribute is unstable and won't be usable by other crates on stable any time soon. We shouldn't take away this tool from library authors.

@apiraino
Copy link
Contributor

I think it can now go back to:

@rustbot author

(but please update the labels if it's the case - thanks)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2022
@DevinR528
Copy link
Contributor Author

Ok, I'm fine with doing whatever so then the only review was from @camelid about removing the special casing of doc(hidden) so I think this PR might be ready for another review if we are leaving doc(hidden) the way that it is since my change here 2a042d6.

I fixed filtering doc(hidden) when crate local changed/added tests for that case.

@DevinR528
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 (such as code changes or more information) from the author. labels Feb 17, 2022
@wesleywiser
Copy link
Member

r? @camelid

@camelid would you mind taking ownership of the review here? Otherwise, feel free to re-roll for a new reviewer or you can assign to me.

Thanks!

@rust-highfive rust-highfive assigned camelid and unassigned estebank Feb 24, 2022
@camelid
Copy link
Member

camelid commented Mar 1, 2022

Sorry, I don't have enough time to take ownership of reviewing. I can try to weigh in on discussions, but that's about it. r? @wesleywiser

@bors
Copy link
Contributor

bors commented Mar 1, 2022

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

@bors
Copy link
Contributor

bors commented Mar 8, 2022

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

jackh726
jackh726 approved these changes Mar 12, 2022
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

A nit, but r=me

src/test/ui/pattern/usefulness/doc-hidden-fields.stderr Outdated Show resolved Hide resolved
src/test/ui/pattern/usefulness/doc-hidden-fields.stderr
|
help: include the missing field in the pattern
|
LL | let HiddenStruct { one, hide, two } = HiddenStruct::default();
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be left as followup, but ideally we should also suggest hiding in one go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if the doc(hidden) field is being explicitly matched on?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Good point. Not sure.

@jackh726
Copy link
Member

Needs rebase

r? @jackh726

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2022
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2022

📌 Commit 492d8d7 has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2022
@bors
Copy link
Contributor

bors commented Mar 12, 2022

⌛ Testing commit 492d8d7 with merge bbbd48f...

@bors
Copy link
Contributor

bors commented Mar 13, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing bbbd48f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2022
@bors bors merged commit bbbd48f into rust-lang:master Mar 13, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 13, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bbbd48f): comparison url.

Summary: This benchmark run did not return any relevant results. 15 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@DevinR528 DevinR528 deleted the omitted-field-fix branch March 13, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

non_exhaustive_omitted_patterns lint exposes unstable and hidden fields