Skip to content

Commit 6ab1f05

Browse files
jyn514jyn514
committed
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.
1 parent e37f25a commit 6ab1f05

File tree

2 files changed

+98
-123
lines changed

2 files changed

+98
-123
lines changed

Diff for: ‎src/librustdoc/passes/collect_intra_doc_links.rs

+62-123
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,55 @@ 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+
use ty::TyKind;
851+
if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) {
852+
// using `ty.to_string()` directly has issues with shortening paths
853+
// FIXME: this is a hack, isn't there a better way?
854+
let ty = self.cx.tcx.type_of(self_id);
855+
let name = match ty.kind() {
856+
TyKind::Adt(def, _) => Some(self.cx.tcx.item_name(def.did).to_string()),
857+
other if other.is_primitive() => Some(ty.to_string()),
858+
_ => None,
859+
};
860+
debug!("using type_of(): {:?}", name);
861+
name
862+
} else {
863+
let name = self.cx.tcx.opt_item_name(self_id).map(|sym| sym.to_string());
864+
debug!("using item_name(): {:?}", name);
865+
name
866+
}
867+
});
868+
870869
if item.is_mod() && item.attrs.inner_docs {
871870
self.mod_ids.push(item.def_id);
872871
}
873872

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-
913873
// We want to resolve in the lexical scope of the documentation.
914874
// In the presence of re-exports, this is not the same as the module of the item.
915875
// 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> {
945905
let link = self.resolve_link(
946906
&item,
947907
&combined_docs,
948-
&current_item,
908+
&self_name,
949909
parent_node,
950-
&parent_name,
951910
krate,
952911
ori_link,
953912
link_range,
@@ -980,9 +939,8 @@ impl LinkCollector<'_, '_> {
980939
&self,
981940
item: &Item,
982941
dox: &str,
983-
current_item: &Option<String>,
942+
self_name: &Option<String>,
984943
parent_node: Option<DefId>,
985-
parent_name: &Option<String>,
986944
krate: CrateNum,
987945
ori_link: String,
988946
link_range: Option<Range<usize>>,
@@ -1069,7 +1027,7 @@ impl LinkCollector<'_, '_> {
10691027
let resolved_self;
10701028
// replace `Self` with suitable item's parent name
10711029
if path_str.starts_with("Self::") {
1072-
if let Some(ref name) = parent_name {
1030+
if let Some(ref name) = self_name {
10731031
resolved_self = format!("{}::{}", name, &path_str[6..]);
10741032
path_str = &resolved_self;
10751033
}
@@ -1122,7 +1080,6 @@ impl LinkCollector<'_, '_> {
11221080
item,
11231081
dox,
11241082
path_str,
1125-
current_item,
11261083
module_id,
11271084
extra_fragment,
11281085
&ori_link,
@@ -1243,15 +1200,14 @@ impl LinkCollector<'_, '_> {
12431200
item: &Item,
12441201
dox: &str,
12451202
path_str: &str,
1246-
current_item: &Option<String>,
12471203
base_node: DefId,
12481204
extra_fragment: Option<String>,
12491205
ori_link: &str,
12501206
link_range: Option<Range<usize>>,
12511207
) -> Option<(Res, Option<String>)> {
12521208
match disambiguator.map(Disambiguator::ns) {
12531209
Some(ns @ (ValueNS | TypeNS)) => {
1254-
match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {
1210+
match self.resolve(path_str, ns, base_node, &extra_fragment) {
12551211
Ok(res) => Some(res),
12561212
Err(ErrorKind::Resolve(box mut kind)) => {
12571213
// We only looked in one namespace. Try to give a better error if possible.
@@ -1264,7 +1220,6 @@ impl LinkCollector<'_, '_> {
12641220
new_ns,
12651221
path_str,
12661222
base_node,
1267-
&current_item,
12681223
&extra_fragment,
12691224
) {
12701225
kind = ResolutionFailure::WrongNamespace(res, ns);
@@ -1298,13 +1253,7 @@ impl LinkCollector<'_, '_> {
12981253
macro_ns: self
12991254
.resolve_macro(path_str, base_node)
13001255
.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-
) {
1256+
type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
13081257
Ok(res) => {
13091258
debug!("got res in TypeNS: {:?}", res);
13101259
Ok(res)
@@ -1315,13 +1264,7 @@ impl LinkCollector<'_, '_> {
13151264
}
13161265
Err(ErrorKind::Resolve(box kind)) => Err(kind),
13171266
},
1318-
value_ns: match self.resolve(
1319-
path_str,
1320-
ValueNS,
1321-
&current_item,
1322-
base_node,
1323-
&extra_fragment,
1324-
) {
1267+
value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
13251268
Ok(res) => Ok(res),
13261269
Err(ErrorKind::AnchorFailure(msg)) => {
13271270
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
@@ -1389,13 +1332,9 @@ impl LinkCollector<'_, '_> {
13891332
Err(mut kind) => {
13901333
// `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
13911334
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-
) {
1335+
if let Some(res) =
1336+
self.check_full_res(ns, path_str, base_node, &extra_fragment)
1337+
{
13991338
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
14001339
break;
14011340
}
@@ -1734,7 +1673,7 @@ fn resolution_failure(
17341673
name = start;
17351674
for &ns in &[TypeNS, ValueNS, MacroNS] {
17361675
if let Some(res) =
1737-
collector.check_full_res(ns, &start, module_id, &None, &None)
1676+
collector.check_full_res(ns, &start, module_id, &None)
17381677
{
17391678
debug!("found partial_res={:?}", res);
17401679
*partial_res = Some(res);
@@ -2131,7 +2070,7 @@ fn strip_generics_from_path(path_str: &str) -> Result<String, ResolutionFailure<
21312070
}
21322071
_ => segment.push(chr),
21332072
}
2134-
debug!("raw segment: {:?}", segment);
2073+
trace!("raw segment: {:?}", segment);
21352074
}
21362075

21372076
if !segment.is_empty() {

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

+36
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)