Skip to content

Commit b7ebc6b

Browse files
borsbors
committed
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.
2 parents 28b86e0 + 2b17f02 commit b7ebc6b

File tree

4 files changed

+105
-123
lines changed

4 files changed

+105
-123
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs‎

Lines changed: 56 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ use std::cell::Cell;
3131
use std::mem;
3232
use std::ops::Range;
3333

34-
use crate::clean::{self, Crate, GetDefId, Import, Item, ItemLink, PrimitiveType};
34+
use crate::clean::{self, Crate, GetDefId, Item, ItemLink, PrimitiveType};
3535
use crate::core::DocContext;
3636
use crate::fold::DocFolder;
3737
use crate::html::markdown::markdown_links;
@@ -195,7 +195,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
195195
fn variant_field(
196196
&self,
197197
path_str: &'path str,
198-
current_item: &Option<String>,
199198
module_id: DefId,
200199
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
201200
let cx = self.cx;
@@ -218,14 +217,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
218217
split.next().map(|f| (f, Symbol::intern(f))).ok_or_else(no_res)?;
219218
let path = split
220219
.next()
221-
.map(|f| {
222-
if f == "self" || f == "Self" {
223-
if let Some(name) = current_item.as_ref() {
224-
return name.clone();
225-
}
226-
}
227-
f.to_owned()
228-
})
220+
.map(|f| f.to_owned())
229221
// If there's no third component, we saw `[a::b]` before and it failed to resolve.
230222
// So there's no partial res.
231223
.ok_or_else(no_res)?;
@@ -405,8 +397,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
405397
&self,
406398
path_str: &'path str,
407399
ns: Namespace,
408-
// FIXME(#76467): This is for `Self`, and it's wrong.
409-
current_item: &Option<String>,
410400
module_id: DefId,
411401
extra_fragment: &Option<String>,
412402
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
@@ -449,14 +439,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
449439
let (item_str, item_name) = split.next().map(|i| (i, Symbol::intern(i))).unwrap();
450440
let path_root = split
451441
.next()
452-
.map(|f| {
453-
if f == "self" || f == "Self" {
454-
if let Some(name) = current_item.as_ref() {
455-
return name.clone();
456-
}
457-
}
458-
f.to_owned()
459-
})
442+
.map(|f| f.to_owned())
460443
// If there's no `::`, it's not an associated item.
461444
// So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved.
462445
.ok_or_else(|| {
@@ -477,7 +460,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
477460
} else {
478461
// FIXME: this is duplicated on the end of this function.
479462
return if ns == Namespace::ValueNS {
480-
self.variant_field(path_str, current_item, module_id)
463+
self.variant_field(path_str, module_id)
481464
} else {
482465
Err(ResolutionFailure::NotResolved {
483466
module_id,
@@ -617,7 +600,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
617600
};
618601
res.unwrap_or_else(|| {
619602
if ns == Namespace::ValueNS {
620-
self.variant_field(path_str, current_item, module_id)
603+
self.variant_field(path_str, module_id)
621604
} else {
622605
Err(ResolutionFailure::NotResolved {
623606
module_id,
@@ -640,15 +623,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
640623
ns: Namespace,
641624
path_str: &str,
642625
module_id: DefId,
643-
current_item: &Option<String>,
644626
extra_fragment: &Option<String>,
645627
) -> Option<Res> {
646628
// resolve() can't be used for macro namespace
647629
let result = match ns {
648630
Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from),
649-
Namespace::TypeNS | Namespace::ValueNS => self
650-
.resolve(path_str, ns, current_item, module_id, extra_fragment)
651-
.map(|(res, _)| res),
631+
Namespace::TypeNS | Namespace::ValueNS => {
632+
self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res)
633+
}
652634
};
653635

654636
let res = match result {
@@ -839,77 +821,49 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
839821
trace!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.def_id);
840822
}
841823

842-
let current_item = match item.kind {
843-
clean::ModuleItem(..) => {
844-
if item.attrs.inner_docs {
845-
if item.def_id.is_top_level_module() { item.name.clone() } else { None }
846-
} else {
847-
match parent_node.or(self.mod_ids.last().copied()) {
848-
Some(parent) if !parent.is_top_level_module() => {
849-
Some(self.cx.tcx.item_name(parent).to_string())
850-
}
851-
_ => None,
852-
}
853-
}
854-
}
855-
clean::ImplItem(clean::Impl { ref for_, .. }) => {
856-
for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string())
857-
}
858-
// we don't display docs on `extern crate` items anyway, so don't process them.
859-
clean::ExternCrateItem(..) => {
860-
debug!("ignoring extern crate item {:?}", item.def_id);
861-
return Some(self.fold_item_recur(item));
862-
}
863-
clean::ImportItem(Import { kind: clean::ImportKind::Simple(ref name, ..), .. }) => {
864-
Some(name.clone())
865-
}
866-
clean::MacroItem(..) => None,
867-
_ => item.name.clone(),
824+
// find item's parent to resolve `Self` in item's docs below
825+
debug!("looking for the `Self` type");
826+
let self_id = if item.is_fake() {
827+
None
828+
} else if matches!(
829+
self.cx.tcx.def_kind(item.def_id),
830+
DefKind::AssocConst
831+
| DefKind::AssocFn
832+
| DefKind::AssocTy
833+
| DefKind::Variant
834+
| DefKind::Field
835+
) {
836+
self.cx.tcx.parent(item.def_id)
837+
// HACK(jynelson): `clean` marks associated types as `TypedefItem`, not as `AssocTypeItem`.
838+
// Fixing this breaks `fn render_deref_methods`.
839+
// As a workaround, see if the parent of the item is an `impl`; if so this must be an associated item,
840+
// regardless of what rustdoc wants to call it.
841+
} else if let Some(parent) = self.cx.tcx.parent(item.def_id) {
842+
let parent_kind = self.cx.tcx.def_kind(parent);
843+
Some(if parent_kind == DefKind::Impl { parent } else { item.def_id })
844+
} else {
845+
Some(item.def_id)
868846
};
869847

848+
// FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly
849+
let self_name = self_id.and_then(|self_id| {
850+
if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) {
851+
// using `ty.to_string()` directly has issues with shortening paths
852+
let ty = self.cx.tcx.type_of(self_id);
853+
let name = ty::print::with_crate_prefix(|| ty.to_string());
854+
debug!("using type_of(): {}", name);
855+
Some(name)
856+
} else {
857+
let name = self.cx.tcx.opt_item_name(self_id).map(|sym| sym.to_string());
858+
debug!("using item_name(): {:?}", name);
859+
name
860+
}
861+
});
862+
870863
if item.is_mod() && item.attrs.inner_docs {
871864
self.mod_ids.push(item.def_id);
872865
}
873866

874-
// find item's parent to resolve `Self` in item's docs below
875-
// FIXME(#76467, #75809): this is a mess and doesn't handle cross-crate
876-
// re-exports
877-
let parent_name = self.cx.as_local_hir_id(item.def_id).and_then(|item_hir| {
878-
let parent_hir = self.cx.tcx.hir().get_parent_item(item_hir);
879-
let item_parent = self.cx.tcx.hir().find(parent_hir);
880-
match item_parent {
881-
Some(hir::Node::Item(hir::Item {
882-
kind:
883-
hir::ItemKind::Impl {
884-
self_ty:
885-
hir::Ty {
886-
kind:
887-
hir::TyKind::Path(hir::QPath::Resolved(
888-
_,
889-
hir::Path { segments, .. },
890-
)),
891-
..
892-
},
893-
..
894-
},
895-
..
896-
})) => segments.first().map(|seg| seg.ident.to_string()),
897-
Some(hir::Node::Item(hir::Item {
898-
ident, kind: hir::ItemKind::Enum(..), ..
899-
}))
900-
| Some(hir::Node::Item(hir::Item {
901-
ident, kind: hir::ItemKind::Struct(..), ..
902-
}))
903-
| Some(hir::Node::Item(hir::Item {
904-
ident, kind: hir::ItemKind::Union(..), ..
905-
}))
906-
| Some(hir::Node::Item(hir::Item {
907-
ident, kind: hir::ItemKind::Trait(..), ..
908-
})) => Some(ident.to_string()),
909-
_ => None,
910-
}
911-
});
912-
913867
// We want to resolve in the lexical scope of the documentation.
914868
// In the presence of re-exports, this is not the same as the module of the item.
915869
// 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> {
945899
let link = self.resolve_link(
946900
&item,
947901
&combined_docs,
948-
&current_item,
902+
&self_name,
949903
parent_node,
950-
&parent_name,
951904
krate,
952905
ori_link,
953906
link_range,
@@ -980,9 +933,8 @@ impl LinkCollector<'_, '_> {
980933
&self,
981934
item: &Item,
982935
dox: &str,
983-
current_item: &Option<String>,
936+
self_name: &Option<String>,
984937
parent_node: Option<DefId>,
985-
parent_name: &Option<String>,
986938
krate: CrateNum,
987939
ori_link: String,
988940
link_range: Option<Range<usize>>,
@@ -1069,7 +1021,7 @@ impl LinkCollector<'_, '_> {
10691021
let resolved_self;
10701022
// replace `Self` with suitable item's parent name
10711023
if path_str.starts_with("Self::") {
1072-
if let Some(ref name) = parent_name {
1024+
if let Some(ref name) = self_name {
10731025
resolved_self = format!("{}::{}", name, &path_str[6..]);
10741026
path_str = &resolved_self;
10751027
}
@@ -1122,7 +1074,6 @@ impl LinkCollector<'_, '_> {
11221074
item,
11231075
dox,
11241076
path_str,
1125-
current_item,
11261077
module_id,
11271078
extra_fragment,
11281079
&ori_link,
@@ -1243,15 +1194,14 @@ impl LinkCollector<'_, '_> {
12431194
item: &Item,
12441195
dox: &str,
12451196
path_str: &str,
1246-
current_item: &Option<String>,
12471197
base_node: DefId,
12481198
extra_fragment: Option<String>,
12491199
ori_link: &str,
12501200
link_range: Option<Range<usize>>,
12511201
) -> Option<(Res, Option<String>)> {
12521202
match disambiguator.map(Disambiguator::ns) {
12531203
Some(ns @ (ValueNS | TypeNS)) => {
1254-
match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {
1204+
match self.resolve(path_str, ns, base_node, &extra_fragment) {
12551205
Ok(res) => Some(res),
12561206
Err(ErrorKind::Resolve(box mut kind)) => {
12571207
// We only looked in one namespace. Try to give a better error if possible.
@@ -1264,7 +1214,6 @@ impl LinkCollector<'_, '_> {
12641214
new_ns,
12651215
path_str,
12661216
base_node,
1267-
&current_item,
12681217
&extra_fragment,
12691218
) {
12701219
kind = ResolutionFailure::WrongNamespace(res, ns);
@@ -1298,13 +1247,7 @@ impl LinkCollector<'_, '_> {
12981247
macro_ns: self
12991248
.resolve_macro(path_str, base_node)
13001249
.map(|res| (res, extra_fragment.clone())),
1301-
type_ns: match self.resolve(
1302-
path_str,
1303-
TypeNS,
1304-
&current_item,
1305-
base_node,
1306-
&extra_fragment,
1307-
) {
1250+
type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
13081251
Ok(res) => {
13091252
debug!("got res in TypeNS: {:?}", res);
13101253
Ok(res)
@@ -1315,13 +1258,7 @@ impl LinkCollector<'_, '_> {
13151258
}
13161259
Err(ErrorKind::Resolve(box kind)) => Err(kind),
13171260
},
1318-
value_ns: match self.resolve(
1319-
path_str,
1320-
ValueNS,
1321-
&current_item,
1322-
base_node,
1323-
&extra_fragment,
1324-
) {
1261+
value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
13251262
Ok(res) => Ok(res),
13261263
Err(ErrorKind::AnchorFailure(msg)) => {
13271264
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
@@ -1389,13 +1326,9 @@ impl LinkCollector<'_, '_> {
13891326
Err(mut kind) => {
13901327
// `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
13911328
for &ns in &[TypeNS, ValueNS] {
1392-
if let Some(res) = self.check_full_res(
1393-
ns,
1394-
path_str,
1395-
base_node,
1396-
&current_item,
1397-
&extra_fragment,
1398-
) {
1329+
if let Some(res) =
1330+
self.check_full_res(ns, path_str, base_node, &extra_fragment)
1331+
{
13991332
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
14001333
break;
14011334
}
@@ -1734,7 +1667,7 @@ fn resolution_failure(
17341667
name = start;
17351668
for &ns in &[TypeNS, ValueNS, MacroNS] {
17361669
if let Some(res) =
1737-
collector.check_full_res(ns, &start, module_id, &None, &None)
1670+
collector.check_full_res(ns, &start, module_id, &None)
17381671
{
17391672
debug!("found partial_res={:?}", res);
17401673
*partial_res = Some(res);
@@ -2131,7 +2064,7 @@ fn strip_generics_from_path(path_str: &str) -> Result<String, ResolutionFailure<
21312064
}
21322065
_ => segment.push(chr),
21332066
}
2134-
debug!("raw segment: {:?}", segment);
2067+
trace!("raw segment: {:?}", segment);
21352068
}
21362069

21372070
if !segment.is_empty() {

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#![crate_name = "cross_crate_self"]
2+
pub struct S;
3+
4+
impl S {
5+
/// Link to [Self::f]
6+
pub fn f() {}
7+
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// aux-build:self.rs
2+
3+
extern crate cross_crate_self;
4+
5+
// @has self/struct.S.html '//a[@href="../self/struct.S.html#method.f"]' "Self::f"
6+
pub use cross_crate_self::S;

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// ignore-tidy-linelength
2+
#![deny(broken_intra_doc_links)]
3+
#![feature(lang_items)]
4+
#![feature(no_core)]
5+
#![no_core]
6+
7+
#[lang = "usize"]
8+
/// [Self::f]
9+
/// [Self::MAX]
10+
// @has intra_link_prim_self/primitive.usize.html
11+
// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#method.f"]' 'Self::f'
12+
// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX"]' 'Self::MAX'
13+
impl usize {
14+
/// Some docs
15+
pub fn f() {}
16+
17+
/// 10 and 2^32 are basically the same.
18+
pub const MAX: usize = 10;
19+
20+
// FIXME(#8995) uncomment this when associated types in inherent impls are supported
21+
// @ has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedtype.ME"]' 'Self::ME'
22+
// / [Self::ME]
23+
//pub type ME = usize;
24+
}
25+
26+
#[doc(primitive = "usize")]
27+
/// This has some docs.
28+
mod usize {}
29+
30+
/// [S::f]
31+
/// [Self::f]
32+
pub struct S;
33+
34+
impl S {
35+
pub fn f() {}
36+
}

0 commit comments

Comments
 (0)