Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Nov 30, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ab1f05
Fix intra-doc links for `Self` on primitives
jyn514 Sep 8, 2020
aa8c9b0
Remove `TypeKind` hack in favor of `with_crate_prefix`
jyn514 Nov 29, 2020
2b17f02
Add test for cross-crate Self
jyn514 Nov 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 56 additions & 123 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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,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 {
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| {
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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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>>,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1122,7 +1074,6 @@ impl LinkCollector<'_, '_> {
item,
dox,
path_str,
current_item,
module_id,
extra_fragment,
&ori_link,
Expand Down Expand Up @@ -1243,15 +1194,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 +1214,6 @@ impl LinkCollector<'_, '_> {
new_ns,
path_str,
base_node,
&current_item,
&extra_fragment,
) {
kind = ResolutionFailure::WrongNamespace(res, ns);
Expand Down Expand Up @@ -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(
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 +1258,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 +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(
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 +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);
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 7 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![crate_name = "cross_crate_self"]
pub struct S;

impl S {
/// Link to [Self::f]
pub fn f() {}
}
6 changes: 6 additions & 0 deletions src/test/rustdoc/intra-doc-crate/self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
36 changes: 36 additions & 0 deletions src/test/rustdoc/intra-link-prim-self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
}