Skip to content

Commit

Permalink
Fix intra-doc links for Self on primitives
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@jyn514
jyn514 committed Nov 29, 2020
1 parent e37f25a commit 6ab1f05
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 123 deletions.
185 changes: 62 additions & 123 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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)?;
Expand Down Expand Up @@ -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>> {
Expand Down Expand Up @@ -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(|| {
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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
.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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -945,9 +905,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,
Expand Down Expand Up @@ -980,9 +939,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>>,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1122,7 +1080,6 @@ impl LinkCollector<'_, '_> {
item,
dox,
path_str,
current_item,
module_id,
extra_fragment,
&ori_link,
Expand Down Expand Up @@ -1243,15 +1200,14 @@ impl LinkCollector<'_, '_> {
item: &Item,
dox: &str,
path_str: &str,
current_item: &Option<String>,
base_node: DefId,
extra_fragment: Option<String>,
ori_link: &str,
link_range: Option<Range<usize>>,
) -> 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.
Expand All @@ -1264,7 +1220,6 @@ impl LinkCollector<'_, '_> {
new_ns,
path_str,
base_node,
&current_item,
&extra_fragment,
) {
kind = ResolutionFailure::WrongNamespace(res, ns);
Expand Down Expand Up @@ -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,
&current_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)
Expand All @@ -1315,13 +1264,7 @@ impl LinkCollector<'_, '_> {
}
Err(ErrorKind::Resolve(box kind)) => Err(kind),
},
value_ns: match self.resolve(
path_str,
ValueNS,
&current_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);
Expand Down Expand Up @@ -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,
&current_item,
&extra_fragment,
) {
if let Some(res) =
self.check_full_res(ns, path_str, base_node, &extra_fragment)
{
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
break;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -2131,7 +2070,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() {
Expand Down
36 changes: 36 additions & 0 deletions src/test/rustdoc/intra-link-prim-self.rs
View file
@@ -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() {}
}

0 comments on commit 6ab1f05

Please sign in to comment.