Auto merge of #76467 - jyn514:intra-link-self, r=Manishearth

Fix intra-doc links for `Self` on cross-crate items and primitives

- Remove the difference between `parent_item` and `current_item`; these
  should never have been different.
- Remove `current_item` from `resolve` and `variant_field` so that
  `Self` is only substituted in one place at the very start.
- Resolve the current item as a `DefId`, not a `HirId`. This is what
  actually fixed the bug.

Hacks:
- `clean` uses `TypedefItem` when it _really_ should be
  `AssociatedTypeItem`. I tried fixing this without success and hacked
  around it instead (see comments)
- This second-guesses the `to_string()` impl since it wants
  fully-qualified paths. Possibly there's a better way to do this.

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -31,7 +31,7 @@ use std::cell::Cell;
 use std::mem;
 use std::ops::Range;
 
-use crate::clean::{self, Crate, GetDefId, Import, Item, ItemLink, PrimitiveType};
+use crate::clean::{self, Crate, GetDefId, Item, ItemLink, PrimitiveType};
 use crate::core::DocContext;
 use crate::fold::DocFolder;
 use crate::html::markdown::markdown_links;
@@ -195,7 +195,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     fn variant_field(
         &self,
         path_str: &'path str,
-        current_item: &Option<String>,
         module_id: DefId,
     ) -> Result<(Res, Option<String>), ErrorKind<'path>> {
         let cx = self.cx;
@@ -218,14 +217,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
             split.next().map(|f| (f, Symbol::intern(f))).ok_or_else(no_res)?;
         let path = split
             .next()
-            .map(|f| {
+            .map(|f| f.to_owned())
-                if f == "self" || f == "Self" {
-                    if let Some(name) = current_item.as_ref() {
-                        return name.clone();
-                    }
-                }
-                f.to_owned()
-            })
             // If there's no third component, we saw `[a::b]` before and it failed to resolve.
             // So there's no partial res.
             .ok_or_else(no_res)?;
@@ -405,8 +397,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         &self,
         path_str: &'path str,
         ns: Namespace,
-        // FIXME(#76467): This is for `Self`, and it's wrong.
-        current_item: &Option<String>,
         module_id: DefId,
         extra_fragment: &Option<String>,
     ) -> Result<(Res, Option<String>), ErrorKind<'path>> {
@@ -449,14 +439,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         let (item_str, item_name) = split.next().map(|i| (i, Symbol::intern(i))).unwrap();
         let path_root = split
             .next()
-            .map(|f| {
+            .map(|f| f.to_owned())
-                if f == "self" || f == "Self" {
-                    if let Some(name) = current_item.as_ref() {
-                        return name.clone();
-                    }
-                }
-                f.to_owned()
-            })
             // If there's no `::`, it's not an associated item.
             // So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved.
             .ok_or_else(|| {
@@ -477,7 +460,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         } else {
             // FIXME: this is duplicated on the end of this function.
             return if ns == Namespace::ValueNS {
-                self.variant_field(path_str, current_item, module_id)
+                self.variant_field(path_str, module_id)
             } else {
                 Err(ResolutionFailure::NotResolved {
                     module_id,
@@ -617,7 +600,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         };
         res.unwrap_or_else(|| {
             if ns == Namespace::ValueNS {
-                self.variant_field(path_str, current_item, module_id)
+                self.variant_field(path_str, module_id)
             } else {
                 Err(ResolutionFailure::NotResolved {
                     module_id,
@@ -640,15 +623,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         ns: Namespace,
         path_str: &str,
         module_id: DefId,
-        current_item: &Option<String>,
         extra_fragment: &Option<String>,
     ) -> Option<Res> {
         // resolve() can't be used for macro namespace
         let result = match ns {
             Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from),
-            Namespace::TypeNS | Namespace::ValueNS => self
+            Namespace::TypeNS | Namespace::ValueNS => {
-                .resolve(path_str, ns, current_item, module_id, extra_fragment)
+                self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res)
-                .map(|(res, _)| res),
+            }
         };
 
         let res = match result {
@@ -839,77 +821,49 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
             trace!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.def_id);
         }
 
-        let current_item = match item.kind {
+        // find item's parent to resolve `Self` in item's docs below
-            clean::ModuleItem(..) => {
+        debug!("looking for the `Self` type");
-                if item.attrs.inner_docs {
+        let self_id = if item.is_fake() {
-                    if item.def_id.is_top_level_module() { item.name.clone() } else { None }
+            None
-                } else {
+        } else if matches!(
-                    match parent_node.or(self.mod_ids.last().copied()) {
+            self.cx.tcx.def_kind(item.def_id),
-                        Some(parent) if !parent.is_top_level_module() => {
+            DefKind::AssocConst
-                            Some(self.cx.tcx.item_name(parent).to_string())
+                | DefKind::AssocFn
-                        }
+                | DefKind::AssocTy
-                        _ => None,
+                | DefKind::Variant
-                    }
+                | DefKind::Field
-                }
+        ) {
-            }
+            self.cx.tcx.parent(item.def_id)
-            clean::ImplItem(clean::Impl { ref for_, .. }) => {
+        // HACK(jynelson): `clean` marks associated types as `TypedefItem`, not as `AssocTypeItem`.
-                for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string())
+        // Fixing this breaks `fn render_deref_methods`.
-            }
+        // As a workaround, see if the parent of the item is an `impl`; if so this must be an associated item,
-            // we don't display docs on `extern crate` items anyway, so don't process them.
+        // regardless of what rustdoc wants to call it.
-            clean::ExternCrateItem(..) => {
+        } else if let Some(parent) = self.cx.tcx.parent(item.def_id) {
-                debug!("ignoring extern crate item {:?}", item.def_id);
+            let parent_kind = self.cx.tcx.def_kind(parent);
-                return Some(self.fold_item_recur(item));
+            Some(if parent_kind == DefKind::Impl { parent } else { item.def_id })
-            }
+        } else {
-            clean::ImportItem(Import { kind: clean::ImportKind::Simple(ref name, ..), .. }) => {
+            Some(item.def_id)
-                Some(name.clone())
-            }
-            clean::MacroItem(..) => None,
-            _ => item.name.clone(),
         };
 
+        // FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly
+        let self_name = self_id.and_then(|self_id| {
+            if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) {
+                // using `ty.to_string()` directly has issues with shortening paths
+                let ty = self.cx.tcx.type_of(self_id);
+                let name = ty::print::with_crate_prefix(|| ty.to_string());
+                debug!("using type_of(): {}", name);
+                Some(name)
+            } else {
+                let name = self.cx.tcx.opt_item_name(self_id).map(|sym| sym.to_string());
+                debug!("using item_name(): {:?}", name);
+                name
+            }
+        });
+
         if item.is_mod() && item.attrs.inner_docs {
             self.mod_ids.push(item.def_id);
         }
 
-        // find item's parent to resolve `Self` in item's docs below
-        // FIXME(#76467, #75809): this is a mess and doesn't handle cross-crate
-        // re-exports
-        let parent_name = self.cx.as_local_hir_id(item.def_id).and_then(|item_hir| {
-            let parent_hir = self.cx.tcx.hir().get_parent_item(item_hir);
-            let item_parent = self.cx.tcx.hir().find(parent_hir);
-            match item_parent {
-                Some(hir::Node::Item(hir::Item {
-                    kind:
-                        hir::ItemKind::Impl {
-                            self_ty:
-                                hir::Ty {
-                                    kind:
-                                        hir::TyKind::Path(hir::QPath::Resolved(
-                                            _,
-                                            hir::Path { segments, .. },
-                                        )),
-                                    ..
-                                },
-                            ..
-                        },
-                    ..
-                })) => segments.first().map(|seg| seg.ident.to_string()),
-                Some(hir::Node::Item(hir::Item {
-                    ident, kind: hir::ItemKind::Enum(..), ..
-                }))
-                | Some(hir::Node::Item(hir::Item {
-                    ident, kind: hir::ItemKind::Struct(..), ..
-                }))
-                | Some(hir::Node::Item(hir::Item {
-                    ident, kind: hir::ItemKind::Union(..), ..
-                }))
-                | Some(hir::Node::Item(hir::Item {
-                    ident, kind: hir::ItemKind::Trait(..), ..
-                })) => Some(ident.to_string()),
-                _ => None,
-            }
-        });
-
         // We want to resolve in the lexical scope of the documentation.
         // In the presence of re-exports, this is not the same as the module of the item.
         // Rather than merging all documentation into one, resolve it one attribute at a time
@@ -945,9 +899,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                 let link = self.resolve_link(
                     &item,
                     &combined_docs,
-                    &current_item,
+                    &self_name,
                     parent_node,
-                    &parent_name,
                     krate,
                     ori_link,
                     link_range,
@@ -980,9 +933,8 @@ impl LinkCollector<'_, '_> {
         &self,
         item: &Item,
         dox: &str,
-        current_item: &Option<String>,
+        self_name: &Option<String>,
         parent_node: Option<DefId>,
-        parent_name: &Option<String>,
         krate: CrateNum,
         ori_link: String,
         link_range: Option<Range<usize>>,
@@ -1069,7 +1021,7 @@ impl LinkCollector<'_, '_> {
         let resolved_self;
         // replace `Self` with suitable item's parent name
         if path_str.starts_with("Self::") {
-            if let Some(ref name) = parent_name {
+            if let Some(ref name) = self_name {
                 resolved_self = format!("{}::{}", name, &path_str[6..]);
                 path_str = &resolved_self;
             }
@@ -1122,7 +1074,6 @@ impl LinkCollector<'_, '_> {
             item,
             dox,
             path_str,
-            current_item,
             module_id,
             extra_fragment,
             &ori_link,
@@ -1243,7 +1194,6 @@ impl LinkCollector<'_, '_> {
         item: &Item,
         dox: &str,
         path_str: &str,
-        current_item: &Option<String>,
         base_node: DefId,
         extra_fragment: Option<String>,
         ori_link: &str,
@@ -1251,7 +1201,7 @@ impl LinkCollector<'_, '_> {
     ) -> Option<(Res, Option<String>)> {
         match disambiguator.map(Disambiguator::ns) {
             Some(ns @ (ValueNS | TypeNS)) => {
-                match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {
+                match self.resolve(path_str, ns, base_node, &extra_fragment) {
                     Ok(res) => Some(res),
                     Err(ErrorKind::Resolve(box mut kind)) => {
                         // We only looked in one namespace. Try to give a better error if possible.
@@ -1264,7 +1214,6 @@ impl LinkCollector<'_, '_> {
                                     new_ns,
                                     path_str,
                                     base_node,
-                                    &current_item,
                                     &extra_fragment,
                                 ) {
                                     kind = ResolutionFailure::WrongNamespace(res, ns);
@@ -1298,13 +1247,7 @@ impl LinkCollector<'_, '_> {
                     macro_ns: self
                         .resolve_macro(path_str, base_node)
                         .map(|res| (res, extra_fragment.clone())),
-                    type_ns: match self.resolve(
+                    type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
-                        path_str,
-                        TypeNS,
-                        &current_item,
-                        base_node,
-                        &extra_fragment,
-                    ) {
                         Ok(res) => {
                             debug!("got res in TypeNS: {:?}", res);
                             Ok(res)
@@ -1315,13 +1258,7 @@ impl LinkCollector<'_, '_> {
                         }
                         Err(ErrorKind::Resolve(box kind)) => Err(kind),
                     },
-                    value_ns: match self.resolve(
+                    value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
-                        path_str,
-                        ValueNS,
-                        &current_item,
-                        base_node,
-                        &extra_fragment,
-                    ) {
                         Ok(res) => Ok(res),
                         Err(ErrorKind::AnchorFailure(msg)) => {
                             anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
@@ -1389,13 +1326,9 @@ impl LinkCollector<'_, '_> {
                     Err(mut kind) => {
                         // `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
                         for &ns in &[TypeNS, ValueNS] {
-                            if let Some(res) = self.check_full_res(
+                            if let Some(res) =
-                                ns,
+                                self.check_full_res(ns, path_str, base_node, &extra_fragment)
-                                path_str,
+                            {
-                                base_node,
-                                &current_item,
-                                &extra_fragment,
-                            ) {
                                 kind = ResolutionFailure::WrongNamespace(res, MacroNS);
                                 break;
                             }
@@ -1734,7 +1667,7 @@ fn resolution_failure(
                         name = start;
                         for &ns in &[TypeNS, ValueNS, MacroNS] {
                             if let Some(res) =
-                                collector.check_full_res(ns, &start, module_id, &None, &None)
+                                collector.check_full_res(ns, &start, module_id, &None)
                             {
                                 debug!("found partial_res={:?}", res);
                                 *partial_res = Some(res);
@@ -2131,7 +2064,7 @@ fn strip_generics_from_path(path_str: &str) -> Result<String, ResolutionFailure<
             }
             _ => segment.push(chr),
         }
-        debug!("raw segment: {:?}", segment);
+        trace!("raw segment: {:?}", segment);
     }
 
     if !segment.is_empty() {

src/test/rustdoc/intra-doc-crate/auxiliary/self.rs

@@ -0,0 +1,7 @@
+#![crate_name = "cross_crate_self"]
+pub struct S;
+
+impl S {
+    /// Link to [Self::f]
+    pub fn f() {}
+}

src/test/rustdoc/intra-doc-crate/self.rs

@@ -0,0 +1,6 @@
+// aux-build:self.rs
+
+extern crate cross_crate_self;
+
+// @has self/struct.S.html '//a[@href="../self/struct.S.html#method.f"]' "Self::f"
+pub use cross_crate_self::S;

src/test/rustdoc/intra-link-prim-self.rs

@@ -0,0 +1,36 @@
+// ignore-tidy-linelength
+#![deny(broken_intra_doc_links)]
+#![feature(lang_items)]
+#![feature(no_core)]
+#![no_core]
+
+#[lang = "usize"]
+/// [Self::f]
+/// [Self::MAX]
+// @has intra_link_prim_self/primitive.usize.html
+// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#method.f"]' 'Self::f'
+// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX"]' 'Self::MAX'
+impl usize {
+    /// Some docs
+    pub fn f() {}
+
+    /// 10 and 2^32 are basically the same.
+    pub const MAX: usize = 10;
+
+    // FIXME(#8995) uncomment this when associated types in inherent impls are supported
+    // @ has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedtype.ME"]' 'Self::ME'
+    // / [Self::ME]
+    //pub type ME = usize;
+}
+
+#[doc(primitive = "usize")]
+/// This has some docs.
+mod usize {}
+
+/// [S::f]
+/// [Self::f]
+pub struct S;
+
+impl S {
+    pub fn f() {}
+}