-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Add new doc(attribute = "...") attribute
#142472
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
Add new doc(attribute = "...") attribute
#142472
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.
cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi
Some changes occurred in compiler/rustc_passes/src/check_attr.rs
Some changes occurred in HTML/CSS/JS.
This comment has been minimized.
This comment has been minimized.
1ab0dc7 to
e2fd7f7
Compare
This comment has been minimized.
This comment has been minimized.
cc: @ehuss you might want to make sure the reference documents this too? Unsure whether this is interesting to you but just to be safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh, I'll put the doc attribute high up on my todo list to get a proper parser... (this is fine for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we make sure this stays in sync with the compiler's view of attributes? Is the plan to keep this manual, or could we somehow generate this from for example rustc_attr_data_structures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like keywords: we can only check that the attribute is used on an existing one. If new ones are added, there is no check for that.
We could add a tidy check for it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'd be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding a clean::Item::is_std_synthetic method for all 3 of these cases (name heavily subject to bikeshedding).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about the same but wasn't sure it was worth it. But since I'm not the only one thinking that, adding it.
e2fd7f7 to
a0fb9d0
Compare
These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.
Please ensure that if you've changed the output:
- It's intentional.
- The
FORMAT_VERSIONinsrc/librustdoc-json-typesis bumped if necessary.
This comment has been minimized.
This comment has been minimized.
a0fb9d0 to
14b0aa7
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
4a47724 to
094ed61
Compare
This comment has been minimized.
This comment has been minimized.
094ed61 to
10c8903
Compare
Ok, hopefully this time I didn't forget yet another different check I didn't use in years. T_T
Yeay! \o/
@notriddle It was an argument against having the search result link directly to the reference. I'm fine with having a link to an external resource in an item page as long as there is some content before it and not just a link.
4b3fb64 to
93fbd25
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.
Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
Fixed merge conflicts. Since we got approval from the lang team, all that remains is to review I guess. :)
I have fully reviewed here: #142472 (review).
I realized it just after commenting. ^^' I'm working on it. :)
Applied @fmease's suggestions. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens tho if you add #![feature(rustdoc_internals)]? Does it error with unknown attribute or does it get accepted? If it gets accepted, is rustdoc-search able to find it when you search for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg, I tested with a file with all attributes and forgot to copy/paste this one. ><
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more "for internal use in the std library."; section Document keywords also has that incorrect statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then taking it as well.
@bors rollup (internal feature)
r=me with latest nits addressed in some way or another + with some of the smaller commits squashed where it makes sense.
..._primitive`, `is_keyword` and `is_attribute` methods
...tes with namespace
65d329a to
f3c0234
Compare
@bors r=fmease
Rollup of 6 pull requests Successful merges: - #142472 (Add new `doc(attribute = "...")` attribute) - #145368 (CFI: Make `lto` and `linker-plugin-lto` work the same for `compiler_builtins`) - #145853 (Improve error messages around invalid literals in attribute arguments) - #145920 (bootstrap: Explicitly mark the end of a failed test's captured output) - #145937 (add doc-hidden to exports in attribute prelude) - #145965 (Move exporting of profiler and sanitizer symbols to the LLVM backend) r? `@ghost` `@rustbot` modify labels: rollup
For multiple reasons:
- It is not item doc oriented but item kind oriented, meaning that one chapter will cover all of the items of the same kind (so for example attributes), making it nice when you want to go through all items, but definitely not when you're looking for one in particular and want a lot of information on this in particular.
- It is not so much about explaining but about describing. Which is not bad or whatever, just not aimed at the same target audience.
- It uses a lot of terms that a rust beginner would have never heard of.
- There is a lot of visual information, like the anchors on the side of each item. Again not an issue, but very different to what someone coming from rust docs would expect and definitely need an extra time to adapt (this point is honestly pretty low on the issues, just being exhaustive).
So overall, the target audience is the main issue imo and why on top of the previously mentioned reasons we should not just generate links to the reference but instead add some extra context around it (context that doesn't need to be big or anything, but that might allow to give the information the user is looking for more quickly).
Thanks for these details. You're not wrong about any of this. These are valid reasons to want to write and maintain a separate set of documentation for attributes and keywords. It's just a lot of work to do well.
That would also mean that we would end up in another page that wouldn't work with no internet connection and it would be bad UX to directly lead the users in a resource that isn't available.
FWIW, I'm pretty sure we ship the reference (and all other books) in the same component, rust-docs. So as long as our URLs are relative and the HTML loads on file:// URLs, I would expect that to work locally (offline) + in doc.r-l.o just fine.
Rollup merge of #142472 - GuillaumeGomez:doc-attribute-attribute, r=fmease Add new `doc(attribute = "...")` attribute Fixes #141123. The implementation and purpose of this new `#[doc(attribute = "...")]` attribute is very close to `#[doc(keyword = "...")]`. Which means that luckily for us, most of the code needed was already in place and `@Noratrieb` nicely wrote a first draft that helped me implement this new attribute very fast. Now with all this said, there is one thing I didn't do yet: adding a `rustdoc-js-std` test. I added GUI tests with search results for attributes so should be fine but I still plan on adding one for it once documentation for builtin attributes will be written into the core/std libs. You can test it [here](https://rustdoc.crud.net/imperio/doc-attribute-attribute/foo/index.html). cc `@Noratrieb` `@Veykril`
Thanks for these details. You're not wrong about any of this. These are valid reasons to want to write and maintain a separate set of documentation for attributes and keywords. It's just a lot of work to do well.
Guillaume and I have sort of decided to go with a one-sentence synopsis + one example maximum + a link to the Reference. Nothing is set in stone here of course. However, having some sort of synopsis in rustdoc-generated pages (as opposed to linking or redirecting to the Reference) would "basically automatically" allow rust-analyzer to show docs for built-in attrs on LSP-hover which was actually the original motivation for this feature as stated in the linked issue #141123.
Another reason for a proper page over a link would be the hope that these new attribute docs would be more accessible, not just phrasing-wise (as previously mentioned) but also presentation-wise (re. the latter, there's no "context switch" necessary for the brain because the page layout stays the same which is good for various obvious reasons).
Of course, there are downsides to this approach. E.g., more experienced users will probably care less about these "stubs" and want to jump to the Reference directly in which case two clicks over one click could become increasingly annoying (T-lang's stance IIUC) and could also "hurt line of thought"1 .
Footnotes
-
At the other end of the spectrum however, we have actual power users who use parametrized browser keywords to jump to various documents like the Reference with a given query param. ↩
That would also mean that we would end up in another page that wouldn't work with no internet connection and it would be bad UX to directly lead the users in a resource that isn't available.
FWIW, I'm pretty sure we ship the reference (and all other books) in the same component, rust-docs. So as long as our URLs are relative and the HTML loads on file:// URLs, I would expect that to work locally (offline) + in doc.r-l.o just fine.
Nice, didn't know about that. We can make relative links then. :)
It's just a lot of work to do well.
Indeed. In this case, I'd recommend opening an issue with the list of attributes so that the work can be done in parallel, allowing to potentially greatly reduce the total time required.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #141123.
The implementation and purpose of this new
#[doc(attribute = "...")]attribute is very close to#[doc(keyword = "...")]. Which means that luckily for us, most of the code needed was already in place and @Noratrieb nicely wrote a first draft that helped me implement this new attribute very fast.Now with all this said, there is one thing I didn't do yet: adding a
rustdoc-js-stdtest. I added GUI tests with search results for attributes so should be fine but I still plan on adding one for it once documentation for builtin attributes will be written into the core/std libs.You can test it here.
cc @Noratrieb @Veykril