Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustdoc: Strip broken links in summaries #79781

Closed
wants to merge 1 commit into from
Closed

rustdoc: Strip broken links in summaries #79781

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/librustdoc/html/markdown.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,14 @@ fn markdown_summary_with_limit(md: &str, length_limit: usize) -> (String, bool)
*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`
// 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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@camelid camelid Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


'outer: for event in
Parser::new_with_broken_link_callback(md, summary_opts(), Some(&mut broken_link_callback))
{
match &event {
Event::Text(text) => {
for word in text.split_inclusive(char::is_whitespace) {
Expand Down Expand Up @@ -1113,7 +1120,14 @@ crate fn plain_text_summary(md: &str) -> String {

let mut s = String::with_capacity(md.len() * 3 / 2);

for event in Parser::new_ext(md, summary_opts()) {
// NOTE: Make sure to update the same variable in `markdown_summary_with_limit`
// 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()));

for event in
Parser::new_with_broken_link_callback(md, summary_opts(), Some(&mut broken_link_callback))
{
match &event {
Event::Text(text) => s.push_str(text),
Event::Code(code) => {
Expand Down