rustdoc: Add lint redundant_explicit_links

[ChAoSUnItY]

Closes #87799.

• Lint warns by default
• Reworks link parser to cache original link's display text

r? @jyn514

[jyn514]

i don't have time for reviews, please don't assign me.

r? rustdoc cc @petrochenkov

[GuillaumeGomez]

Changes look good to me. One thing though: I think this should be a rustdoc lint and not a rustc one as it only makes sense in the case you're generating documentation. Can you move it into rustdoc please? It'll also allow to remove the duplication in parse_links.

[notriddle]

Looking at this output messages (I haven't read the code yet), there's a few other things I'd like to see.

First, very minor nitpicks

• This should be a rustdoc pass instead of a rustc pass, like GuillaumeGomez mentioned.

• Standard rust error message formatting is to not capitalize sentences: the note "Explicit link does not affect the original link" violates that rule.

  Yes, it's an arbitrary rule, but in this example, "Explicit" is literally the only capitalized word other than type names, and that's distracting.

• There should be test cases for the syntax [dummy_target][dummy_target]

• There should be test cases for syntax [dummy_target]: dummy_target

Now, the more detailed criticism

I suggest reading the ideas from https://static.barik.net/barik/publications/fse2018/barik_fse18.pdf, where compiler error messages were tested for understandability, and they make recommendations for how error messages should be presented, based on practical argument theory.

A good error message should present:

• Claim: In this case, that the link destination is redundant, and may be removed without breaking the documentation.
• Grounds: In this case, your error message should mention that the label of the intra-doc link resolves to the same item as the explicitly-provided destination (this is obvious for dummy_target, since they're identical, but for [usize](std::primitive::usize) it would help to mention it).
• Warrant: In this case, you need to prove that the fact that the link label resolves to the item is relevant, by mentioning that rustdoc will use the label as the path for resolving an intra-doc link if an explicit destination is not provided.

Just for giggles, here's a suggested form. Feel free to criticize, or let me know if parts of it are too difficult to implement.

error: redundant explicit link target
  --> $DIR/redundant_explicit_links.rs:5:20
   |
LL | /// [dummy_target](dummy_target)
   |      ------------  ^^^^^^^^^^^^ explicit target is redundant
   |      |
   |      because label contains path that resolves to same destination
   |
   = note: when a link has no explicit target,
           the label is used as the destination instead

help: remove explicit link target
   |
LL - /// [dummy_target](dummy_target)
LL + /// [dummy_target][]

In particular, in my counterproposal, the note doesn't just say that it's redundant. It directly teaches the operator what rustdoc does: "when a link's destination is not specified, the label is used to resolve intra-doc links".

[ChAoSUnItY]

Thanks for the diagnostic message suggestion! however, I've noticed some implementation limitations:

1. Currently I could not find any reliable way to fetch symbol text d from reference link syntax [d]: dummy_target.
2. I agree suggestion with diff shown is better, however, I could not find a way to implement it afaik.

[ChAoSUnItY]

Currently there are 2 things I still need to figure out:

1. There's a weird bug for documentation link resolution map encoding, see here.
2. This PR might be a breaking change to the stdlib doc, do we need to either downgrade the lint level, allow rustdoc::redundant_explicit_links in stdlib, or run rustfix on stdlib doc?

[jyn514]

I'd prefer to fix the places in the standard library where this lint triggers. If those fixes break the docs, that means there's a false positive and the lint itself should be changed to not trigger in those cases.

[notriddle]

Agreed. If someone says deny(warn), and we add a warning, it's not a breaking change that their build fails. It's what they asked for.

The false positive on [the documentation for f32](f32) is a problem. Please fix it.

[ChAoSUnItY]

Seems the current mingw-check requires both doc in core & std to be same while resolution might failed on one side.

[notriddle]

In a situation like that, just add #![cfg_attr(not(bootstrap), allow(rustdoc::redundant_explicit_links))]

[petrochenkov]

@ChAoSUnItY
Could you fix the errors until CI is green?
This PR needs performance testing, and the build needs to pass to run it.
@rustbot author

[bors]

⌛ Testing commit 297ff8c with merge 9c699a4...

[bors]

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 9c699a4 to master...

[rust-timer]

Finished benchmarking commit (9c699a4): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.4%, 0.9%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.4%, 0.9%]	7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-3.3%	[-3.9%, -2.7%]	2
All ❌✅ (primary)	-0.4%	[-0.4%, -0.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.8%	[1.0%, 5.8%]	13
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[1.0%, 5.8%]	13

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	62
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	62

Bootstrap: 636.469s -> 634.076s (-0.38%)
Artifact size: 346.85 MiB -> 346.97 MiB (0.03%)

[Mark-Simulacrum]

Marking as triaged. It looks like the regression in libc might be genuine. At least, I don't see subsequent recovery. But it's limited to a few incremental scenarios and I don't see an obvious tie in to the changes in this PR. So I don't think it's worth further investigation.