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: some trait methods appear as "hidden undocumented items" #56546

Closed
eminence opened this issue Dec 5, 2018 · 44 comments
Closed

rustdoc: some trait methods appear as "hidden undocumented items" #56546

eminence opened this issue Dec 5, 2018 · 44 comments
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@eminence
Copy link
Contributor

eminence commented Dec 5, 2018

Using the Clone impl for Vec as an example, here's what the latest stable docs look like:

image

Whereas the latest nightly docs look like this:

image

This issue was mentioned #56073 but I believe this issue is important enough to pull into its own issue. I also think this issue is a candidate for a "regression-from-stable-to-nightly" flag.

@estebank estebank added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 7, 2018
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Dec 8, 2018

What's the problem with that? Those items don't have documentation and are therefore using the one from the trait definition. They are undocumented, that's a fact. What do you suggest we should do instead?

@eminence
Copy link
Contributor Author

eminence commented Dec 8, 2018

In my specific example with the Clone trait, I believe the old behavior produces good results because the trait method documentation is written in such a way to make sense for all types that derive Clone. So my initial suggesting would be to switch back to the old behavior of taking the documentation from the trait if the impl does not provide one.

However, I can imagine that this isn't true for all traits, and there may be examples where applying the trait documentation to the implementation might not make sense. Do you know if there are specific examples of this? Was this related to why this behavior was changed?

The root of my complaint is that categorizing important methods like clone() as "hidden undocumented" is confusing. To me a "hidden undocumented" method is one that should be avoided (my thought process is something like: "If the authors wanted me to use this method, they would have documented it and unhidden it")

I suppose one way to resolve my complaint would be to manually duplicate the trait documentation onto the impls, but this seems tedious and error-prone 😃

@QuietMisdreavus
Copy link
Member

Hiding the methods was deliberate. The idea is that when looking at a type that implements a bunch of standard traits but hadn't put special notes on any of them, the only important thing the user needs to know is that the trait is implemented. Seeing your comment, i bet the verbiage could be improved, but the feature itself was deliberately added to try to keep the page length from getting excessive.

@QuietMisdreavus
Copy link
Member

I suppose one way to resolve my complaint would be to manually duplicate the trait documentation onto the impls, but this seems tedious and error-prone 😃

If you expand the "undocumented" items, rustdoc already does that for you. The impls are undocumented, so we copy over the trait's documentation. Again, the idea is that if you see the same trait everywhere, and a given implementation doesn't add any extra documentation, just knowing it's there is enough.

The behavior was added in #54162 if you want to read over that thread.

@eminence
Copy link
Contributor Author

Thanks for sharing the relevant issue number. I am certainly sympathetic to the issue of page length. I think that collapsing the trait section by default is a good step, but I still maintain that classifying these methods as "hidden undocumented" is confusing and misleading (especially to new users who may not be familiar with these standard traits).

Are you open to suggestions on how to improve this? I think one possibility would be to automatically expand the method docs when expanding the trait (since currently expanding the trait shows nothing of value).

@birktj
Copy link

birktj commented Feb 1, 2019

I think this would be better if the default impls also appeared under the "undocumented" items. Currently it looks sort of strange when a bunch of default impls are shown but the most important items are hidden.

@nstoddard
Copy link

I ran into this issue when reading the docs for the cgmath crate. On the Vector3 docs, for the MetricSpace impl, the distance2 method is hidden by default, which initially makes it look like it doesn't exist. The same problem occurs for the InnerSpace impl.

I see how this feature is helpful for traits like Clone for which there's nothing interesting about the implementation other than the fact that it's implemented, but for many other traits, it often hides important methods. Would it be possible to restrict this feature to traits like Clone?

@TheZoq2
Copy link
Contributor

TheZoq2 commented Mar 17, 2019

The same issue is present in the embedded_hal world. Nearly all the functions from the embedded_hal crate are hidden in the stm32f1xx_hal docs.

It seems weird to always hide trait impls as there could be some very useful functions in there, as is the case in stm32f1xx_hal

@GuillaumeGomez
Copy link
Member

@TheZoq2: You don't look at the problem as a whole: in this case they could all be expanded, it wouldn't change much. But in some cases where a type has a lot of trait implementations, it becomes very complicated to read. The current orientation would be to reformulate the sentence, not to re-expand everything automatically.

@agrif
Copy link

agrif commented Mar 18, 2019

Perhaps a trait could indicate at the definition point that its documentation should be expanded automatically? Like @nstoddard's suggestion. The issue seems to be that there are two types of traits:

  1. Traits like Clone, where you are likely to see it very often and already have an idea of what it provides. Hiding these traits reduces visual noise and makes it easier to get at more important info.

  2. Traits like InnerSpace in cgmath, that most people will probably not see often enough to learn each of the 8 methods it provides. Especially for cgmath, hiding 'undocumented' traits by default makes it very hard to learn what methods a type like Vector3 even provides. If InnerSpace could opt-in to showing trait items and docs by default, that would be a pretty good compromise.

Whether this behavior should be opt-in or opt-out is a good question, though.

Edit: another possible solution, let the documentation for a given type 'promote' certain traits it implements, to be shown expanded directly under the inherent impl. Vector3 could promote InnerSpace, etc, and improve its documentation considerably.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 4, 2019

Not all undocumented items under impl blocks are hidden by 'Show undocumented items':

For example: https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#implementors

Here, some of the ne are shown, and some are not, even though all are undocumented:

blah

Is this an unrelated bug?

@RalfJung
Copy link
Member

RalfJung commented Apr 10, 2019

In fact sometimes this leads to only showing deprecated items per default while not showing the actual public API:

Two sub-items appear: "Show hidden undocumented items" and before_exec. That is very strange, since the items that are shown when unfolding the "undocumented" part are not at all undocumented -- in fact, they are the main API surface while before_exec is deprecated!

Similarly, for the second "impl CommandExt for Command" (for Windows), the only item is also folded as "hidden undocumented".

So there is certainly a bug here where some methods are hidden and others are not, for no apparent reason. But beyond that, I think hiding them at all is a bug in the case noted above: Note that I already clicked on the [+] to even see the trait methods! Then it shows nothing except for some scary note about undocumented items, and now I have to click a second time and ignore the factually incorrect message to see the documentation I am looking for. How can this be intended behavior?

I understand not unfold trait impls per default as there can be many of them, but do they really have to be hidden behind two clicks?

@phil-opp
Copy link
Contributor

phil-opp commented Jul 8, 2019

I think it would be more intuitive to also hide functions with default implementations when they're not overridden. Yes, they're technically documented since there is some documentation on the default implementation, but there is no "overridden" documentation inside the impl block.

Consider the following example on why the current behavior is confusing:

pub trait Foo {
    /// Do something
    fn do_something(&self);

    /// Do something twice
    fn do_something_twice(&self) {
        self.do_something(); self.do_something();
    }
}

pub struct Baz;

impl Foo for Baz {
    /// Do something special since this is a Baz instance and bla bla…
    fn do_something(&self) {
        println!("Foo");
    }
}

The generated docs look like this:

image

The current implementation shows the default method do_something_twice in the same way as the method do_something that has "overridden" documentation.

This leads to confusing docs, especially when no implementations contain "overridden" documentation:

pub struct Bar;

impl Foo for Bar {
    fn do_something(&self) {
        println!("Bar");
    }
}

Now do_something is hidden as "undocumented", but do_something_twice is still shown:

image

The user that sees this documentation doesn't know if do_something_twice does something special and thus has "overridden" documentation, or if it's just a simple default implementation.

@GuillaumeGomez
Copy link
Member

@phil-opp Can you open a specific issue about this one please? (and ping me on it)

@phil-opp
Copy link
Contributor

phil-opp commented Jul 8, 2019

@GuillaumeGomez Sure! I opened #62499 for this.

@GuillaumeGomez
Copy link
Member

Thanks! :)

@eminence
Copy link
Contributor Author

Here's another example of a headache with the current behavior:

  1. Browse to https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html
  2. Use your browser find dialog (ctrl-f) and search for fn as_ref
  3. There will be no results, which I found confusing because I'm pretty sure such a function exists
  4. Click the "Collapse all docs" button, and then click the "expand all docs" button.
  5. Search again for fn as_ref and again find nothing.
  6. Be annoyed that "expand all docs" didn't actually expand all docs.

@ghost
Copy link

ghost commented Apr 13, 2020

I also find it a bit confusing, I wanted to look up the count() function on Chars, but I did not realize that I have to look at the Iterator trait documentation. I also not found the (i) hint left from the function signature on the return type of std::String::chars(). As a beginner this was confusing and a bit time consuming to find what I wanted to know.

@birkenfeld
Copy link
Contributor

At the very least, it would be nice not to label the hidden methods as "undocumented" (which they aren't) but as "documented in the trait" or somesuch.

"Undocumented" suggests "I won't need them anyway".

@codesections
Copy link

codesections commented Apr 13, 2020

@Amityville-DE mentioned the "Important traits" icon to the left of the String::chars() method, which (when clicked) notes that Iterator is an important trait for Chars. This shows that Rustdoc already has some notion of "important traits".

Specifically, a trait is "important" if it is documented with the (currently unstable) #[doc(spotlight)] attribute; for the standard library, this currently applies to Iterator, io::Read, io::Write, and Future.

Given that, would it be possible to show the documentation for important traits, and hide it for other traits? That might present a bit of a compromise and prevent some of the confusion mentioned above.

EDIT: Actually, it looks like the #[doc(spotlight)] attribute is being/has been removed, #69514. So it's probably not a good idea to tie a solution here to that feature. Never mind, then!

@aloucks
Copy link
Contributor

aloucks commented May 10, 2020

The generated rustdocs seem to have regressed over the past few years.

For example the cgmath Point3 docs from January of 2019 have the traits expanded by default and the traits from the crate seem to be sorted to the top:

image

Today, there there appears to be more traits referenced in the docs but nothing is expanded and finding the traits from the crate is difficult:

image

(Scrolling all the way down....)

image

@kangalio
Copy link

kangalio commented Jul 7, 2020

In my MIDI device interfacing library I'm using traits with default functions to share the code for common capabilities between device

trait FunctionalitySet1 {
	/// Elaborate documentation...
	fn do_something(&mut self) {
		// ...
	}

	// ... more methods, all documented here within the trait
}

trait FunctionalitySet2 {
	/// Elaborate documentation...
	fn do_fancy_thing(&mut self) {
		// ...
	}

	// ... more methods, all documented here within the trait
}

trait FunctionalitySet3 {
	/// Elaborate documentation...
	fn do_something_exotic(&mut self) {
		// ...
	}

	// ... more methods, all documented here within the trait
}

// Depending on which capability sets each hardware device supports, we can just implement the
// corresponding traits.

struct DeviceA { /* ... */ }
impl FunctionalitySet1 for DeviceA {}
impl FunctionalitySet2 for DeviceA {}

struct DeviceB { /* ... */ }
impl FunctionalitySet3 for DeviceB {}

struct DeviceC { /* ... */ }
impl FunctionalitySet1 for DeviceC {}
impl FunctionalitySet3 for DeviceC {}

This approach is really great for concisely modeling hardware with shared functionality. It's elegant and completely eliminates duplicate code across devices.

But unfortunately, rustdoc yields really bad results on this kind of code. The documentation is super scattered and incredibly hard to find. Look at how much work and page navigation needs to be done just to see the documentation on the methods of DeviceA: https://streamable.com/nhrqrr

What to do instead?

The proposal by @agrif seems ideal to me. It would allow you to annotate traits with #[doc(inline)] - similar to how it's already possible with pub use. When a trait is annotated with #[doc(inline)], its functions are not hidden under a "show undocumented items" dropdown, but directly embedded in the implementor's documentation.

#[doc(inline)]
trait FunctionalitySet1 {
	// ...
}

#[doc(inline)]
trait FunctionalitySet2 {
	// ...
}

#[doc(inline)]
trait FunctionalitySet3 {
	// ...
}

// ...

Below is a shopped image to demonstrate what the rustdoc output should look like, after applying #[doc(inline)] to the traits
image

And here, again, the video showing how convoluted it is right now: https://streamable.com/nhrqrr

Or explore for yourself!

Final words

This writeup was quite some work.

I would greatly appreciate if my #[doc(inline)] idea for traits was seriously taken into consideration.

@agrif
Copy link

agrif commented Jul 7, 2020

This seems like it would be a reasonable first project -- what is the process for getting this done? Just a PR, or is there some other thing needed first?

@GuillaumeGomez
Copy link
Member

Sorry, I just got in the conversation. The reason behind the fact that there isn't all the documentation there is simply because we had very huge browser issues (just imagine the Debug trait docs). The performances were terribly bad and this is why we simply kept as little as possible.

@aloucks
Copy link
Contributor

aloucks commented Jul 8, 2020

The reason behind the fact that there isn't all the documentation there is simply because we had very huge browser issues (just imagine the Debug trait docs).

There's two issues here though.

  1. There's vastly more trait impls showing up in the docs, but the current crate (i.e. the crate that you ran cargo docs on) trait impls are non prioritized - they show up towards the end of the list.
  2. The hidden docs make the overall documentation less useful. You can't really find or use the trait docs as it exists today.

Could rustdoc be made smart enough to prioritize trait impl docs from the current crate (i.e. the crate you ran cargo docs on) so that:

  1. Those trait docs are sorted to the top of the impls list
  2. The method for those docs are never hidden

@GuillaumeGomez
Copy link
Member

This is not optimal in my opinion. It would seem very weird that some impls have full documentation while others don't. This is just not coherent. Also, impls are sorted alphabetically and I don't want to change it. You can see the hidden documentation by clicking on the trait, which is an annoying extra step but remains an acceptable compromise. Still better than having your browser crashing when trying to get a page.

@birkenfeld
Copy link
Contributor

@GuillaumeGomez what about my minimal suggestion to change "undocumented" to "documented in trait"?

@GuillaumeGomez
Copy link
Member

That wording however would be a great improvement so I totally support it. 👍 (sorry, I caught up really quickly with this thread)

@kangalio
Copy link

kangalio commented Jul 8, 2020

I'll try to summarize a bit:

Problem: Showing all trait functions' documentation even the ones that aren't explicitly documented in the implementor's code file, leads to overblown page sizes and browser crashes.

Possible solutions:

  1. Don't show those functions, ever (current system)
  2. Only show those functions if they come from the current crate (proposed by aloucks)
  3. Only show those functions if the crate was annotated with #[doc(inline)] (my proposal)
  4. Only show those functions if the impl Trait for Struct block was annotated with #[doc(inline)] (another idea of mine)

1 has the advantage that there's no special treatment of any kind; as GuillaumeGomez said: It would seem very weird that some impls have full documentation while others don't. This is just not coherent. Also, impls are sorted alphabetically and I don't want to change it. I personally don't care at all about this mild incoherence, plus these existing features are not coherent either, but they still exist, fortunately. But I suppose Guillaume does mind, so I'm including it.

The disadvantage of 1 is that, well, it makes it almost impossible to navigate the docs of a trait-heavy crate (like embedded_hal apparently). This difficulty is what I tried to get across in my writeup comment above.

Any of the solutions 2, 3 and 4 solve the big navigation problems that 1 has.

So, weighing advantages against disadvantages; I strongly prefer navigable docs in trait-heavy crates, even if it does come with mild incoherence issues (and we've accepted those in the past already, why not now).

@GuillaumeGomez
Copy link
Member

If you're talking about not hiding functions inside the "show undocumented items" thingy, it can be discussed. But adding more content to the pages will bring browser issues and on that we can't do much unfortunately (we're talking about HTML more than 20MB in size, maybe even more, I need to find the original issue).

@kangalio
Copy link

kangalio commented Jul 8, 2020

I see. Solution 4 gives you control over the amount of embedded trait documentations on a given page, and with such, over the page size. What do you think about that?

@GuillaumeGomez
Copy link
Member

The whole std impls are using inline, so again, not possible. :-/

@kangalio
Copy link

kangalio commented Jul 8, 2020

What do you mean "are using inline"? The feature I'm proposing doesn't exist yet, how can they be using it

@GuillaumeGomez
Copy link
Member

Sorry, I made the confusion with #[inline]. I have to admit that this might be "badly" used and end up in going back to the initial issue: having way too big HTML because you have used it on the wrong one. Also, since you have to mark each impl you don't want "stripped", it increases the difficulty of the code's maintenance. But anyway, what do you think @rust-lang/rustdoc ?

@kangalio
Copy link

kangalio commented Jul 8, 2020

I have to admit that this might be "badly" used and end up in going back to the initial issue: having way too big HTML because you have used it on the wrong one.

I'd argue that this is on the crate author. If the crate author explicitly instructs rustdoc to embed trait documentation by blindly sticking #[doc(inline)] on all their trait implementation, they are misusing the tool and it's their fault that rustdoc spits out a huge file.

In any case, it would also be possible to gate this functionality behind a flag or something. That makes it even more unlikely that users mistakenly abuse #[doc(inline)].

Also, since you have to mark each impl you don't want "stripped", it increases the difficulty of the code's maintenance.

That's a valid concern. This is also why initially I preferred solution 3 over 4: with solution 3, #[doc(inline)] is placed on the trait definition, not the implementation. As traits are implemented way more often than they are defined, this would cut down on the #[doc(inline)] clutter.

However, the disadvantage of solution 3 is that the trait implementor has no control over the size of their rustdoc output. If they happen to have implemented many #[doc(inline)] traits, their html file will be overblown - with no way to fix it.

Weighing advantages against disadvantages:

  • solution 4: potential #[doc(inline)] clutter
  • solution 3: the risk of overblown doc files with no chance of resolving it

I'd prefer solution 4 (#[doc(inline)] on trait implementation) over 3 (#[doc(inline)] on trait definition).

Also, explicitness is generally good - and solution 4 is definitely more explicit and obvious than the slightly-magic solution 3.

@eminence
Copy link
Contributor Author

eminence commented Jul 8, 2020

I think we have to trust documentation writers to be responsible users of the tools given to them. There is always the possibility of abuse. But if that happens, then we must educate the crate author about best practices for documenting their code so that their end users have the best experience.

I am also a fan of #[doc(inline)] or something like it, because it gives more control to documentation writers.

I suspect that stdlib might not make use of this feature, and so I would like to reiterate the point in my original issue: Describing important methods like clone() as "hidden undocumented" is confusing, and so I strongly support the idea to reword this text as "documented in trait" or something similar.

@GuillaumeGomez
Copy link
Member

I feel like we're back in the early debates of Rust when C and C++ developers were arguing that it was up to the developers to not mismanage memory and pointers. :p

My stand here is as follow: we should strongly limit any possibilities of letting developers do something wrong. And I think that adding #[doc(inline)] would definitely break this stance. But this is also why I asked other people for the rustdoc team to give their opinion.

@birktj
Copy link

birktj commented Jul 8, 2020

Given the case where the trait is not expanded by default would it at least be possible to automatically expand the hidden items when expanding the trait. Usually nothing is explicitly documented so one needs to click two times for anything to appear. I find this quite annoying and it doesn't seem to serve any purpose. If this change is supported I would like create a PR.

@aloucks
Copy link
Contributor

aloucks commented Jul 8, 2020

I feel like we're back in the early debates of Rust when C and C++ developers were arguing that it was up to the developers to not mismanage memory and pointers. :p

I don' think that's really the case. We're just saying that the old behavior was more useful, although it had other issues. Finding the best of both worlds seems to be the challenge.

I'm not terribly fond of having to opt-in to better documentation via an attribute.

Only show those functions if they come from the current crate (proposed by aloucks)

I think the current behavior is that they are hidden unless you override the docs. I'm suggesting that foreign implementations (that don't override the docs or methods) could be collapsed by default while local implementations would always be expanded by default. The later half is the only real change that I'm proposing. I think that the first part is how it currently works.

@birktj
Copy link

birktj commented Jul 9, 2020

In general I think a better solution would simply be to have trait impls contracted by default. And showing all items when expanding them. This will lead to even shorter pages than today and would in my opinion be more intuitive and require less clicking. Then if a user wants to see everything they can simply press the expand all button.

I think the other issue with the Read more button and large html sizes is mostly separate from this one and I don't really mind it.

@GuillaumeGomez
Copy link
Member

In general I think a better solution would simply be to have trait impls contracted by default. And showing all items when expanding them.

Just to be sure: when you say this, you only mean with the current content right? Because if not, the problem is simply that the HTML is too big, whether or not it's displayed.

@birktj
Copy link

birktj commented Jul 9, 2020

Yes, that is correct. I don't mind having to navigate to a new page to see the full documentation and the HTML issue is quite understandable.

@GuillaumeGomez
Copy link
Member

Ok, on this we can definitely try to do things better.

@eminence
Copy link
Contributor Author

eminence commented Feb 8, 2023

These days, the latest docs no longer hide the fn clone() function as a "hidden undocumented" item. This directly solves the original complaint in my original issue. Additionally, the rustdocs have naturally evolved over the intervening years, so most of the discussion in this thread are about old versions that are no longer around. Thus I'm going to close this issue as "Done". Thank you everyone.

@eminence eminence closed this as completed Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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