rustdoc: Fix invalid suggestions on ambiguous intra doc links v2 #109104

GuillaumeGomez · 2023-03-13T22:24:54Z

This is another approach to fixing the same issue. This time, we keep the computed information around instead of re-computing it.

Strangely enough, the order for ambiguities seem to have been changed. Not an issue but it creates a lot of diff...

So which version do you prefer?

r? @notriddle

jyn514 · 2023-03-13T22:48:32Z

this seems better than before. you should probably get someone from t-compiler to review the change to AssocItems.

GuillaumeGomez · 2023-03-13T22:52:05Z

Let's ask to @oli-obk then if they think it looks ok.

notriddle · 2023-03-13T22:47:14Z

src/librustdoc/passes/collect_intra_doc_links.rs

-    )
+fn is_derive_trait_collision<T>(ns: &PerNS<Result<Vec<(Res, T)>, ResolutionFailure<'_>>>) -> bool {
+    if let (&Ok(ref type_ns), &Ok(ref macro_ns)) = (&ns.type_ns, &ns.macro_ns) {
+        type_ns.iter().any(|(res, _)| matches!(res, Res::Def(DefKind::Trait, _)))


Should this be any or should it be all? What happens if there's a three-way conflict between a macro_rules macro, a derive macro, and a trait?

I think it's not an issue since we only want to check if there is a derive macro and a trait. In one case, we run this check if we have two elements which then allow to only return the types, and in the other case, we remove the macros from the candidates being returned.

I went ahead and tried to write a test for this one.

It doesn't seem to matter, since if I try to bring a macro_rules macro and a derive macro into scope at once, Rust says no. The difference between any and all doesn't seem to matter, because you're usually not going to get more than one match in the same namespace anyway.

error[E0255]: the name `Dev` is defined multiple times --> $DIR/issue-108653-associated-items-9.rs:9:3 | LL | use proc_macro_derive::Dev; | ---------------------- previous import of the macro `Dev` here ... LL | macro_rules! Dev { | ^^^^^^^^^^^^^^^^ `Dev` redefined here | = note: `Dev` must be defined only once in the macro namespace of this module

It seems the function wasn't needed so I removed it completely.

No, it was needed:

error: `PartialEq` is both a trait and a derive macro --> library/core/src/macros/mod.rs:65:71 | 65 | /// Asserts that two expressions are not equal to each other (using [`PartialEq`]). | ^^^^^^^^^ ambiguous link | help: to link to the trait, prefix with `trait@` | 65 | /// Asserts that two expressions are not equal to each other (using [`trait@PartialEq`]). | ++++++ help: to link to the derive macro, prefix with `derive@` | 65 | /// Asserts that two expressions are not equal to each other (using [`derive@PartialEq`]). | +++++++

I'll add a regression test.

oli-obk

None of

this PR
the issue
the other PR

explain what the problem on the impl side is and what is being changed. Please add some information to the main post of this PR.

If regular rustc will never encounter filter_by_name_and_namespace that return more than one element, it should stay as it is and you could make the filtering function a free function inside rustdoc.

compiler/rustc_middle/src/ty/assoc.rs

GuillaumeGomez · 2023-03-14T14:08:56Z

@oli-obk Thanks to your comment, I realized the change in the compiler was unnecessary and thus completely removed it (and the commit as well).

The other thing I changed in the new added commit is in case we have multiple candidates with the same "kind". In such case, it's not possible to disambiguate so the function doesn't emit a warning and we handle the candidates as if we only had one. It seems to be what we had before this PR so I brought back this behaviour.

src/librustdoc/passes/collect_intra_doc_links.rs

GuillaumeGomez · 2023-03-17T23:23:46Z

Removed unnecessary variable.

notriddle · 2023-03-17T23:28:31Z

@jyn514 @oli-obk does this new version look good for you?

jyn514

I don't really understand how this is supposed to work ...

jyn514 · 2023-03-18T05:59:15Z

tests/rustdoc-ui/intra-doc/issue-108653-associated-items-2.rs

+//~^ ERROR
+pub trait Trait {
+    type IDENT;
+    const IDENT: usize;
+}
+
+/// [`Trait2::IDENT`]
+//~^ ERROR


please include the message in the ERROR highlight.

Suggested change

//~^ ERROR

pub trait Trait {

type IDENT;

const IDENT: usize;

}

/// [`Trait2::IDENT`]

//~^ ERROR

//~^ ERROR both an associated constant and an associated type

pub trait Trait {

type IDENT;

const IDENT: usize;

}

/// [`Trait2::IDENT`]

//~^ ERROR both an associated function and an associated type

jyn514 · 2023-03-18T05:59:33Z

tests/rustdoc-ui/intra-doc/issue-108653-associated-items-3.rs

+//~^ ERROR
+/// [`Trait::Trait`]
+//~^ ERROR


jyn514 · 2023-03-18T06:03:33Z

tests/rustdoc-ui/intra-doc/issue-108653-associated-items-6.stderr

+error: `u32::MAX` is both an associated constant and a builtin type
+  --> $DIR/issue-108653-associated-items-6.rs:4:7
+   |
+LL | /// [`u32::MAX`]
+   |       ^^^^^^^^ ambiguous link
+   |
+note: the lint level is defined here
+  --> $DIR/issue-108653-associated-items-6.rs:1:9
+   |
+LL | #![deny(rustdoc::broken_intra_doc_links)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: to link to the associated constant, prefix with `const@`
+   |
+LL | /// [`const@u32::MAX`]
+   |       ++++++
+help: to link to the builtin type, prefix with `prim@`
+   |
+LL | /// [`prim@u32::MAX`]
+   |       +++++


This still doesn't seem right? One of these is a constant in a module, and the other is an associated constant to the u32 primitive type (I think the error has those backwards as well and thinks the one in the module is an associated constant).

Ah interesting: it considers MAX as a primitive from the module and as an associated constant of u32. The naming isn't great though...

Also, dictionary.com is convinced that it should be spelled "built-in".

Well I renamed it "primitive type" so that fixes this issue too. 😆

jyn514 · 2023-03-18T06:05:29Z

tests/rustdoc-ui/intra-doc/issue-108653-associated-items-8.stderr

+help: to link to the associated constant, prefix with `const@`
+   |
+LL | /// [`const@u32::MAX`]
+   |       ++++++
+help: to link to the associated type, prefix with `type@`
+   |
+LL | /// [`type@u32::MAX`]
+   |       +++++


unrelated to this pr, but I wonder if we could include the span of the definition in this suggestion? i.e. point to line 11 here.

You mean adding a "item defined here" note?

jyn514 · 2023-03-18T06:06:49Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -324,14 +335,21 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
        prim_ty: PrimitiveType,
        ns: Namespace,
        item_name: Symbol,
-    ) -> Option<(Res, DefId)> {
+    ) -> Vec<(Res, DefId)> {


Can you return a PerNS here instead to enforce at compile time that there's only one result per namespace?

I don't get what you mean by "enforce at compile time". We'd need to construct a PerNS by calling prim_ty.impls(tcx) which is not a constant method and cannot be one since it makes allocations in simplified_types. Did you have something else in mind?

Is this function going to return a single item per namespace? I can figure you'd have multiple items in the same namespace, but different disambiguators:

#![allow(nonstandard_style)] /// [x](method@Foo::data) pub struct Foo; pub trait Bar { fn data(&self) {} } impl Bar for Foo { fn data(&self) {} } pub trait Baz { const data: u32; } impl Baz for Foo { const data: u32 = 0; }

It's only for primitive types. So normally, this method should never return more than one item. It's possible to create duplicates with trait impls on primitive though, hence why I did this change.

jyn514 · 2023-03-18T06:08:53Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                if !items.is_empty() {
+                    items
+                } else {


Suggested change

if !items.is_empty() {

items

} else {

// Inherent associated items take precedence over items that come from trait impls.

if !items.is_empty() {

items

} else {

jyn514 · 2023-03-18T06:16:08Z

src/librustdoc/passes/collect_intra_doc_links.rs

    // FIXME(#74563): warn about ambiguity
-    debug!("the candidates were {:?}", candidates.clone().collect::<Vec<_>>());
-    candidates.next()
+    debug!("the candidates were {:?}", candidates);
+    candidates


I don't think this behaves as expected when there are multiple traits with the same associated item. Consider:

//! [usize::Item] pub trait Foo { type Item; } pub trait Bar { type Item; } impl Foo for usize { type Item = u32; } impl Bar for usize { type Item = i32; }

Today that compiles without warning; after your change it gives an ambiguity error. That's correct, but we don't have a way to let users disambiguate it today (#74563), so I'm not sure the warning makes sense to emit.

It was compiling without warning with this version too. I added a UI test with this code in tests/rustdoc-ui/intra-doc/issue-108653-associated-items-9.rs just in case.

jyn514 · 2023-03-18T06:17:24Z

src/librustdoc/passes/collect_intra_doc_links.rs

-            ..
-        }
-    )
+fn is_derive_trait_collision<T>(ns: &PerNS<Result<Vec<(Res, T)>, ResolutionFailure<'_>>>) -> bool {


This change doesn't make sense to me. Why are we considering multiple items in the same namespace? At that point it's already ambiguous and the result of this function doesn't matter.

It seems the function wasn't needed so I removed it completely.

jyn514 · 2023-03-18T06:19:00Z

src/librustdoc/passes/collect_intra_doc_links.rs

+        if candidates.len() > 1 && !ambiguity_error(self.cx, &diag, &key.path_str, &candidates) {
+            candidates = vec![candidates[0]];
+        }


I don't understand what's going on here. Why are you arbitrarily picking a candidate here? This happens too late, you can no longer be sure that all the candidates came from trait selection instead of from somewhere where you need to report the ambiguity.

It allows to fix the following case (using the code example you provided):

//! [usize::Item] pub trait Foo { type Item; } pub trait Bar { type Item; } impl Foo for usize { type Item = u32; } impl Bar for usize { type Item = i32; }

Without the deduplication logic present in ambiguity_error, it would output:

error: `usize::Item` is both an associated type and an associated type --> tests/rustdoc-ui/intra-doc/issue-108653-associated-items-9.rs:5:6 | 5 | //! [usize::Item] | ^^^^^^^^^^^ ambiguous link | note: the lint level is defined here --> tests/rustdoc-ui/intra-doc/issue-108653-associated-items-9.rs:3:9 | 3 | #![deny(warnings)] | ^^^^^^^^ = note: `#[deny(rustdoc::broken_intra_doc_links)]` implied by `#[deny(warnings)]` help: to link to the associated type, prefix with `type@` | 5 | //! [type@usize::Item] | +++++ help: to link to the associated type, prefix with `type@` | 5 | //! [type@usize::Item] | +++++ error: aborting due to previous error

I'll add a comment to explain why this is needed.

bors · 2023-03-22T22:22:52Z

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

…ype"

GuillaumeGomez · 2023-03-24T14:09:37Z

Applied suggestions where possible and added a new UI test to prevent regression.

jyn514 · 2023-03-24T14:18:20Z

I don't have time for reviews right now. Happy for @notriddle or @oli-obk to be in charge here.

GuillaumeGomez · 2023-03-24T14:22:14Z

Thanks a lot for your input on this! Let's set @notriddle then.

r? @notriddle

rustbot · 2023-03-24T14:22:21Z

Could not assign reviewer from: notriddle.
User(s) notriddle are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

GuillaumeGomez · 2023-03-24T14:38:37Z

So is_derive_trait_collision was actually useful.

notriddle · 2023-03-30T18:54:32Z

@oli-obk Now that this PR has no changes to rustc in it, do the rest of the changes look reasonable to you?

oli-obk · 2023-03-31T15:40:02Z

I only looked at filter_assoc_items_by_name_and_namespace and its uses, but those seemed fine to me

notriddle · 2023-03-31T17:57:04Z

@bors r=oli-obk,notriddle

bors · 2023-03-31T17:57:07Z

📌 Commit 415a3ca has been approved by oli-obk,notriddle

It is now in the queue for this repository.

…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#109104 (rustdoc: Fix invalid suggestions on ambiguous intra doc links v2) - rust-lang#109443 (Move `doc(primitive)` future incompat warning to `invalid_doc_attributes`) - rust-lang#109680 (Fix subslice capture in closure) - rust-lang#109798 (fluent_messages macro: don't emit the OS error in a note) - rust-lang#109805 (Source map cleanups) - rust-lang#109818 (rustdoc: Add GUI test for jump to collapsed item) - rust-lang#109820 (rustdoc-search: update docs for comma in `?` help popover) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2023-03-31T23:13:26Z

⌛ Testing commit 415a3ca with merge 5e1d329...

rustbot assigned notriddle Mar 13, 2023

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

GuillaumeGomez force-pushed the fix-invalid-suggestion-ambiguous-intra-doc2 branch from 131633d to 3cafb6d Compare March 13, 2023 23:05

notriddle reviewed Mar 13, 2023

View reviewed changes

oli-obk requested changes Mar 14, 2023

View reviewed changes

compiler/rustc_middle/src/ty/assoc.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the fix-invalid-suggestion-ambiguous-intra-doc2 branch from 3cafb6d to 2b9e953 Compare March 14, 2023 14:06

GuillaumeGomez mentioned this pull request Mar 15, 2023

rustdoc: Fix invalid suggestions on ambiguous intra doc links #108699

Closed

GuillaumeGomez requested a review from notriddle March 16, 2023 12:55

notriddle approved these changes Mar 17, 2023

View reviewed changes

notriddle reviewed Mar 17, 2023

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the fix-invalid-suggestion-ambiguous-intra-doc2 branch from 2b9e953 to ac4ee88 Compare March 17, 2023 23:23

jyn514 suggested changes Mar 18, 2023

View reviewed changes

GuillaumeGomez added 6 commits March 24, 2023 13:41

Fix rustdoc intra-doc link invalid ambiguity error message

e981738

Special case ambiguity_error if all candidates have the same "kind"

e193950

Add regression tests for rust-lang#108653

c78ebab

Rename description of primitive from "builtin type" into "primitive t…

95926b2

…ype"

Update UI tests for primitive type ambiguity error renaming

ec43cb3

Strenghten disambiguation in ambiguity_error and improve documentation

537fdbd

GuillaumeGomez force-pushed the fix-invalid-suggestion-ambiguous-intra-doc2 branch from ac4ee88 to 537fdbd Compare March 24, 2023 14:06

Put back is_derive_trait_collision check

415a3ca

notriddle approved these changes Mar 31, 2023

View reviewed changes

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 Mar 31, 2023

GuillaumeGomez mentioned this pull request Mar 31, 2023

Rollup of 7 pull requests #109824

Merged

bors merged commit 8fe5b56 into rust-lang:master Mar 31, 2023

rustbot added this to the 1.70.0 milestone Mar 31, 2023

GuillaumeGomez deleted the fix-invalid-suggestion-ambiguous-intra-doc2 branch April 1, 2023 08:31

rustdoc: Fix invalid suggestions on ambiguous intra doc links v2 #109104

rustdoc: Fix invalid suggestions on ambiguous intra doc links v2 #109104

Conversation

GuillaumeGomez commented Mar 13, 2023

jyn514 commented Mar 13, 2023

GuillaumeGomez commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 14, 2023

GuillaumeGomez commented Mar 17, 2023

notriddle commented Mar 17, 2023

jyn514 left a 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

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

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

bors commented Mar 22, 2023

GuillaumeGomez commented Mar 24, 2023

jyn514 commented Mar 24, 2023

GuillaumeGomez commented Mar 24, 2023

rustbot commented Mar 24, 2023

GuillaumeGomez commented Mar 24, 2023

notriddle commented Mar 30, 2023

oli-obk commented Mar 31, 2023

notriddle commented Mar 31, 2023

bors commented Mar 31, 2023

bors commented Mar 31, 2023

notriddle Mar 13, 2023 •

edited

Loading