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

jyn514 · 2021-12-18T17:07:33Z

This has almost no effect in practice, since most places were using unwrap_or_default,
and the rest were doing checks that should have been checking for an empty string.

The only change in behavior is that the JSON backend no longer distinguishes
"undocumented" and "documented with the empty string". This doesn't seem like a particularly useful distinction,
but I can add it back for that code only if you think it's important.

Builds on #92077. cc #91072 (comment)

cc @CraftSpider @HeroicKatora
r? @camelid

rust-highfive · 2021-12-18T17:07:36Z

rustdoc-json-types is a public (although nightly-only) API. Consider changing src/librustdoc/json/conversions.rs instead; otherwise, make sure you update format_version.

cc @CraftSpider,@aDotInTheVoid

Some changes occurred in clean/types.rs.

cc @camelid

jyn514 · 2021-12-18T17:13:20Z

cc @GuillaumeGomez since you said in #91072 (comment) you thought the distinction was useful, although I'm not quite sure why ... IMO the new explicit checks are more clear.

rustdoc: Write doc-comments directly instead of using FromIterator The FromIterator impl made the code much harder to understand. The types don't make sense until you realize there's a custom FromIterator impl. This is the first commit from rust-lang#91305; since `@camelid` wrote it originally I don't feel bad unilaterally approving it. r? `@ghost` `@bors` r+ Note that this will conflict with rust-lang#92078.

GuillaumeGomez · 2021-12-18T19:15:58Z

It's because of how the code is written: you can forget to check that a string is new but you can't forget to check if an Option is Some.

jyn514 · 2021-12-18T19:35:38Z

@GuillaumeGomez right, but often you don't need to check if it's empty or not. I encourage you to look at the diff: very few places actually keep the check and those that do only keep it as a small optimization.

GuillaumeGomez · 2021-12-18T19:45:50Z

I agree with you on that but the problem is not more the majority but for the minority. It's not a hard blocker for me, but still something that needs to be considered. I relied on the compiler to avoid that whereas you rely on the developer. It's a paradigm shift.

jyn514 · 2021-12-18T20:17:43Z

@GuillaumeGomez if it helps, another motivation for this was making collapsed_doc_value more consistent with doc_value: right now collapsed* treats it as "no lines" but doc_value" treats it as "no text".

GuillaumeGomez · 2021-12-18T20:29:45Z

Leet's just say I'm not convinced but you seem to be sure so like I said: I'll remain neutral.

camelid · 2021-12-18T20:44:38Z

src/librustdoc/doctest.rs

-        if let Some(doc) = attrs.collapsed_doc_value() {
+        // `doc_value()` doesn't combine sugared/raw doc attributes, this will combine them for us.
+        let docs = attrs.collapsed_doc_value();
+        if !docs.is_empty() {


Are we sure this slight behavior change here and elsewhere doesn't matter? Before, this checked for if there were no DocFragments; now, it checks if the collapsed docs is an empty string.

I think it's probably "more correct" now, but I just wanted to double-check.

If you look at https://github.com/rust-lang/rust/blob/c7476adc38bc9348398afcf24117d7d48e7ba9f1/src/librustdoc/html/markdown.rs#L732, it doesn't do anything for empty strings any way; having a check here at all is a microoptimization we could probably remove. So the change shouldn't have an impact.

But this code is looking for doctests, not rendering Markdown.

I don't think there should be a behavior change, but there is a very slight possibility.

This if calls find_testable_code immediately below. The parser there is parsing the code for doctests.

rustdoc: Write doc-comments directly instead of using FromIterator The FromIterator impl made the code much harder to understand. The types don't make sense until you realize there's a custom FromIterator impl. This is the first commit from rust-lang#91305; since ``@camelid`` wrote it originally I don't feel bad unilaterally approving it. r? ``@ghost`` ``@bors`` r+ Note that this will conflict with rust-lang#92078.

aDotInTheVoid · 2021-12-19T10:11:22Z

src/rustdoc-json-types/lib.rs

-    /// 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>,
+    /// The full markdown docstring of this item.


The only change in behavior is that the JSON backend no longer distinguishes
"undocumented" and "documented with the empty string". This doesn't seem like a particularly useful distinction,
but I can add it back for that code only if you think it's important.

This change seems good to me, but I think this sould be added to the docs field documentation.

Also their should probably be a test for this

Sure thing, done :)

This has almost no effect in practice, since most places were using `unwrap_or_default`, and the rest were doing checks that *should* have been checking for an empty string. The only change in behavior is that the JSON backend no longer distinguishes "undocumented" and "documented with the empty string". This doesn't seem like a particularly useful distinction, but I can add it back for that code only if you think it's important.

This also fixes a few bugs in jsondocck and makes some of its errors easier to read.

rust-log-analyzer · 2021-12-20T17:52:03Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
error[E0432]: unresolved import `crate::clean::collapse_doc_fragments`
 --> src/librustdoc/passes/unindent_comments/tests.rs:3:5
  |
3 | use crate::clean::collapse_doc_fragments;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `collapse_doc_fragments` in `clean`
For more information about this error, try `rustc --explain E0432`.
error: could not compile `rustdoc` due to previous error



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rustdoc/Cargo.toml" "-p" "rustdoc:0.0.0" "--" "--quiet"


Build completed unsuccessfully in 0:30:56

bors · 2021-12-21T19:48:47Z

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

jyn514 · 2022-01-08T02:45:30Z

ditto #92079 (comment)

jyn514 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 Dec 18, 2021

rust-highfive assigned camelid Dec 18, 2021

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

jyn514 force-pushed the no-option-collapsed-docs branch from d418cfe to c7476ad Compare December 18, 2021 17:08

This was referenced Dec 18, 2021

rustdoc: Return String from doc_value, not Option #92079

Closed

rustdoc: Write doc-comments directly instead of using FromIterator #92082

Merged

jyn514 changed the title ~~Return a String from collapsed_doc_value, not an Option~~ rustdoc: Return a String from collapsed_doc_value, not an Option Dec 18, 2021

camelid added S-blocked Status: Marked as 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 Dec 18, 2021

camelid reviewed Dec 18, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

aDotInTheVoid reviewed Dec 19, 2021

View reviewed changes

vacuus mentioned this pull request Dec 19, 2021

rustdoc: Remove 'need_backline' field from DocFragment #92095

Merged

jyn514 force-pushed the no-option-collapsed-docs branch from 3677299 to 026b1b3 Compare December 20, 2021 16:46

This comment has been minimized.

Sign in to view

jyn514 force-pushed the no-option-collapsed-docs branch from 026b1b3 to 2ef6ae2 Compare December 20, 2021 17:02

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

This comment has been minimized.

Sign in to view

Add test for json output

112ed0b

This also fixes a few bugs in jsondocck and makes some of its errors easier to read.

jyn514 force-pushed the no-option-collapsed-docs branch from 2ef6ae2 to 112ed0b Compare December 20, 2021 17:17

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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2021

jyn514 closed this Jan 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jyn514 commented Dec 18, 2021 •

edited

Loading

rust-highfive commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

camelid Dec 18, 2021

jyn514 Dec 18, 2021

camelid Dec 18, 2021

camelid Dec 18, 2021

jyn514 Dec 20, 2021

This comment has been minimized.

aDotInTheVoid Dec 19, 2021

jyn514 Dec 20, 2021

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Dec 20, 2021

bors commented Dec 21, 2021

jyn514 commented Jan 8, 2022

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

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

Conversation

jyn514 commented Dec 18, 2021 • edited Loading

rust-highfive commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

jyn514 commented Dec 18, 2021

GuillaumeGomez commented Dec 18, 2021

camelid Dec 18, 2021

Choose a reason for hiding this comment

jyn514 Dec 18, 2021

Choose a reason for hiding this comment

camelid Dec 18, 2021

Choose a reason for hiding this comment

camelid Dec 18, 2021

Choose a reason for hiding this comment

jyn514 Dec 20, 2021

Choose a reason for hiding this comment

This comment has been minimized.

aDotInTheVoid Dec 19, 2021

Choose a reason for hiding this comment

jyn514 Dec 20, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Dec 20, 2021

bors commented Dec 21, 2021

jyn514 commented Jan 8, 2022

jyn514 commented Dec 18, 2021 •

edited

Loading