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

RFC 2008: Enum Variants #59376

Merged
merged 4 commits into from
Mar 30, 2019
Merged

Conversation

davidtwco
Copy link
Member

Part of #44109. See Zulip topic for previous discussion.

r? @petrochenkov
cc @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2019
@davidtwco
Copy link
Member Author

I anticipate a lot of review comments so I'd appreciate any feedback and I'm happy to make any requested changes. I tried to split some of the later commits up to make it easier to review but unfortunately wasn't able to with the earlier commits that did the bulk of the refactoring.

@petrochenkov
Copy link
Contributor

Looks like something went wrong during rebase because ppaux.rs and item_path.rs no longer exist on master.

@petrochenkov
Copy link
Contributor

This work would benefit from splitting into two PRs - one for the DefId separation refactoring and no functional changes, and another for the non_exhaustive part.

@petrochenkov petrochenkov 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 23, 2019
@davidtwco
Copy link
Member Author

Looks like something went wrong during rebase because ppaux.rs and item_path.rs no longer exist on master.

I may be mistaken, but isn’t this due to #58140 landing?

This work would benefit from splitting into two PRs - one for the DefId separation refactoring and no functional changes, and another for the non_exhaustive part.

I’ll split this into two PRs. I wanted to make the RFC 2008 changes on top so that I could confirm that the refactoring did work as expected.

@petrochenkov
Copy link
Contributor

@davidtwco

I may be mistaken, but isn’t this due to #58140 landing?

Exactly.

@davidtwco
Copy link
Member Author

davidtwco commented Mar 23, 2019

Exactly.

Oh, I see. I didn’t realise this PR still had those files, will fix that too. Fixed this in force push below.

@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch 2 times, most recently from 516fadf to fa89ba1 Compare March 23, 2019 11:51
@davidtwco
Copy link
Member Author

davidtwco commented Mar 23, 2019

Filed #59382 with the refactoring, marking this as S-blocked and will rebase atop master when/if #59382 lands.

@davidtwco davidtwco added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2019
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch 2 times, most recently from 516f410 to 1b382f3 Compare March 24, 2019 20:01
bors added a commit that referenced this pull request Mar 24, 2019
Separate `DefId`s for variants and their constructors

Part of #44109. Split off from #59376. See [Zulip topic](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/rfc-2008/near/132663140) for previous discussion.

r? @petrochenkov
@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch from 1b382f3 to 147a385 Compare March 25, 2019 06:56
@davidtwco
Copy link
Member Author

Rebased atop master now #59382 has landed, unblocked.

@davidtwco davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 25, 2019
@petrochenkov petrochenkov 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 26, 2019
@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch from 08c4943 to 07de867 Compare March 28, 2019 21:05
@davidtwco
Copy link
Member Author

davidtwco commented Mar 28, 2019

cc @rust-lang/rustdoc for the final commit which modifies rustdoc to extend existing non-exhaustive support to enum variants.

@petrochenkov
Copy link
Contributor

r=me for non-rustdoc changes
r? @QuietMisdreavus

This commit removes the check that disallows the `#[non_exhaustive]`
attribute from being placed on enum variants and removes the associated
tests.

Further, this commit lowers the visibility of enum variant constructors
when the variant is marked as non-exhaustive.
@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch from 07de867 to 4de38fa Compare March 28, 2019 21:29
@GuillaumeGomez
Copy link
Member

Does it generate a whole page for enum variants now?

This commit updates the unstable book and diagnostics to reflect that
the `#[non_exhaustive]` attribute is now available for enum variants.
This commit adds support for non-exhaustive enum variants in rustdoc,
extending the existing support for non-exhaustive enums and structs.
@davidtwco davidtwco force-pushed the finally-rfc-2008-variants branch from 4de38fa to 49a6da2 Compare March 29, 2019 10:03
@davidtwco
Copy link
Member Author

Does it generate a whole page for enum variants now?

@GuillaumeGomez When an enum variant is marked as #[non_exhaustive], such as below:

enum Foo {
    #[non_exhaustive]
    Unit,
    #[non_exhaustive]
    Tuple(u8, u8),
    #[non_exhaustive]
    Struct { x: u8, y: u8 },
}

Then it is displayed like this:

Screenshot from 2019-03-29 09-47-05

This expands to:

Screenshot from 2019-03-29 09-58-12

For reference, here are the equivalent messages for structs (expanded) and enums (expanded).

All that the final commit of this PR does is add support for these messages and make it so that the non-exhaustive sections always start collapsed (since they're a lot of repetitive text otherwise).

@GuillaumeGomez
Copy link
Member

Ah I see! Looks good to me then.

@QuietMisdreavus
Copy link
Member

Rustdoc code looks good. Thanks for the screenshots!

@bors r=petrochenkov,QuietMisdreavus

@bors
Copy link
Contributor

bors commented Mar 29, 2019

📌 Commit 49a6da2 has been approved by petrochenkov,QuietMisdreavus

@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 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 30, 2019
bors added a commit that referenced this pull request Mar 30, 2019
Rollup of 10 pull requests

Successful merges:

 - #59376 (RFC 2008: Enum Variants)
 - #59453 (Recover from parse error in tuple syntax)
 - #59455 (Account for short-hand field syntax when suggesting borrow)
 - #59499 (Fix broken download link in the armhf-gnu image)
 - #59512 (implement `AsRawFd` for stdio locks)
 - #59525 (Whitelist some rustc attrs)
 - #59528 (Improve the dbg! macro docs )
 - #59532 (In doc examples, don't ignore read/write results)
 - #59534 (rustdoc: collapse blanket impls in the same way as normal impls)
 - #59537 (Fix OnceWith docstring.)

Failed merges:

r? @ghost
@bors bors merged commit 49a6da2 into rust-lang:master Mar 30, 2019
@davidtwco davidtwco deleted the finally-rfc-2008-variants branch March 30, 2019 18:02
Centril added a commit to Centril/rust that referenced this pull request Apr 14, 2019
Fix cross-crate visibility of fictive variant constructors

After merging rust-lang#59376 I realized that the code in the decoder wasn't entirely correct - we "decoded" fictive variant constructors with their variant's visibility, which could be public, rather than demoted to `pub(crate)`.

Fictive constructors are not directly usable in expression/patterns, but the effect still can be observed with imports.

r? @davidtwco
Centril added a commit to Centril/rust that referenced this pull request Apr 14, 2019
Fix cross-crate visibility of fictive variant constructors

After merging rust-lang#59376 I realized that the code in the decoder wasn't entirely correct - we "decoded" fictive variant constructors with their variant's visibility, which could be public, rather than demoted to `pub(crate)`.

Fictive constructors are not directly usable in expression/patterns, but the effect still can be observed with imports.

r? @davidtwco
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants