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

Clean up in intra-doc link collector #77743

Merged
merged 6 commits into from
Oct 11, 2020
+198 −160
Merged

Clean up in intra-doc link collector #77743

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90e0fed
Clean up check_full_res
bugadani Oct 9, 2020
3a640f2
Clean up hard to follow control flow
bugadani Oct 9, 2020
3858c0f
Apply suggestions from code review
bugadani Oct 9, 2020
a5fbfab
Refactor path resolution and use Symbols instead of &str
bugadani Oct 10, 2020
b385598
Apply suggestions from code review
bugadani Oct 10, 2020
725e3d1
Re-enable test case
bugadani Oct 10, 2020
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
Prev Previous commit
Next Next commit
Refactor path resolution and use Symbols instead of &str 
Co-authored-by: Joshua Nelson <[email protected]>
@bugadani
bugadani and Joshua Nelson committed Oct 10, 2020
commit a5fbfab8c0c2f36dbb631d77df272b3965bd074c
23 changes: 23 additions & 0 deletions src/librustdoc/clean/types.rs
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ use std::{slice, vec};
use rustc_ast::attr;
use rustc_ast::util::comments::beautify_doc_string;
use rustc_ast::{self as ast, AttrStyle};
use rustc_ast::{FloatTy, IntTy, UintTy};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
use rustc_hir::def::Res;
@@ -1279,6 +1280,28 @@ impl GetDefId for Type {
}

impl PrimitiveType {
pub fn from_hir(prim: hir::PrimTy) -> PrimitiveType {
match prim {
hir::PrimTy::Int(IntTy::Isize) => PrimitiveType::Isize,
hir::PrimTy::Int(IntTy::I8) => PrimitiveType::I8,
hir::PrimTy::Int(IntTy::I16) => PrimitiveType::I16,
hir::PrimTy::Int(IntTy::I32) => PrimitiveType::I32,
hir::PrimTy::Int(IntTy::I64) => PrimitiveType::I64,
hir::PrimTy::Int(IntTy::I128) => PrimitiveType::I128,
hir::PrimTy::Uint(UintTy::Usize) => PrimitiveType::Usize,
hir::PrimTy::Uint(UintTy::U8) => PrimitiveType::U8,
hir::PrimTy::Uint(UintTy::U16) => PrimitiveType::U16,
hir::PrimTy::Uint(UintTy::U32) => PrimitiveType::U32,
hir::PrimTy::Uint(UintTy::U64) => PrimitiveType::U64,
hir::PrimTy::Uint(UintTy::U128) => PrimitiveType::U128,
hir::PrimTy::Float(FloatTy::F32) => PrimitiveType::F32,
hir::PrimTy::Float(FloatTy::F64) => PrimitiveType::F64,
hir::PrimTy::Str => PrimitiveType::Str,
hir::PrimTy::Bool => PrimitiveType::Bool,
hir::PrimTy::Char => PrimitiveType::Char,
}
}

pub fn from_symbol(s: Symbol) -> Option<PrimitiveType> {
match s {
sym::isize => Some(PrimitiveType::Isize),
228 changes: 113 additions & 115 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@ use rustc_session::lint::{
Lint,
};
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::sym;
use rustc_span::symbol::Ident;
use rustc_span::symbol::Symbol;
use rustc_span::DUMMY_SP;
@@ -234,6 +235,52 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
}

fn resolve_primitive_associated_item(
&self,
prim_ty: hir::PrimTy,
prim: Res,
Copy link
Member

Choose a reason for hiding this comment

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

Why does this take both? If you need the full Res I'd expect this to generate the prim_ty from the Res.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a shame I can't somehow tell in code that "I know this must be a Res::PrimTy. I still need the whole enum value, but I also want it's contents without pattern matching". Sooo, I kinda have to go the other way and wrap the prim_ty inside the function instead. I'm not so keen on adding an unreachable!() for pattern matching, or an unnecessary unwrap somewhere here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I ran into the same issue a few days ago: https://discordapp.com/channels/273534239310479360/274215136414400513/761661592898633748. This would be partially addressed by rust-lang/rfcs#2593 I think.

ns: Namespace,
module_id: DefId,
item_name: Symbol,
item_str: &'path str,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let cx = self.cx;

PrimitiveType::from_hir(prim_ty)
.impls(cx.tcx)
.into_iter()
.find_map(|&impl_| {
cx.tcx
.associated_items(impl_)
.find_by_name_and_namespace(
cx.tcx,
Ident::with_dummy_span(item_name),
ns,
impl_,
)
.map(|item| match item.kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
})
.map(|out| (prim, Some(format!("{}#{}.{}", prim_ty.name(), out, item_str))))
})
.ok_or_else(|| {
debug!(
"returning primitive error for {}::{} in {} namespace",
prim_ty.name(),
item_name,
ns.descr()
);
ResolutionFailure::NotResolved {
module_id,
partial_res: Some(prim),
unresolved: item_str.into(),
}
.into()
})
}

/// Resolves a string as a macro.
fn macro_resolve(
&self,
@@ -275,6 +322,20 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
})
}

fn resolve_path(&self, path_str: &str, ns: Namespace, module_id: DefId) -> Option<Res> {
let result = self.cx.enter_resolver(|resolver| {
resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id)
});
debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns);
match result.map(|(_, res)| res) {
Ok(Res::Err) | Err(()) => is_bool_value(path_str, ns).map(|(_, res)| res),

// resolver doesn't know about true and false so we'll have to resolve them
// manually as bool
Ok(res) => Some(res.map_id(|_| panic!("unexpected node_id"))),
}
}

/// Resolves a string as a path within a particular namespace. Also returns an optional
/// URL fragment in the case of variants and methods.
fn resolve<'path>(
@@ -287,32 +348,15 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let cx = self.cx;

let result = cx.enter_resolver(|resolver| {
resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id)
});
debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns);
let result = match result.map(|(_, res)| res) {
Ok(Res::Err) | Err(()) => {
// resolver doesn't know about true and false so we'll have to resolve them
// manually as bool
if let Some((_, res)) = is_bool_value(path_str, ns) { Ok(res) } else { Err(()) }
}
Ok(res) => Ok(res.map_id(|_| panic!("unexpected node_id"))),
};

if let Ok(res) = result {
if let Some(res) = self.resolve_path(path_str, ns, module_id) {
match res {
Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => {
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 starting to wonder if traits_implemented_by is actually necessary ... do we already have the info here?
Related: #74489 (comment). Maybe rustc_resolve handles inherent impls but not anything else? @petrochenkov might know.

Anyway, not your problem to solve.

if ns != ValueNS {
return Err(ResolutionFailure::WrongNamespace(res, ns).into());
}
assert_eq!(ns, ValueNS);
// Fall through: In case this is a trait item, skip the
// early return and try looking for the trait.
}
Res::Def(DefKind::AssocTy, _) => {
if ns != TypeNS {
return Err(ResolutionFailure::WrongNamespace(res, ns).into());
}
assert_eq!(ns, TypeNS);
// Fall through: In case this is a trait item, skip the
// early return and try looking for the trait.
}
@@ -362,70 +406,29 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
})?;

if let Some((path, prim)) = is_primitive(&path_root, TypeNS) {
let impls =
primitive_impl(cx, &path).ok_or_else(|| ResolutionFailure::NotResolved {
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
Comment on lines +413 to +415
Copy link
Member

Choose a reason for hiding this comment

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

After my spiel about separating refactors and changes, I don't want to make the change here, but I don't think these are both necessary:

Suggested change
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
// FIXME: are these both necessary?
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the strictest sense, they are, because removing is_primitive breaks tests, because modules aren't getting resolved. But even adding is_primitive back, it's only the modules that are called the same as the primitives that are... I wonder if this is because otherwise we'd end up returning early out of resolve()?

Copy link
Member

Choose a reason for hiding this comment

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

Let's fix this in a follow-up.

{
ty_res
} else {
// FIXME: this is duplicated on the end of this function.
return if ns == Namespace::ValueNS {
self.variant_field(path_str, current_item, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
partial_res: Some(prim),
unresolved: item_str.into(),
})?;
for &impl_ in impls {
let link = cx
.tcx
.associated_items(impl_)
.find_by_name_and_namespace(
cx.tcx,
Ident::with_dummy_span(item_name),
ns,
impl_,
)
.map(|item| match item.kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
ty::AssocKind::Type => "associatedtype",
})
.map(|out| (prim, Some(format!("{}#{}.{}", path, out, item_str))));
if let Some(link) = link {
return Ok(link);
partial_res: None,
unresolved: path_root.into(),
}
}
debug!(
"returning primitive error for {}::{} in {} namespace",
path,
item_name,
ns.descr()
);
return Err(ResolutionFailure::NotResolved {
module_id,
partial_res: Some(prim),
unresolved: item_str.into(),
}
.into());
}

let ty_res = cx
.enter_resolver(|resolver| {
// only types can have associated items
resolver.resolve_str_path_error(DUMMY_SP, &path_root, TypeNS, module_id)
})
.map(|(_, res)| res);
let ty_res = match ty_res {
Err(()) | Ok(Res::Err) => {
return if ns == Namespace::ValueNS {
self.variant_field(path_str, current_item, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
partial_res: None,
unresolved: path_root.into(),
}
.into())
};
}
Ok(res) => res,
.into())
};
};
let ty_res = ty_res.map_id(|_| panic!("unexpected node_id"));

let res = match ty_res {
Res::PrimTy(prim) => Some(self.resolve_primitive_associated_item(
prim, ty_res, ns, module_id, item_name, item_str,
)),
Res::Def(DefKind::Struct | DefKind::Union | DefKind::Enum | DefKind::TyAlias, did) => {
debug!("looking for associated item named {} for item {:?}", item_name, did);
// Checks if item_name belongs to `impl SomeItem`
@@ -465,7 +468,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Some(if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res)))
} else {
// HACK(jynelson): `clean` expects the type, not the associated item.
// 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)));
@@ -511,13 +514,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
_ => None,
}
} else {
// We already know this isn't in ValueNS, so no need to check variant_field
return Err(ResolutionFailure::NotResolved {
module_id,
partial_res: Some(ty_res),
unresolved: item_str.into(),
}
.into());
None
}
}
Res::Def(DefKind::Trait, did) => cx
@@ -1089,7 +1086,7 @@ impl LinkCollector<'_, '_> {
return None;
}
res = prim;
fragment = Some(path.to_owned());
fragment = Some((*path.as_str()).to_owned());
} else {
// `[char]` when a `char` module is in scope
let candidates = vec![res, prim];
@@ -1956,44 +1953,45 @@ fn handle_variant(
)
}

const PRIMITIVES: &[(&str, Res)] = &[
("u8", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U8))),
("u16", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U16))),
("u32", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U32))),
("u64", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U64))),
("u128", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U128))),
("usize", Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::Usize))),
("i8", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I8))),
("i16", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I16))),
("i32", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I32))),
("i64", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I64))),
("i128", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I128))),
("isize", Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::Isize))),
("f32", Res::PrimTy(hir::PrimTy::Float(rustc_ast::FloatTy::F32))),
("f64", Res::PrimTy(hir::PrimTy::Float(rustc_ast::FloatTy::F64))),
("str", Res::PrimTy(hir::PrimTy::Str)),
("bool", Res::PrimTy(hir::PrimTy::Bool)),
("char", Res::PrimTy(hir::PrimTy::Char)),
// FIXME: At this point, this is basically a copy of the PrimitiveTypeTable
const PRIMITIVES: &[(Symbol, Res)] = &[
Comment on lines +1960 to +1961
Copy link
Member

Choose a reason for hiding this comment

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

Could we make that public instead? I'm all for reusing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a moment, I read "refusing". I'll see what I can do here, maybe there's a way to access the hashmap through the resolver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The straight forward solution would be to get the table from the Resolver, but I'm not sure if I am allowed to make a modification like that, or if I should do that for a single use case.

The other possibility is to make PrimitiveTypeTable and it's private member public but that still feels fishy.

Copy link
Member

Choose a reason for hiding this comment

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

Let's fix this in a follow-up.

(sym::u8, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U8))),
(sym::u16, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U16))),
(sym::u32, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U32))),
(sym::u64, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U64))),
(sym::u128, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::U128))),
(sym::usize, Res::PrimTy(hir::PrimTy::Uint(rustc_ast::UintTy::Usize))),
(sym::i8, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I8))),
(sym::i16, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I16))),
(sym::i32, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I32))),
(sym::i64, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I64))),
(sym::i128, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::I128))),
(sym::isize, Res::PrimTy(hir::PrimTy::Int(rustc_ast::IntTy::Isize))),
(sym::f32, Res::PrimTy(hir::PrimTy::Float(rustc_ast::FloatTy::F32))),
(sym::f64, Res::PrimTy(hir::PrimTy::Float(rustc_ast::FloatTy::F64))),
(sym::str, Res::PrimTy(hir::PrimTy::Str)),
(sym::bool, Res::PrimTy(hir::PrimTy::Bool)),
(sym::char, Res::PrimTy(hir::PrimTy::Char)),
];

fn is_primitive(path_str: &str, ns: Namespace) -> Option<(&'static str, Res)> {
fn is_primitive(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
is_bool_value(path_str, ns).or_else(|| {
if ns == TypeNS { PRIMITIVES.iter().find(|x| x.0 == path_str).copied() } else { None }
if ns == TypeNS {
PRIMITIVES.iter().find(|x| x.0.as_str() == path_str).copied()
} else {
None
}
})
}

fn is_bool_value(path_str: &str, ns: Namespace) -> Option<(&'static str, Res)> {
fn is_bool_value(path_str: &str, ns: Namespace) -> Option<(Symbol, Res)> {
if ns == TypeNS && (path_str == "true" || path_str == "false") {
Some(("bool", Res::PrimTy(hir::PrimTy::Bool)))
Some((sym::bool, Res::PrimTy(hir::PrimTy::Bool)))
} else {
None
}
}

fn primitive_impl(cx: &DocContext<'_>, path_str: &str) -> Option<&'static SmallVec<[DefId; 4]>> {
Some(PrimitiveType::from_symbol(Symbol::intern(path_str))?.impls(cx.tcx))
}

fn strip_generics_from_path(path_str: &str) -> Result<String, ResolutionFailure<'static>> {
let mut stripped_segments = vec![];
let mut path = path_str.chars().peekable();