rustdoc: Strip broken links in summaries

[camelid]

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]

Before

After

[camelid]

Before

After

[jyn514]

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?

[camelid]

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?

[jyn514]

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.

[camelid]

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

[jyn514]

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

[camelid]

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?

[camelid]

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.

[camelid]

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.

[camelid]

@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.

[jyn514]

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

[jyn514]

Also, please add a test for this.

[camelid]

Hmm, why did I mark this PR as blocked?

[jyn514]

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

[camelid]

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

[camelid]

Starting with a rebase.

[camelid]

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

[camelid]

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]

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]

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

[jyn514]

@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]

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.