From 6ab1f05697c3f2df4e439a05ebcee479a9a16d80 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Tue, 8 Sep 2020 00:07:40 -0400 Subject: [PATCH 1/3] Fix intra-doc links for `Self` on 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 stringifies DefIds, then resolves them a second time. This is really silly and rustdoc should just use DefIds throughout. Fixing this is a larger task than I want to take on right now. --- .../passes/collect_intra_doc_links.rs | 185 ++++++------------ src/test/rustdoc/intra-link-prim-self.rs | 36 ++++ 2 files changed, 98 insertions(+), 123 deletions(-) create mode 100644 src/test/rustdoc/intra-link-prim-self.rs diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 6aa46b24a0e96..c1fbf6d9f5884 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/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, module_id: DefId, ) -> Result<(Res, Option), 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| { - if f == "self" || f == "Self" { - if let Some(name) = current_item.as_ref() { - return name.clone(); - } - } - f.to_owned() - }) + .map(|f| 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, module_id: DefId, extra_fragment: &Option, ) -> Result<(Res, Option), 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| { - if f == "self" || f == "Self" { - if let Some(name) = current_item.as_ref() { - return name.clone(); - } - } - f.to_owned() - }) + .map(|f| 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, extra_fragment: &Option, ) -> Option { // 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 - .resolve(path_str, ns, current_item, module_id, extra_fragment) - .map(|(res, _)| res), + Namespace::TypeNS | Namespace::ValueNS => { + self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res) + } }; let res = match result { @@ -839,77 +821,55 @@ 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 { - clean::ModuleItem(..) => { - if item.attrs.inner_docs { - if item.def_id.is_top_level_module() { item.name.clone() } else { None } - } else { - match parent_node.or(self.mod_ids.last().copied()) { - Some(parent) if !parent.is_top_level_module() => { - Some(self.cx.tcx.item_name(parent).to_string()) - } - _ => None, - } - } - } - clean::ImplItem(clean::Impl { ref for_, .. }) => { - for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string()) - } - // we don't display docs on `extern crate` items anyway, so don't process them. - clean::ExternCrateItem(..) => { - debug!("ignoring extern crate item {:?}", item.def_id); - return Some(self.fold_item_recur(item)); - } - clean::ImportItem(Import { kind: clean::ImportKind::Simple(ref name, ..), .. }) => { - Some(name.clone()) - } - clean::MacroItem(..) => None, - _ => item.name.clone(), + // find item's parent to resolve `Self` in item's docs below + debug!("looking for the `Self` type"); + let self_id = if item.is_fake() { + None + } else if matches!( + self.cx.tcx.def_kind(item.def_id), + DefKind::AssocConst + | DefKind::AssocFn + | DefKind::AssocTy + | DefKind::Variant + | DefKind::Field + ) { + self.cx.tcx.parent(item.def_id) + // HACK(jynelson): `clean` marks associated types as `TypedefItem`, not as `AssocTypeItem`. + // 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, + // regardless of what rustdoc wants to call it. + } else if let Some(parent) = self.cx.tcx.parent(item.def_id) { + let parent_kind = self.cx.tcx.def_kind(parent); + Some(if parent_kind == DefKind::Impl { parent } else { item.def_id }) + } else { + Some(item.def_id) }; + // FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly + let self_name = self_id.and_then(|self_id| { + use ty::TyKind; + if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) { + // using `ty.to_string()` directly has issues with shortening paths + // FIXME: this is a hack, isn't there a better way? + let ty = self.cx.tcx.type_of(self_id); + let name = match ty.kind() { + TyKind::Adt(def, _) => Some(self.cx.tcx.item_name(def.did).to_string()), + other if other.is_primitive() => Some(ty.to_string()), + _ => None, + }; + debug!("using type_of(): {:?}", name); + 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 +905,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { let link = self.resolve_link( &item, &combined_docs, - ¤t_item, + &self_name, parent_node, - &parent_name, krate, ori_link, link_range, @@ -980,9 +939,8 @@ impl LinkCollector<'_, '_> { &self, item: &Item, dox: &str, - current_item: &Option, + self_name: &Option, parent_node: Option, - parent_name: &Option, krate: CrateNum, ori_link: String, link_range: Option>, @@ -1069,7 +1027,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 +1080,6 @@ impl LinkCollector<'_, '_> { item, dox, path_str, - current_item, module_id, extra_fragment, &ori_link, @@ -1243,7 +1200,6 @@ impl LinkCollector<'_, '_> { item: &Item, dox: &str, path_str: &str, - current_item: &Option, base_node: DefId, extra_fragment: Option, ori_link: &str, @@ -1251,7 +1207,7 @@ impl LinkCollector<'_, '_> { ) -> Option<(Res, Option)> { match disambiguator.map(Disambiguator::ns) { Some(ns @ (ValueNS | TypeNS)) => { - match self.resolve(path_str, ns, ¤t_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 +1220,6 @@ impl LinkCollector<'_, '_> { new_ns, path_str, base_node, - ¤t_item, &extra_fragment, ) { kind = ResolutionFailure::WrongNamespace(res, ns); @@ -1298,13 +1253,7 @@ impl LinkCollector<'_, '_> { macro_ns: self .resolve_macro(path_str, base_node) .map(|res| (res, extra_fragment.clone())), - type_ns: match self.resolve( - path_str, - TypeNS, - ¤t_item, - base_node, - &extra_fragment, - ) { + type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) { Ok(res) => { debug!("got res in TypeNS: {:?}", res); Ok(res) @@ -1315,13 +1264,7 @@ impl LinkCollector<'_, '_> { } Err(ErrorKind::Resolve(box kind)) => Err(kind), }, - value_ns: match self.resolve( - path_str, - ValueNS, - ¤t_item, - base_node, - &extra_fragment, - ) { + value_ns: match self.resolve(path_str, ValueNS, 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 +1332,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( - ns, - path_str, - base_node, - ¤t_item, - &extra_fragment, - ) { + if let Some(res) = + self.check_full_res(ns, path_str, base_node, &extra_fragment) + { kind = ResolutionFailure::WrongNamespace(res, MacroNS); break; } @@ -1734,7 +1673,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 +2070,7 @@ fn strip_generics_from_path(path_str: &str) -> Result segment.push(chr), } - debug!("raw segment: {:?}", segment); + trace!("raw segment: {:?}", segment); } if !segment.is_empty() { diff --git a/src/test/rustdoc/intra-link-prim-self.rs b/src/test/rustdoc/intra-link-prim-self.rs new file mode 100644 index 0000000000000..1189d266c536e --- /dev/null +++ b/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() {} +} From aa8c9b0d294717a566b0d69721e8c7648309667c Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 29 Nov 2020 13:37:43 -0500 Subject: [PATCH 2/3] Remove `TypeKind` hack in favor of `with_crate_prefix` --- src/librustdoc/passes/collect_intra_doc_links.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index c1fbf6d9f5884..3dcc0f240a3f8 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -847,18 +847,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { // FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly let self_name = self_id.and_then(|self_id| { - use ty::TyKind; if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) { // using `ty.to_string()` directly has issues with shortening paths - // FIXME: this is a hack, isn't there a better way? let ty = self.cx.tcx.type_of(self_id); - let name = match ty.kind() { - TyKind::Adt(def, _) => Some(self.cx.tcx.item_name(def.did).to_string()), - other if other.is_primitive() => Some(ty.to_string()), - _ => None, - }; - debug!("using type_of(): {:?}", name); - name + 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); From 2b17f025610077b2b45bdb6401756e404e6c34b2 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 29 Nov 2020 13:49:44 -0500 Subject: [PATCH 3/3] Add test for cross-crate Self --- src/test/rustdoc/intra-doc-crate/auxiliary/self.rs | 7 +++++++ src/test/rustdoc/intra-doc-crate/self.rs | 6 ++++++ 2 files changed, 13 insertions(+) create mode 100644 src/test/rustdoc/intra-doc-crate/auxiliary/self.rs create mode 100644 src/test/rustdoc/intra-doc-crate/self.rs diff --git a/src/test/rustdoc/intra-doc-crate/auxiliary/self.rs b/src/test/rustdoc/intra-doc-crate/auxiliary/self.rs new file mode 100644 index 0000000000000..cdfe842f3ccbb --- /dev/null +++ b/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() {} +} diff --git a/src/test/rustdoc/intra-doc-crate/self.rs b/src/test/rustdoc/intra-doc-crate/self.rs new file mode 100644 index 0000000000000..62aef8e85afc9 --- /dev/null +++ b/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;