rustdoc: add new "Implementations on Foreign Types" section to traits #43849

QuietMisdreavus · 2017-08-13T18:04:22Z

Demo screenshot:

Full demo available at https://tonberry.quietmisdreavus.net/foreign-test/foreign_test/trait.CheckIt.html

This PR splits the "Implementors" section on trait pages into two: First, for impls on types local to the crate, their impls are kept as-is, printing one line for the impl line, and any additional lines for associated types. However, for types external to the crate, they are now pulled up over the others and are printed (almost) like the summary impl on the type page itself. This gives any doc comments on these impls or methods to be exposed in the documentation.

There's just one small problem, though: libstd docs apparently surface impls for libc and rand, possibly among others. This adds this section to pages in the std docs where we might not want them to show up in the first place. I think this is a bug distinct from this PR, but it does make it drastically apparent.

My question, then, is this: Do we want this here? Taking it out involves fixing which impls are visible to rustdoc, possibly specifically when rendering the std facade. I'm convinced this is fine to land as-is, since it adds a feature specifically for non-std crates (i'm thinking of things like num or related crates that implement things on primitives or std types as part of their functionality). (EDIT: I have an open PR to fix this: #44026)

rust-highfive · 2017-08-13T18:04:26Z

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

QuietMisdreavus · 2017-08-13T18:04:44Z

r? @GuillaumeGomez

cc @rust-lang/docs @rust-lang/dev-tools

kennytm · 2017-08-13T19:02:34Z

I don't see why impl for foreign types needs to have a separate section. The new style can be applied to local types as well.

If the methods of the traits are also shown, please add the ability to [-] the whole impl to hide every associated thing in it. Imagine the repetition when reading num_traits::PrimInt...

GuillaumeGomez · 2017-08-13T19:42:00Z

I'm not sure to understand as well. So I'm not against it, but a great big explanation would be appreciated.

frewsxcv · 2017-08-13T20:03:03Z

src/librustdoc/html/render.rs

+
+            for implementor in foreign {
+                // need to get from a clean::Impl to a clean::Item so i can use render_impl
+                if let Some(t_did) = implementor.impl_.for_.def_id() {


alternatively, i think the body of this block could be something like:

if let Some(impl_item) = impls .find(|i| i.impl_item.def_id == implementor.def_id) .next() { let i = &implementor.impl_; let impl_ = Impl { impl_item: impl_item.clone() }; let assoc_link = AssocItemLink::GotoSource( impl_item.def_id, &i.provided_trait_methods ); render_impl( w, cx, &impl_, assoc_link, RenderMode::Normal, impl_item.stable_since(), false )?; }

Ah right, i guess i did manually rewrite find there. Here's what i got:

for implementor in foreign { // need to get from a clean::Impl to a clean::Item so i can use render_impl if let Some(t_did) = implementor.impl_.for_.def_id() { if let Some(impl_item) = cache.impls.get(&t_did).and_then(|i| i.iter() .find(|i| i.impl_item.def_id == implementor.def_id)) { let i = &impl_item.impl_item; let impl_ = Impl { impl_item: i.clone() }; let assoc_link = AssocItemLink::GotoSource( i.def_id, &implementor.impl_.provided_trait_methods ); render_impl(w, cx, &impl_, assoc_link, RenderMode::Normal, i.stable_since(), false)?; } } }

QuietMisdreavus · 2017-08-13T22:13:59Z

The main reason behind this is what's sketched out in the opening screenshot: If you want to apply doc comments to an impl or to a method in an impl, it will only show up if the type being impl'd is a local type. This PR is an attempt to surface doc comments from impls on non-local types, by printing the full impl (sans "default" comments from the trait itself) for those types that aren't part of the local crate. It allows more documentation to be written and surfaced.

QuietMisdreavus · 2017-08-14T16:01:41Z

@kennytm I just added a change to auto-hide impl items when printing in this manner.

Demo screenshot:

Full demo available at https://tonberry.quietmisdreavus.net/foreign-test/foreign_test/trait.CheckIt.html

arielb1 · 2017-08-22T22:39:07Z

Does this look ready for landing @GuillaumeGomez ?

kennytm · 2017-08-23T06:52:15Z

@QuietMisdreavus The placement of "Expand description" seems strange on Firefox 54.0.1. Otherwise LGTM.

GuillaumeGomez · 2017-08-23T07:54:05Z

@arielb1: Remains some ui issues now. :)

QuietMisdreavus · 2017-08-23T14:07:07Z

@kennytm That's what I opened #43862 for. When this is merged in with that, it'll look fine. Would you like me to merge my fork forward so i can include that change?

kennytm · 2017-08-23T14:12:05Z

@QuietMisdreavus I see. Either way is fine.

QuietMisdreavus · 2017-08-23T20:11:53Z

We took a look at this in the docs team meeting today (since three of the four members are also dev-tool peers for rustdoc >_>) and were generally positive, but wanted some time to consider the utility of the change.

To restate the intent of this PR:

Currently, you can apply doc comments to impl blocks or individual impl items, and the comments will appear when the full impl is rendered on the type's page. However, when you implement a trait on a type that's not part of the crate, that impl is never fully rendered. Thus, those doc comments are lost. This PR aims to fix this, by rendering the "full" trait impls when the type in question isn't part of the crate. This way, these doc comments have a place where they can be visible, and we have an opportunity to write even more docs! :3

carols10cents · 2017-08-28T14:26:11Z

but wanted some time to consider the utility of the change.

@QuietMisdreavus how much time? what considerations are being made? What I'm really asking is, what should the infra team PR triagers do with this PR? :)

QuietMisdreavus · 2017-08-28T14:30:59Z

@carols10cents I guess the most blunt thing to do would be to ping @steveklabnik and get this on his queue since he was the only one to express hesitation on it. Looking back at the log it seemed more like he wanted time to properly review it more than anything.

carols10cents · 2017-08-28T14:37:22Z

Sounds good! let's make it official then :)

r? @steveklabnik

bors · 2017-09-01T16:40:28Z

☔ The latest upstream changes (presumably #44238) made this pull request unmergeable. Please resolve the merge conflicts.

steveklabnik · 2017-09-05T15:09:16Z

Seems good to me, though it's now out of date.

QuietMisdreavus · 2017-09-05T18:31:17Z

I've rebased to fix the merge conflict.

steveklabnik · 2017-09-05T19:19:04Z

@bors: r+

bors · 2017-09-05T19:19:05Z

📌 Commit a9f0f6d has been approved by steveklabnik

bors · 2017-09-05T19:56:39Z

⌛ Testing commit a9f0f6d with merge f2cdf2fb4df529d6eea7aa607eb6de0eff7c5891...

bors · 2017-09-05T20:30:12Z

💔 Test failed - status-travis

alexcrichton · 2017-09-05T21:07:21Z

@bors: retry

good ol' network troubles

bors · 2017-09-06T03:12:11Z

⌛ Testing commit a9f0f6d with merge fc76d3ac85265db732ebe2bbfe5ed1b84c7209bc...

bors · 2017-09-06T04:14:21Z

💔 Test failed - status-travis

alexcrichton · 2017-09-06T04:46:52Z

@bors: retry

network travis issues

bors · 2017-09-06T05:12:33Z

⌛ Testing commit a9f0f6d with merge 09b101b4a490ed430aa01361c2da20655e4b3a71...

bors · 2017-09-06T06:54:11Z

💔 Test failed - status-travis

kennytm · 2017-09-06T10:40:31Z

@bors retry

check x86_64-apple-darwin no output in 30 minutes etc 😴.

bors · 2017-09-06T11:24:24Z

⌛ Testing commit a9f0f6d with merge a209539...

rustdoc: add new "Implementations on Foreign Types" section to traits Demo screenshot: ![image](https://user-images.githubusercontent.com/5217170/29281219-c547f758-80e3-11e7-808f-49f592c65c5b.png) Full demo available at https://tonberry.quietmisdreavus.net/foreign-test/foreign_test/trait.CheckIt.html This PR splits the "Implementors" section on trait pages into two: First, for impls on types local to the crate, their impls are kept as-is, printing one line for the impl line, and any additional lines for associated types. However, for types external to the crate, they are now pulled up over the others and are printed (almost) like the summary impl on the type page itself. This gives any doc comments on these impls or methods to be exposed in the documentation. There's just one small problem, though: [libstd docs apparently surface impls for libc and rand, possibly among others](https://tonberry.quietmisdreavus.net/foreign-std/std/marker/trait.Copy.html#foreign-impls). This adds this section to pages in the std docs where we might not want them to show up in the first place. I think this is a bug distinct from this PR, but it does make it drastically apparent. ~~My question, then, is this: Do we want this here? Taking it out involves fixing which impls are visible to rustdoc, possibly specifically when rendering the std facade. I'm convinced this is fine to land as-is, since it adds a feature specifically for non-std crates (i'm thinking of things like `num` or related crates that implement things on primitives or std types as part of their functionality).~~ (EDIT: I have an open PR to fix this: #44026)

bors · 2017-09-06T14:14:12Z

☀️ Test successful - status-appveyor, status-travis
Approved by: steveklabnik
Pushing a209539 to master...

rust-highfive assigned steveklabnik Aug 13, 2017

rust-highfive assigned GuillaumeGomez and unassigned steveklabnik Aug 13, 2017

frewsxcv reviewed Aug 13, 2017

View reviewed changes

QuietMisdreavus force-pushed the foreign-impls branch 2 times, most recently from d616e36 to 6406c92 Compare August 13, 2017 23:24

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2017

rust-highfive assigned steveklabnik and unassigned GuillaumeGomez Aug 28, 2017

QuietMisdreavus added 3 commits September 5, 2017 11:21

rustdoc: add new "Implementations on Foreign Types" section to traits

30e5268

if not showing default docs on an impl, autohide the items

b357a9c

add "Implementations on Foreign Types" to the sidebar

a9f0f6d

QuietMisdreavus force-pushed the foreign-impls branch from fb15ef5 to a9f0f6d Compare September 5, 2017 16:21

bors merged commit a9f0f6d into rust-lang:master Sep 6, 2017

QuietMisdreavus deleted the foreign-impls branch February 26, 2018 23:25

kennytm mentioned this pull request May 27, 2018

rustdoc: The default style of items inside "Implementations on Foreign Types" is very ugly #51113

Closed

kzys mentioned this pull request Sep 14, 2018

rustdoc: Sort implementors #53941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: add new "Implementations on Foreign Types" section to traits #43849

rustdoc: add new "Implementations on Foreign Types" section to traits #43849

QuietMisdreavus commented Aug 13, 2017 •

edited

Loading

rust-highfive commented Aug 13, 2017

QuietMisdreavus commented Aug 13, 2017

kennytm commented Aug 13, 2017

GuillaumeGomez commented Aug 13, 2017

frewsxcv Aug 13, 2017 •

edited

Loading

QuietMisdreavus Aug 13, 2017

QuietMisdreavus commented Aug 13, 2017

QuietMisdreavus commented Aug 14, 2017 •

edited

Loading

arielb1 commented Aug 22, 2017 •

edited

Loading

kennytm commented Aug 23, 2017

GuillaumeGomez commented Aug 23, 2017

QuietMisdreavus commented Aug 23, 2017

kennytm commented Aug 23, 2017

QuietMisdreavus commented Aug 23, 2017

carols10cents commented Aug 28, 2017 •

edited

Loading

QuietMisdreavus commented Aug 28, 2017

carols10cents commented Aug 28, 2017

bors commented Sep 1, 2017

steveklabnik commented Sep 5, 2017

QuietMisdreavus commented Sep 5, 2017

steveklabnik commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

alexcrichton commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

kennytm commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

rustdoc: add new "Implementations on Foreign Types" section to traits #43849

rustdoc: add new "Implementations on Foreign Types" section to traits #43849

Conversation

QuietMisdreavus commented Aug 13, 2017 • edited Loading

rust-highfive commented Aug 13, 2017

QuietMisdreavus commented Aug 13, 2017

kennytm commented Aug 13, 2017

GuillaumeGomez commented Aug 13, 2017

frewsxcv Aug 13, 2017 • edited Loading

Choose a reason for hiding this comment

QuietMisdreavus Aug 13, 2017

Choose a reason for hiding this comment

QuietMisdreavus commented Aug 13, 2017

QuietMisdreavus commented Aug 14, 2017 • edited Loading

arielb1 commented Aug 22, 2017 • edited Loading

kennytm commented Aug 23, 2017

GuillaumeGomez commented Aug 23, 2017

QuietMisdreavus commented Aug 23, 2017

kennytm commented Aug 23, 2017

QuietMisdreavus commented Aug 23, 2017

carols10cents commented Aug 28, 2017 • edited Loading

QuietMisdreavus commented Aug 28, 2017

carols10cents commented Aug 28, 2017

bors commented Sep 1, 2017

steveklabnik commented Sep 5, 2017

QuietMisdreavus commented Sep 5, 2017

steveklabnik commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

bors commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

alexcrichton commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

kennytm commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

QuietMisdreavus commented Aug 13, 2017 •

edited

Loading

frewsxcv Aug 13, 2017 •

edited

Loading

QuietMisdreavus commented Aug 14, 2017 •

edited

Loading

arielb1 commented Aug 22, 2017 •

edited

Loading

carols10cents commented Aug 28, 2017 •

edited

Loading