Auto merge of #130587 - coolreader18:field-variant-doclink-disambig, r=notriddle,jyn514

Add `field@` and `variant@` doc-link disambiguators

I'm not sure if this is big enough to need an fcp or not, but this is something I found missing when trying to refer to a field in macro-generated docs, not knowing if a method might be defined as well. Obviously, there are definitely other uses.

In the case where it's not disambiguated, methods (and I suppose other associated items in the value namespace) still take priority, which `@jyn514` said was an oversight but I think is probably the desired behavior 99% of the time anyway - shadowing a field with an accessor method is a very common pattern. If fields and methods with the same name started conflicting, it would be a breaking change. Though, to quote them:

> jyn: maybe you can break this only if both [the method and the field] are public
> jyn: rustc has some future-incompat warning level
> jyn: that gets through -A warnings and --cap-lints from cargo

That'd be out of scope of this PR, though.

Fixes #80283

src/doc/rustdoc/src/write-documentation/linking-to-items-by-name.md

@@ -89,8 +89,8 @@ fn Foo() {}
 
 These prefixes will be stripped when displayed in the documentation, so `[struct@Foo]` will be
 rendered as `Foo`. The following prefixes are available: `struct`, `enum`, `trait`, `union`,
-`mod`, `module`, `const`, `constant`, `fn`, `function`, `method`, `derive`, `type`, `value`,
+`mod`, `module`, `const`, `constant`, `fn`, `function`, `field`, `variant`, `method`, `derive`,
-`macro`, `prim` or `primitive`.
+`type`, `value`, `macro`, `prim` or `primitive`.
 
 You can also disambiguate for functions by adding `()` after the function name,
 or for macros by adding `!` after the macro name. The macro `!` can be followed by `()`, `{}`,

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -110,7 +110,6 @@ impl Res {
 
         let prefix = match kind {
             DefKind::Fn | DefKind::AssocFn => return Suggestion::Function,
-            DefKind::Field => return Suggestion::RemoveDisambiguator,
             DefKind::Macro(MacroKind::Bang) => return Suggestion::Macro,
 
             DefKind::Macro(MacroKind::Derive) => "derive",
@@ -123,6 +122,8 @@ impl Res {
                 "const"
             }
             DefKind::Static { .. } => "static",
+            DefKind::Field => "field",
+            DefKind::Variant | DefKind::Ctor(..) => "variant",
             // Now handle things that don't have a specific disambiguator
             _ => match kind
                 .ns()
@@ -415,6 +416,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         &mut self,
         path_str: &'path str,
         ns: Namespace,
+        disambiguator: Option<Disambiguator>,
         item_id: DefId,
         module_id: DefId,
     ) -> Result<Vec<(Res, Option<DefId>)>, UnresolvedPath<'path>> {
@@ -454,7 +456,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         match resolve_primitive(path_root, TypeNS)
             .or_else(|| self.resolve_path(path_root, TypeNS, item_id, module_id))
             .map(|ty_res| {
-                self.resolve_associated_item(ty_res, item_name, ns, module_id)
+                self.resolve_associated_item(ty_res, item_name, ns, disambiguator, module_id)
                     .into_iter()
                     .map(|(res, def_id)| (res, Some(def_id)))
                     .collect::<Vec<_>>()
@@ -557,6 +559,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         root_res: Res,
         item_name: Symbol,
         ns: Namespace,
+        disambiguator: Option<Disambiguator>,
         module_id: DefId,
     ) -> Vec<(Res, DefId)> {
         let tcx = self.cx.tcx;
@@ -583,7 +586,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                 // FIXME: if the associated item is defined directly on the type alias,
                 // it will show up on its documentation page, we should link there instead.
                 let Some(res) = self.def_id_to_res(did) else { return Vec::new() };
-                self.resolve_associated_item(res, item_name, ns, module_id)
+                self.resolve_associated_item(res, item_name, ns, disambiguator, module_id)
             }
             Res::Def(
                 def_kind @ (DefKind::Struct | DefKind::Union | DefKind::Enum | DefKind::ForeignTy),
@@ -604,6 +607,39 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     }
                 }
 
+                let search_for_field = || {
+                    let (DefKind::Struct | DefKind::Union) = def_kind else { return vec![] };
+                    debug!("looking for fields named {item_name} for {did:?}");
+                    // FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?)
+                    // NOTE: it's different from variant_field because it only resolves struct fields,
+                    // not variant fields (2 path segments, not 3).
+                    //
+                    // We need to handle struct (and union) fields in this code because
+                    // syntactically their paths are identical to associated item paths:
+                    // `module::Type::field` and `module::Type::Assoc`.
+                    //
+                    // On the other hand, variant fields can't be mistaken for associated
+                    // items because they look like this: `module::Type::Variant::field`.
+                    //
+                    // Variants themselves don't need to be handled here, even though
+                    // they also look like associated items (`module::Type::Variant`),
+                    // because they are real Rust syntax (unlike the intra-doc links
+                    // field syntax) and are handled by the compiler's resolver.
+                    let ty::Adt(def, _) = tcx.type_of(did).instantiate_identity().kind() else {
+                        unreachable!()
+                    };
+                    def.non_enum_variant()
+                        .fields
+                        .iter()
+                        .filter(|field| field.name == item_name)
+                        .map(|field| (root_res, field.did))
+                        .collect::<Vec<_>>()
+                };
+
+                if let Some(Disambiguator::Kind(DefKind::Field)) = disambiguator {
+                    return search_for_field();
+                }
+
                 // Checks if item_name belongs to `impl SomeItem`
                 let mut assoc_items: Vec<_> = tcx
                     .inherent_impls(did)
@@ -646,32 +682,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                 if ns != Namespace::ValueNS {
                     return Vec::new();
                 }
-                debug!("looking for fields named {item_name} for {did:?}");
+
-                // FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?)
+                search_for_field()
-                // NOTE: it's different from variant_field because it only resolves struct fields,
-                // not variant fields (2 path segments, not 3).
-                //
-                // We need to handle struct (and union) fields in this code because
-                // syntactically their paths are identical to associated item paths:
-                // `module::Type::field` and `module::Type::Assoc`.
-                //
-                // On the other hand, variant fields can't be mistaken for associated
-                // items because they look like this: `module::Type::Variant::field`.
-                //
-                // Variants themselves don't need to be handled here, even though
-                // they also look like associated items (`module::Type::Variant`),
-                // because they are real Rust syntax (unlike the intra-doc links
-                // field syntax) and are handled by the compiler's resolver.
-                let def = match tcx.type_of(did).instantiate_identity().kind() {
-                    ty::Adt(def, _) if !def.is_enum() => def,
-                    _ => return Vec::new(),
-                };
-                def.non_enum_variant()
-                    .fields
-                    .iter()
-                    .filter(|field| field.name == item_name)
-                    .map(|field| (root_res, field.did))
-                    .collect::<Vec<_>>()
             }
             Res::Def(DefKind::Trait, did) => filter_assoc_items_by_name_and_namespace(
                 tcx,
@@ -1297,7 +1309,7 @@ impl LinkCollector<'_, '_> {
 
         match disambiguator.map(Disambiguator::ns) {
             Some(expected_ns) => {
-                match self.resolve(path_str, expected_ns, item_id, module_id) {
+                match self.resolve(path_str, expected_ns, disambiguator, item_id, module_id) {
                     Ok(candidates) => candidates,
                     Err(err) => {
                         // We only looked in one namespace. Try to give a better error if possible.
@@ -1306,8 +1318,9 @@ impl LinkCollector<'_, '_> {
                         let mut err = ResolutionFailure::NotResolved(err);
                         for other_ns in [TypeNS, ValueNS, MacroNS] {
                             if other_ns != expected_ns {
-                                if let Ok(&[res, ..]) =
+                                if let Ok(&[res, ..]) = self
-                                    self.resolve(path_str, other_ns, item_id, module_id).as_deref()
+                                    .resolve(path_str, other_ns, None, item_id, module_id)
+                                    .as_deref()
                                 {
                                     err = ResolutionFailure::WrongNamespace {
                                         res: full_res(self.cx.tcx, res),
@@ -1327,7 +1340,7 @@ impl LinkCollector<'_, '_> {
             None => {
                 // Try everything!
                 let mut candidate = |ns| {
-                    self.resolve(path_str, ns, item_id, module_id)
+                    self.resolve(path_str, ns, None, item_id, module_id)
                         .map_err(ResolutionFailure::NotResolved)
                 };
 
@@ -1531,6 +1544,8 @@ impl Disambiguator {
                 }),
                 "function" | "fn" | "method" => Kind(DefKind::Fn),
                 "derive" => Kind(DefKind::Macro(MacroKind::Derive)),
+                "field" => Kind(DefKind::Field),
+                "variant" => Kind(DefKind::Variant),
                 "type" => NS(Namespace::TypeNS),
                 "value" => NS(Namespace::ValueNS),
                 "macro" => NS(Namespace::MacroNS),
@@ -1569,6 +1584,8 @@ impl Disambiguator {
     fn ns(self) -> Namespace {
         match self {
             Self::Namespace(n) => n,
+            // for purposes of link resolution, fields are in the value namespace.
+            Self::Kind(DefKind::Field) => ValueNS,
             Self::Kind(k) => {
                 k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
             }
@@ -1603,8 +1620,6 @@ enum Suggestion {
     Function,
     /// `m!`
     Macro,
-    /// `foo` without any disambiguator
-    RemoveDisambiguator,
 }
 
 impl Suggestion {
@@ -1613,7 +1628,6 @@ impl Suggestion {
             Self::Prefix(x) => format!("prefix with `{x}@`").into(),
             Self::Function => "add parentheses".into(),
             Self::Macro => "add an exclamation mark".into(),
-            Self::RemoveDisambiguator => "remove the disambiguator".into(),
         }
     }
 
@@ -1623,13 +1637,11 @@ impl Suggestion {
             Self::Prefix(prefix) => format!("{prefix}@{path_str}"),
             Self::Function => format!("{path_str}()"),
             Self::Macro => format!("{path_str}!"),
-            Self::RemoveDisambiguator => path_str.into(),
         }
     }
 
     fn as_help_span(
         &self,
-        path_str: &str,
         ori_link: &str,
         sp: rustc_span::Span,
     ) -> Vec<(rustc_span::Span, String)> {
@@ -1677,7 +1689,6 @@ impl Suggestion {
                 }
                 sugg
             }
-            Self::RemoveDisambiguator => vec![(sp, path_str.into())],
         }
     }
 }
@@ -1826,7 +1837,9 @@ fn resolution_failure(
                         };
                         name = start;
                         for ns in [TypeNS, ValueNS, MacroNS] {
-                            if let Ok(v_res) = collector.resolve(start, ns, item_id, module_id) {
+                            if let Ok(v_res) =
+                                collector.resolve(start, ns, None, item_id, module_id)
+                            {
                                 debug!("found partial_res={v_res:?}");
                                 if let Some(&res) = v_res.first() {
                                     *partial_res = Some(full_res(tcx, res));
@@ -2164,7 +2177,7 @@ fn suggest_disambiguator(
     };
 
     if let (Some(sp), Some(ori_link)) = (sp, ori_link) {
-        let mut spans = suggestion.as_help_span(path_str, ori_link, sp);
+        let mut spans = suggestion.as_help_span(ori_link, sp);
         if spans.len() > 1 {
             diag.multipart_suggestion(help, spans, Applicability::MaybeIncorrect);
         } else {

tests/rustdoc-ui/intra-doc/disambiguator-mismatch.rs

@@ -1,6 +1,8 @@
 #![deny(rustdoc::broken_intra_doc_links)]
 //~^ NOTE lint level is defined
-pub enum S {}
+pub enum S {
+    A,
+}
 fn S() {}
 
 #[macro_export]
@@ -13,6 +15,10 @@ const c: usize = 0;
 
 trait T {}
 
+struct X {
+    y: usize,
+}
+
 /// Link to [struct@S]
 //~^ ERROR incompatible link kind for `S`
 //~| NOTE this link resolved
@@ -78,4 +84,14 @@ trait T {}
 //~^ ERROR unresolved link to `std`
 //~| NOTE this link resolves to the crate `std`
 //~| HELP to link to the crate, prefix with `mod@`
+
+/// Link to [method@X::y]
+//~^ ERROR incompatible link kind for `X::y`
+//~| NOTE this link resolved
+//~| HELP prefix with `field@`
+
+/// Link to [field@S::A]
+//~^ ERROR incompatible link kind for `S::A`
+//~| NOTE this link resolved
+//~| HELP prefix with `variant@`
 pub fn f() {}

tests/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr

@@ -1,5 +1,5 @@
 error: incompatible link kind for `S`
-  --> $DIR/disambiguator-mismatch.rs:16:14
+  --> $DIR/disambiguator-mismatch.rs:22:14
    |
 LL | /// Link to [struct@S]
    |              ^^^^^^^^ this link resolved to an enum, which is not a struct
@@ -15,7 +15,7 @@ LL | /// Link to [enum@S]
    |              ~~~~~
 
 error: incompatible link kind for `S`
-  --> $DIR/disambiguator-mismatch.rs:21:14
+  --> $DIR/disambiguator-mismatch.rs:27:14
    |
 LL | /// Link to [mod@S]
    |              ^^^^^ this link resolved to an enum, which is not a module
@@ -26,7 +26,7 @@ LL | /// Link to [enum@S]
    |              ~~~~~
 
 error: incompatible link kind for `S`
-  --> $DIR/disambiguator-mismatch.rs:26:14
+  --> $DIR/disambiguator-mismatch.rs:32:14
    |
 LL | /// Link to [union@S]
    |              ^^^^^^^ this link resolved to an enum, which is not a union
@@ -37,7 +37,7 @@ LL | /// Link to [enum@S]
    |              ~~~~~
 
 error: incompatible link kind for `S`
-  --> $DIR/disambiguator-mismatch.rs:31:14
+  --> $DIR/disambiguator-mismatch.rs:37:14
    |
 LL | /// Link to [trait@S]
    |              ^^^^^^^ this link resolved to an enum, which is not a trait
@@ -48,7 +48,7 @@ LL | /// Link to [enum@S]
    |              ~~~~~
 
 error: incompatible link kind for `T`
-  --> $DIR/disambiguator-mismatch.rs:36:14
+  --> $DIR/disambiguator-mismatch.rs:42:14
    |
 LL | /// Link to [struct@T]
    |              ^^^^^^^^ this link resolved to a trait, which is not a struct
@@ -59,7 +59,7 @@ LL | /// Link to [trait@T]
    |              ~~~~~~
 
 error: incompatible link kind for `m`
-  --> $DIR/disambiguator-mismatch.rs:41:14
+  --> $DIR/disambiguator-mismatch.rs:47:14
    |
 LL | /// Link to [derive@m]
    |              ^^^^^^^^ this link resolved to a macro, which is not a derive macro
@@ -71,7 +71,7 @@ LL + /// Link to [m!]
    |
 
 error: unresolved link to `m`
-  --> $DIR/disambiguator-mismatch.rs:46:14
+  --> $DIR/disambiguator-mismatch.rs:52:14
    |
 LL | /// Link to [m()]
    |              ^^^ this link resolves to the macro `m`, which is not in the value namespace
@@ -82,7 +82,7 @@ LL | /// Link to [m!()]
    |               +
 
 error: incompatible link kind for `s`
-  --> $DIR/disambiguator-mismatch.rs:52:14
+  --> $DIR/disambiguator-mismatch.rs:58:14
    |
 LL | /// Link to [const@s]
    |              ^^^^^^^ this link resolved to a static, which is not a constant
@@ -93,7 +93,7 @@ LL | /// Link to [static@s]
    |              ~~~~~~~
 
 error: incompatible link kind for `c`
-  --> $DIR/disambiguator-mismatch.rs:57:14
+  --> $DIR/disambiguator-mismatch.rs:63:14
    |
 LL | /// Link to [static@c]
    |              ^^^^^^^^ this link resolved to a constant, which is not a static
@@ -104,7 +104,7 @@ LL | /// Link to [const@c]
    |              ~~~~~~
 
 error: incompatible link kind for `c`
-  --> $DIR/disambiguator-mismatch.rs:62:14
+  --> $DIR/disambiguator-mismatch.rs:68:14
    |
 LL | /// Link to [fn@c]
    |              ^^^^ this link resolved to a constant, which is not a function
@@ -115,7 +115,7 @@ LL | /// Link to [const@c]
    |              ~~~~~~
 
 error: incompatible link kind for `c`
-  --> $DIR/disambiguator-mismatch.rs:67:14
+  --> $DIR/disambiguator-mismatch.rs:73:14
    |
 LL | /// Link to [c()]
    |              ^^^ this link resolved to a constant, which is not a function
@@ -127,7 +127,7 @@ LL + /// Link to [const@c]
    |
 
 error: incompatible link kind for `f`
-  --> $DIR/disambiguator-mismatch.rs:72:14
+  --> $DIR/disambiguator-mismatch.rs:78:14
    |
 LL | /// Link to [const@f]
    |              ^^^^^^^ this link resolved to a function, which is not a constant
@@ -139,7 +139,7 @@ LL + /// Link to [f()]
    |
 
 error: unresolved link to `std`
-  --> $DIR/disambiguator-mismatch.rs:77:14
+  --> $DIR/disambiguator-mismatch.rs:83:14
    |
 LL | /// Link to [fn@std]
    |              ^^^^^^ this link resolves to the crate `std`, which is not in the value namespace
@@ -149,5 +149,27 @@ help: to link to the crate, prefix with `mod@`
 LL | /// Link to [mod@std]
    |              ~~~~
 
-error: aborting due to 13 previous errors
+error: incompatible link kind for `X::y`
+  --> $DIR/disambiguator-mismatch.rs:88:14
+   |
+LL | /// Link to [method@X::y]
+   |              ^^^^^^^^^^^ this link resolved to a field, which is not a function
+   |
+help: to link to the field, prefix with `field@`
+   |
+LL | /// Link to [field@X::y]
+   |              ~~~~~~
+
+error: incompatible link kind for `S::A`
+  --> $DIR/disambiguator-mismatch.rs:93:14
+   |
+LL | /// Link to [field@S::A]
+   |              ^^^^^^^^^^ this link resolved to a unit variant, which is not a field
+   |
+help: to link to the unit variant, prefix with `variant@`
+   |
+LL | /// Link to [variant@S::A]
+   |              ~~~~~~~~
+
+error: aborting due to 15 previous errors
 

tests/rustdoc-ui/intra-doc/field-ice.rs

@@ -4,8 +4,8 @@
 /// [`Foo::bar`]
 /// [`Foo::bar()`]
 //~^ERROR incompatible link kind for `Foo::bar`
-//~|HELP to link to the field, remove the disambiguator
+//~|HELP to link to the field, prefix with `field@`
 //~|NOTE this link resolved to a field, which is not a function
 pub struct Foo {
-    pub bar: u8
+    pub bar: u8,
 }

tests/rustdoc-ui/intra-doc/field-ice.stderr

@@ -9,10 +9,11 @@ note: the lint level is defined here
    |
 LL | #![deny(rustdoc::broken_intra_doc_links)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-help: to link to the field, remove the disambiguator
+help: to link to the field, prefix with `field@`
+   |
+LL - /// [`Foo::bar()`]
+LL + /// [`field@Foo::bar`]
    |
-LL | /// [`Foo::bar`]
-   |       ~~~~~~~~
 
 error: aborting due to 1 previous error
 

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

@@ -43,10 +43,10 @@ help: to link to the associated function, add parentheses
    |
 LL | /// [`Self::IDENT()`]
    |                  ++
-help: to link to the variant, prefix with `type@`
+help: to link to the variant, prefix with `variant@`
    |
-LL | /// [`type@Self::IDENT`]
+LL | /// [`variant@Self::IDENT`]
-   |       +++++
+   |       ++++++++
 
 error: `Self::IDENT2` is both an associated constant and an associated type
   --> $DIR/issue-108653-associated-items.rs:30:7

tests/rustdoc/intra-doc/field.rs

@@ -1,4 +1,24 @@
 //@ has field/index.html '//a[@href="{{channel}}/core/ops/range/struct.Range.html#structfield.start"]' 'start'
 //@ has field/index.html '//a[@href="{{channel}}/std/io/error/enum.ErrorKind.html#variant.NotFound"]' 'not_found'
+//@ has field/index.html '//a[@href="struct.FieldAndMethod.html#structfield.x"]' 'x'
+//@ has field/index.html '//a[@href="enum.VariantAndMethod.html#variant.X"]' 'X'
 //! [start][std::ops::Range::start]
 //! [not_found][std::io::ErrorKind::NotFound]
+//! [x][field@crate::FieldAndMethod::x]
+//! [X][variant@crate::VariantAndMethod::X]
+
+pub struct FieldAndMethod {
+    pub x: i32,
+}
+
+impl FieldAndMethod {
+    pub fn x(&self) {}
+}
+
+pub enum VariantAndMethod {
+    X {},
+}
+
+impl VariantAndMethod {
+    fn X() {}
+}