rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

camelid · 2021-11-20T04:11:27Z

There actually seems to be no difference.

r? @GuillaumeGomez
cc @jyn514

They seem to be very similar; I think they don't need to be separate.

They're unneeded now that `doc_value()` and `collapsed_doc_value()` are the same.

rust-highfive · 2021-11-20T04:11:30Z

Some changes occurred in intra-doc-links.

cc @jyn514

camelid · 2021-11-20T04:12:11Z

Might as well see if the Crate shrinkage improved perf.

@bors try @rust-timer queue

rust-timer · 2021-11-20T04:12:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-20T04:12:22Z

⌛ Trying commit 157d1db9b17ae9637cf92273bab45da869e22b78 with merge 68b8e770b5409b48c66862743fdff57a63ae8bfd...

jyn514

I'm not sure this has the same behavior ... some things are obviously a win like turning FromIterator into a function, but I would like to see more examples of the behavior for different doc types before and after.

jyn514 · 2021-11-20T04:26:15Z

src/librustdoc/clean/types.rs

-        let mut iter = self.doc_strings.iter();
-
-        let ori = iter.next()?;
-        let mut out = String::new();
-        add_doc_fragment(&mut out, ori);
-        for new_frag in iter {
-            if new_frag.kind != ori.kind || new_frag.parent_module != ori.parent_module {
-                break;
-            }
-            add_doc_fragment(&mut out, new_frag);
-        }
-        if out.is_empty() { None } else { Some(out) }
+        self.collapsed_doc_value()


This is not the same behavior. The FromIterator impl below is missing the new_frag.kind != ori.kind || new_frag.parent_module != ori.parent_module check.

I'm not sure if it matters in practice, but it seems like something we should have tests for ... could you try and find one or two, or add a new one if not?

Before merging collapsed and un-collapsed, I tried removing one part of this conditional and all the tests passed. I'll try removing the whole conditional too (with no other changes) to see what happens.

I agree it'd be good to have more test coverage. I'll see if I can come up with some test cases.

All tests pass with master + this diff:

diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 41ebf270ba6..5a02a6c6259 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1083,9 +1083,6 @@ fn update_need_backline(doc_strings: &mut Vec<DocFragment>) { let mut out = String::new(); add_doc_fragment(&mut out, ori); for new_frag in iter { - if new_frag.kind != ori.kind || new_frag.parent_module != ori.parent_module { - break; - } add_doc_fragment(&mut out, new_frag); } if out.is_empty() { None } else { Some(out) }

So either that conditional is useless or we don't have any test coverage for it.

Adding enough test coverage around this code to know for sure that nothing is changing will take a long time. What about if I open a PR to remove this conditional, we land it, and wait to see if any regressions pop up? If there are any regressions, then we can do a quick revert since it's just a few lines. If we don't find any regressions, then we can move forward with a bigger change.

Hmm, I'm ok with that, but let's wait until after the beta branch on friday.

Opened #91305.

jyn514 · 2021-11-20T04:30:14Z

src/librustdoc/clean/types.rs

@@ -1083,7 +1080,11 @@ impl Attributes {
    /// Finds all `doc` attributes as NameValues and returns their corresponding values, joined
    /// with newlines.
    crate fn doc_value(&self) -> Option<String> {


not necessarily in this pr, but it would be nice to remove the Option here

I think we check the is_some to generate differently the doc block. From my point of view, this forces us to keep this check so might be better to keep it.

I don't really understand why None is treated differently from Some("") though. What if someone writes #[doc = ""]? Won't that return Some("") instead of None? And why do we need to special-case the absence of doc attrs so much?

Another difference between doc_value and collapsed_doc_value - doc_value returns None if the combined string is empty, collapsed_doc_value returns None if there are no lines. The difference matters if there's empty lines, like below:

/// /// /// pub fn foo() {}

@CraftSpider I see the following comment in rustdoc-json-types:

/// The full markdown docstring of this item. Absent if there is no documentation at all, /// Some("") if there is some documentation but it is empty (EG `#[doc = ""]`). pub docs: Option<String>,

Is that an important distinction? Or can I just make that field a simple String?

bors · 2021-11-20T05:21:54Z

💔 Test failed - checks-actions

src/librustdoc/clean/types.rs

GuillaumeGomez · 2021-11-20T11:13:56Z

Let's restart the perf check:

@bors try @rust-timer queue

rust-timer · 2021-11-20T11:13:58Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-20T11:14:04Z

⌛ Trying commit 157d1db9b17ae9637cf92273bab45da869e22b78 with merge c5a55b2dfca7383f1932664496103fe59a330016...

bors · 2021-11-20T12:31:38Z

💔 Test failed - checks-actions

camelid · 2021-11-20T15:30:08Z

Ugh, it'll be nice to have cargo check --doc 😅

error: unresolved link to `DocFragments`
32468
   --> src/librustdoc/clean/types.rs:947:32
32469
    |
32470
947 | /// Collapse a collection of [`DocFragments`] into one string,
32471
    |                                ^^^^^^^^^^^^ no item named `DocFragments` in scope
32472
    |
32473
    = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
32474
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
32475

The FromIterator was only used once and made the code much harder to understand. The types don't make sense until you realize there's a custom FromIterator impl.

bors · 2021-11-20T15:40:26Z

⌛ Trying commit 1891548 with merge 33fc15df6db1037b483198fb978ecdd109b9776d...

bors · 2021-11-20T17:15:24Z

☀️ Try build successful - checks-actions
Build commit: 33fc15df6db1037b483198fb978ecdd109b9776d (33fc15df6db1037b483198fb978ecdd109b9776d)

rust-timer · 2021-11-20T17:15:25Z

Queued 33fc15df6db1037b483198fb978ecdd109b9776d with parent 3d78974, future comparison URL.

rust-timer · 2021-11-20T18:52:44Z

Finished benchmarking commit (33fc15df6db1037b483198fb978ecdd109b9776d): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

GuillaumeGomez · 2021-11-20T20:12:37Z

Very small improvements but overall we're moving in the right direction! :D

camelid · 2021-11-28T01:37:25Z

Blocked on #91305.

I need to remove this conditional for rust-lang#91072, but while it seems unnecessary, we are not certain. So, the plan is to first remove the conditional and see if any regressions pop up before doing the refactor. This way, it will be easier to revert if there are subtle regressions.

rustdoc: Remove apparently unnecessary conditional in `doc_value` I need to remove this conditional for rust-lang#91072, but while it seems unnecessary, we are not certain. So, the plan is to first remove the conditional and see if any regressions pop up before doing the refactor. This way, it will be easier to revert if there are subtle regressions. r? `@jyn514`

I need to remove this conditional for rust-lang#91072, but while it seems unnecessary, we are not certain. So, the plan is to first remove the conditional and see if any regressions pop up before doing the refactor. This way, it will be easier to revert if there are subtle regressions.

camelid · 2022-03-06T01:32:59Z

The PR this has been blocked on, #91305, is on stable as of a week and a half ago. As of yet, I don't know of any regressions that have been reported. So, I think in a month or two, we should be able to merge this PR as well!

Dylan-DPC · 2022-04-07T21:48:18Z

@camelid this is waiting on a rebase

camelid · 2022-04-07T22:03:28Z

Thanks, it's on my list. I might not get to it for a bit since there are so many conflicts.

JohnCSimon · 2022-11-27T01:50:50Z

@camelid
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

camelid added 3 commits November 19, 2021 22:34

rustdoc: Make doc_value() dispatch to collapsed_doc_value()

7be69ba

They seem to be very similar; I think they don't need to be separate.

Remove doc_value() and strip collapsed_ prefix

da26b51

rustdoc: Remove {Crate,SharedContext}.collapsed

22dc5c4

They're unneeded now that `doc_value()` and `collapsed_doc_value()` are the same.

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 20, 2021

rust-highfive assigned GuillaumeGomez Nov 20, 2021

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2021

jyn514 reviewed Nov 20, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

bors 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 20, 2021

GuillaumeGomez reviewed Nov 20, 2021

View reviewed changes

src/librustdoc/clean/types.rs Show resolved Hide resolved

GuillaumeGomez approved these changes Nov 20, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

camelid added 2 commits November 20, 2021 10:31

Write code directly instead of using FromIterator

8be135b

The FromIterator was only used once and made the code much harder to understand. The types don't make sense until you realize there's a custom FromIterator impl.

Remove outdated comment

1891548

camelid force-pushed the collapsed branch from 157d1db to 1891548 Compare November 20, 2021 15:31

camelid 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 Nov 20, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 20, 2021

jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2021

camelid mentioned this pull request Nov 28, 2021

rustdoc: Remove apparently unnecessary conditional in doc_value #91305

Merged

This was referenced Dec 18, 2021

rustdoc: Return a String from collapsed_doc_value, not an Option #92078

Closed

rustdoc: Return String from doc_value, not Option #92079

Closed

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 1, 2022

JohnCSimon closed this Nov 27, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

camelid commented Nov 20, 2021 •

edited

Loading

rust-highfive commented Nov 20, 2021

camelid commented Nov 20, 2021

rust-timer commented Nov 20, 2021

bors commented Nov 20, 2021

jyn514 left a comment

jyn514 Nov 20, 2021

camelid Nov 20, 2021

camelid Nov 20, 2021

camelid Nov 24, 2021

jyn514 Nov 24, 2021

camelid Nov 28, 2021

jyn514 Nov 20, 2021

GuillaumeGomez Nov 20, 2021

camelid Nov 20, 2021

jyn514 Dec 18, 2021 •

edited

Loading

jyn514 Dec 18, 2021

This comment has been minimized.

bors commented Nov 20, 2021

GuillaumeGomez commented Nov 20, 2021

rust-timer commented Nov 20, 2021

bors commented Nov 20, 2021

This comment has been minimized.

bors commented Nov 20, 2021

camelid commented Nov 20, 2021

bors commented Nov 20, 2021

bors commented Nov 20, 2021

rust-timer commented Nov 20, 2021

rust-timer commented Nov 20, 2021

GuillaumeGomez commented Nov 20, 2021

camelid commented Nov 28, 2021

camelid commented Mar 6, 2022

Dylan-DPC commented Apr 7, 2022

camelid commented Apr 7, 2022

JohnCSimon commented Nov 27, 2022

rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

Conversation

camelid commented Nov 20, 2021 • edited Loading

rust-highfive commented Nov 20, 2021

camelid commented Nov 20, 2021

rust-timer commented Nov 20, 2021

bors commented Nov 20, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Nov 20, 2021

GuillaumeGomez commented Nov 20, 2021

rust-timer commented Nov 20, 2021

bors commented Nov 20, 2021

This comment has been minimized.

bors commented Nov 20, 2021

camelid commented Nov 20, 2021

bors commented Nov 20, 2021

bors commented Nov 20, 2021

rust-timer commented Nov 20, 2021

rust-timer commented Nov 20, 2021

GuillaumeGomez commented Nov 20, 2021

camelid commented Nov 28, 2021

camelid commented Mar 6, 2022

Dylan-DPC commented Apr 7, 2022

camelid commented Apr 7, 2022

JohnCSimon commented Nov 27, 2022

camelid commented Nov 20, 2021 •

edited

Loading

jyn514 Dec 18, 2021 •

edited

Loading