rustdoc: Strip broken links in summaries #79781

camelid · 2020-12-07T04:27:05Z

The primary reason to do this is because intra-doc links are treated as
"broken" in the summary since the resolution context is not available.
Previously, search results with intra-doc links would look like:

This method returns an [Ordering]

but now they look like:

This method returns an Ordering

as search results with regular links do.

The one drawback I can see is if people are using [ and ] literally,
but I think that case is much rarer than using intra-doc links, and they
can and should escape the brackets with a backslash or use inline code.
Plus, this is just for summaries.

r? @jyn514

camelid · 2020-12-07T04:29:02Z

Before

After

camelid · 2020-12-07T04:29:52Z

Before

After

src/librustdoc/html/markdown.rs

jyn514 · 2020-12-07T14:40:15Z

src/librustdoc/html/markdown.rs

+    // NOTE: Make sure to update the same variable in `plain_text_summary`
+    //       if/when you update this one. They have to be duplicated because of a typesystem thing.
+    let mut broken_link_callback =
+        |broken_link: BrokenLink<'_>| Some(("#".into(), broken_link.reference.to_owned().into()));


This treats every broken link as valid, right? Can we instead use the same logic as for intra-doc links and only replace it if it was resolved?

Well we could, but we’d have to thread the intra-doc link information through somehow. If you would like me to do that, could you give me some instructions?

This info is available from item.attrs.links: https://doc.rust-lang.org/nightly/nightly-rustc/rustdoc/clean/types/struct.Attributes.html#structfield.links. It looks like that's not currently threaded through to short_markdown_summary, but everywhere that calls summary calls it on an Item. I'd suggest making this a method on Item instead and calling item.short_markdown_summary() instead, which gives you access to all the info on the item.

See

rust/src/librustdoc/html/markdown.rs

Line 365 in 5019791

// Replace intra-doc links and remove disambiguators from shortcut links (`[fn@f]`).

for an example of actually using the link - I don't expect this to be as complicated since it's just stripping the link altogether, and only for [] style links.

Hmm, will I then I have to duplicate broken_link_callback as a closure after all?

I don't see why? You're stripping the links in both cases, right?

It's been a while since we last discussed this. It looks like the last thing we talked about is letting their be a little duplication and moving the summary functions to be on Item?

It's coming back to me now: The reason I temporarily abandoned this is because I was having trouble with the lifetimes of the callback for the summary functions not on Item. I think it might have been the dreaded higher-ranked subtype error. I'll post the error if/when I get it.

So I tried this type signature:

fn markdown_summary_with_limit( md: &str, length_limit: usize, broken_link_callback: Option<F>, ) -> (String, bool) where F: for<'a> FnMut(BrokenLink<'_>) -> Option<(CowStr<'a>, CowStr<'a>)>, { /* ... */ }

but that gives a bunch of errors like:

error[E0582]: binding for associated type `Output` references lifetime `'a`, which does not appear in the trait input types --> src/librustdoc/html/markdown.rs:1030:41 | 1030 | F: for<'a> FnMut(BrokenLink<'_>) -> Option<(CowStr<'a>, CowStr<'a>)>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What are the correct lifetimes? I feel like this needs higher-ranked lifetimes (which I added) but as you can see it doesn't work. I need to communicate to the compiler that the return type lifetimes are totally unrelated from the input lifetimes.

@jyn514 friendly ping :)

If you need to focus on other stuff, don't worry about looking at this now, but you seemed eager to get this working.

Try F: FnMut(BrokenLink<'_>) -> Option<(CowStr<'static>, CowStr<'static>)>. That has the same meaning without introducing a new lifetime.

jyn514 · 2020-12-07T14:40:41Z

Also, please add a test for this.

camelid · 2020-12-26T18:12:27Z

Hmm, why did I mark this PR as blocked?

jyn514 · 2021-02-15T02:09:27Z

@camelid friendly ping :) I think this would be a nice feature to have, happy to help out if you need it.

camelid · 2021-02-15T02:20:46Z

Yeah, I've been trying to catch up on my S-waiting-on-author PRs. This one is on my list :)

The primary reason to do this is because intra-doc links are treated as "broken" in the summary since the resolution context is not available. Previously, search results with intra-doc links would look like: > This method returns an \[`Ordering`\] but now they look like: > This method returns an `Ordering` as search results with regular links do. The one drawback I can see is if people are using `[` and `]` literally, but I think that case is much rarer than using intra-doc links, and they can and should escape the brackets with a backslash or use inline code. Plus, this is just for summaries.

camelid · 2021-02-16T06:20:44Z

Starting with a rebase.

camelid · 2021-02-16T06:21:16Z

Oops, looks like my rebase included some changes... oh well 😅

camelid · 2021-02-16T06:21:43Z

Weird, I used git range-diff master @{u} HEAD and it showed no changes:

1:  d32c80467db ! 1:  6c9a5805b0c rustdoc: Strip broken links in summaries
    @@ Commit message
      ## src/librustdoc/html/markdown.rs ##
     @@ src/librustdoc/html/markdown.rs: fn push(s: &mut String, text_length: &mut usize, text: &str) {
              *text_length += text.len();
    -     };
    +     }
      
     -    'outer: for event in Parser::new_ext(md, summary_opts()) {
     +    // NOTE: Make sure to update the same variable in `plain_text_summary`
2:  3f685a724e9 < -:  ----------- Deduplicate broken-link callback

jyn514 · 2021-02-23T04:47:51Z

I think this is waiting on #79781 (comment). If I'm wrong, feel free to ping me for a review :)

@rustbot label: -S-waiting-on-review +S-waiting-on-author

camelid · 2021-02-24T04:55:16Z

This is blocked on getting a new pulldown-cmark release that includes your lifetimes fix. @rustbot label: -S-waiting-on-author +S-blocked

jyn514 · 2021-07-02T08:48:52Z

@camelid can you test this with a git dependency to make sure that actually fixes it? It seems silly to wait all this time if it doesn't actually affect the error.

camelid · 2021-08-09T21:01:57Z

Actually, it looks like this was fixed in the meantime by #86451! (Although that's only for HTML summaries; plain text summaries that are used for alt-text have not yet been fixed.) I'll close this then.

rust-highfive assigned jyn514 Dec 7, 2020

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

camelid force-pushed the summary-render-intra-doc branch from 0bbe443 to d32c804 Compare December 7, 2020 04:43

jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 7, 2020

jyn514 reviewed Dec 7, 2020

View reviewed changes

camelid force-pushed the summary-render-intra-doc branch from 4d130d0 to 3f685a7 Compare December 8, 2020 21:39

camelid mentioned this pull request Jan 19, 2021

[NLL] Bad higher ranked subtype error #57374

Open

camelid force-pushed the summary-render-intra-doc branch from 3f685a7 to 6c9a580 Compare February 16, 2021 06:20

camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2021

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 24, 2021

rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Feb 24, 2021

camelid added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 12, 2021

camelid added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 28, 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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 2, 2021

camelid closed this Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Strip broken links in summaries #79781

rustdoc: Strip broken links in summaries #79781

camelid commented Dec 7, 2020 •

edited

Loading

camelid commented Dec 7, 2020

camelid commented Dec 7, 2020

jyn514 Dec 7, 2020

camelid Dec 7, 2020

jyn514 Dec 8, 2020

camelid Dec 8, 2020

jyn514 Dec 8, 2020

camelid Feb 16, 2021 •

edited

Loading

camelid Feb 16, 2021

camelid Feb 16, 2021

camelid Feb 21, 2021

jyn514 Feb 21, 2021

jyn514 commented Dec 7, 2020

camelid commented Dec 26, 2020

jyn514 commented Feb 15, 2021

camelid commented Feb 15, 2021

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

jyn514 commented Feb 23, 2021

camelid commented Feb 24, 2021

jyn514 commented Jul 2, 2021

camelid commented Aug 9, 2021

rustdoc: Strip broken links in summaries #79781

rustdoc: Strip broken links in summaries #79781

Conversation

camelid commented Dec 7, 2020 • edited Loading

camelid commented Dec 7, 2020

Before

After

camelid commented Dec 7, 2020

Before

After

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

camelid Feb 16, 2021 • edited Loading

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 commented Dec 7, 2020

camelid commented Dec 26, 2020

jyn514 commented Feb 15, 2021

camelid commented Feb 15, 2021

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

jyn514 commented Feb 23, 2021

camelid commented Feb 24, 2021

jyn514 commented Jul 2, 2021

camelid commented Aug 9, 2021

camelid commented Dec 7, 2020 •

edited

Loading

camelid Feb 16, 2021 •

edited

Loading