show in docs whether the return type of a function impls Iterator/Read/Write

[QuietMisdreavus]

Closes#25928

This PR makes it so that when rustdoc documents a function, it checks the return type to see whether it implements a handful of specific traits. If so, it will print the impl and any associated types. Rather than doing this via a whitelist within rustdoc, i chose to do this by a new #[doc] attribute parameter, so things like Future could tap into this if desired.

Known shortcomings

The printing of impls currently uses the where class over the whole thing to shrink the font size relative to the function definition itself. Naturally, when the impl has a where clause of its own, it gets shrunken even further: (This is no longer a problem because the design changed and rendered this concern moot.)

The lookup currently just looks at the top-level type, not looking inside things like Result or Option, which renders the spotlights on Read/Write a little less useful:

`File::{open, create}` don't have spotlight info (pic of old design)

All three of the initially spotlighted traits are generically implemented on &mut references. Rustdoc currently treats a &mut T reference-to-a-generic as an impl on the reference primitive itself. &mut Self counts as a generic in the eyes of rustdoc. All this combines to create this lovely scene on Iterator::by_ref:

`Iterator::by_ref` spotlights Iterator, Read, and Write (pic of old design)

[rust-highfive]

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

[rust-highfive]

r? @GuillaumeGomez

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

[QuietMisdreavus]

@rust-lang/docs Bikeshed request: Is this layout okay? Also which of the "Known shortcomings" above should i work to resolve before merging this, and which are okay to keep around while the spotlight parameter is still unstable?

[GuillaumeGomez]

I don't like it. It feels like the function is implementing all this. We should try to think about how to give this information but not by default (the display is already very heavy). Maybe when we hover a little symbol close to the returned type? Or maybe even when hovering the returned type directly?

[QuietMisdreavus]

If it's a matter of the layout on the final page, that can easily be changed. The problem with making it a hover on the return type directly is that it's not very discoverable. It's already a bit of a secret that those type names are links, so it feels like adding something that's not visible or easily explained would render this change much less useful. Making it a new icon (and putting the impl in a hover off that) at least provides some affordance, but risks being easily overlooked.

There is some precedent here with the "warning" icon on unsafe functions, and now the extra CSS on ignored doc blocks. I'm not too familiar with how something like that would be implemented, if we want the links in this impl to be clickable? Maybe putting it in a separate docblock that's automatically hidden? That may create some strange interactions with the toggle wrapper, but saves on a bit of the noise you're talking about.

(EDIT: Also i force-pushed to fix the test that broke travis.)

[QuietMisdreavus]

New travis error: Turns out i was generating links wrong and created a lot of broken links. I think i have a fix, but i'm running linkchecker locally to make sure i solved it.

[bors]

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

[QuietMisdreavus]

Force-pushed to resolve linkchecker and the merge conflict.

[QuietMisdreavus]

I tried to put the trait impls into an auto-hide docblock, but i think the js that creates the toggle-wrapper got confused by something in this setup, since it didn't work:

What does everyone think about that "Important traits for Filter" heading? Would that help solve some of the ambiguity?

[GuillaumeGomez]

I have a lot of difficulties to appreciate this PR so I think I'll just let someone else handles all this.

[carols10cents]

Sounds like this pr is waiting for some feedback from @rust-lang/docs members? I'm going to change this to S-waiting-for-team, please adjust if that's not where this is at!

[QuietMisdreavus]

We did talk about this in the docs team meeting last week, and it seemed like everyone would be better on board with this if the way it was laid out on the page was cleaned up some. I need to dig into the "toggle wrapper" logic in the rustdoc javascript to see if i can get my last screenshot to work the way i wanted, where it was foldable and automatically hidden. Haven't taken the chance to do that, though.

[bors]

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

[carols10cents]

ping @QuietMisdreavus, looks like there are some merge conflicts in addition to the changes you wanted to make!

[QuietMisdreavus]

I think I cracked it. The main.js was only applying toggle wrappers to .docblocks that were direct children of the main element, so my docblock-within-a-docblock wasn't working. Tweaking the wrapper code a little now makes it work, and this is how it looks now:

@rust-lang/docs What do you think about this?

[QuietMisdreavus]

I asked @GuillaumeGomez about the latest design during today's docs team meeting, and he offered to help out with designing a better display for this, basing it on the existing modal shown for the ? menu. As long as it doesn't blur out the background like the ? menu does - it's not so important as to drown out literally everything else on the page - that could work out.

[GuillaumeGomez]

Will write the modal code starting monday.

[QuietMisdreavus]

imperio and i just walked through the design of the new spotlight modal, and here's what we came up with:

Associated methods get a little circle-i next to their toggle wrapper if they return something that implements a spotlighted trait:

Example with `slice::{reverse, iter, iter_mut}`

Bare functions get the circle-i next to their definition:

Example with `std::iter::once`

Clicking the circle-i opens up a modal window that shows the impl(s) of any spotlighted traits on the return type:

"Important traits for `Once<T>`

The circle-i also shows up on trait pages:

Several examples from Iterator with docs collapsed

@GuillaumeGomez Before i start doing fine-grain code review, i have one more suggestion: Right now there's no title attribute on the circle-i. Could we put one in there, with the same text as the heading inside the modal, just so there's some more indication what it's there for, before clicking?

[QuietMisdreavus]

Also, uh, it looks like htmldocck.py choked on the output? I'm not quite sure what's going on here.

[00:54:41] ---- [rustdoc] rustdoc/doc-spotlight.rs stdout ----
[00:54:41] [00:54:41] error: htmldocck failed!
[00:54:41] status: exit code: 1
[00:54:41] command: "/usr/bin/python2.7" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/doc-spotlight.stage2-x86_64-unknown-linux-gnu" "/checkout/src/test/rustdoc/doc-spotlight.rs"
[00:54:41] stdout:
[00:54:41] ------------------------------------------
[00:54:41] [00:54:41] ------------------------------------------
[00:54:41] stderr:
[00:54:41] ------------------------------------------
[00:54:41] Traceback (most recent call last):
[00:54:41] File "/checkout/src/etc/htmldocck.py", line 455, in <module>
[00:54:41] check(sys.argv[1], get_commands(sys.argv[2]))
[00:54:41] File "/checkout/src/etc/htmldocck.py", line 448, in check
[00:54:41] check_command(c, cache)
[00:54:41] File "/checkout/src/etc/htmldocck.py", line 397, in check_command
[00:54:41] tree = cache.get_tree(c.args[0])
[00:54:41] File "/checkout/src/etc/htmldocck.py", line 309, in get_tree
[00:54:41] raise RuntimeError('Cannot parse an HTML file {!r}: {}'.format(path, e))
[00:54:41] RuntimeError: Cannot parse an HTML file 'doc_spotlight/trait.SomeTrait.html': pop from empty stack
[00:54:41] [00:54:41] ------------------------------------------

[GuillaumeGomez]

Maybe a missing closing tag? I'll investigate. Also, I was thinking about the title as well. Can you come up with a text to put while I work on the title design?

[QuietMisdreavus]

@GuillaumeGomez I didn't want it to be a tooltip like this, i literally meant the title attribute, to use standard hover-text. Also, you didn't print the type name. I wanted "Important traits for {return type}", the exact same text as the heading. Although now that i see it, i think something like "Show important traits for {type}" may be better, since it's a link.

Also the doc-spotlight test failed:

[00:58:27] ---- [rustdoc] rustdoc/doc-spotlight.rs stdout ----
[00:58:27] [00:58:27] error: htmldocck failed!
[00:58:27] status: exit code: 1
[00:58:27] command: "/usr/bin/python2.7" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/doc-spotlight.stage2-x86_64-unknown-linux-gnu" "/checkout/src/test/rustdoc/doc-spotlight.rs"
[00:58:27] stdout:
[00:58:27] ------------------------------------------
[00:58:27] [00:58:27] ------------------------------------------
[00:58:27] stderr:
[00:58:27] ------------------------------------------
[00:58:27] 22: @has check failed
[00:58:27] `XPATH PATTERN` did not match
[00:58:27] // @has - '//code[@class="spotlight"]' 'impl<T: SomeTrait> SomeTrait for Wrapper<T>'
[00:58:27] 35: @has check failed
[00:58:27] `XPATH PATTERN` did not match
[00:58:27] // @has - '//code[@class="spotlight"]' 'impl SomeTrait for SomeStruct'
[00:58:27] 36: @has check failed
[00:58:27] `XPATH PATTERN` did not match
[00:58:27] // @has - '//code[@class="spotlight"]' 'impl<T: SomeTrait> SomeTrait for Wrapper<T>'
[00:58:27] 43: @has check failed
[00:58:27] `XPATH PATTERN` did not match
[00:58:27] // @has - '//code[@class="spotlight"]' 'impl SomeTrait for SomeStruct'
[00:58:27] [00:58:27] Encountered 4 errors

[GuillaumeGomez]

Ok, fixed the test and added the wanted thing in the tooltip.

[QuietMisdreavus]

Okay, so this is how it looks now:

I think at this point i'm ready to call it good? The tooltip having a link in there is awkward, and possibly leads to bad contrast levels depending on what font gets pulled. (On a different machine it used the sans-serif font from the headers? Not sure what's going on on this one.) If it leads to problems, the fix is literally two additional characters, so i'm not all that concerned. Here we gooooooo~

@bors r=GuillaumeGomez,QuietMisdreavus

[bors]

📌 Commit 6047a03 has been approved by GuillaumeGomez,QuietMisdreavus

[bors]

⌛ Testing commit 6047a03 with merge 421a211...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez,QuietMisdreavus
Pushing 421a211 to master...