Rustdoc: only report broken ref-style links once #77859
Conversation
|
Some changes occurred in intra-doc-links. cc @jyn514 |
|
r? @jyn514 (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
2fc34d9
to
73fe98f
Compare
jyn514
added
A-intra-doc-links
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
2 times, most recently
from
c69d3eb
to
025d676
Compare
jyn514
added
S-blocked
This comment has been minimized.
This comment has been minimized.
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
025d676
to
435cdb0
Compare
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
4 times, most recently
from
6aac7f5
to
093f190
Compare
This comment has been minimized.
This comment has been minimized.
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
093f190
to
0228f78
Compare
This comment has been minimized.
This comment has been minimized.
jyn514
added
S-waiting-on-author
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
8ec0a57
to
9254f86
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
2 times, most recently
from
fd381ab
to
a91de1a
Compare
| extra_fragment, | ||
| cache_resolution_failure: matches!( | ||
| ori_link.kind, | ||
| LinkType::Reference | LinkType::Shortcut |
There was a problem hiding this comment.
I don't particularly like this, but:
- LinkType isn't hashable, so it can't be included in the ResolutionInfo struct directly
- even if LinkType were hashable, including it would result in random test failures (didn't bother investigating)
- I'm not sure why this doesn't break shortcut links completely (maybe because they are parsed as
ShortcutUnknown?)
There was a problem hiding this comment.
Calculating whether to cache it ahead of time seems fine. The reason you check explicitly for the kind is because inline links shouldn't be cached (things like [x](Ok))? Can you check for those explicitly instead? Or if that's not what you meant, maybe Collapsed should also be checked?
There was a problem hiding this comment.
And I have another question - I don't think this needs to be part of the key, right? It would work if you passed it as a parameter instead? That would avoid having to hash it at all.
There was a problem hiding this comment.
It has to be, otherwise "normal" links to the same target as a reference-style link would hit the cache. See the test case for exactly this :) Depends on what you expect, I guess, but this patch aims to emit a single error for each incorrect link definition, or at least that was my goal.
There was a problem hiding this comment.
Ah, it's also not that links don't get cached, but resolution failures don't get cached. Which is only interesting in the reference style links case, where we don't want to emit an error for each usage of the incorrect link. I think :)
There was a problem hiding this comment.
Ah, it's also not that links don't get cached, but resolution failures don't get cached.
Right, that's what I meant, thanks.
It has to be, otherwise "normal" links to the same target as a reference-style link would hit the cache.
Ok, I see the issue: if you have something like
/// [reference][link] and [inline](link)both links should give an error, but if you don't include cache_resolution_failure in the cache key, then the second link will pick up the None and be returned.
So I have a further question now - when will you hit the cache and not have cache_resolution_failure set? That would only happen for [inline](link) twice in a row, right? But we explicitly choose not to insert it into the cache on error. I think there's an optimization here where you can make the size of the cache smaller by explicitly checking cache_resolution_failure on a cache hit, that way you don't need that extra bit of info; and the logic seems more clear to me too.
There was a problem hiding this comment.
I think there's an optimization here
Interesting thought, that might be something I missed.
| /// | ||
| /// [a]: ref | ||
| //~^ ERROR unresolved link to | ||
| /// [b]: ref2 |
There was a problem hiding this comment.
In the current patch, if this is [b]: ref, the error emitted for [reference link][b] will point to the definition of a. This is because the link cache ignores the name of the reference and handles a and b as if identical. This isn't ideal, but I consider this acceptable: we don't report all errors, but if the user fixes link a, we will report an error for link b, so eventually, every error can be caught :)
We could fix this by adding the span to ResolutionInfo (Optionally, only for Shortcut/Reference links), but I'm not sure if the complexity is worth it. This would also make ResolutionInfo and DiagnosticInfo overlap, if only optionally.
There was a problem hiding this comment.
Wait, I'm confused what those have to do with each other. We never cache the span, so there's nowhere for it to look up the span of a in the first place, the only span available is that of b.
Can you post the error that's wrong?
There was a problem hiding this comment.
Assume the following:
[a] [b]
[a]: ref
[b]: ref
We would only report an error for a.
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
76e1763
to
6238941
Compare
This comment has been minimized.
This comment has been minimized.
src/librustdoc/html/markdown.rs
Outdated
| // For reference-style links (where the target is defined in footnote) we want the | ||
| // span to point to the definition, not the usage in order to generate useful error | ||
| // messages. | ||
| let start = link.as_ptr() as usize - md.as_ptr() as usize; |
There was a problem hiding this comment.
This actually doesn't work because the parser may clone the string and the pointer will no longer point into md. Sad :(
There was a problem hiding this comment.
Can you open an issue for pulldown? I think pointing to the usage is fine for now though.
There was a problem hiding this comment.
To be honest, I don't consider this a bug in pulldown. Current behaviour is strange, but unless pulldown can provide two spans for a single event, this is at least consistent enough.
Pointing at the usage feels weird, though. This doesn't point me to what I need to modify:
+ LL | /// Links to [a] [link][a]
+ | ^^^ no item named `ref` in scope
But I guess we can live with it if we can't fix it.
On the other hand, if it were possible to look up the link's title in the list of definitions, we could maybe point at the definition itself. I guess we can build a collector pass that build a table from footnotes? I need to look into how pulldows actually parses text and which events are relevant here. That will probably have to wait for next year, though.
There was a problem hiding this comment.
Hmm, yeah that error is pretty bad. I think I would rather have duplicate errors than an error pointing to the wrong place :/
There was a problem hiding this comment.
You broke it ;)
To clarify: I believe this is an unintended, unnoticed side effect of the refactor that removed locate(). Maybe we should revert that and add a test? Or is there one and I believe it wrong and this error is my fault, which would be preferable.
There was a problem hiding this comment.
Oh oof, you're right, this is latest nightly:
warning: unresolved link to `std::process::Comman`
--> link.rs:1:14
|
1 | //! Links to [a] [link][a]
| ^^^ no item named `Comman` in module `process`
|
= note: `#[warn(broken_intra_doc_links)]` on by default
warning: unresolved link to `std::process::Comman`
--> link.rs:1:18
|
1 | //! Links to [a] [link][a]
| ^^^^^^^^^ no item named `Comman` in module `process`
and this is a couple days ago, before the PR removing locate():
warning: unresolved link to `std::process::Comman`
--> link.rs:3:10
|
3 | //! [a]: std::process::Comman
| ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process`
|
= note: `#[warn(broken_intra_doc_links)]` on by default
warning: unresolved link to `std::process::Comman`
--> link.rs:3:10
|
3 | //! [a]: std::process::Comman
| ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process`
| /// | ||
| /// [a]: ref | ||
| //~^ ERROR unresolved link to | ||
| /// [b]: ref2 |
There was a problem hiding this comment.
Wait, I'm confused what those have to do with each other. We never cache the span, so there's nowhere for it to look up the span of a in the first place, the only span available is that of b.
Can you post the error that's wrong?
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #80181) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
…llaumeGomez Revert "Cleanup markdown span handling" Reverts rust-lang#80244. This caused a diagnostic regression, originally it was: ``` warning: unresolved link to `std::process::Comman` --> link.rs:3:10 | 3 | //! [a]: std::process::Comman | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` but after that PR rustdoc now displays ``` warning: unresolved link to `std::process::Comman` --> link.rs:1:14 | 1 | //! Links to [a] [link][a] | ^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` which IMO is much less clear. cc `@bugadani,` thanks for catching this in rust-lang#77859. r? `@GuillaumeGomez`
…llaumeGomez Revert "Cleanup markdown span handling" Reverts rust-lang#80244. This caused a diagnostic regression, originally it was: ``` warning: unresolved link to `std::process::Comman` --> link.rs:3:10 | 3 | //! [a]: std::process::Comman | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` but after that PR rustdoc now displays ``` warning: unresolved link to `std::process::Comman` --> link.rs:1:14 | 1 | //! Links to [a] [link][a] | ^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` which IMO is much less clear. cc `@bugadani,` thanks for catching this in rust-lang#77859. r? `@GuillaumeGomez`
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
39eec33
to
648ab30
Compare
|
☔ The latest upstream changes (presumably #80261) made this pull request unmergeable. Please resolve the merge conflicts. |
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
648ab30
to
2892d4a
Compare
|
@bors delegate=bugadani |
|
✌️ @bugadani can now approve this pull request |
|
Sure thing! Want me to squash before merging? |
|
Sure, if you think it makes the git history more clear. My idea of 'more clear' is 'no commits that should have just been in the original' (e.g. fixing typos) and "don't have major rewrites of the same functionality". |
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
bugadani
force-pushed
the
no-duplicate-ref-link-error
branch
from
13c5aa1
to
4b612dd
Compare
|
Makes sense. @bors r=jyn514 |
|
📌 Commit 4b612dd has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
This PR assigns the markdown
LinkTypeto each parsed link and passes this information into the link collector.If a link can't be resolved in
resolve_with_disambiguator, the failure is cached for the link types where we only want to report the error once (namelyShortcutandReference).Fixes #77681