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

Removes references to hidden elementes in rustdoc #66715

Closed
wants to merge 1 commit into from

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Nov 24, 2019

This is my first PR dealing with rustc/rustdoc internals, usual warnings apply 😅

This is a partial attempt to fix #13698. The approach is pretty blunt: we keep a reference to the defining item in a trait implementation, and we then strip all items with a "parent" marked as #[doc(hidden)]. It is not finished but I wanted to gather feedback early.

TODOs / Questions / Doubts / Alternatives

  • TODO: handle all trait items, not only Method.
  • Digging a bit, I see that the Clean implementation that I have modified is called during the collect_trait_impls pass. I'm not sure whether it would be better to try to tackle hidden elements at that point, instead of waiting to strip-hidden.
  • Is parent a good name for defining item? Maybe definition would be better?
  • Where should we place the parent field? Right now, I'm stuffing it into each ItemEnum variant, but maybe we could just add it to the Item struct.

@Dylan-DPC-zz
Copy link

r? @GuillaumeGomez

@Dylan-DPC-zz Dylan-DPC-zz added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2019
impl<'a> DocFolder for ReferencesStripper<'a> {
fn fold_item(&mut self, i: Item) -> Option<Item> {
if let clean::ItemEnum::MethodItem(ref method) = &i.inner {
if method.parent.map(|did| !self.retained.contains(&did)).unwrap_or(false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if method.parent.map(|did| !self.retained.contains(&did)).unwrap_or(false) {
if method.parent.map_or(false, |did| !self.retained.contains(&did)) {

impl issue_13698::Foo for Foo {}
// @!has - '//*[@id="method.foo2"]' 'fn foo2'
impl issue_13698::FooAux for Foo {
fn foo2(&self) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indent of 2 instead of 4. ;)

impl Bar for Foo {}
// @!has - '//*[@id="method.qux"]' 'fn qux'
impl Bar for Foo {
fn qux(&self) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indent of 2 instead of 4. ;)

@GuillaumeGomez
Copy link
Member

So far, code looks good to me (you even added tests!). However I wonder: if the child implementation of a trait wants to display a method implementation, how can it be done with this change? Currently, you have to re-use #[doc(hidden)] to hide it and I think this is the good way of handling it.

However, I'm open to other opinions on this topic. :)

@jrvidal
Copy link
Contributor Author

jrvidal commented Nov 25, 2019

if the child implementation of a trait wants to display a method implementation, how can it be done with this change? Currently, you have to re-use #[doc(hidden)] to hide it and I think this is the good way of handling it.

Oh, now I'm confused about the scope of the bug I'm supposed to be fixing 🙃 If I understood what you're saying, you are arguing that the current behavior is preferable since it is more flexible? I don't have a strong opinion, I assumed it was undesired because of the (repeated) chatter in #13698 about being unwanted/surprising.

If that's the case, I guess this PR does not make much sense, and that in cases like #51147 (comment) what we want is to explicitly mark the implementations with #[doc(hidden)].

@GuillaumeGomez
Copy link
Member

Well, my point was that you can hide elements but you can't "unhide" them. ;)

@jrvidal
Copy link
Contributor Author

jrvidal commented Dec 5, 2019

#13698 is a WONTFIX

@jrvidal jrvidal closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hidden methods still appear in trait implementation docs
5 participants