-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
rustdoc: introduce the #[doc(keyword="")] attribute for documenting keywords #51140
Conversation
fcbcfb9
to
02df550
Compare
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.
Yay, this is exciting! With this feature we can really extend the docs for some highly-requested features. Overall it looks nice, but i have some small comments.
Can you also put #[doc(keyword="")]
behind a feature flag and make a tracking issue for it? We missed the boat with #[doc(primitive="")]
but we can at least get this one.
We'll also want an issue to track the keywords we want to document. It could get folded into the #[doc(keyword)]
tracking issue, since i imagine we wouldn't really want to stabilize that, but it could also be its own standalone issue.
src/librustdoc/clean/mod.rs
Outdated
hir::ItemUse(ref path, hir::UseKind::Single) | ||
if item.vis == hir::Visibility::Public => { | ||
as_keyword(path.def).map(|(_, prim, attrs)| { | ||
// Pretend the primitive is local. |
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.
Left this comment as-is from the primitives version. 😆
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.
🐑
src/librustdoc/html/render.rs
Outdated
@@ -1556,6 +1557,7 @@ impl AllTypes { | |||
typedefs: HashSet::with_capacity(100), | |||
statics: HashSet::with_capacity(100), | |||
constants: HashSet::with_capacity(100), | |||
keywords: HashSet::with_capacity(10), |
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.
Wondering if this should join the rest at 100 default capacity, since there are currently 62 valid elements in the Keyword
enum, which could feasibly each get documentation.
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.
Let's put 62 then. :D
src/librustdoc/clean/mod.rs
Outdated
@@ -194,6 +195,18 @@ impl<'a, 'tcx, 'rcx> Clean<Crate> for visit_ast::RustdocVisitor<'a, 'tcx, 'rcx> | |||
inner: PrimitiveItem(prim), | |||
} | |||
})); | |||
m.items.extend(keywords.iter().map(|&(def_id, ref kw, ref attrs)| { |
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 wonder if you could use into_iter
with this to prevent the clones below. keywords
isn't really used outside of this, so there's no use keeping it around.
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.
Excellent idea!
src/test/rustdoc/keyword.rs
Outdated
// @has foo/keyword.match.html '//section[@id="main"]//div[@class="docblock"]//p' 'this is a test!' | ||
#[doc(keyword = "match")] | ||
/// this is a test! | ||
mod foo{} |
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.
Should we also include a test here to make sure the foo/foo/index.html
and related links are missing? Seems like a good chance to use the @!has_dir
command.
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 was hoping to have this checked directly once we have added keywords into std docs directly.
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.
...wait, how would that work? Isn't it the job of these tests to make sure the feature works, without depending on how it's used to pull that together?
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 I meant is that if it fails to link once we use the keyword feature in std docs, it'll fail tests at this moment. :)
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'm not worried about it placing broken links in the docs - the test is already checking for that. I'm worried about rustdoc creating useless unlinked pages for the original module that's supposed to not exist once rustdoc processes the keyword information out of it. (To be sure, since this code was adapted from the code for primitives, this issue shouldn't exist, but I like more tests.)
What I am suggesting is adding lines like this:
// @!has foo/index.html '//a/@href' 'foo/index.html'
// @!has foo/foo/index.html
// @!has_dir foo/foo
src/librustdoc/html/render.rs
Outdated
it: &clean::Item, | ||
_p: &str) -> fmt::Result { | ||
document(w, cx, it)?; | ||
render_assoc_items(w, cx, it, it.def_id, AssocItemRender::All) |
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.
Does this need to have associated items? I don't expect us to link keywords to anything.
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 primitives. :)
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.
Actually, we link loads of things to primitives, because function signatures will have links to them. Keywords, on the other hand, don't have the same association. Since they're not types, per se, there's nothing that will automatically want to reference them. (Unless we want to link every function's fn
keyword to that page, every impl
block to that page, the trait
/struct
/enum
/etc in type/trait definitions to those pages, etc... I don't quite think it will be useful enough to edge out over the noise of having yet another link.)
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.
True, I'll remove it then.
src/librustdoc/clean/mod.rs
Outdated
@@ -204,7 +204,7 @@ impl<'a, 'tcx, 'rcx> Clean<Crate> for visit_ast::RustdocVisitor<'a, 'tcx, 'rcx> | |||
stability: get_stability(cx, def_id), |
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.
attrs
doesn't need a clone here either.
d4a6c0e
to
793e041
Compare
Ping from triage, @QuietMisdreavus ! |
I'm still waiting on @GuillaumeGomez to respond to #51140 (comment) and follow up on #51140 (comment). |
Updated. |
Yay, thanks so much! r=me pending travis. |
The job Click to expand the log.
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 |
@bors: r=QuietMisdreavus |
📌 Commit b9ede5d has been approved by |
☔ The latest upstream changes (presumably #50338) made this pull request unmergeable. Please resolve the merge conflicts. |
🔒 Merge conflict |
💔 Test failed - status-appveyor |
Failure is legit.
|
Oh right, I forget every time that travis doesn't run everything... I'll add the missing piece. |
@bors r+ |
📌 Commit 2f7fa24 has been approved by |
⌛ Testing commit 2f7fa24 with merge 61fc80b27d9b1c1a08ed3b6a0c8f7a1f8bbae870... |
💔 Test failed - status-travis |
The job Click to expand the log.
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 |
The job Click to expand the log.
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 |
The error being:
I assume we can just retry... @bors: retry |
rustdoc: introduce the #[doc(keyword="")] attribute for documenting keywords Part of #34601. r? @QuietMisdreavus
☀️ Test successful - status-appveyor, status-travis |
Part of #34601.
r? @QuietMisdreavus