Add lint for broken doc links

[maxclaus]

fixes #2179

changelog: [doc_broken_link]: Add pedantic lint to catch broken doc links that won't produce a link tag by rustdoc.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[maxclaus]

Just noticed there are tests failing due to a false positive like this:

/// Referencing an slice [T]

This will be considered a broken link although actually it isn't. I guess in order to fix it I won't be able to check fake value from pulldown_cmark::Parser::new_with_broken_link_callback anymore, since it doesn't provide the raw text to check why it was considered a broken link. I will try to work on a different solution, appreciate if there are any suggestions to achieve it.

[maxclaus]

I will try to use similar approach used on https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/tabs_in_doc_comments.rs

[bors]

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

[Jarcho]

Sorry for the long wait. Left a couple of specific comments.

One thing you'll need to do is take the text input from the markdown parser. Currently you'll be linting inside code and html sections. Doc attribute can also contain multiple lines.

[maxclaus]

@Jarcho thanks for taking the time to review this.

I applied two of your code improvement suggestions. I am a bit confused though about these others comments:

One thing you'll need to do is take the text input from the markdown parser.

Do you mean this lint should run on this parser's output instead of running before that step, which is done against the rust AST attributes? I tried doing that as the first approach, but the new_with_broken_link_callback we use sanitizes broken links replacing them with a fake text and link values, and that makes impossible to run any of this lint logic.

Currently you'll be linting inside code and html sections.

It is being applied only for AttrKind::DocComment attributes. Doesn't it guarantees only code doc comments are covered by this lint, which I imagine is what we want?

Doc attribute can also contain multiple lines.

This current logic is checking for broken links across multiple lines, so I am confused what you mean on this one, as it is already checking for multiple lines.

[maxclaus]

@rustbot review

[Jarcho]

It is being applied only for AttrKind::DocComment attributes. Doesn't it guarantees only code doc comments are covered by this lint, which I imagine is what we want?

Yes. And markdown contain code and html sections. Neither of which will try to parse links.

This current logic is checking for broken links across multiple lines, so I am confused what you mean on this one, as it is already checking for multiple lines.

Right now you're assuming that each attribute contains a single line. This is normally true, but isn't guaranteed.

[maxclaus]

@Jarcho So, to make sure I got it right:

Yes. And markdown contain code and html sections. Neither of which will try to parse links.

Are you saying AttrKind::DocComment attributes are markdown, which include code and html sections, and we should not try to parse links for those types, applying it on real document's content only? If that is what you mean, is your suggestion to follow the same approach from doc/mod.rs which uses the pulldown_cmark to parse the doc comments? But in this case we would not use new_with_broken_link_callback in order to properly handle those broken links, since as mentioned before, they get replaced with fake values in that case.

About this other one:

Right now you're assuming that each attribute contains a single line. This is normally true, but isn't guaranteed.

Would attributes with multiple lines be represented with \n so I should handle that case or are multiple lines represented in a different format? Also, is there are way to reproduce those multiple lines without actually adding \n to comments so I can properly have tests for that case?

[Jarcho]

Are you saying AttrKind::DocComment attributes are markdown, which include code and html sections, and we should not try to parse links for those types, applying it on real document's content only? If that is what you mean, is your suggestion to follow the same approach from doc/mod.rs which uses the pulldown_cmark to parse the doc comments? But in this case we would not use new_with_broken_link_callback in order to properly handle those broken links, since as mentioned before, they get replaced with fake values in that case.

I don't see why that would stop this from working. You can use the span given to the callback to know where to start parsing the input string for the link's destination.

Would attributes with multiple lines be represented with \n so I should handle that case or are multiple lines represented in a different format? Also, is there are way to reproduce those multiple lines without actually adding \n to comments so I can properly have tests for that case?

rustdoc joins all the doc attributes together with \n as the separator and then passes that to the markdown parser.

[maxclaus]

@Jarcho

I don't see why that would stop this from working. You can use the span given to the callback to know where to start parsing the input string for the link's destination.

Ok. I will give the markdown parser another try.

rustdoc joins all the doc attributes together with \n as the separator and then passes that to the markdown parser.

rustdoc might do that, but that is not what I have seen on clippy. Each line is a different AttrKind::DocComment, the linter does not provide a single AttrKind::DocComment merged with \n for all sibling lines of AttrKind::DocComment.

[maxclaus]

hey @Jarcho, I just pushed some temporary changes to confirm I am on the right direction.

I added a new function to the pulldown_cmark fake_broken_link_callback handler based on your suggestions. This is the data we have available within that function:

doc="Test invalid link, url part broken across multiple lines.\n[doc invalid link broken url scheme part part](https://\ntest.fake/doc_invalid_link_broken_url_scheme_part)"

 fragments=[
    DocFragment {
        span: tests/ui/doc_broken_link.rs:40:1: 40:62 (#0),
        item_id: None,
        doc: " Test invalid link, url part broken across multiple lines.",
        kind: SugaredDoc,
        indent: 1,
    },
    DocFragment {
        span: tests/ui/doc_broken_link.rs:41:1: 41:60 (#0),
        item_id: None,
        doc: " [doc invalid link broken url scheme part part](https://",
        kind: SugaredDoc,
        indent: 1,
    },
    DocFragment {
        span: tests/ui/doc_broken_link.rs:42:1: 42:55 (#0),
        item_id: None,
        doc: " test.fake/doc_invalid_link_broken_url_scheme_part)",
        kind: SugaredDoc,
        indent: 1,
    },
]

 bl=BrokenLink {
    span: 58..104,
    link_type: Shortcut,
    reference: Borrowed(
        "doc invalid link broken url scheme part part",
    ),
}

 text based on 'bl.span' range="[doc invalid link broken url scheme part part]"

So, as we can see the BrokenLink data we get from fake_broken_link_callback gives a span just for the link title. I could use that span initial position to start reading the link and get the url part too. Is that what you had in mind?

@rustbot review

[maxclaus]

hi @Jarcho, I have not heard back from you in a month, hopefully everything is alright with you and you are only taking a time off. If you are not back yet, is there someone else that could assist me with this PR?

[Jarcho]

Sorry for the wait. Reading from the link title span is what I had in mind.

rustdoc might do that, but that is not what I have seen on clippy. Each line is a different AttrKind::DocComment, the linter does not provide a single AttrKind::DocComment merged with \n for all sibling lines of AttrKind::DocComment.

Clippy is combining attrs_to_doc_fragments with add_doc_fragment from rustdoc. Between the two of them it adds the necessary line breaks and indentation edits to get the final markdown document. My main point here is doc comments are sequences of doc attributes, each doc attribute is just a string, and each of those strings can themselves contain line breaks. Each line is usually a separate attribute, but that isn't guaranteed to be the case.

With the implementation running through the broken links handler this will be handled properly.

[maxclaus]

@rustbot review

[maxclaus]

I had to move this function declaration down so it has access to doc and fragments variables.

[maxclaus]

hey @Jarcho could you review this please?

[Jarcho]

Again, sorry for the long delay. Behaviour wise this looks to be fine. Can you rebase this so we can get a lintcheck run on it please?

[maxclaus]

@rustbot review

[Jarcho]

Looks like everything is good. Thank you.

Can you just squash the commits down please.

[Jarcho]

Ping @maxclaus from triage. This is only waiting on the commits being squashed.

[maxclaus]

@Jarcho done