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 non-exhaustive enums/structs: Rustdoc #51854

Merged
merged 8 commits into from Jul 19, 2018

Conversation

Projects
None yet
8 participants
@davidtwco
Copy link
Member

davidtwco commented Jun 27, 2018

Part of #44109. Not sure how those who maintain rustdoc primarily would prefer this addition look or where it should be placed, happy to make any changes required.

r? @QuietMisdreavus (not sure if this is the right person, just guessing)

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Jun 27, 2018

Mind sharing a screenshot of what this looks like?

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jun 27, 2018

@clarcharr
Here's a link to a screenshot.
I'm not sure it's perfect, but I'm more than happy to adjust it to something better.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jun 27, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:23:24] travis_time:end:stage0-rustdoc-themes:start=1530135345900665780,finish=1530135347050704467,duration=1150038687

[01:23:24] rustdoc: [theme-checker] Starting tests!
[01:23:24]  - Checking "/checkout/src/librustdoc/html/static/themes/dark.css"... FAILED
[01:23:24]   Missing ".stab.non-exhaustive" rule
[01:23:24] 
[01:23:24] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rustdoc-themes" "/checkout/obj/build/bootstrap/debug/rustdoc" "/checkout/src/librustdoc/html/static/themes"
[01:23:24] expected success, got: exit code: 1
[01:23:24] 
[01:23:24] 
[01:23:24] 
[01:23:24] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:23:24] Build completed unsuccessfully in 0:42:42
[01:23:24] make: *** [check] Error 1
[01:23:24] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:123a773c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Jun 27, 2018

Ah, looks good! Although the message is a bit odd, and personally I'd recommend customising the message depending on whether it's a struct or enum.

I'm also a bit confused on the microscope icon, as I get the sentiment behind "a closer look" although it looks a bit off for me. Although without an icon it'd look worse, so, idk.

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jun 27, 2018

I've made it say what it is (struct, enum, etc.). I only included the microscope icon as that is (as far as I can tell) always included in the little boxes like that one.

I agree that the message is a bit off, I can't think of a better one at the moment (else I'd have included it) but if anyone reading this can think of one, I'll happily change it.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Jun 27, 2018

What I thought of for enums: This enum is marked as non-exhaustive, and additional variants may be added in the future. When matching over values of this type, an extra _ arm must be added to account for future extensions.

However, for structs, I think that it might be better to do the opposite and only add a message if all of these things can be done normally. Something along the lines of "all of this struct's fields are public, and it can be constructed normally with the Struct { .. } syntax."

The RFC suggested using the /* fields omitted */ comment in the definition, although the latest rustdoc hides declarations. I'm filing a bug for this.

@clarfon

This comment has been minimized.

Copy link
Contributor

clarfon commented Jun 27, 2018

(I edited that comment enough times I'm going to add a comment that I did so for people viewing the response via email)

@QuietMisdreavus
Copy link
Member

QuietMisdreavus left a comment

cc @rust-lang/rustdoc for the implementation and @rust-lang/docs for the placement of the "non-exhaustive" disclaimer and its wording. I'm not totally sure what was agreed upon for how #[non_exhaustive] was supposed to be noted in docs, but i'd like to make sure there's some agreement in the docs team for how we do it.

@@ -366,6 +368,7 @@ pub struct Item {
pub def_id: DefId,
pub stability: Option<Stability>,
pub deprecation: Option<Deprecation>,
pub non_exhaustive: bool,

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Jul 5, 2018

Member

I think i'd prefer seeing the non_exhaustive marker just on the kinds where it's relevant, so just Struct/Enum/etc, instead of on Item here where literally everything has to care about it.

On the other hand, if i remember right we do keep all the ast::Attributes around in the Attributes struct, so we could just query them as needed, rather than passing this flag around all the way from clean_ast to html/render.

This comment has been minimized.

@davidtwco

davidtwco Jul 5, 2018

Author Member

This should be resolved now.

@@ -2194,6 +2194,7 @@ fn document(w: &mut fmt::Formatter, cx: &Context, item: &clean::Item) -> fmt::Re
info!("Documenting {}", name);
}
document_stability(w, cx, item)?;
document_non_exhaustive(w, item)?;

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Jul 5, 2018

Member

...though now i see why you did it this way. I commented in #51860 (comment) that this marker could go into the "Fields" or "Variants" sections of the struct/enum pages, rather than at the top of the full docs.

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 12, 2018

image

@QuietMisdreavus @clarcharr I've updated this to move the message to the suggested location.

@davidtwco davidtwco force-pushed the davidtwco:rfc-2008-rustdoc branch from bfd2b55 to 9527d6a Jul 12, 2018

pub fn is_non_exhaustive(&self) -> bool {
self.attrs.other_attrs.iter()
.filter(|a| a.name().as_str() == "non_exhaustive")
.count() > 0

This comment has been minimized.

@frewsxcv

frewsxcv Jul 13, 2018

Member

It may be more peformant to use .any() instead of .count() > 0

This comment has been minimized.

@davidtwco

davidtwco Jul 13, 2018

Author Member

Fixed this.

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 17, 2018

@rust-lang/rustdoc @rust-lang/docs Would appreciate some feedback on whether the message and placement are appropriate so we can get this landed!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jul 17, 2018

This is very confusing and it takes a lot of place. Not very convinced by the current add...

Updated wording and placement of non-exhaustive notice so it is colla…
…psed by default and easier to understand.
@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 18, 2018

I've updated the PR to hide the message by default and to update the wording slightly.

When collapsed:
image

When expanded:
image

Thoughts? @rust-lang/rustdoc @rust-lang/docs

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jul 19, 2018

With the message hidden by default, it's a bit better. However, why do we have a #[non_exhaustive] attribute?

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 19, 2018

@GuillaumeGomez #[non_exhaustive] was added in RFC 2008. In short, it requires that downstream crates add wildcard arms to any matches on structs/enums that are marked as #[non_exhaustive] so that new variants or fields being added to those types isn't a breaking change.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jul 19, 2018

Ah I didn't know. Then no problem with it. If messages are hidden by default, then all good for me.

Therefore, non-exhaustive structs cannot be constructed in external crates \
using the traditional <code>Struct {{ .. }}</code> syntax; cannot be \
matched against without a wildcard <code>..</code>; and \
functional-record-updates do not work.")?;

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Jul 19, 2018

Member

Is "functional-record-updates" a term we use elsewhere? I'm not familiar with what it could mean.

This comment has been minimized.

@davidtwco

davidtwco Jul 19, 2018

Author Member

It seems like the book refers to it as struct update syntax, I'll change the PR to reflect this.

This comment has been minimized.

@davidtwco

davidtwco Jul 19, 2018

Author Member

Pushed a commit that changes this.

This comment has been minimized.

@clarfon

clarfon Jul 19, 2018

Contributor

The book actually didn't mention FRU at all so I was equally confused when the RFC came about. Struct update syntax is a much better way of putting it, and I'm glad it's finally in the book now.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jul 19, 2018

This looks really nice! Thanks so much for working on this. I just have the one terminology question, but once that's done i'm ready to push the button on this!

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jul 19, 2018

Thanks for updating! Let's get this rolling!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2018

📌 Commit b671bdc has been approved by QuietMisdreavus

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2018

⌛️ Testing commit b671bdc with merge 11864c4...

bors added a commit that referenced this pull request Jul 19, 2018

Auto merge of #51854 - davidtwco:rfc-2008-rustdoc, r=QuietMisdreavus
RFC 2008 non-exhaustive enums/structs: Rustdoc

Part of #44109. Not sure how those who maintain rustdoc primarily would prefer this addition look or where it should be placed, happy to make any changes required.

r? @QuietMisdreavus (not sure if this is the right person, just guessing)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 19, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 11864c4 to master...

@bors bors merged commit b671bdc into rust-lang:master Jul 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:rfc-2008-rustdoc branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.