Add empty impl blocks if they have documentation #90905

GuillaumeGomez · 2021-11-14T16:16:26Z

The update for the test script is needed to count the number of impl blocks we have with only the struct. To be noted that with #89676 merged, it wouldn't be needed (I don't know what is the status of it btw. cc @Mark-Simulacrum).

It looks like this:

cc @jyn514
r? @camelid

GuillaumeGomez · 2021-11-14T17:23:55Z

Every time I run cargo fmt on librustdoc I get this error...

camelid · 2021-11-14T21:13:53Z

Sorry, I likely won't be able to review this for some time. r? @jyn514

GuillaumeGomez · 2021-11-20T21:09:58Z

r? @jsha

jyn514 · 2021-11-24T15:59:26Z

Every time I run cargo fmt on librustdoc I get this error...

You need to use x.py fmt src/librustdoc.

jyn514 · 2021-11-24T16:02:24Z

@GuillaumeGomez why did you add a warning note to the documentation? It seems like unnecessary clutter, you can already see it's empty just by looking at it.

jyn514 · 2021-11-24T16:05:06Z

The update for the test script is needed to count the number of impl blocks we have with only the struct. To be noted that with #89676 merged, it wouldn't be needed (I don't know what is the status of it btw).

Can you explain this a little more? Why was the existing test framework insufficient? How would #89676 have helped?

GuillaumeGomez · 2021-11-24T16:39:54Z

Every time I run cargo fmt on librustdoc I get this error...

You need to use x.py fmt src/librustdoc.

Absolutely, just the force of habit. :)

@GuillaumeGomez why did you add a warning note to the documentation? It seems like unnecessary clutter, you can already see it's empty just by looking at it.

Because I found that this suggestion made sense. But we can talk more about it if you think it's not useful. (cc @jsha too)

Can you explain this a little more? Why was the existing test framework insufficient? How would #89676 have helped?

Sure, I needed to count the number of items with this text. Currently we can't count how many items have this text, only how many items have this attribute or how many items with this XPath exist. With #89676, I could have used the *[text()="some text"] directly into the XPath and count the number of items matching it. Currently, we can't use *[text() = "..."] to filter items because of the limitations of the tool we use.

jyn514 · 2021-11-24T17:37:54Z

src/librustdoc/html/render/mod.rs

@@ -1634,6 +1634,18 @@ fn render_impl(
        }

        if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) {
+            if render_mode == RenderMode::Normal


Hmm, shouldn't it be impossible for render_mode to be RenderMode::Deref and for trait.is_some() to be true at the same time, since the recursive deref check only documents inherent impls? Why check both?

I wonder if we could combine those somehow maybe? Or maybe it makes more sense to show traits that the deref target implements 🤔

It's already checked just a few lines above so completely unneeded, you're absolutely right.

As for showing the traits on the Deref types, I think it would just be too much information (or we will need to think how to display them). But that's for another time. 😄 Maybe open an issue so we can talk about it later on?

src/librustdoc/html/render/mod.rs

src/librustdoc/passes/stripper.rs

src/test/rustdoc/empty-impl-block.rs

GuillaumeGomez · 2021-12-02T09:47:28Z

Updated!

bors · 2021-12-19T15:55:35Z

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

jsha · 2022-05-01T16:51:41Z

Earlier, @jyn514 said:

why did you add a warning note to the documentation? It seems like unnecessary clutter, you can already see it's empty just by looking at it.

and @GuillaumeGomez replied:

Because I found that #90866 (comment) made sense. But we can talk more about it if you think it's not useful. (cc @jsha too)

I see a 👍🏻 from @jyn514 on the linked comment so I'm taking that as agreement that the warning for empty impl blocks is a good idea.

jsha

It's not clear to me why a new kind of matcher in the test infrastructure is needed. Can you explain that more?

src/etc/htmldocck.py

src/librustdoc/html/render/mod.rs

src/librustdoc/passes/stripper.rs

GuillaumeGomez · 2022-05-03T09:33:40Z

It's not clear to me why a new kind of matcher in the test infrastructure is needed. Can you explain that more?

It's because the current XPath support is very bad. You can't do: //*[text() = "whatever"] and I need to count elements with a given text. Hence why I added this new case. To fix the XPath handling, I have #89676 open.

GuillaumeGomez · 2022-05-03T10:25:58Z

Updated!

bors · 2022-05-28T04:13:33Z

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

…the @count command

GuillaumeGomez · 2022-05-31T12:12:41Z

Updated!

jsha · 2022-06-05T06:06:11Z

@bors r+ rollup

GuillaumeGomez · 2022-06-05T15:22:44Z

Weird, didn't work.

@bors=jsha rollup

GuillaumeGomez · 2022-06-05T15:23:02Z

And I failed the command of course. ^^'

@bors r=jsha rollup

bors · 2022-06-05T15:23:04Z

📌 Commit eca12e3 has been approved by jsha

…=jsha Add empty impl blocks if they have documentation Fixes rust-lang#90866. The update for the test script is needed to count the number of impl blocks we have with only the struct. To be noted that with rust-lang#89676 merged, it wouldn't be needed (I don't know what is the status of it btw. cc `@Mark-Simulacrum).` It looks like this: ![Screenshot from 2021-11-14 16-51-28](https://user-images.githubusercontent.com/3050060/141689100-e57123c0-bf50-4c42-adf5-d991e169a0e4.png) cc `@jyn514` r? `@camelid`

…=jsha Add empty impl blocks if they have documentation Fixes rust-lang#90866. The update for the test script is needed to count the number of impl blocks we have with only the struct. To be noted that with rust-lang#89676 merged, it wouldn't be needed (I don't know what is the status of it btw. cc ``@Mark-Simulacrum).`` It looks like this: ![Screenshot from 2021-11-14 16-51-28](https://user-images.githubusercontent.com/3050060/141689100-e57123c0-bf50-4c42-adf5-d991e169a0e4.png) cc ``@jyn514`` r? ``@camelid``

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#90905 (Add empty impl blocks if they have documentation) - rust-lang#97683 (Fail gracefully when encountering an HRTB in APIT. ) - rust-lang#97721 (Do `suggest_await_before_try` with infer variables in self, and clean up binders) - rust-lang#97752 (typo: `-Zcodegen-backend=llvm -Cpasses=list` should work now) - rust-lang#97759 (Suggest adding `{}` for `'label: non_block_expr`) - rust-lang#97764 (use strict provenance APIs) - rust-lang#97765 (Restore a test that was intended to test `as` cast to ptr) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned camelid Nov 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2021

GuillaumeGomez changed the title ~~Empty impl blocks~~ Add empty impl blocks if they have documentation Nov 14, 2021

GuillaumeGomez added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 14, 2021

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the empty-impl-blocks branch from b1b5389 to 52c86d5 Compare November 14, 2021 17:23

rust-highfive assigned jyn514 and unassigned camelid Nov 14, 2021

rust-highfive assigned jsha and unassigned jyn514 Nov 20, 2021

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2021

jyn514 reviewed Nov 24, 2021

View reviewed changes

camelid reviewed Nov 24, 2021

View reviewed changes

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

camelid reviewed Nov 24, 2021

View reviewed changes

src/librustdoc/passes/stripper.rs Outdated Show resolved Hide resolved

camelid reviewed Nov 24, 2021

View reviewed changes

src/test/rustdoc/empty-impl-block.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the empty-impl-blocks branch from 52c86d5 to f518d89 Compare December 2, 2021 09:46

GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2021

GuillaumeGomez force-pushed the empty-impl-blocks branch from f518d89 to 65e4288 Compare January 6, 2022 14:07

jsha requested changes May 2, 2022

View reviewed changes

src/etc/htmldocck.py Show resolved Hide resolved

src/etc/htmldocck.py Show resolved Hide resolved

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

src/librustdoc/passes/stripper.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the empty-impl-blocks branch 2 times, most recently from 2eebfbd to 4511d24 Compare May 3, 2022 10:25

GuillaumeGomez added 4 commits May 31, 2022 14:02

Display empty impl blocks if they have documentations

5881fd5

Update test script for src/test/rustdoc to allow to add a filter for …

198dea5

…the @count command

Add test for empty impl blocks

51dd090

Fix theme checks

eca12e3

GuillaumeGomez force-pushed the empty-impl-blocks branch from 4511d24 to eca12e3 Compare May 31, 2022 12:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2022

matthiaskrgr mentioned this pull request Jun 5, 2022

Rollup of 5 pull requests #97766

Closed

Dylan-DPC mentioned this pull request Jun 6, 2022

Rollup of 5 pull requests #97781

Closed

matthiaskrgr mentioned this pull request Jun 6, 2022

Rollup of 7 pull requests #97783

Merged

bors merged commit 77f0209 into rust-lang:master Jun 6, 2022

rustbot added this to the 1.63.0 milestone Jun 6, 2022

GuillaumeGomez deleted the empty-impl-blocks branch June 6, 2022 11:42

aDotInTheVoid mentioned this pull request May 14, 2023

rustdoc: Inconsistant decision to include impl with docs but no visible items #111564

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add empty impl blocks if they have documentation #90905

Add empty impl blocks if they have documentation #90905

GuillaumeGomez commented Nov 14, 2021

This comment has been minimized.

GuillaumeGomez commented Nov 14, 2021

camelid commented Nov 14, 2021

GuillaumeGomez commented Nov 20, 2021

jyn514 commented Nov 24, 2021 •

edited

Loading

jyn514 commented Nov 24, 2021

jyn514 commented Nov 24, 2021 •

edited

Loading

GuillaumeGomez commented Nov 24, 2021

jyn514 Nov 24, 2021

GuillaumeGomez Dec 2, 2021

GuillaumeGomez Dec 2, 2021

GuillaumeGomez commented Dec 2, 2021

bors commented Dec 19, 2021

jsha commented May 1, 2022

jsha left a comment

GuillaumeGomez commented May 3, 2022

GuillaumeGomez commented May 3, 2022

bors commented May 28, 2022

GuillaumeGomez commented May 31, 2022

jsha commented Jun 5, 2022

GuillaumeGomez commented Jun 5, 2022

GuillaumeGomez commented Jun 5, 2022

bors commented Jun 5, 2022

Add empty impl blocks if they have documentation #90905

Add empty impl blocks if they have documentation #90905

Conversation

GuillaumeGomez commented Nov 14, 2021

This comment has been minimized.

GuillaumeGomez commented Nov 14, 2021

camelid commented Nov 14, 2021

GuillaumeGomez commented Nov 20, 2021

jyn514 commented Nov 24, 2021 • edited Loading

jyn514 commented Nov 24, 2021

jyn514 commented Nov 24, 2021 • edited Loading

GuillaumeGomez commented Nov 24, 2021

jyn514 Nov 24, 2021

Choose a reason for hiding this comment

GuillaumeGomez Dec 2, 2021

Choose a reason for hiding this comment

GuillaumeGomez Dec 2, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 2, 2021

bors commented Dec 19, 2021

jsha commented May 1, 2022

jsha left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 3, 2022

GuillaumeGomez commented May 3, 2022

bors commented May 28, 2022

GuillaumeGomez commented May 31, 2022

jsha commented Jun 5, 2022

GuillaumeGomez commented Jun 5, 2022

GuillaumeGomez commented Jun 5, 2022

bors commented Jun 5, 2022

jyn514 commented Nov 24, 2021 •

edited

Loading

jyn514 commented Nov 24, 2021 •

edited

Loading