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

Add intra-doc field@ disambiguator #83546

Closed
wants to merge 2 commits into from
Closed

Add intra-doc field@ disambiguator #83546

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58de79f
Add intra-doc `field@` disambiguator
camelid Mar 27, 2021
04d5a32
Add an enum test case
camelid Apr 1, 2021
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
250 changes: 150 additions & 100 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 @@ -487,6 +487,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
&mut self,
path_str: &'path str,
ns: Namespace,
expect_field: bool,
module_id: DefId,
extra_fragment: &Option<String>,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
Expand Down Expand Up @@ -565,92 +566,60 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
| DefKind::ForeignTy,
did,
) => {
debug!("looking for associated item named {} for item {:?}", item_name, did);
// Checks if item_name belongs to `impl SomeItem`
let assoc_item = tcx
.inherent_impls(did)
.iter()
.flat_map(|&imp| {
tcx.associated_items(imp).find_by_name_and_namespace(
tcx,
Ident::with_dummy_span(item_name),
ns,
imp,
)
})
.map(|item| (item.kind, item.def_id))
// There should only ever be one associated item that matches from any inherent impl
.next()
// Check if item_name belongs to `impl SomeTrait for SomeItem`
// FIXME(#74563): This gives precedence to `impl SomeItem`:
// Although having both would be ambiguous, use impl version for compatibility's sake.
// To handle that properly resolve() would have to support
// something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
.or_else(|| {
let kind =
resolve_associated_trait_item(did, module_id, item_name, ns, self.cx);
debug!("got associated item kind {:?}", kind);
kind
});

if let Some((kind, id)) = assoc_item {
let out = match kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
};
Some(if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
if expect_field {
self.resolve_variant_or_field(item_name, did, ty_res, extra_fragment)
} else {
Comment on lines +569 to +571
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is already super long with lots of indentation - could you change this to a separate match arm? Res::Def(...) if expect_field => { ... }

Alternatively you could turn this whole match into a separate function, which lets you do an early return.

debug!("looking for associated item named {} for item {:?}", item_name, did);
// Checks if item_name belongs to `impl SomeItem`
let assoc_item = tcx
.inherent_impls(did)
.iter()
.flat_map(|&imp| {
tcx.associated_items(imp).find_by_name_and_namespace(
tcx,
Ident::with_dummy_span(item_name),
ns,
imp,
)
})
.map(|item| (item.kind, item.def_id))
// There should only ever be one associated item that matches from any inherent impl
.next()
// Check if item_name belongs to `impl SomeTrait for SomeItem`
// FIXME(#74563): This gives precedence to `impl SomeItem`:
// Although having both would be ambiguous, use impl version for compatibility's sake.
// To handle that properly resolve() would have to support
// something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
.or_else(|| {
let kind = resolve_associated_trait_item(
did, module_id, item_name, ns, self.cx,
);
debug!("got associated item kind {:?}", kind);
kind
});

if let Some((kind, id)) = assoc_item {
let out = match kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
};
Some(if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
ty_res,
)))
} else {
// HACK(jynelson): `clean` expects the type, not the associated item
// but the disambiguator logic expects the associated item.
// Store the kind in a side channel so that only the disambiguator logic looks at it.
self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
Ok((ty_res, Some(format!("{}.{}", out, item_str))))
})
} else if ns == Namespace::ValueNS {
self.resolve_variant_or_field(item_name, did, ty_res, extra_fragment)
} else {
// HACK(jynelson): `clean` expects the type, not the associated item
// but the disambiguator logic expects the associated item.
// Store the kind in a side channel so that only the disambiguator logic looks at it.
self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
Ok((ty_res, Some(format!("{}.{}", out, item_str))))
})
} else if ns == Namespace::ValueNS {
debug!("looking for variants or fields named {} for {:?}", item_name, did);
// FIXME(jynelson): why is this different from
// `variant_field`?
match tcx.type_of(did).kind() {
ty::Adt(def, _) => {
let field = if def.is_enum() {
def.all_fields().find(|item| item.ident.name == item_name)
} else {
def.non_enum_variant()
.fields
.iter()
.find(|item| item.ident.name == item_name)
};
field.map(|item| {
if extra_fragment.is_some() {
let res = Res::Def(
if def.is_enum() {
DefKind::Variant
} else {
DefKind::Field
},
item.did,
);
Err(ErrorKind::AnchorFailure(
AnchorFailure::RustdocAnchorConflict(res),
))
} else {
Ok((
ty_res,
Some(format!(
"{}.{}",
if def.is_enum() { "variant" } else { "structfield" },
item.ident
)),
))
}
})
}
_ => None,
None
}
} else {
None
}
}
Res::Def(DefKind::Trait, did) => tcx
Expand Down Expand Up @@ -692,6 +661,46 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
})
}

fn resolve_variant_or_field(
&self,
item_name: Symbol,
did: DefId,
ty_res: Res,
extra_fragment: &Option<String>,
) -> Option<Result<(Res, Option<String>), ErrorKind<'path>>> {
debug!("looking for variants or fields named {} for {:?}", item_name, did);
// FIXME(jynelson): why is this different from
// `variant_field`?
match self.cx.tcx.type_of(did).kind() {
ty::Adt(def, _) => {
Comment on lines +674 to +675
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would make this an early return so you don't have so much indentation.

Suggested change
match self.cx.tcx.type_of(did).kind() {
ty::Adt(def, _) => {
let def = match self.cx.tcx.type_of(did).kind() {
ty::Adt(def, _) => def,
_ => return None,
};

let field = if def.is_enum() {
def.all_fields().find(|item| item.ident.name == item_name)
} else {
def.non_enum_variant().fields.iter().find(|item| item.ident.name == item_name)
};
field.map(|item| {
if extra_fragment.is_some() {
let res = Res::Def(
if def.is_enum() { DefKind::Variant } else { DefKind::Field },
item.did,
);
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res)))
} else {
Ok((
ty_res,
Some(format!(
"{}.{}",
if def.is_enum() { "variant" } else { "structfield" },
item.ident
)),
))
}
})
}
_ => None,
}
}

/// Used for reporting better errors.
///
/// Returns whether the link resolved 'fully' in another namespace.
Expand All @@ -701,16 +710,17 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn check_full_res(
&mut self,
ns: Namespace,
expect_field: bool,
path_str: &str,
module_id: DefId,
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, module_id, extra_fragment).map(|(res, _)| res)
}
Namespace::TypeNS | Namespace::ValueNS => self
.resolve(path_str, ns, expect_field, module_id, extra_fragment)
.map(|(res, _)| res),
};

let res = match result {
Expand Down Expand Up @@ -1165,6 +1175,9 @@ impl LinkCollector<'_, '_> {
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
match (kind, disambiguator) {
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const)))
// Fields are resolved to their parent type's Res.
// FIXME: what about DefKind::Variant?
| (DefKind::Struct | DefKind::Enum, Some(Disambiguator::Kind(DefKind::Field)))
Comment on lines +1178 to +1180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspicious ... I'd have to look into how exactly rustc_resolve behaves here, but I'd rather not allow people to write

/// [field@S]
struct S;

and have it work. Maybe you could add this to yet another side channel? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspicious ... I'd have to look into how exactly rustc_resolve behaves here, but I'd rather not allow people to write

/// [field@S]
struct S;

and have it work.

Your suspicion is correct, because that code works fine....

I don't think it's rustc_resolve behavior; we seem to use rustc_resolve to resolve everything up until the field, and then resolve that manually ourselves somehow. It seems like this code could use some cleanup; I think it would be a good idea for us to have our own Res enum that includes fields. Maybe it would be a good idea to do that first before implementing this feature?

Or, is there some way that rustc_resolve can resolve fields for us?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, is there some way that rustc_resolve can resolve fields for us?

I wouldn't expect so, since that needs type information (put another way, it can only resolve things that have a namespace).

I think it would be a good idea for us to have our own Res enum that includes fields.

Ok, there's already a custom Res type you could modify:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 71 in 58de79f

enum Res {
. But I'm not sure why that needs a new variant, I would expect using a normal Res::Def(DefKind::Field) to work.

Copy link
Member Author

@camelid camelid Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea for us to have our own Res enum that includes fields.

Ok, there's already a custom Res type you could modify:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 71 in 58de79f

enum Res {

. But I'm not sure why that needs a new variant, I would expect using a normal Res::Def(DefKind::Field) to work.

Sounds good! I said that because I didn't understand why we didn't just use Res::Def(DefKind::Field) for the field resolution. So I think a good cleanup to do before I add this feature is to use Res::Def(DefKind::Field) to resolve fields instead of resolving them to their parent struct/enum. I think I'll play around with that a bit to see if I can get it working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think a good cleanup to do before I add this feature is to use Res::Def(DefKind::Field) to resolve fields instead of resolving them to their parent struct/enum. I think I'll play around with that a bit to see if I can get it working.

Buyer beware:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 605 to 608 in d474075

// HACK(jynelson): `clean` expects the type, not the associated item
// but the disambiguator logic expects the associated item.
// Store the kind in a side channel so that only the disambiguator logic looks at it.
self.kind_side_channel.set(Some((kind.as_def_kind(), id)));

but if you get it working that would be amazing :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh-oh... ;)

// NOTE: this allows 'method' to mean both normal functions and associated functions
// This can't cause ambiguity because both are in the same namespace.
| (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn)))
Expand Down Expand Up @@ -1315,10 +1328,11 @@ impl LinkCollector<'_, '_> {
let path_str = &key.path_str;
let base_node = key.module_id;
let extra_fragment = &key.extra_fragment;
let expect_field = disambiguator.map_or(false, Disambiguator::is_field);

match disambiguator.map(Disambiguator::ns) {
Some(expected_ns @ (ValueNS | TypeNS)) => {
match self.resolve(path_str, expected_ns, base_node, extra_fragment) {
match self.resolve(path_str, expected_ns, expect_field, 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 @@ -1327,9 +1341,13 @@ impl LinkCollector<'_, '_> {
// FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator`
// See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach
for &new_ns in &[other_ns, MacroNS] {
if let Some(res) =
self.check_full_res(new_ns, path_str, base_node, extra_fragment)
{
if let Some(res) = self.check_full_res(
new_ns,
expect_field,
path_str,
base_node,
extra_fragment,
) {
kind = ResolutionFailure::WrongNamespace { res, expected_ns };
break;
}
Expand Down Expand Up @@ -1368,7 +1386,13 @@ 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, base_node, extra_fragment) {
type_ns: match self.resolve(
path_str,
TypeNS,
expect_field,
base_node,
extra_fragment,
) {
Ok(res) => {
debug!("got res in TypeNS: {:?}", res);
Ok(res)
Expand All @@ -1386,7 +1410,13 @@ impl LinkCollector<'_, '_> {
}
Err(ErrorKind::Resolve(box kind)) => Err(kind),
},
value_ns: match self.resolve(path_str, ValueNS, base_node, extra_fragment) {
value_ns: match self.resolve(
path_str,
ValueNS,
expect_field,
base_node,
extra_fragment,
) {
Ok(res) => Ok(res),
Err(ErrorKind::AnchorFailure(msg)) => {
anchor_failure(
Expand Down Expand Up @@ -1463,9 +1493,13 @@ 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, extra_fragment)
{
if let Some(res) = self.check_full_res(
ns,
expect_field,
path_str,
base_node,
extra_fragment,
) {
kind =
ResolutionFailure::WrongNamespace { res, expected_ns: MacroNS };
break;
Expand Down Expand Up @@ -1542,6 +1576,7 @@ impl Disambiguator {
"enum" => Kind(DefKind::Enum),
"trait" => Kind(DefKind::Trait),
"union" => Kind(DefKind::Union),
"field" => Kind(DefKind::Field),
"module" | "mod" => Kind(DefKind::Mod),
"const" | "constant" => Kind(DefKind::Const),
"static" => Kind(DefKind::Static),
Expand Down Expand Up @@ -1604,11 +1639,22 @@ impl Disambiguator {
Suggestion::Prefix(prefix)
}

fn is_field(self) -> bool {
self == Self::Kind(DefKind::Field)
}

fn ns(self) -> Namespace {
match self {
Self::Namespace(n) => n,
Self::Kind(k) => {
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
match k {
// Fields technically don't have a namespace, but we treat
// fields as if they belong to the value namespace.
DefKind::Field => TypeNS,
Comment on lines +1651 to +1653
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Fields technically don't have a namespace, but we treat
// fields as if they belong to the value namespace.
DefKind::Field => TypeNS,
// Fields technically don't have a namespace, but we treat
// fields as if they belong to the value namespace.
DefKind::Field => ValueNS,

Do we need to return a namespace here? Would it be better to return an Option, the same way DefKind::ns does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops... I said they belong to the value namespace because I saw that elsewhere, but I think in my mind I thought type namespace fitted better. Need to figure out if it makes sense to update elsewhere to use TypeNS or to switch to using ValueNS here...

_ => {
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
}
}
}
Self::Primitive => TypeNS,
}
Expand Down Expand Up @@ -1795,9 +1841,13 @@ fn resolution_failure(
};
name = start;
for &ns in &[TypeNS, ValueNS, MacroNS] {
if let Some(res) =
collector.check_full_res(ns, &start, module_id, &None)
{
if let Some(res) = collector.check_full_res(
ns,
disambiguator.map_or(false, Disambiguator::is_field),
&start,
module_id,
&None,
) {
debug!("found partial_res={:?}", res);
*partial_res = Some(res);
*unresolved = end.into();
Expand Down
15 changes: 15 additions & 0 deletions src/test/rustdoc/intra-doc/field-disambiguator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![crate_name = "foo"]

pub struct Foo {
pub bar: i32,
}

impl Foo {
/// [`Foo::bar()`] gets a reference to [`field@Foo::bar`].
/// What about [without disambiguators](Foo::bar)?
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#method.bar"]' 'Foo::bar()'
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#structfield.bar"]' 'Foo::bar'
// @!has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#structfield.bar"]' 'field'
// @has foo/struct.Foo.html '//a[@href="../foo/struct.Foo.html#method.bar"]' 'without disambiguators'
pub fn bar(&self) -> i32 { self.bar }
}