Delay ambiguous intra-doc link resolution after `Cache` has been populated #131691

GuillaumeGomez · 2024-10-14T16:00:28Z

I was getting nowhere with #130278. I took a wrong turn at some point and ended making way too many changes so instead I started again back from 0 and this time it worked out as expected.

r? @notriddle

…lated

rustbot · 2024-10-14T16:00:38Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

notriddle · 2024-10-14T17:21:36Z

src/librustdoc/passes/collect_intra_doc_links.rs

+    /// These links are ambiguous. We need for the cache to have its paths filled. Unfortunately,
+    /// if we run the `LinkCollector` pass after `Cache::populate`, a lot of items that we need
+    /// to go through will be removed, making a lot of intra-doc links to not be inferred.
+    ///
+    /// So instead, we store the ambiguous links and we wait for cache paths to be filled before
+    /// inferring them (if possible).


This is how I would write it.

According to rustc_resolve, these links are ambiguous.

However, we cannot link to an item that has been stripped from the documentation. If all but one of the "possibilities" are stripped, then there is no real ambiguity. To determine if an ambiguity is real, we delay resolving them until after Cache::populate, then filter every item that doesn't have a cached path.

We could get correct results by simply delaying everything. This would have fewer happy codepaths, but we want to distinguish different kinds of error conditions, and this is easy to do by resolving links as soon as possible.

I actually might still try refactoring this to use a single happy codepath, if I can do it in a way that doesn't regress the diagnostics and actually reduces the total amount of code.

Can be done as a follow-up but if you have ideas for improvements, it's very welcome!

notriddle · 2024-10-14T20:41:25Z

src/librustdoc/passes/collect_intra_doc_links.rs

+}
+
+pub(crate) struct AmbiguousLinks {
+    disambiguator: Option<Disambiguator>,


How could this possibly be Some? Doesn't using a disambiguator imply that the link can't be ambiguous?

One word: reexports. :)

I'm still confused. Is there a test case for this?

So with:

//! [struct@X] mod a { pub struct X; } mod b { pub struct X; } pub use a::*; pub use b::*;

We get:

warning: public documentation for `bar` links to private item `X` --> bar.rs:1:6 | 1 | //! [struct@X] | ^^^^^^^^ this item is private | = note: this link will resolve properly if you pass `--document-private-items` = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default

I think it's a bug but for now, let's simply remove this disambiguator as it doesn't seem to work.

notriddle · 2024-10-14T23:14:45Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                            self.cx.tcx,
+                            BROKEN_INTRA_DOC_LINKS,
+                            format!(
+                                "all items matching `{path_str}` are either private or doc(hidden)"


Suggested change

"all items matching `{path_str}` are either private or doc(hidden)"

"all items matching `{path_str}` are private or doc(hidden)"

notriddle · 2024-10-14T23:22:39Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -2061,6 +2251,8 @@ fn ambiguity_error(
        // There is no way for users to disambiguate at this point, so better return the first
        // candidate and not show a warning.
        return false;
+    } else if !emit_error {
+        return true;


Can this first chunk be separated out into a has_ambiguity_error function that ambiguity_error simply calls?

Ah actually not really: we need the kinds variable that is computed at the beginning...

GuillaumeGomez · 2024-10-14T23:58:45Z

Updated!

GuillaumeGomez · 2024-10-15T13:44:43Z

Updated!

GuillaumeGomez · 2024-10-15T15:47:42Z

@bors ready

notriddle · 2024-10-15T21:41:27Z

@bors r+ rollup

bors · 2024-10-15T21:41:30Z

📌 Commit 10f2395 has been approved by notriddle

It is now in the queue for this repository.

…er-out-2, r=notriddle Delay ambiguous intra-doc link resolution after `Cache` has been populated Fixes rust-lang#130233. I was getting nowhere with rust-lang#130278. I took a wrong turn at some point and ended making way too many changes so instead I started again back from 0 and this time it worked out as expected. r? `@notriddle`

…er-out-2, r=notriddle Delay ambiguous intra-doc link resolution after `Cache` has been populated Fixes rust-lang#130233. I was getting nowhere with rust-lang#130278. I took a wrong turn at some point and ended making way too many changes so instead I started again back from 0 and this time it worked out as expected. r? ``@notriddle``

Rollup of 10 pull requests Successful merges: - rust-lang#129181 (Pass end position of span through inline ASM cookie) - rust-lang#130989 (Don't check unsize goal in MIR validation when opaques remain) - rust-lang#131657 (Rustfmt `for<'a> async` correctly) - rust-lang#131691 (Delay ambiguous intra-doc link resolution after `Cache` has been populated) - rust-lang#131730 (Refactor some `core::fmt` macros) - rust-lang#131751 (Rename `can_coerce` to `may_coerce`, and then structurally resolve correctly in the probe) - rust-lang#131753 (Unify `secondary_span` and `swap_secondary_and_primary` args in `note_type_err`) - rust-lang#131776 (Emscripten: Xfail backtrace ui tests) - rust-lang#131777 (Fix trivially_copy_pass_by_ref in stable_mir) - rust-lang#131778 (Fix needless_lifetimes in stable_mir) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#130989 (Don't check unsize goal in MIR validation when opaques remain) - rust-lang#131657 (Rustfmt `for<'a> async` correctly) - rust-lang#131691 (Delay ambiguous intra-doc link resolution after `Cache` has been populated) - rust-lang#131730 (Refactor some `core::fmt` macros) - rust-lang#131751 (Rename `can_coerce` to `may_coerce`, and then structurally resolve correctly in the probe) - rust-lang#131753 (Unify `secondary_span` and `swap_secondary_and_primary` args in `note_type_err`) - rust-lang#131776 (Emscripten: Xfail backtrace ui tests) - rust-lang#131777 (Fix trivially_copy_pass_by_ref in stable_mir) - rust-lang#131778 (Fix needless_lifetimes in stable_mir) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131691 - GuillaumeGomez:intra-doc-link-filter-out-2, r=notriddle Delay ambiguous intra-doc link resolution after `Cache` has been populated Fixes rust-lang#130233. I was getting nowhere with rust-lang#130278. I took a wrong turn at some point and ended making way too many changes so instead I started again back from 0 and this time it worked out as expected. r? ```@notriddle```

GuillaumeGomez added 2 commits October 14, 2024 17:51

Delay ambiguous intra-doc link resolution after Cache has been popu…

73fc00f

…lated

Add regression tests for rust-lang#130233

d540e72

GuillaumeGomez changed the title ~~Delay ambiguous intra-doc link resolution after Cache has been populated~~ Delay ambiguous intra-doc link resolution after Cache has been populated Oct 14, 2024

rustbot assigned notriddle Oct 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 14, 2024

GuillaumeGomez mentioned this pull request Oct 14, 2024

Remove intra doc link items if they are private/hidden and the matching option is not enabled #130278

Closed

notriddle requested changes Oct 14, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2024

This comment has been minimized.

Sign in to view

GuillaumeGomez added 2 commits October 15, 2024 15:42

Improve documentation for intra-doc links computation

2b9e41c

Remove AmbiguousLinks::disambiguator

10f2395

GuillaumeGomez force-pushed the intra-doc-link-filter-out-2 branch from e0fb0dc to 10f2395 Compare October 15, 2024 13:43

GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024

matthiaskrgr mentioned this pull request Oct 16, 2024

Rollup of 8 pull requests #131768

Closed

Urgau mentioned this pull request Oct 16, 2024

Rollup of 10 pull requests #131785

Closed

matthiaskrgr mentioned this pull request Oct 16, 2024

Rollup of 9 pull requests #131797

Merged

bors merged commit 87c31fe into rust-lang:master Oct 16, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Oct 16, 2024

GuillaumeGomez deleted the intra-doc-link-filter-out-2 branch October 20, 2024 02:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay ambiguous intra-doc link resolution after `Cache` has been populated #131691

Delay ambiguous intra-doc link resolution after `Cache` has been populated #131691

GuillaumeGomez commented Oct 14, 2024

rustbot commented Oct 14, 2024

notriddle Oct 14, 2024 •

edited

Loading

GuillaumeGomez Oct 14, 2024

notriddle Oct 14, 2024

GuillaumeGomez Oct 14, 2024

notriddle Oct 15, 2024

GuillaumeGomez Oct 15, 2024

notriddle Oct 14, 2024

notriddle Oct 14, 2024 •

edited

Loading

GuillaumeGomez Oct 14, 2024

GuillaumeGomez Oct 14, 2024

GuillaumeGomez commented Oct 14, 2024

This comment has been minimized.

GuillaumeGomez commented Oct 15, 2024

GuillaumeGomez commented Oct 15, 2024

notriddle commented Oct 15, 2024

bors commented Oct 15, 2024

	"all items matching `{path_str}` are either private or doc(hidden)"
	"all items matching `{path_str}` are private or doc(hidden)"

Delay ambiguous intra-doc link resolution after Cache has been populated #131691

Delay ambiguous intra-doc link resolution after Cache has been populated #131691

Conversation

GuillaumeGomez commented Oct 14, 2024

rustbot commented Oct 14, 2024

notriddle Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Oct 14, 2024

This comment has been minimized.

GuillaumeGomez commented Oct 15, 2024

GuillaumeGomez commented Oct 15, 2024

notriddle commented Oct 15, 2024

bors commented Oct 15, 2024

Delay ambiguous intra-doc link resolution after `Cache` has been populated #131691

Delay ambiguous intra-doc link resolution after `Cache` has been populated #131691

notriddle Oct 14, 2024 •

edited

Loading

notriddle Oct 14, 2024 •

edited

Loading