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: remove redundant toggles for "undocumented items" #84326

Open
jsha opened this issue Apr 19, 2021 · 34 comments
Open

rustdoc: remove redundant toggles for "undocumented items" #84326

jsha opened this issue Apr 19, 2021 · 34 comments
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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

@jsha
Copy link
Contributor

jsha commented Apr 19, 2021

Rustdoc has some code to hide "undocumented items." As far as I can tell, that applies to trait implementors (on trait pages) and trait implementations (on struct pages). However, there's already an outer layer of toggles for both the implementors and implementations. For instance, this is what the "implementations" section looks like for the Read trait, if your settings expand implementations by default:



The toggle for each implementation is redundant with the inner toggle for the undocumented items. I think we should remove either the outer toggle or the inner. I'm not sure which is better. One argument in favor of removing the outer toggles: If someone has gone to the trouble of documenting a trait implementation, we should try to display that by default. For instance, here's what the implementations section looks like for String, by default:



However, if you know to expand it, there's actually some really nice documentation there:



There's a similar situation in rustls: Its Read implementation has important documentation on how to use it properly, but you wouldn't know that documentation was there because it's toggled closed by default.

@jsha
Copy link
Contributor Author

jsha commented Apr 19, 2021

Also worth noting that the "Show hidden undocumented items" don't respond to the "show all / collapse all" toggle in the upper right, and don't have a corresponding settings.html entry to show or hide them by default.

Part of #83332

@jsha jsha added A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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. labels Apr 23, 2021
@GuillaumeGomez
Copy link
Member

Done in #84754. Don't hesitate to re-open if I missed something but from what I read, it's now done.

@jsha
Copy link
Contributor Author

jsha commented May 9, 2021

In #84754, we migrated the existing toggles to <details> But what I'd like to consider is either removing the toggles for "undocumented items", or removing the toggles for "implementors," or otherwise finding some improvement so we don't have two levels of toggles when it's generally not necessary.

@jsha jsha reopened this May 9, 2021
@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

I think we should split this into two cases:

Implementations on struct and enum pages

Example (default view)

image

Example (toggles expanded)

image

This demonstrates that the wording "Show hidden undocumented items" is incorrect - these items actually are documented, on the trait itself. That documentation gets copied to the struct or enum page. We're paying the cost in bytes and DOM size for these docs; we should show them by default. And that default can be modified by the "Auto-hide trait implementations" setting.

Or, another way of looking at it: If you are looking at some code that calls a method on ErrorKind, and you want to know what that method does, you'll go to the ErrorKind page and Ctrl-F for it. You should be able to find the method regardless of whether it was implemented as part of a trait or not. This is a use case I definitely had early in my Rust journey, and still occasionally do.

Implementors on trait pages

Example (default view)

image

Example (toggles expanded)

image

When you're on a trait page, the thing that's most useful to know is that type T implements the trait. The "undocumented methods" really are repetitive on a trait page and should be hidden. But I think, more than hidden, they should actually be entirely unrendered, since they duplicate information already on the page in the trait definition.

There are two things that should potentially be shown under each implementation in the Implementors section of a trait page:

  • Methods that have documentation (but this probably belongs on the docs for the implementing type, not on the trait page)
  • Whether the type implements any of the optional methods of the trait.

Proposal

I propose to remove the toggle for "undocumented items" on struct and enum pages, making the contents always visible. I propose to remove the method documentation for Implementors on trait pages, leaving that information to the implementor's own doc page.

@GuillaumeGomez
Copy link
Member

That would simplify the code a bit. I don't have a strong opinion on this topic though.

What do you think @rust-lang/rustdoc?

@jyn514
Copy link
Member

jyn514 commented May 17, 2021

I really like both those ideas :) although maybe it makes sense to collapse the method descriptions by default on type pages, since they're always the same and can be long? I don't feel strongly about that though.

@Nemo157
Copy link
Member

Nemo157 commented May 17, 2021

One thing I'm unsure about is how this will feel on pages like Box that have all the traits.

I somewhat commonly skim through the trait list looking for specific traits I care about, but I could just turn "Auto-hide trait implementation documentation" back on (sidenote, it appears to be broken on nightly).

@GuillaumeGomez
Copy link
Member

Please open an issue for the broken part. ;)

@Manishearth
Copy link
Member

I propose to remove the toggle for "undocumented items" on struct and enum pages, making the contents always visible.

Strong +1

I propose to remove the method documentation for Implementors on trait pages, leaving that information to the implementor's own doc page.

Should we show it only if there are explicit method docs on the implementors?

@GuillaumeGomez
Copy link
Member

Should we show it only if there are explicit method docs on the implementors?

Currently it's kinda done, but badly. If there is no doc on the implementor method, we put it under "undocumented item" (which is really not clear). So any improvement would be nice.

I think that if there is implementor doc, we should show it, otherwise just display something like "read more" and that's it.

@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

One thing I'm unsure about is how this will feel on pages like Box that have all the traits.

@Nemo157 this is a good point. As an example, Box has 5 implementations of Default, for various specializations. Here they are with everything fully expanded:

image

This can be repetitive, and also potentially confusing for a new user. If I see default being called, which of these is it?

One potential approach would be to "group" these implementations. So we would have one sub-heading for the Default trait, which would list all the implementations of Default for this type, and then all the methods. Something like (forgive the lack of formatting):

Default

impl<T> Default for Box<[T], Global>
impl Default for Box<str, Global>
impl<T> Default for Box<T, Global>
impl Default for Box<CStr>
impl Default for Box<OsStr>

fn default() -> Box<OsStr>
  Returns the “default value” for a type. Read more

I think that if there is implementor doc, we should show it, otherwise just display something like "read more" and that's it.

I think to keep things simpler, on trait Foo's page we should only show comments that are set on the impl Foo for Bar of a struct or enum. Comments on individual methods within impl Foo for Bar can be found on Bar's page on don't need be on Foo's page. This is simpler for the doc author and the doc reader to reason about, and as a side benefit I suspect it will be a little easier for us to implement as well.

@GuillaumeGomez
Copy link
Member

I think to keep things simpler, on trait Foo's page we should only show comments that are set on the impl Foo for Bar of a struct or enum. Comments on individual methods within impl Foo for Bar can be found on Bar's page on don't need be on Foo's page. This is simpler for the doc author and the doc reader to reason about, and as a side benefit I suspect it will be a little easier for us to implement as well.

Not sure to understand your comment so instead, code:

trait Foo {
    /// default doc
    fn default_fn();
    fn another_one();
}

struct Bar;

impl Foo for Bar {
    fn default_fn() {}
    /// item doc!
    fn another_one() {}
}

So on Bar page, only show show the doc of another_one, default_fn will just have "read more". Is it what you meant?

@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

On Foo's page: Show only impl Foo for Bar, no methods.

On Bar's page: Show default_fn with default doc and another_one with item doc!.

@GuillaumeGomez
Copy link
Member

On Foo's page: Show only impl Foo for Bar, no methods.

Agreed, that'd be duplicated (I actually thought we already did but I guess not so nice idea!).

On Bar's page: Show default_fn with default doc and another_one with item doc!.

However I'm not sure to agree on that point: that'd make a lot of duplicated content in all pages...

@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

On Bar's page: Show default_fn with default doc and another_one with item doc!.

However I'm not sure to agree on that point: that'd make a lot of duplicated content in all pages...

This is how we currently do it. For example https://doc.rust-lang.org/nightly/std/fs/struct.File.html#impl-Read:

So I was proposing this way to be conservative with regards to what we currently do. You're right that there's a lot of duplicated content across many pages!

Is your proposal that we would show no documentation for Read::read on File's page? Or just the method signature but not copy the doccomment from Read::read onto File's page?

@GuillaumeGomez
Copy link
Member

Currently we display the first line/paragraph on "default docs". I'm not sure it's very useful so I would only keep the "read more" part.

Is your proposal that we would show no documentation for Read::read on File's page? Or just the method signature but not copy the doccomment from Read::read onto File's page?

The second one: it would show the method signature (very important to keep that part!) but no documentation unless it's this implementation which added it.

@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

The second one: it would show the method signature (very important to keep that part!) but no documentation unless it's this implementation which added it.

Great. I like this approach.

@jyn514
Copy link
Member

jyn514 commented May 17, 2021

no documentation unless it's this implementation which added it.

Hmm, that seems like it might confuse people into thinking the trait method has no description - maybe instead we could collapse the description by default?

@GuillaumeGomez
Copy link
Member

That's what we currently do. :p Also, there will still be a "read more" link.

@jyn514
Copy link
Member

jyn514 commented May 17, 2021

(also, I'm not fully following all the proposed changes, maybe someone could write up the current proposal in the issue description? Presumably it will be in the PR too which seems fine)

@GuillaumeGomez
Copy link
Member

I *think* we just agreed on all details, so I guess we'll have the sum up written shortly.

@jsha
Copy link
Contributor Author

jsha commented May 17, 2021

Current proposal, which I think should be a few separate PRs:

  1. On struct and enum pages: remove the toggle for "undocumented items," making the contents always visible.
  2. On struct and enum pages: group implementations by the trait implemented. This reduces duplication when the struct or enum implements the same trait for various different specializations
  3. On struct and enum pages: remove the doccomment on trait methods, if that doccomment was copied from the trait. Keep the doccomment if it was written on the struct or enum's implementation of that method.
  4. On trait pages: remove the method documentation for Implementors on trait pages, leaving that information to the implementor's own doc page.

@Nemo157
Copy link
Member

Nemo157 commented May 17, 2021

1. On struct and enum pages: remove the toggle for "undocumented items," making the contents always visible.

But note that the default setting of "Auto-hide trait implementation documentation" is on, so you still won't see this content by default.

@jyn514
Copy link
Member

jyn514 commented May 18, 2021

On struct and enum pages: remove the doccomment on trait methods, if that doccomment was copied from the trait. Keep the doccomment if it was written on the struct or enum's implementation of that method.

Ok yes, this still seems confusing to me: #84326 (comment)

@jsha
Copy link
Contributor Author

jsha commented May 18, 2021

no documentation unless it's this implementation which added it.

Hmm, that seems like it might confuse people into thinking the trait method has no description - maybe instead we could collapse the description by default?

I'd like to avoid adding more things that we collapse by default. It adds page weight for stuff that is usually never seen, and it feels like a way of not addressing head-on the questions of what information needs to be visible where.

We could add a link for those trait methods to the trait page, e.g. "Read documentation at Foo."

I'm also not particularly opposed to keeping doccomments that came from the trait (as we currently do).

@GuillaumeGomez
Copy link
Member

I'm also not particularly opposed to keeping doccomments that came from the trait (as we currently do).

We don't, we just have the first line. ;)

We could add a link for those trait methods to the trait page, e.g. "Read documentation at Foo."

As long as the trait's items are listed, I'm ok with it.

  1. On struct and enum pages: group implementations by the trait implemented. This reduces duplication when the struct or enum implements the same trait for various different specializations

I'm not too sure about this. It's actually quite useful in some cases like wrapping types where the generic wrapped type can implement (or not) some traits based on what they implement themselves.

On trait pages: remove the method documentation for Implementors on trait pages, leaving that information to the implementor's own doc page.

Just listing the implementors and not their documentation sounds like a good idea!

jsha added a commit to jsha/rust that referenced this issue May 31, 2021
Per discussion in rust-lang#84326. For trait implementations, this was
misleading: the items actually do have documentation (but it comes from
the trait definition).

For both trait implementations and trait implementors, this was
redundant: in both of those cases, the items are default-hidden by
different toggle at the level above.

Update tests: Remove XPath selectors that over-specified on details tag,
in cases that weren't testing toggles. Add an explicit test for toggles
on methods. Rename item-hide-threshold to toggle-item-contents for
consistency.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 1, 2021
…aumeGomez

Remove toggle for "undocumented items."

Per discussion in rust-lang#84326. For trait implementations, this was
misleading: the items actually do have documentation (but it comes from
the trait definition).

For both trait implementations and trait implementors, this was
redundant: in both of those cases, the items are default-hidden by
different toggle at the level above.

Update tests: Remove XPath selectors that over-specified on details tag,
in cases that weren't testing toggles. Add an explicit test for toggles
on methods. Rename item-hide-threshold to toggle-item-contents for
consistency.

Demo:
https://hoffman-andrews.com/rust/untoggle-undocumented/std/string/struct.String.html
https://hoffman-andrews.com/rust/untoggle-undocumented/std/io/trait.Read.html
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 17, 2021
…=GuillaumeGomez

Remove methods under Implementors on trait pages

As discussed at rust-lang#84326 (comment).

On a trait page, the "Implementors" section currently lists all methods of each implementor. That duplicates the method definitions on the trait itself, and is usually not very useful. So the implementors are collapsed by default. This PR changes rustdoc to just not render them at all. Any documentation specific to an implementor can be found by clicking through to the implementor's page.

This moves the "portability" info inside the `<summary>` tags so it is still visible on trait pages (as originally implemented in rust-lang#79201). That also means it will be visible on struct/enum pages when methods are collapsed.

Add `#[doc(hidden)]` to all implementations of `Iterator::__iterator_get_unchecked` that didn't already have it. Otherwise, due to rust-lang#86145, the structs/enums with those implementations would generate documentation for them, and that documentation would have a broken link into the Iterator page. Those links were already "broken" but not detected by the link-checker, because they pointed to one of the Implementors on the Iterator page, which happened to have the right anchor name.

This reduces the Read trait's page size from 128kB to 68kB (uncompressed) and from 12,125 bytes to 9,989 bytes (gzipped
Demo:

https://hoffman-andrews.com/rust/remove-methods-implementors/std/string/struct.String.html#trait-implementations
https://hoffman-andrews.com/rust/remove-methods-implementors/std/io/trait.Read.html#implementors

r? `@GuillaumeGomez`
@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

@jsha this can be closed now that #85970 is merged, right?

@jsha
Copy link
Contributor Author

jsha commented Jun 21, 2021

Yes. Note that these two items are unimplemented:

On struct and enum pages: group implementations by the trait implemented. This reduces duplication when the struct or enum implements the same trait for various different specializations
On struct and enum pages: remove the doccomment on trait methods, if that doccomment was copied from the trait. Keep the doccomment if it was written on the struct or enum's implementation of that method.

But it seems from subsequent comments that these were not particularly popular, so I'm going to close this without doing them.

@jsha jsha closed this as completed Jun 21, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

Oh hmm I really liked the idea of grouping by trait, I forgot that was suggested.

I'm not too sure about this. It's actually quite useful in some cases like wrapping types where the generic wrapped type can implement (or not) some traits based on what they implement themselves.

@GuillaumeGomez I'm not sure what you mean by this. Rustdoc will still show just as many impl blocks and they'll have generic parameters, it will just be grouped by the trait.

@jyn514 jyn514 reopened this Jun 21, 2021
@GuillaumeGomez
Copy link
Member

Then all good. I thought it meant having one impl displayed if the same trait was implemented multiple times.

@cynecx
Copy link
Contributor

cynecx commented Aug 30, 2021

On trait pages: remove the method documentation for Implementors on trait pages, leaving that information to the implementor's own doc page.

Instead of completely removing the methods from the implementors pages. Can we just show their signature without docs? That would certainly help with navigation and seeing what kind of methods are implemented (excluding default methods), the same way it does with the associate types (which #88490 adds back).

@GuillaumeGomez, @jyn514 ?

cc #88490

@joseluis
Copy link
Contributor

I was searching a way to remove the "read more" link from the doc-comments in the automatically implemented methods and constants from the trait, and be able to show instead the full doc-comment. But it seems there's not currently a way to customize this behaviour, right?

For certain APIS, specially those having a lot of default methods, I find it very inconvenient to have to navigate back and forth between the trait page documentation and the implementation type documentation in order to read more than 1 line of information for many of the methods, and it would be a much better experience if the complete doc-comment was shown for every method in the type page, independently of whether it's a default implementation or not.

@GuillaumeGomez
Copy link
Member

We removed the complete doc comment and replaced it with "read more" because the size of some trait pages was just completely out of control.

@joseluis
Copy link
Contributor

We removed the complete doc comment and replaced it with "read more" because the size of some trait pages was just completely out of control.

I see.

Fortunately I could make a workaround for my usecase, with a macro that duplicates the items in both the trait definition and the implementation. This way the doc-comments can be seen fully both in the trait and in the type doc pages:

macro_rules! impl_api {
    ($type:ident, $trait:ident, $($i:item),*) => {
        #[doc = concat!("Associates methods and constants to the [`", stringify!($type), "`] type.")]
        pub trait $trait {
            $($i)*
        }
        impl $trait for $type {
            $($i)*
        }
    };
}

// usage:
struct MyType;
impl_api![MyType, MyTrait,
    /// `BAR` is 0
    const BAR: u8 = 0;,
    /// `foo` returns 1
    fn foo() -> u8 { 1 }
];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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

7 participants