Skip to content

Add field@ and variant@ doc-link disambiguators #130587

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

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Add field@ and variant@ doc-link disambiguators #130587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/doc/rustdoc/src/write-documentation/linking-to-items-by-name.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ fn Foo() {}

These prefixes will be stripped when displayed in the documentation, so `[struct@Foo]` will be
rendered as `Foo`. The following prefixes are available: `struct`, `enum`, `trait`, `union`,
`mod`, `module`, `const`, `constant`, `fn`, `function`, `method`, `derive`, `type`, `value`,
`macro`, `prim` or `primitive`.
`mod`, `module`, `const`, `constant`, `fn`, `function`, `field`, `variant`, `method`, `derive`,
`type`, `value`, `macro`, `prim` or `primitive`.

You can also disambiguate for functions by adding `()` after the function name,
or for macros by adding `!` after the macro name. The macro `!` can be followed by `()`, `{}`,
Expand Down
95 changes: 54 additions & 41 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 @@ -110,7 +110,6 @@ impl Res {

let prefix = match kind {
DefKind::Fn | DefKind::AssocFn => return Suggestion::Function,
DefKind::Field => return Suggestion::RemoveDisambiguator,
DefKind::Macro(MacroKind::Bang) => return Suggestion::Macro,

DefKind::Macro(MacroKind::Derive) => "derive",
Expand All @@ -123,6 +122,8 @@ impl Res {
"const"
}
DefKind::Static { .. } => "static",
DefKind::Field => "field",
DefKind::Variant | DefKind::Ctor(..) => "variant",
// Now handle things that don't have a specific disambiguator
_ => match kind
.ns()
Expand Down Expand Up @@ -415,6 +416,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
&mut self,
path_str: &'path str,
ns: Namespace,
disambiguator: Option<Disambiguator>,
item_id: DefId,
module_id: DefId,
) -> Result<Vec<(Res, Option<DefId>)>, UnresolvedPath<'path>> {
Expand Down Expand Up @@ -454,7 +456,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
match resolve_primitive(path_root, TypeNS)
.or_else(|| self.resolve_path(path_root, TypeNS, item_id, module_id))
.map(|ty_res| {
self.resolve_associated_item(ty_res, item_name, ns, module_id)
self.resolve_associated_item(ty_res, item_name, ns, disambiguator, module_id)
.into_iter()
.map(|(res, def_id)| (res, Some(def_id)))
.collect::<Vec<_>>()
Expand Down Expand Up @@ -557,6 +559,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
root_res: Res,
item_name: Symbol,
ns: Namespace,
disambiguator: Option<Disambiguator>,
module_id: DefId,
) -> Vec<(Res, DefId)> {
let tcx = self.cx.tcx;
Expand All @@ -583,7 +586,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// FIXME: if the associated item is defined directly on the type alias,
// it will show up on its documentation page, we should link there instead.
let Some(res) = self.def_id_to_res(did) else { return Vec::new() };
self.resolve_associated_item(res, item_name, ns, module_id)
self.resolve_associated_item(res, item_name, ns, disambiguator, module_id)
}
Res::Def(
def_kind @ (DefKind::Struct | DefKind::Union | DefKind::Enum | DefKind::ForeignTy),
Expand All @@ -604,6 +607,39 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
}

let search_for_field = || {
let (DefKind::Struct | DefKind::Union) = def_kind else { return vec![] };
debug!("looking for fields named {item_name} for {did:?}");
// FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?)
// NOTE: it's different from variant_field because it only resolves struct fields,
// not variant fields (2 path segments, not 3).
//
// We need to handle struct (and union) fields in this code because
// syntactically their paths are identical to associated item paths:
// `module::Type::field` and `module::Type::Assoc`.
//
// On the other hand, variant fields can't be mistaken for associated
// items because they look like this: `module::Type::Variant::field`.
//
// Variants themselves don't need to be handled here, even though
// they also look like associated items (`module::Type::Variant`),
// because they are real Rust syntax (unlike the intra-doc links
// field syntax) and are handled by the compiler's resolver.
let ty::Adt(def, _) = tcx.type_of(did).instantiate_identity().kind() else {
unreachable!()
};
def.non_enum_variant()
.fields
.iter()
.filter(|field| field.name == item_name)
.map(|field| (root_res, field.did))
.collect::<Vec<_>>()
};

if let Some(Disambiguator::Kind(DefKind::Field)) = disambiguator {
return search_for_field();
}

// Checks if item_name belongs to `impl SomeItem`
let mut assoc_items: Vec<_> = tcx
.inherent_impls(did)
Expand Down Expand Up @@ -647,32 +683,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
if ns != Namespace::ValueNS {
return Vec::new();
}
debug!("looking for fields named {item_name} for {did:?}");
// FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?)
// NOTE: it's different from variant_field because it only resolves struct fields,
// not variant fields (2 path segments, not 3).
//
// We need to handle struct (and union) fields in this code because
// syntactically their paths are identical to associated item paths:
// `module::Type::field` and `module::Type::Assoc`.
//
// On the other hand, variant fields can't be mistaken for associated
// items because they look like this: `module::Type::Variant::field`.
//
// Variants themselves don't need to be handled here, even though
// they also look like associated items (`module::Type::Variant`),
// because they are real Rust syntax (unlike the intra-doc links
// field syntax) and are handled by the compiler's resolver.
let def = match tcx.type_of(did).instantiate_identity().kind() {
ty::Adt(def, _) if !def.is_enum() => def,
_ => return Vec::new(),
};
def.non_enum_variant()
.fields
.iter()
.filter(|field| field.name == item_name)
.map(|field| (root_res, field.did))
.collect::<Vec<_>>()

search_for_field()
}
Res::Def(DefKind::Trait, did) => filter_assoc_items_by_name_and_namespace(
tcx,
Expand Down Expand Up @@ -1298,7 +1310,7 @@ impl LinkCollector<'_, '_> {

match disambiguator.map(Disambiguator::ns) {
Some(expected_ns) => {
match self.resolve(path_str, expected_ns, item_id, module_id) {
match self.resolve(path_str, expected_ns, disambiguator, item_id, module_id) {
Ok(candidates) => candidates,
Err(err) => {
// We only looked in one namespace. Try to give a better error if possible.
Expand All @@ -1307,8 +1319,9 @@ impl LinkCollector<'_, '_> {
let mut err = ResolutionFailure::NotResolved(err);
for other_ns in [TypeNS, ValueNS, MacroNS] {
if other_ns != expected_ns {
if let Ok(&[res, ..]) =
self.resolve(path_str, other_ns, item_id, module_id).as_deref()
if let Ok(&[res, ..]) = self
.resolve(path_str, other_ns, None, item_id, module_id)
.as_deref()
{
err = ResolutionFailure::WrongNamespace {
res: full_res(self.cx.tcx, res),
Expand All @@ -1328,7 +1341,7 @@ impl LinkCollector<'_, '_> {
None => {
// Try everything!
let mut candidate = |ns| {
self.resolve(path_str, ns, item_id, module_id)
self.resolve(path_str, ns, None, item_id, module_id)
.map_err(ResolutionFailure::NotResolved)
};

Expand Down Expand Up @@ -1532,6 +1545,8 @@ impl Disambiguator {
}),
"function" | "fn" | "method" => Kind(DefKind::Fn),
"derive" => Kind(DefKind::Macro(MacroKind::Derive)),
"field" => Kind(DefKind::Field),
"variant" => Kind(DefKind::Variant),
"type" => NS(Namespace::TypeNS),
"value" => NS(Namespace::ValueNS),
"macro" => NS(Namespace::MacroNS),
Expand Down Expand Up @@ -1570,6 +1585,8 @@ impl Disambiguator {
fn ns(self) -> Namespace {
match self {
Self::Namespace(n) => n,
// for purposes of link resolution, fields are in the value namespace.
Self::Kind(DefKind::Field) => ValueNS,
Comment on lines +1588 to +1589
Copy link
Member

Choose a reason for hiding this comment

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

i'm confused, i thought you were going to remove this? is the problem that DefKind::ns is defined in rustc so you can't modify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was, but it turns out it didn't make sense to move the disambiguator == Field so far up and out - that would mean putting it in resolve_disambiguator(), while it makes much more sense (and is easier to keep existing field-last resolution behavior) if it just gets considered to be in the value ns and fails to resolve to anything before getting picked up in the special-case in resolve_associated_item().

Self::Kind(k) => {
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
}
Expand Down Expand Up @@ -1604,8 +1621,6 @@ enum Suggestion {
Function,
/// `m!`
Macro,
/// `foo` without any disambiguator
RemoveDisambiguator,
}

impl Suggestion {
Expand All @@ -1614,7 +1629,6 @@ impl Suggestion {
Self::Prefix(x) => format!("prefix with `{x}@`").into(),
Self::Function => "add parentheses".into(),
Self::Macro => "add an exclamation mark".into(),
Self::RemoveDisambiguator => "remove the disambiguator".into(),
}
}

Expand All @@ -1624,13 +1638,11 @@ impl Suggestion {
Self::Prefix(prefix) => format!("{prefix}@{path_str}"),
Self::Function => format!("{path_str}()"),
Self::Macro => format!("{path_str}!"),
Self::RemoveDisambiguator => path_str.into(),
}
}

fn as_help_span(
&self,
path_str: &str,
ori_link: &str,
sp: rustc_span::Span,
) -> Vec<(rustc_span::Span, String)> {
Expand Down Expand Up @@ -1678,7 +1690,6 @@ impl Suggestion {
}
sugg
}
Self::RemoveDisambiguator => vec![(sp, path_str.into())],
}
}
}
Expand Down Expand Up @@ -1827,7 +1838,9 @@ fn resolution_failure(
};
name = start;
for ns in [TypeNS, ValueNS, MacroNS] {
if let Ok(v_res) = collector.resolve(start, ns, item_id, module_id) {
if let Ok(v_res) =
collector.resolve(start, ns, None, item_id, module_id)
{
debug!("found partial_res={v_res:?}");
if let Some(&res) = v_res.first() {
*partial_res = Some(full_res(tcx, res));
Expand Down Expand Up @@ -2165,7 +2178,7 @@ fn suggest_disambiguator(
};

if let (Some(sp), Some(ori_link)) = (sp, ori_link) {
let mut spans = suggestion.as_help_span(path_str, ori_link, sp);
let mut spans = suggestion.as_help_span(ori_link, sp);
if spans.len() > 1 {
diag.multipart_suggestion(help, spans, Applicability::MaybeIncorrect);
} else {
Expand Down
18 changes: 17 additions & 1 deletion tests/rustdoc-ui/intra-doc/disambiguator-mismatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#![deny(rustdoc::broken_intra_doc_links)]
//~^ NOTE lint level is defined
pub enum S {}
pub enum S {
A,
}
fn S() {}

#[macro_export]
Expand All @@ -13,6 +15,10 @@ const c: usize = 0;

trait T {}

struct X {
y: usize,
}

/// Link to [struct@S]
//~^ ERROR incompatible link kind for `S`
//~| NOTE this link resolved
Expand Down Expand Up @@ -78,4 +84,14 @@ trait T {}
//~^ ERROR unresolved link to `std`
//~| NOTE this link resolves to the crate `std`
//~| HELP to link to the crate, prefix with `mod@`

/// Link to [method@X::y]
//~^ ERROR incompatible link kind for `X::y`
//~| NOTE this link resolved
//~| HELP prefix with `field@`

/// Link to [field@S::A]
//~^ ERROR incompatible link kind for `S::A`
//~| NOTE this link resolved
//~| HELP prefix with `variant@`
pub fn f() {}
Loading
Loading