rustdoc: do not use plain summary for trait impls #73819

euclio · 2020-06-27T23:44:45Z

Fixes #38386.
Fixes #48332.
Fixes #49430.
Fixes #62741.
Fixes #73474.

Unfortunately this is not quite ready to go because the newly-working links trigger a bunch of linkcheck failures. The failures are tough to fix because the links are resolved relative to the implementor, which could be anywhere in the module hierarchy.

(In the current docs, these links end up rendering as uninterpreted markdown syntax, so I don't think these failures are any worse than the status quo. It might be acceptable to just add them to the linkchecker whitelist.)

Ideally this could be fixed with intra-doc links ~~but it isn't working for me: I am currently investigating if it's possible to solve it this way.~~ Opened #73829.

EDIT: This is now ready!

rust-highfive · 2020-06-27T23:44:48Z

r? @steveklabnik

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

jyn514 · 2020-06-28T14:28:23Z

src/librustdoc/html/markdown.rs

-                Some(String::new())
+    let mut s = String::with_capacity(md.len() * 3 / 2);
+
+    // FIXME: This doesn't handle intra-doc links: "[`std::io::Error`]" will go through unchanged.


You shouldn't need to handle them here, they're handled in a separate pass. See for example the links on https://docs.rs/rand/0.7.3/rand/#traits

src/librustdoc/html/markdown.rs

GuillaumeGomez · 2020-07-09T11:44:27Z

As far as I remember, the intra-links are currently not resolved in all shorten doc comments. As long as you don't change the current behavior, I think it's fine to fix it in another PR?

euclio · 2020-07-09T20:20:24Z

The problem is that linkcheck will break for basically anything that implements a trait with a link in the description, because there's no way to write a link that will work for all locations without intra-doc links.

GuillaumeGomez · 2020-07-09T20:22:10Z

Absolutely, that needs to be added and there is an issue open for this missing part. I started to take a look at it but some parts in intra-doc links were missing. Once @jyn514's big PR is merged, we'll be able to go back into it.

jyn514 · 2020-07-09T21:42:49Z

Once @jyn514's big PR is merged, we'll be able to go back into it.

See #43466 (comment) for status on that, it's currently waiting on #74195.

crlf0710 · 2020-07-24T14:19:44Z

Ping from triage. What's the current status? Is this blocked on #74430?

euclio · 2020-07-24T14:30:09Z

Blocked on #74489. Planning to add linkcheck exceptions for links that can't be resolved due to #73829.

jyn514 · 2020-07-24T15:59:00Z

I'm not sure I follow. Why do intra-doc links need to be stabilized for this to work? The relevant bugs in intra-doc links have been fixed, except for #73829.

jyn514 · 2020-07-24T15:59:30Z

Oh oops I didn't realized you linked to two different PRs. I'll try and work on #74489 sometime this weekend.

euclio · 2020-08-25T22:43:13Z

This is ready for another look. I put in a FIXME in linkcheck for the links that need #73829 to work.

src/tools/linkchecker/main.rs

jyn514 · 2020-08-25T22:57:20Z

A lot of the links you changed were also changed in #75832, #75852, and #75853. So you may want to wait until some of those land.

ollie27 · 2020-08-26T21:22:31Z

How about we filter out all links in short summaries when used for trait impls?

GuillaumeGomez · 2020-08-31T15:36:37Z

src/librustdoc/html/render/mod.rs

-        render_markdown(w, cx, &markdown, item.links(), prefix, is_hidden);
+        let mut summary_html = MarkdownSummaryLine(s, &item.links()).into_string();
+
+        if s.contains('\n') {


Does it take into account when you only have an indented code block like:

/// hello

?

GuillaumeGomez · 2020-08-31T15:37:23Z

src/test/rustdoc/trait-impl.rs

+
+    /// Escaped formatting a\*b\*c\* works
+    fn d();
+}


Please add a test with indented codeblock like this:

/// hello

GuillaumeGomez · 2020-08-31T15:38:00Z

Just one doubt about the indented codeblock but otherwise sounds good. Once the indented codeblock test has been added, it'll be good to go for me!

euclio · 2020-09-02T22:51:13Z

@GuillaumeGomez What's the expected behavior? unindent-comments trims the leading spaces so that

///    hello

is rendered as a paragraph, not a code block. There's nothing special about trait methods here.

GuillaumeGomez · 2020-09-02T22:53:33Z

@euclio It should be a codeblock. :o

euclio · 2020-09-02T22:55:40Z

My point is that

///    hello
pub struct Foo;

isn't rendered as a code block either.

GuillaumeGomez · 2020-09-02T22:59:47Z

Ah, I might have forgotten a whitespace. Codeblocks are 4 whitespaces, but with rustdoc we have to add a fifth one. Try again with 5. :)

Normally, it should render as a codeblock (it does with the commonmark spec), if not, we have a bug to report.

euclio · 2020-09-02T23:06:18Z

It's the same output with 4 and 5 spaces. I disagree that it's a bug, seems like expected behavior from the unindent-comments pass.

GuillaumeGomez · 2020-09-03T08:03:30Z

No, it's a bug for me: we can't transform a codeblock into text just like that.

jyn514 · 2020-09-03T12:32:31Z

@GuillaumeGomez you haven't answered the question. This is done for all items - what's special about trait methods that they shouldn't be counted? Why is unindenting relevant to this PR? If you think rustdoc shouldn't be doing it in general, that seems like it should be a separate fix.

GuillaumeGomez · 2020-09-03T12:35:06Z

Unless I misunderstood, it's part of plain_text_summary, no?

jyn514 · 2020-09-03T12:41:52Z

No, it's a separate pass.

rust/src/librustdoc/passes/mod.rs

Line 98 in b543afc

ConditionalPass::always(UNINDENT_COMMENTS),

GuillaumeGomez · 2020-09-03T12:43:24Z

Then I'm just confused. Apart from this, no issue for me.

src/librustdoc/html/markdown.rs

jyn514

LGTM

GuillaumeGomez · 2020-09-03T16:00:32Z

Thanks @euclio !

@bors: r=jyn514,GuillaumeGomez

bors · 2020-09-03T16:00:49Z

📌 Commit 98232ec has been approved by jyn514,GuillaumeGomez

bors · 2020-09-03T19:07:50Z

⌛ Testing commit 98232ec with merge 62dad45...

bors · 2020-09-03T21:20:13Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514,GuillaumeGomez
Pushing 62dad45 to master...

rust-highfive assigned steveklabnik Jun 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2020

This comment has been minimized.

Sign in to view

euclio force-pushed the rustdoc-summaries branch from 1c05711 to 8677281 Compare June 28, 2020 04:55

This comment has been minimized.

Sign in to view

jyn514 reviewed Jun 28, 2020

View reviewed changes

GuillaumeGomez reviewed Jul 9, 2020

View reviewed changes

src/librustdoc/html/markdown.rs Show resolved Hide resolved

crlf0710 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 Jul 24, 2020

This comment has been minimized.

Sign in to view

euclio force-pushed the rustdoc-summaries branch from 8677281 to 184772e Compare August 25, 2020 21:45

euclio changed the title ~~[WIP] rustdoc: do not use plain summary for trait impls~~ rustdoc: do not use plain summary for trait impls Aug 25, 2020

euclio force-pushed the rustdoc-summaries branch 2 times, most recently from cf63b25 to 6498e63 Compare August 25, 2020 22:42

jyn514 reviewed Aug 25, 2020

View reviewed changes

src/tools/linkchecker/main.rs Outdated Show resolved Hide resolved

rust-highfive assigned GuillaumeGomez Aug 31, 2020

GuillaumeGomez reviewed Aug 31, 2020

View reviewed changes

jyn514 reviewed Sep 3, 2020

View reviewed changes

src/librustdoc/html/markdown.rs Show resolved Hide resolved

jyn514 approved these changes Sep 3, 2020

View reviewed changes

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 Sep 3, 2020

bors mentioned this pull request Sep 3, 2020

diagnostics: shorten paths of unique symbols #73996

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2020

bors merged commit 62dad45 into rust-lang:master Sep 3, 2020

This was referenced Sep 3, 2020

Use more intra-doc-links in core::fmt #75853

Merged

Clean up rustdoc tests by moving them into one place #76223

Closed

euclio deleted the rustdoc-summaries branch September 4, 2020 00:13

jyn514 mentioned this pull request Sep 10, 2020

rustdoc: Item summaries have unrendered Markdown and sometimes don't show up #76553

Closed

cuviper added this to the 1.48.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: do not use plain summary for trait impls #73819

rustdoc: do not use plain summary for trait impls #73819

euclio commented Jun 27, 2020 •

edited

Loading

rust-highfive commented Jun 27, 2020

This comment has been minimized.

This comment has been minimized.

jyn514 Jun 28, 2020

GuillaumeGomez commented Jul 9, 2020

euclio commented Jul 9, 2020

GuillaumeGomez commented Jul 9, 2020

jyn514 commented Jul 9, 2020

crlf0710 commented Jul 24, 2020

euclio commented Jul 24, 2020 •

edited

Loading

jyn514 commented Jul 24, 2020

jyn514 commented Jul 24, 2020

This comment has been minimized.

euclio commented Aug 25, 2020

jyn514 commented Aug 25, 2020

ollie27 commented Aug 26, 2020

GuillaumeGomez Aug 31, 2020

GuillaumeGomez Aug 31, 2020

GuillaumeGomez commented Aug 31, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 2, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 2, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 commented Sep 3, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 commented Sep 3, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 left a comment

GuillaumeGomez commented Sep 3, 2020

bors commented Sep 3, 2020

bors commented Sep 3, 2020

bors commented Sep 3, 2020

rustdoc: do not use plain summary for trait impls #73819

rustdoc: do not use plain summary for trait impls #73819

Conversation

euclio commented Jun 27, 2020 • edited Loading

rust-highfive commented Jun 27, 2020

This comment has been minimized.

This comment has been minimized.

jyn514 Jun 28, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 9, 2020

euclio commented Jul 9, 2020

GuillaumeGomez commented Jul 9, 2020

jyn514 commented Jul 9, 2020

crlf0710 commented Jul 24, 2020

euclio commented Jul 24, 2020 • edited Loading

jyn514 commented Jul 24, 2020

jyn514 commented Jul 24, 2020

This comment has been minimized.

euclio commented Aug 25, 2020

jyn514 commented Aug 25, 2020

ollie27 commented Aug 26, 2020

GuillaumeGomez Aug 31, 2020

Choose a reason for hiding this comment

GuillaumeGomez Aug 31, 2020

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 31, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 2, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 2, 2020

euclio commented Sep 2, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 commented Sep 3, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 commented Sep 3, 2020

GuillaumeGomez commented Sep 3, 2020

jyn514 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 3, 2020

bors commented Sep 3, 2020

bors commented Sep 3, 2020

bors commented Sep 3, 2020

euclio commented Jun 27, 2020 •

edited

Loading

euclio commented Jul 24, 2020 •

edited

Loading