get rid of some false negatives in rustdoc::broken_intra_doc_links

[lolbinarycat]

rustdoc will not try to do intra-doc linking if the "path" of a link looks too much like a "real url".

however, only inline links ([text](url)) can actually contain a url, other types of links (reference links, shortcut links) contain a reference which is later resolved to an actual url.

the "path" in this case cannot be a url, and therefore it should not be skipped due to looking like a url.

fixes #54191

to minimize the number of false positives that will be introduced, the following heuristic is used:

If there's no backticks, be lenient revert to old behavior.
This is to prevent churn by linting on stuff that isn't meant to be a link.
only shortcut links have simple enough syntax that they
are likely to be written accidentlly, collapsed and reference links
need 4 metachars, and reference links will not usually use
backticks in the reference name.
therefore, only shortcut syntax gets the lenient behavior.
here's a truth table for how link kinds that cannot be urls are handled:

	is shortcut link	not shortcut link
has backtick	never ignore	never ignore
no backtick	ignore if url-like	never ignore

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[notriddle]

This looks like a good idea, but I have found one problem. Consider a sample like this:

//@ check-pass
#![no_std]
#![deny(rustdoc::broken_intra_doc_links)]

// regression test for https://github.com/rust-lang/rust/issues/54191
// false positives

//! This is not an intra-doc link: [`foobar`]
//!
//! [`foobar`]: /index.html

That test case fails, because it thinks /index.html is an intra-doc link. You're right that certain kinds of links aren't allowed to be URLs—that's the Unknown type links.

[lolbinarycat]

welp, turns out the standard library itself contains several of these false negatives!

[lolbinarycat]

...or perhaps there's some deeper bug with intra-doc link generation?

there seems to be some false positives containing spaces, even though there is a matching reference definition...

I guess we can just re-enable unconditionally ignoring links with spaces?

[lolbinarycat]

Ok, this breaks a lot of ui tests. This might actually have a pretty big impact, maybe we should do a crater run or something..?

[notriddle]

It's looks fine to me. Here's the PRs where each of these test cases were added:

test case	PR
disambiguator-endswith-named-suffix.rs	#127016
weird-syntax.rs	#112014
redundant_explicit_links-utf8.rs	#115070

The last two are both contrived ICE tests, and all of them are intended to be parsed as intra-doc links. Making it so that they warn seems fine to me.

r? @GuillaumeGomez do you also think this is a suitable bug fix?

[GuillaumeGomez]

I think so. I'm not completely satisfied with the current impact though. Intra-doc links should not be triggered on items that contain characters which can't be in an ident or a path, like Clone\(\). Maybe a different warning in these cases?

[lolbinarycat]

@GuillaumeGomez what about just amending the current warning to account for other common mistakes?

honestly makes me wish we had --explain for lints, putting a full help message for each one could get a bit verbose.

regardless, i think that should be a separate issue, since the lint output isn't the most helpful in general (it just recommends escaping the brackets, and doesn't mention other common issues, such as misspelled link references)

[GuillaumeGomez]

what about just amending the current warning to account for other common mistakes?

Do you have an example of before/after of what you have in mind by any chance?

[lolbinarycat]

Do you have an example of before/after of what you have in mind by any chance?

well, the most obvious one is some sort of "consider adding a reference: [whatever]: //some-url.example/", but i also think trying to catch typos by comparing the edit distance of the missing reference to that of existing references could be handy.

and perhaps we should also mention surrounding code by backticks, since code snippets should generally be in code blocks instead of individually escaping chars.

[GuillaumeGomez]

well, the most obvious one is some sort of "consider adding a reference: [whatever]: //some-url.example/", but i also think trying to catch typos by comparing the edit distance of the missing reference to that of existing references could be handy.

Sounds good to me. The distance between two items could be nice too (as a follow-up?).

[GuillaumeGomez]

Then let's go, thanks everyone!

@bors r+ rollup

[bors]

📌 Commit 2d38996 has been approved by GuillaumeGomez

It is now in the queue for this repository.

[fmease]

#144384 (comment)
@bors r-

[fmease]

@bors try jobs=dist-x86_64-linux-alt

[rust-bors]

⌛ Trying commit 2d38996 with merge 0419140…

To cancel the try build, run the command @bors try cancel.

[lolbinarycat]

weirdly the erroring code doesn't seem to exist in master or nightly source.

EDIT: nvm there's multiple comments referencing README.md in that file, i was looking at the wrong one.

[rust-bors]

💔 Test failed (CI). Failed jobs:

• bors build finished (web logs, extended logs)
• try - dist-x86_64-linux-alt (web logs, extended logs)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt