Skip to content

Commit 58de79f

Browse files
camelidcamelid
committed
Add intra-doc field@ disambiguator
This change required some refactoring because fields (1) don't get their own `Res` (instead they seem to use the `Res` of their parent node) and (2) `DefKind::Field::ns()` returns `None`, so rustdoc has to special-case fields. For some reason, we treat fields as if they're in the value namespace - I'm not sure why that is.
1 parent 5e65467 commit 58de79f

File tree

2 files changed

+165
-100
lines changed

2 files changed

+165
-100
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs‎

Lines changed: 150 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
487487
&mut self,
488488
path_str: &'path str,
489489
ns: Namespace,
490+
expect_field: bool,
490491
module_id: DefId,
491492
extra_fragment: &Option<String>,
492493
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
@@ -565,92 +566,60 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
565566
| DefKind::ForeignTy,
566567
did,
567568
) => {
568-
debug!("looking for associated item named {} for item {:?}", item_name, did);
569-
// Checks if item_name belongs to `impl SomeItem`
570-
let assoc_item = tcx
571-
.inherent_impls(did)
572-
.iter()
573-
.flat_map(|&imp| {
574-
tcx.associated_items(imp).find_by_name_and_namespace(
575-
tcx,
576-
Ident::with_dummy_span(item_name),
577-
ns,
578-
imp,
579-
)
580-
})
581-
.map(|item| (item.kind, item.def_id))
582-
// There should only ever be one associated item that matches from any inherent impl
583-
.next()
584-
// Check if item_name belongs to `impl SomeTrait for SomeItem`
585-
// FIXME(#74563): This gives precedence to `impl SomeItem`:
586-
// Although having both would be ambiguous, use impl version for compatibility's sake.
587-
// To handle that properly resolve() would have to support
588-
// something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
589-
.or_else(|| {
590-
let kind =
591-
resolve_associated_trait_item(did, module_id, item_name, ns, self.cx);
592-
debug!("got associated item kind {:?}", kind);
593-
kind
594-
});
595-
596-
if let Some((kind, id)) = assoc_item {
597-
let out = match kind {
598-
ty::AssocKind::Fn => "method",
599-
ty::AssocKind::Const => "associatedconstant",
600-
ty::AssocKind::Type => "associatedtype",
601-
};
602-
Some(if extra_fragment.is_some() {
603-
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
569+
if expect_field {
570+
self.resolve_variant_or_field(item_name, did, ty_res, extra_fragment)
571+
} else {
572+
debug!("looking for associated item named {} for item {:?}", item_name, did);
573+
// Checks if item_name belongs to `impl SomeItem`
574+
let assoc_item = tcx
575+
.inherent_impls(did)
576+
.iter()
577+
.flat_map(|&imp| {
578+
tcx.associated_items(imp).find_by_name_and_namespace(
579+
tcx,
580+
Ident::with_dummy_span(item_name),
581+
ns,
582+
imp,
583+
)
584+
})
585+
.map(|item| (item.kind, item.def_id))
586+
// There should only ever be one associated item that matches from any inherent impl
587+
.next()
588+
// Check if item_name belongs to `impl SomeTrait for SomeItem`
589+
// FIXME(#74563): This gives precedence to `impl SomeItem`:
590+
// Although having both would be ambiguous, use impl version for compatibility's sake.
591+
// To handle that properly resolve() would have to support
592+
// something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
593+
.or_else(|| {
594+
let kind = resolve_associated_trait_item(
595+
did, module_id, item_name, ns, self.cx,
596+
);
597+
debug!("got associated item kind {:?}", kind);
598+
kind
599+
});
600+
601+
if let Some((kind, id)) = assoc_item {
602+
let out = match kind {
603+
ty::AssocKind::Fn => "method",
604+
ty::AssocKind::Const => "associatedconstant",
605+
ty::AssocKind::Type => "associatedtype",
606+
};
607+
Some(if extra_fragment.is_some() {
608+
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
609+
ty_res,
610+
)))
611+
} else {
612+
// HACK(jynelson): `clean` expects the type, not the associated item
613+
// but the disambiguator logic expects the associated item.
614+
// Store the kind in a side channel so that only the disambiguator logic looks at it.
615+
self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
616+
Ok((ty_res, Some(format!("{}.{}", out, item_str))))
617+
})
618+
} else if ns == Namespace::ValueNS {
619+
self.resolve_variant_or_field(item_name, did, ty_res, extra_fragment)
604620
} else {
605-
// HACK(jynelson): `clean` expects the type, not the associated item
606-
// but the disambiguator logic expects the associated item.
607-
// Store the kind in a side channel so that only the disambiguator logic looks at it.
608-
self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
609-
Ok((ty_res, Some(format!("{}.{}", out, item_str))))
610-
})
611-
} else if ns == Namespace::ValueNS {
612-
debug!("looking for variants or fields named {} for {:?}", item_name, did);
613-
// FIXME(jynelson): why is this different from
614-
// `variant_field`?
615-
match tcx.type_of(did).kind() {
616-
ty::Adt(def, _) => {
617-
let field = if def.is_enum() {
618-
def.all_fields().find(|item| item.ident.name == item_name)
619-
} else {
620-
def.non_enum_variant()
621-
.fields
622-
.iter()
623-
.find(|item| item.ident.name == item_name)
624-
};
625-
field.map(|item| {
626-
if extra_fragment.is_some() {
627-
let res = Res::Def(
628-
if def.is_enum() {
629-
DefKind::Variant
630-
} else {
631-
DefKind::Field
632-
},
633-
item.did,
634-
);
635-
Err(ErrorKind::AnchorFailure(
636-
AnchorFailure::RustdocAnchorConflict(res),
637-
))
638-
} else {
639-
Ok((
640-
ty_res,
641-
Some(format!(
642-
"{}.{}",
643-
if def.is_enum() { "variant" } else { "structfield" },
644-
item.ident
645-
)),
646-
))
647-
}
648-
})
649-
}
650-
_ => None,
621+
None
651622
}
652-
} else {
653-
None
654623
}
655624
}
656625
Res::Def(DefKind::Trait, did) => tcx
@@ -692,6 +661,46 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
692661
})
693662
}
694663

664+
fn resolve_variant_or_field(
665+
&self,
666+
item_name: Symbol,
667+
did: DefId,
668+
ty_res: Res,
669+
extra_fragment: &Option<String>,
670+
) -> Option<Result<(Res, Option<String>), ErrorKind<'path>>> {
671+
debug!("looking for variants or fields named {} for {:?}", item_name, did);
672+
// FIXME(jynelson): why is this different from
673+
// `variant_field`?
674+
match self.cx.tcx.type_of(did).kind() {
675+
ty::Adt(def, _) => {
676+
let field = if def.is_enum() {
677+
def.all_fields().find(|item| item.ident.name == item_name)
678+
} else {
679+
def.non_enum_variant().fields.iter().find(|item| item.ident.name == item_name)
680+
};
681+
field.map(|item| {
682+
if extra_fragment.is_some() {
683+
let res = Res::Def(
684+
if def.is_enum() { DefKind::Variant } else { DefKind::Field },
685+
item.did,
686+
);
687+
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res)))
688+
} else {
689+
Ok((
690+
ty_res,
691+
Some(format!(
692+
"{}.{}",
693+
if def.is_enum() { "variant" } else { "structfield" },
694+
item.ident
695+
)),
696+
))
697+
}
698+
})
699+
}
700+
_ => None,
701+
}
702+
}
703+
695704
/// Used for reporting better errors.
696705
///
697706
/// Returns whether the link resolved 'fully' in another namespace.
@@ -701,16 +710,17 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
701710
fn check_full_res(
702711
&mut self,
703712
ns: Namespace,
713+
expect_field: bool,
704714
path_str: &str,
705715
module_id: DefId,
706716
extra_fragment: &Option<String>,
707717
) -> Option<Res> {
708718
// resolve() can't be used for macro namespace
709719
let result = match ns {
710720
Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from),
711-
Namespace::TypeNS | Namespace::ValueNS => {
712-
self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res)
713-
}
721+
Namespace::TypeNS | Namespace::ValueNS => self
722+
.resolve(path_str, ns, expect_field, module_id, extra_fragment)
723+
.map(|(res, _)| res),
714724
};
715725

716726
let res = match result {
@@ -1165,6 +1175,9 @@ impl LinkCollector<'_, '_> {
11651175
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
11661176
match (kind, disambiguator) {
11671177
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const)))
1178+
// Fields are resolved to their parent type's Res.
1179+
// FIXME: what about DefKind::Variant?
1180+
| (DefKind::Struct | DefKind::Enum, Some(Disambiguator::Kind(DefKind::Field)))
11681181
// NOTE: this allows 'method' to mean both normal functions and associated functions
11691182
// This can't cause ambiguity because both are in the same namespace.
11701183
| (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn)))
@@ -1315,10 +1328,11 @@ impl LinkCollector<'_, '_> {
13151328
let path_str = &key.path_str;
13161329
let base_node = key.module_id;
13171330
let extra_fragment = &key.extra_fragment;
1331+
let expect_field = disambiguator.map_or(false, Disambiguator::is_field);
13181332

13191333
match disambiguator.map(Disambiguator::ns) {
13201334
Some(expected_ns @ (ValueNS | TypeNS)) => {
1321-
match self.resolve(path_str, expected_ns, base_node, extra_fragment) {
1335+
match self.resolve(path_str, expected_ns, expect_field, base_node, extra_fragment) {
13221336
Ok(res) => Some(res),
13231337
Err(ErrorKind::Resolve(box mut kind)) => {
13241338
// We only looked in one namespace. Try to give a better error if possible.
@@ -1327,9 +1341,13 @@ impl LinkCollector<'_, '_> {
13271341
// FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator`
13281342
// See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach
13291343
for &new_ns in &[other_ns, MacroNS] {
1330-
if let Some(res) =
1331-
self.check_full_res(new_ns, path_str, base_node, extra_fragment)
1332-
{
1344+
if let Some(res) = self.check_full_res(
1345+
new_ns,
1346+
expect_field,
1347+
path_str,
1348+
base_node,
1349+
extra_fragment,
1350+
) {
13331351
kind = ResolutionFailure::WrongNamespace { res, expected_ns };
13341352
break;
13351353
}
@@ -1368,7 +1386,13 @@ impl LinkCollector<'_, '_> {
13681386
macro_ns: self
13691387
.resolve_macro(path_str, base_node)
13701388
.map(|res| (res, extra_fragment.clone())),
1371-
type_ns: match self.resolve(path_str, TypeNS, base_node, extra_fragment) {
1389+
type_ns: match self.resolve(
1390+
path_str,
1391+
TypeNS,
1392+
expect_field,
1393+
base_node,
1394+
extra_fragment,
1395+
) {
13721396
Ok(res) => {
13731397
debug!("got res in TypeNS: {:?}", res);
13741398
Ok(res)
@@ -1386,7 +1410,13 @@ impl LinkCollector<'_, '_> {
13861410
}
13871411
Err(ErrorKind::Resolve(box kind)) => Err(kind),
13881412
},
1389-
value_ns: match self.resolve(path_str, ValueNS, base_node, extra_fragment) {
1413+
value_ns: match self.resolve(
1414+
path_str,
1415+
ValueNS,
1416+
expect_field,
1417+
base_node,
1418+
extra_fragment,
1419+
) {
13901420
Ok(res) => Ok(res),
13911421
Err(ErrorKind::AnchorFailure(msg)) => {
13921422
anchor_failure(
@@ -1463,9 +1493,13 @@ impl LinkCollector<'_, '_> {
14631493
Err(mut kind) => {
14641494
// `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
14651495
for &ns in &[TypeNS, ValueNS] {
1466-
if let Some(res) =
1467-
self.check_full_res(ns, path_str, base_node, extra_fragment)
1468-
{
1496+
if let Some(res) = self.check_full_res(
1497+
ns,
1498+
expect_field,
1499+
path_str,
1500+
base_node,
1501+
extra_fragment,
1502+
) {
14691503
kind =
14701504
ResolutionFailure::WrongNamespace { res, expected_ns: MacroNS };
14711505
break;
@@ -1542,6 +1576,7 @@ impl Disambiguator {
15421576
"enum" => Kind(DefKind::Enum),
15431577
"trait" => Kind(DefKind::Trait),
15441578
"union" => Kind(DefKind::Union),
1579+
"field" => Kind(DefKind::Field),
15451580
"module" | "mod" => Kind(DefKind::Mod),
15461581
"const" | "constant" => Kind(DefKind::Const),
15471582
"static" => Kind(DefKind::Static),
@@ -1604,11 +1639,22 @@ impl Disambiguator {
16041639
Suggestion::Prefix(prefix)
16051640
}
16061641

1642+
fn is_field(self) -> bool {
1643+
self == Self::Kind(DefKind::Field)
1644+
}
1645+
16071646
fn ns(self) -> Namespace {
16081647
match self {
16091648
Self::Namespace(n) => n,
16101649
Self::Kind(k) => {
1611-
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
1650+
match k {
1651+
// Fields technically don't have a namespace, but we treat
1652+
// fields as if they belong to the value namespace.
1653+
DefKind::Field => TypeNS,
1654+
_ => {
1655+
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
1656+
}
1657+
}
16121658
}
16131659
Self::Primitive => TypeNS,
16141660
}
@@ -1795,9 +1841,13 @@ fn resolution_failure(
17951841
};
17961842
name = start;
17971843
for &ns in &[TypeNS, ValueNS, MacroNS] {
1798-
if let Some(res) =
1799-
collector.check_full_res(ns, &start, module_id, &None)
1800-
{
1844+
if let Some(res) = collector.check_full_res(
1845+
ns,
1846+
disambiguator.map_or(false, Disambiguator::is_field),
1847+
&start,
1848+
module_id,
1849+
&None,
1850+
) {
18011851
debug!("found partial_res={:?}", res);
18021852
*partial_res = Some(res);
18031853
*unresolved = end.into();

‎src/test/rustdoc/intra-doc/field-disambiguator.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#![crate_name = "foo"]
2+
3+
pub struct Foo {
4+
pub bar: i32,
5+
}
6+
7+
impl Foo {
8+
/// [`Foo::bar()`] gets a reference to [`field@Foo::bar`].
9+
/// What about [without disambiguators](Foo::bar)?
10+
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#method.bar"]' 'Foo::bar()'
11+
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#structfield.bar"]' 'Foo::bar'
12+
// @!has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#structfield.bar"]' 'field'
13+
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#method.bar"]' 'without disambiguators'
14+
pub fn bar(&self) -> i32 { self.bar }
15+
}

0 commit comments

Comments
 (0)