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

rustdoc: should we stop showing #[lang = ...] and some other attributes? #84309

Closed
Manishearth opened this issue Apr 18, 2021 · 11 comments
Closed
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

image

It seems like we should not be showing #[lang = ...] (etc) attributes in rustdoc. Perhaps we should allowlist or denylist such attributes?

cc @rust-lang/rustdoc

@Manishearth Manishearth added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 18, 2021
@Manishearth
Copy link
Member Author

Also perhaps we should truncate must_use to not include the full text

image

I know we used to hide attributes unconditionally and now we show them unconditionally since it's almost always one attribute so it's not a space saving thing, but there's the additional concern about it being distracting. We can probably allowlist to a set of attributes we care about (must_use, repr, no_mangle, link, ...)

cc @jsha

@jsha
Copy link
Contributor

jsha commented Apr 18, 2021

There's already an allow list of attributes, but #[lang] is on it:

const ALLOWED_ATTRIBUTES: &[Symbol] = &[
sym::export_name,
sym::lang,
sym::link_section,
sym::must_use,
sym::no_mangle,
sym::repr,
sym::non_exhaustive,
];

I'm removing lang in #84283, along with a reduction in visual weight for all attributes.

I've tried truncating #[must_use] to not include the full text, but I think actually the best thing is to not show #[must_use] at all. It's an attribute on a lot of functions very commonly used by beginners, and the attribute name is not at all self-explanatory. Also, even without must_use listed in the docs, its message will show up at just the time someone needs it - when it generates a compiler warning. In other words, when you're reading the docs, you don't need to know a function is #[must_use] in order to use it correctly.

@Manishearth
Copy link
Member Author

Oh! I feel like I've even seen that list before 😄, probably just forgot.

Reducing the visual weight is a great idea, thanks!

must_use is ... well, I still find it useful to know things are tagged as such, but I don't mind removing it from the allowlist, it doesn't impact semantics anywhere close to as much as the others do.

@camelid
Copy link
Member

camelid commented Apr 19, 2021

must_use is ... well, I still find it useful to know things are tagged as such, but I don't mind removing it from the allowlist, it doesn't impact semantics anywhere close to as much as the others do.

Yeah, I agree. I wonder if we should also stop showing non_exhaustive? Currently, we say that it's non-exhaustive in addition to showing the attribute:

image

@camelid camelid changed the title rustdoc: should we be allowlisting attributes? rustdoc: should we stop showing #[lang = ...] and some other attributes? Apr 19, 2021
@camelid camelid added A-attributes Area: Attributes (`#[…]`, `#![…]`) C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Apr 19, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 19, 2021

I think #[non_exhausive] is important to show. The variants are also duplicated between the declaration and the main docs, but we don't remove the declaration either.

@camelid
Copy link
Member

camelid commented Apr 19, 2021

The variants are also duplicated between the declaration and the main docs, but we don't remove the declaration either.

Yeah, I find that duplication a bit annoying :)

@jsha
Copy link
Contributor

jsha commented Apr 20, 2021

@jyn514 said at #83332 (comment):

I think we should fix this by removing the documentation about what #[non_exhaustive] is and linking to the official docs instead: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute

I agree that we should link all attributes to their official docs. For #[non_exhaustive], that would make it the only clickable hyperlink in the declaration section. That would be a bit of inconsistency since the other items in that section would generally be clickable if they showed up elsewhere. We would also need to highlight it in some way to indicate that it's clickable, which would make it stand out a lot from the rest of the declaration.

@jyn514
Copy link
Member

jyn514 commented Apr 20, 2021

That would be a bit of inconsistency since the other items in that section would generally be clickable if they showed up elsewhere.

I mean, I think it would also be useful to make items in the declaration clickable.

@camelid
Copy link
Member

camelid commented Apr 22, 2021

It might be nice to use syntax highlighting for the declaration too. I don't think we'd even have to use the rustc lexer because rustdoc knows that e.g. it's emitting an attribute vs visibility.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 14, 2021
Remove must_use from ALLOWED_ATTRIBUTES

This is a fairly common attribute on methods, but is not something you need to know when reading the method docs - the purpose of the attribute is for the compiler to tell you about it if you forget to use a value.

Removing reclaims some valuable space in the summary of methods, particularly when the attribute has a long string value.

As discussed in rust-lang#84309. Partially addresses rust-lang#81482.

r? `@Manishearth`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 14, 2021
Remove must_use from ALLOWED_ATTRIBUTES

This is a fairly common attribute on methods, but is not something you need to know when reading the method docs - the purpose of the attribute is for the compiler to tell you about it if you forget to use a value.

Removing reclaims some valuable space in the summary of methods, particularly when the attribute has a long string value.

As discussed in rust-lang#84309. Partially addresses rust-lang#81482.

r? ``@Manishearth``
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 15, 2021
Remove must_use from ALLOWED_ATTRIBUTES

This is a fairly common attribute on methods, but is not something you need to know when reading the method docs - the purpose of the attribute is for the compiler to tell you about it if you forget to use a value.

Removing reclaims some valuable space in the summary of methods, particularly when the attribute has a long string value.

As discussed in rust-lang#84309. Partially addresses rust-lang#81482.

r? ```@Manishearth```
@bjorn3
Copy link
Member

bjorn3 commented Dec 12, 2022

#[lang] is no longer shown since #84283. The full list of attributes that is currently shown makes sense to me:

const ALLOWED_ATTRIBUTES: &[Symbol] =
&[sym::export_name, sym::link_section, sym::no_mangle, sym::repr, sym::non_exhaustive];
Can this issue be closed?

@GuillaumeGomez
Copy link
Member

I think it can be closed now indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants