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

rustdoc: Cleanup handling of associated items for intra-doc links #83849

Merged
merged 3 commits into from
Apr 6, 2021
Merged

rustdoc: Cleanup handling of associated items for intra-doc links #83849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d63b3f9
Remove duplicate unwrap_or_else
jyn514 Apr 4, 2021
ac04dbd
Reduce indentation in `resolve_associated_item`
jyn514 Apr 4, 2021
3b7e654
Use more appropriate return type for `resolve_associated_item`
jyn514 Apr 4, 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
1 change: 1 addition & 0 deletions compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#![feature(nll)]
#![cfg_attr(bootstrap, feature(or_patterns))]
#![recursion_limit = "256"]
#![allow(rustdoc::private_intra_doc_links)]

pub use rustc_hir::def::{Namespace, PerNS};

Expand Down
237 changes: 102 additions & 135 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 @@ -368,55 +368,28 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}

/// Given a primitive type, try to resolve an associated item.
///
/// HACK(jynelson): `item_str` is passed in instead of derived from `item_name` so the
/// lifetimes on `&'path` will work.
fn resolve_primitive_associated_item(
&self,
prim_ty: PrimitiveType,
ns: Namespace,
module_id: DefId,
item_name: Symbol,
item_str: &'path str,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
) -> Option<(Res, String, Option<(DefKind, DefId)>)> {
let tcx = self.cx.tcx;

prim_ty
.impls(tcx)
.into_iter()
.find_map(|&impl_| {
tcx.associated_items(impl_)
.find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_)
.map(|item| {
let kind = item.kind;
self.kind_side_channel.set(Some((kind.as_def_kind(), item.def_id)));
match kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
}
})
.map(|out| {
(
Res::Primitive(prim_ty),
Some(format!("{}#{}.{}", prim_ty.as_str(), out, item_str)),
)
})
})
.ok_or_else(|| {
debug!(
"returning primitive error for {}::{} in {} namespace",
prim_ty.as_str(),
item_name,
ns.descr()
);
ResolutionFailure::NotResolved {
module_id,
partial_res: Some(Res::Primitive(prim_ty)),
unresolved: item_str.into(),
}
.into()
})
prim_ty.impls(tcx).into_iter().find_map(|&impl_| {
tcx.associated_items(impl_)
.find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_)
.map(|item| {
let kind = item.kind;
let out = match kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
};
let fragment = format!("{}#{}.{}", prim_ty.as_str(), out, item_name);
(Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id)))
})
})
}

/// Resolves a string as a macro.
Expand Down Expand Up @@ -490,8 +463,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
module_id: DefId,
extra_fragment: &Option<String>,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let tcx = self.cx.tcx;

if let Some(res) = self.resolve_path(path_str, ns, module_id) {
match res {
// FIXME(#76467): make this fallthrough to lookup the associated
Expand Down Expand Up @@ -534,29 +505,58 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
})?;

// FIXME: are these both necessary?
let ty_res = if let Some(ty_res) = resolve_primitive(&path_root, TypeNS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the error shows why this resolve_primitive might be necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm bisecting this now - I think it's unlikely this is the cause, but it's possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! I figured it out - resolve_path gives precedence to modules over primitives, this does the reverse.

|      ^^^^^^^^^ no item named `MAX` in module `usize`

The issue is that both mod usize and the primitive usize are in scope. I think this might actually be a bug even before - rustdoc should give an error if the base res is ambiguous, but it doesn't:

$ cat primitive-ambiguous.rs 
//! [usize::MAX]

mod usize {
  const MAX: usize = 1;
}
$ rustdoc primitive-ambiguous.rs
$

For this specific test case, I think we could fix it by not giving an ambiguity error if the module has doc(primitive) on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I'm qualified to have a say in this :)

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @Manishearth - do you have an opinion here? For context, the original test case was

//! [usize::MAX]

#[doc(primitive = "usize")]
mod usize {}

Copy link
Member Author

@jyn514 jyn514 Apr 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I think I'm going to wait to fix this for a follow-up PR, it's a change in behavior and shouldn't be mixed with the cleanups. Opened #83862.

// FIXME(#83862): this arbitrarily gives precedence to primitives over modules to support
// links to primitives when `#[doc(primitive)]` is present. It should give an ambiguity
// error instead and special case *only* modules with `#[doc(primitive)]`, not all
// primitives.
resolve_primitive(&path_root, TypeNS)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
{
ty_res
} else {
// FIXME: this is duplicated on the end of this function.
return if ns == Namespace::ValueNS {
self.variant_field(path_str, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
partial_res: None,
unresolved: path_root.into(),
.and_then(|ty_res| {
let (res, fragment, side_channel) =
self.resolve_associated_item(ty_res, item_name, ns, module_id)?;
let result = if extra_fragment.is_some() {
let diag_res = side_channel.map_or(res, |(k, r)| Res::Def(k, r));
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(diag_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.
if let Some((kind, id)) = side_channel {
self.kind_side_channel.set(Some((kind, id)));
}
Ok((res, Some(fragment)))
};
Some(result)
})
.unwrap_or_else(|| {
if ns == Namespace::ValueNS {
self.variant_field(path_str, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
partial_res: None,
unresolved: path_root.into(),
}
.into())
}
.into())
};
};
})
}

/// Returns:
/// - None if no associated item was found
/// - Some((_, _, Some(_))) if an item was found and should go through a side channel
/// - Some((_, _, None)) otherwise
fn resolve_associated_item(
&mut self,
root_res: Res,
item_name: Symbol,
ns: Namespace,
module_id: DefId,
) -> Option<(Res, String, Option<(DefKind, DefId)>)> {
let tcx = self.cx.tcx;

let res = match ty_res {
Res::Primitive(prim) => Some(
self.resolve_primitive_associated_item(prim, ns, module_id, item_name, item_str),
),
match root_res {
Res::Primitive(prim) => self.resolve_primitive_associated_item(prim, ns, item_name),
Res::Def(
DefKind::Struct
| DefKind::Union
Expand Down Expand Up @@ -599,59 +599,42 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
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 {
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,
}
} else {
None
// 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.
return Some((
root_res,
format!("{}.{}", out, item_name),
Some((kind.as_def_kind(), id)),
));
}

if ns != Namespace::ValueNS {
return None;
}
debug!("looking for variants or fields named {} for {:?}", item_name, did);
// FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?)
// NOTE: it's different from variant_field because it resolves fields and variants,
// not variant fields (2 path segments, not 3).
let def = match 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)
}?;
let kind = if def.is_enum() { DefKind::Variant } else { DefKind::Field };
Some((
root_res,
format!(
"{}.{}",
if def.is_enum() { "variant" } else { "structfield" },
field.ident
),
Some((kind, field.did)),
))
}
Res::Def(DefKind::Trait, did) => tcx
.associated_items(did)
Expand All @@ -669,27 +652,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
};

if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
} else {
let res = Res::Def(item.kind.as_def_kind(), item.def_id);
Ok((res, Some(format!("{}.{}", kind, item_str))))
}
let res = Res::Def(item.kind.as_def_kind(), item.def_id);
(res, format!("{}.{}", kind, item_name), None)
}),
_ => None,
};
res.unwrap_or_else(|| {
if ns == Namespace::ValueNS {
self.variant_field(path_str, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
partial_res: Some(ty_res),
unresolved: item_str.into(),
}
.into())
}
})
}
}

/// Used for reporting better errors.
Expand Down
18 changes: 13 additions & 5 deletions src/test/rustdoc-ui/intra-doc/private.private.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
warning: public documentation for `DocMe` links to private item `DontDocMe`
--> $DIR/private.rs:5:11
--> $DIR/private.rs:7:11
|
LL | /// docs [DontDocMe] [DontDocMe::f]
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^ this item is private
|
= note: `#[warn(rustdoc::private_intra_doc_links)]` on by default
= note: this link resolves only because you passed `--document-private-items`, but will break without

warning: public documentation for `DocMe` links to private item `DontDocMe::f`
--> $DIR/private.rs:5:23
--> $DIR/private.rs:7:23
|
LL | /// docs [DontDocMe] [DontDocMe::f]
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^^^^ this item is private
|
= note: this link resolves only because you passed `--document-private-items`, but will break without

warning: 2 warnings emitted
warning: public documentation for `DocMe` links to private item `DontDocMe::x`
--> $DIR/private.rs:7:38
|
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^^^^ this item is private
|
= note: this link resolves only because you passed `--document-private-items`, but will break without

warning: 3 warnings emitted

18 changes: 13 additions & 5 deletions src/test/rustdoc-ui/intra-doc/private.public.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
warning: public documentation for `DocMe` links to private item `DontDocMe`
--> $DIR/private.rs:5:11
--> $DIR/private.rs:7:11
|
LL | /// docs [DontDocMe] [DontDocMe::f]
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^ this item is private
|
= note: `#[warn(rustdoc::private_intra_doc_links)]` on by default
= note: this link will resolve properly if you pass `--document-private-items`

warning: public documentation for `DocMe` links to private item `DontDocMe::f`
--> $DIR/private.rs:5:23
--> $DIR/private.rs:7:23
|
LL | /// docs [DontDocMe] [DontDocMe::f]
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^^^^ this item is private
|
= note: this link will resolve properly if you pass `--document-private-items`

warning: 2 warnings emitted
warning: public documentation for `DocMe` links to private item `DontDocMe::x`
--> $DIR/private.rs:7:38
|
LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
| ^^^^^^^^^^^^ this item is private
|
= note: this link will resolve properly if you pass `--document-private-items`

warning: 3 warnings emitted

10 changes: 7 additions & 3 deletions src/test/rustdoc-ui/intra-doc/private.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@
// revisions: public private
// [private]compile-flags: --document-private-items

/// docs [DontDocMe] [DontDocMe::f]
// make sure to update `rustdoc/intra-doc/private.rs` if you update this file

/// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
//~^ WARNING public documentation for `DocMe` links to private item `DontDocMe`
//~| WARNING public documentation for `DocMe` links to private item `DontDocMe::x`
//~| WARNING public documentation for `DocMe` links to private item `DontDocMe::f`
// FIXME: for [private] we should also make sure the link was actually generated
pub struct DocMe;
struct DontDocMe;
struct DontDocMe {
x: usize,
}

impl DontDocMe {
fn f() {}
Expand Down
15 changes: 13 additions & 2 deletions src/test/rustdoc/intra-doc/private.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
#![crate_name = "private"]
// compile-flags: --document-private-items
/// docs [DontDocMe]

// make sure to update `rustdoc-ui/intra-doc/private.rs` if you update this file

/// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
// @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html"]' 'DontDocMe'
// @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html#method.f"]' 'DontDocMe::f'
// @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html#structfield.x"]' 'DontDocMe::x'
pub struct DocMe;
struct DontDocMe;
struct DontDocMe {
x: usize,
}

impl DontDocMe {
fn f() {}
}