Skip to content

Commit d5918b4

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 doesn't replace `Self` if there's no parent for the current item. In theory this should be possible, but it caused panics when I tried to implement it. - This replaces `Self` in the _displayed_ text, not just when resolving. This should probably be fixed, but it was an existing issue so I didn't fix it here.
1 parent f1dab24 commit d5918b4

File tree

2 files changed

+86
-122
lines changed

2 files changed

+86
-122
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs‎

Lines changed: 50 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
102102
fn variant_field(
103103
&self,
104104
path_str: &'path str,
105-
current_item: &Option<String>,
106105
module_id: DefId,
107106
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
108107
let cx = self.cx;
@@ -125,14 +124,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
125124
split.next().map(|f| (f, Symbol::intern(f))).ok_or_else(no_res)?;
126125
let path = split
127126
.next()
128-
.map(|f| {
129-
if f == "self" || f == "Self" {
130-
if let Some(name) = current_item.as_ref() {
131-
return name.clone();
132-
}
133-
}
134-
f.to_owned()
135-
})
127+
.map(|f| f.to_owned())
136128
// If there's no third component, we saw `[a::b]` before and it failed to resolve.
137129
// So there's no partial res.
138130
.ok_or_else(no_res)?;
@@ -237,7 +229,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
237229
&self,
238230
path_str: &'path str,
239231
ns: Namespace,
240-
current_item: &Option<String>,
241232
module_id: DefId,
242233
extra_fragment: &Option<String>,
243234
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
@@ -296,14 +287,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
296287
let (item_str, item_name) = split.next().map(|i| (i, Symbol::intern(i))).unwrap();
297288
let path_root = split
298289
.next()
299-
.map(|f| {
300-
if f == "self" || f == "Self" {
301-
if let Some(name) = current_item.as_ref() {
302-
return name.clone();
303-
}
304-
}
305-
f.to_owned()
306-
})
290+
.map(|f| f.to_owned())
307291
// If there's no `::`, it's not an associated item.
308292
// So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved.
309293
.ok_or_else(|| {
@@ -365,7 +349,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
365349
let ty_res = match ty_res {
366350
Err(()) | Ok(Res::Err) => {
367351
return if ns == Namespace::ValueNS {
368-
self.variant_field(path_str, current_item, module_id)
352+
self.variant_field(path_str, module_id)
369353
} else {
370354
Err(ResolutionFailure::NotResolved {
371355
module_id,
@@ -501,7 +485,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
501485
};
502486
res.unwrap_or_else(|| {
503487
if ns == Namespace::ValueNS {
504-
self.variant_field(path_str, current_item, module_id)
488+
self.variant_field(path_str, module_id)
505489
} else {
506490
Err(ResolutionFailure::NotResolved {
507491
module_id,
@@ -524,7 +508,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
524508
ns: Namespace,
525509
path_str: &str,
526510
module_id: DefId,
527-
current_item: &Option<String>,
528511
extra_fragment: &Option<String>,
529512
) -> Option<Res> {
530513
let check_full_res_inner = |this: &Self, result: Result<Res, ErrorKind<'_>>| {
@@ -540,7 +523,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
540523
};
541524
// cannot be used for macro namespace
542525
let check_full_res = |this: &Self, ns| {
543-
let result = this.resolve(path_str, ns, current_item, module_id, extra_fragment);
526+
let result = this.resolve(path_str, ns, module_id, extra_fragment);
544527
check_full_res_inner(this, result.map(|(res, _)| res))
545528
};
546529
let check_full_res_macro = |this: &Self| {
@@ -736,32 +719,40 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
736719
trace!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.def_id);
737720
}
738721

739-
let current_item = match item.inner {
740-
ModuleItem(..) => {
741-
if item.attrs.inner_docs {
742-
if item.def_id.is_top_level_module() { item.name.clone() } else { None }
743-
} else {
744-
match parent_node.or(self.mod_ids.last().copied()) {
745-
Some(parent) if !parent.is_top_level_module() => {
746-
// FIXME: can we pull the parent module's name from elsewhere?
747-
Some(self.cx.tcx.item_name(parent).to_string())
748-
}
749-
_ => None,
750-
}
751-
}
752-
}
753-
ImplItem(Impl { ref for_, .. }) => {
754-
for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string())
755-
}
756-
// we don't display docs on `extern crate` items anyway, so don't process them.
757-
ExternCrateItem(..) => {
758-
debug!("ignoring extern crate item {:?}", item.def_id);
759-
return self.fold_item_recur(item);
760-
}
761-
ImportItem(Import::Simple(ref name, ..)) => Some(name.clone()),
762-
MacroItem(..) => None,
763-
_ => item.name.clone(),
722+
// find item's parent to resolve `Self` in item's docs below
723+
debug!("looking for the `Self` type");
724+
let self_id = if item.is_fake() {
725+
None
726+
} else if matches!(
727+
self.cx.tcx.def_kind(item.def_id),
728+
DefKind::AssocConst
729+
| DefKind::AssocFn
730+
| DefKind::AssocTy
731+
| DefKind::Variant
732+
| DefKind::Field
733+
) {
734+
self.cx.tcx.parent(item.def_id)
735+
// HACK(jynelson): `clean` marks associated types as `TypedefItem`, not as `AssocTypeItem`.
736+
// Fixing this breaks `fn render_deref_methods`.
737+
// As a workaround, see if the parent of the item is an `impl`; if so this must be an associated item,
738+
// regardless of what rustdoc wants to call it.
739+
} else if let Some(parent) = self.cx.tcx.parent(item.def_id) {
740+
let parent_kind = self.cx.tcx.def_kind(parent);
741+
Some(if parent_kind == DefKind::Impl { parent } else { item.def_id })
742+
} else {
743+
// FIXME: this should really be `Some(item.def_id)`, but for some reason that panics in `opt_item_name`
744+
None
764745
};
746+
let self_name = self_id.and_then(|self_id| {
747+
if matches!(self.cx.tcx.def_kind(self_id), DefKind::Impl) {
748+
debug!("using type_of()");
749+
// NOTE: uses Debug to avoid shortening paths
750+
Some(format!("{:?}", self.cx.tcx.type_of(self_id)))
751+
} else {
752+
debug!("using item_name()");
753+
self.cx.tcx.opt_item_name(self_id).map(|sym| sym.to_string())
754+
}
755+
});
765756

766757
if item.is_mod() && item.attrs.inner_docs {
767758
self.mod_ids.push(item.def_id);
@@ -770,53 +761,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
770761
let dox = item.attrs.collapsed_doc_value().unwrap_or_else(String::new);
771762
trace!("got documentation '{}'", dox);
772763

773-
// find item's parent to resolve `Self` in item's docs below
774-
let parent_name = self.cx.as_local_hir_id(item.def_id).and_then(|item_hir| {
775-
let parent_hir = self.cx.tcx.hir().get_parent_item(item_hir);
776-
let item_parent = self.cx.tcx.hir().find(parent_hir);
777-
match item_parent {
778-
Some(hir::Node::Item(hir::Item {
779-
kind:
780-
hir::ItemKind::Impl {
781-
self_ty:
782-
hir::Ty {
783-
kind:
784-
hir::TyKind::Path(hir::QPath::Resolved(
785-
_,
786-
hir::Path { segments, .. },
787-
)),
788-
..
789-
},
790-
..
791-
},
792-
..
793-
})) => segments.first().map(|seg| seg.ident.to_string()),
794-
Some(hir::Node::Item(hir::Item {
795-
ident, kind: hir::ItemKind::Enum(..), ..
796-
}))
797-
| Some(hir::Node::Item(hir::Item {
798-
ident, kind: hir::ItemKind::Struct(..), ..
799-
}))
800-
| Some(hir::Node::Item(hir::Item {
801-
ident, kind: hir::ItemKind::Union(..), ..
802-
}))
803-
| Some(hir::Node::Item(hir::Item {
804-
ident, kind: hir::ItemKind::Trait(..), ..
805-
})) => Some(ident.to_string()),
806-
_ => None,
807-
}
808-
});
809-
810764
for (ori_link, link_range) in markdown_links(&dox) {
811-
self.resolve_link(
812-
&mut item,
813-
&dox,
814-
&current_item,
815-
parent_node,
816-
&parent_name,
817-
ori_link,
818-
link_range,
819-
);
765+
self.resolve_link(&mut item, &dox, &self_name, parent_node, ori_link, link_range);
820766
}
821767

822768
if item.is_mod() && !item.attrs.inner_docs {
@@ -840,9 +786,8 @@ impl LinkCollector<'_, '_> {
840786
&self,
841787
item: &mut Item,
842788
dox: &str,
843-
current_item: &Option<String>,
789+
self_name: &Option<String>,
844790
parent_node: Option<DefId>,
845-
parent_name: &Option<String>,
846791
ori_link: String,
847792
link_range: Option<Range<usize>>,
848793
) {
@@ -933,8 +878,10 @@ impl LinkCollector<'_, '_> {
933878

934879
// replace `Self` with suitable item's parent name
935880
if path_str.starts_with("Self::") {
936-
if let Some(ref name) = parent_name {
881+
if let Some(ref name) = self_name {
882+
debug!("replacing Self with {}", name);
937883
resolved_self = format!("{}::{}", name, &path_str[6..]);
884+
// FIXME: this overwrites the link text in both error messages and the link body
938885
path_str = &resolved_self;
939886
}
940887
} else if path_str.starts_with("crate::") {
@@ -955,7 +902,6 @@ impl LinkCollector<'_, '_> {
955902
item,
956903
dox,
957904
path_str,
958-
current_item,
959905
module_id,
960906
extra_fragment,
961907
&ori_link,
@@ -1083,15 +1029,14 @@ impl LinkCollector<'_, '_> {
10831029
item: &mut Item,
10841030
dox: &str,
10851031
path_str: &str,
1086-
current_item: &Option<String>,
10871032
base_node: DefId,
10881033
extra_fragment: Option<String>,
10891034
ori_link: &str,
10901035
link_range: Option<Range<usize>>,
10911036
) -> Option<(Res, Option<String>)> {
10921037
match disambiguator.map(Disambiguator::ns) {
10931038
Some(ns @ (ValueNS | TypeNS)) => {
1094-
match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {
1039+
match self.resolve(path_str, ns, base_node, &extra_fragment) {
10951040
Ok(res) => Some(res),
10961041
Err(ErrorKind::Resolve(box mut kind)) => {
10971042
// We only looked in one namespace. Try to give a better error if possible.
@@ -1104,7 +1049,6 @@ impl LinkCollector<'_, '_> {
11041049
new_ns,
11051050
path_str,
11061051
base_node,
1107-
&current_item,
11081052
&extra_fragment,
11091053
) {
11101054
kind = ResolutionFailure::WrongNamespace(res, ns);
@@ -1138,13 +1082,7 @@ impl LinkCollector<'_, '_> {
11381082
macro_ns: self
11391083
.macro_resolve(path_str, base_node)
11401084
.map(|res| (res, extra_fragment.clone())),
1141-
type_ns: match self.resolve(
1142-
path_str,
1143-
TypeNS,
1144-
&current_item,
1145-
base_node,
1146-
&extra_fragment,
1147-
) {
1085+
type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
11481086
Ok(res) => {
11491087
debug!("got res in TypeNS: {:?}", res);
11501088
Ok(res)
@@ -1155,13 +1093,7 @@ impl LinkCollector<'_, '_> {
11551093
}
11561094
Err(ErrorKind::Resolve(box kind)) => Err(kind),
11571095
},
1158-
value_ns: match self.resolve(
1159-
path_str,
1160-
ValueNS,
1161-
&current_item,
1162-
base_node,
1163-
&extra_fragment,
1164-
) {
1096+
value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
11651097
Ok(res) => Ok(res),
11661098
Err(ErrorKind::AnchorFailure(msg)) => {
11671099
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
@@ -1229,13 +1161,9 @@ impl LinkCollector<'_, '_> {
12291161
Err(mut kind) => {
12301162
// `macro_resolve` only looks in the macro namespace. Try to give a better error if possible.
12311163
for &ns in &[TypeNS, ValueNS] {
1232-
if let Some(res) = self.check_full_res(
1233-
ns,
1234-
path_str,
1235-
base_node,
1236-
&current_item,
1237-
&extra_fragment,
1238-
) {
1164+
if let Some(res) =
1165+
self.check_full_res(ns, path_str, base_node, &extra_fragment)
1166+
{
12391167
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
12401168
break;
12411169
}
@@ -1558,7 +1486,7 @@ fn resolution_failure(
15581486
name = start;
15591487
for &ns in &[TypeNS, ValueNS, MacroNS] {
15601488
if let Some(res) =
1561-
collector.check_full_res(ns, &start, module_id, &None, &None)
1489+
collector.check_full_res(ns, &start, module_id, &None)
15621490
{
15631491
debug!("found partial_res={:?}", res);
15641492
*partial_res = Some(res);

‎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"]' 'usize::f'
12+
// @has - '//a[@href="https://doc.rust-lang.org/nightly/std/primitive.usize.html#associatedconstant.MAX"]' 'usize::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"]' 'usize::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)