squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Require module_id param to resolve to be non-empty

Browse source 
Previously, `resolve` would immediately check that `module_id` was
non-empty and give an error if not. This had two downsides:

- It introduced `Option`s everywhere, even if the calling function knew
it had a valid module, and
- It checked the module on each namespace, which is unnecessary: it only
needed to be checked once.

This makes the caller responsible for checking the module exists, making
the code a lot simpler.
This commit is contained in:
Joshua Nelson 2020-09-13 17:15:40 -04:00
parent 7dc0d335bc
commit 8a13fc494d
1 changed files with 281 additions and 297 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

94
src/librustdoc/passes/collect_intra_doc_links.rs
View file

@ -217,7 +217,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
let kind = if let Some(intermediate) = self.check_full_res(
TypeNS,
&intermediate_path,
Some(module_id),
module_id,
current_item,
extra_fragment,
) {
@ -235,7 +235,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn macro_resolve(
&self,
path_str: &'a str,
parent_id: Option<DefId>,
module_id: DefId,
) -> Result<Res, ResolutionFailure<'a>> {
let cx = self.cx;
let path = ast::Path::from_ident(Ident::from_str(path_str));
@ -254,7 +254,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
if let Some(res) = resolver.all_macros().get(&Symbol::intern(path_str)) {
return Some(Ok(res.map_id(|_| panic!("unexpected id"))));
}
if let Some(module_id) = parent_id {
debug!("resolving {} as a macro in the module {:?}", path_str, module_id);
if let Ok((_, res)) =
resolver.resolve_str_path_error(DUMMY_SP, path_str, MacroNS, module_id)
@ -265,17 +264,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
return Some(Ok(res));
}
}
} else {
debug!("attempting to resolve item without parent module: {}", path_str);
return Some(Err(ResolutionFailure::NoParentItem));
}
None
})
// This weird control flow is so we don't borrow the resolver more than once at a time
.unwrap_or_else(|| {
let mut split = path_str.rsplitn(2, "::");
if let Some((parent, base)) = split.next().and_then(|x| Some((split.next()?, x))) {
if let Some(res) = self.check_full_res(TypeNS, parent, parent_id, &None, &None) {
if let Some(res) = self.check_full_res(TypeNS, parent, module_id, &None, &None) {
return Err(if matches!(res, Res::PrimTy(_)) {
ResolutionFailure::NoPrimitiveAssocItem {
res,
@ -287,12 +282,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
});
}
}
Err(ResolutionFailure::NotInScope {
module_id: parent_id.expect("already saw `Some` when resolving as a macro"),
name: path_str.into(),
})
Err(ResolutionFailure::NotInScope { module_id, name: path_str.into() })
})
}
/// 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>(
@ -300,13 +293,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
path_str: &'path str,
ns: Namespace,
current_item: &Option<String>,
parent_id: Option<DefId>,
module_id: DefId,
extra_fragment: &Option<String>,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let cx = self.cx;
// In case we're in a module, try to resolve the relative path.
if let Some(module_id) = parent_id {
let result = cx.enter_resolver(|resolver| {
resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id)
});
@ -348,9 +339,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
} else if let Some((path, prim)) = is_primitive(path_str, ns) {
if extra_fragment.is_some() {
return Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
prim,
)));
return Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(prim)));
}
return Ok((prim, Some(path.to_owned())));
}
@ -434,9 +423,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Ok((_, Res::Err)) | Err(()) => {}
Ok((_, res)) => {
let res = res.map_id(|_| panic!("unexpected node_id"));
return ResolutionFailure::CannotHaveAssociatedItems(
res, ns,
);
return ResolutionFailure::CannotHaveAssociatedItems(res, ns);
}
}
}
@ -449,10 +436,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
};
let ty_res = ty_res.map_id(|_| panic!("unexpected node_id"));
let res = match ty_res {
Res::Def(
DefKind::Struct | DefKind::Union | DefKind::Enum | DefKind::TyAlias,
did,
) => {
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`
let assoc_item = cx
@ -476,9 +460,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// 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,
);
let kind =
resolve_associated_trait_item(did, module_id, item_name, ns, &self.cx);
debug!("got associated item kind {:?}", kind);
kind
});
@ -490,9 +473,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
ty::AssocKind::Type => "associatedtype",
};
Some(if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
ty_res,
)))
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.
@ -530,11 +511,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
ty_res,
Some(format!(
"{}.{}",
if def.is_enum() {
"variant"
} else {
"structfield"
},
if def.is_enum() { "variant" } else { "structfield" },
item.ident
)),
))
@ -566,9 +543,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
};
if extra_fragment.is_some() {
Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
ty_res,
)))
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_name))))
@ -583,10 +558,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Err(ResolutionFailure::NoAssocItem(ty_res, item_name).into())
}
})
} else {
debug!("attempting to resolve item without parent module: {}", path_str);
Err(ResolutionFailure::NoParentItem.into())
}
}
/// Used for reporting better errors.
@ -599,7 +570,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
&self,
ns: Namespace,
path_str: &str,
base_node: Option<DefId>,
module_id: DefId,
current_item: &Option<String>,
extra_fragment: &Option<String>,
) -> Option<Res> {
@ -616,11 +587,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
};
// cannot be used for macro namespace
let check_full_res = |this: &Self, ns| {
let result = this.resolve(path_str, ns, current_item, base_node, extra_fragment);
let result = this.resolve(path_str, ns, current_item, module_id, extra_fragment);
check_full_res_inner(this, result.map(|(res, _)| res))
};
let check_full_res_macro = |this: &Self| {
let result = this.macro_resolve(path_str, base_node);
let result = this.macro_resolve(path_str, module_id);
check_full_res_inner(this, result.map_err(ErrorKind::from))
};
match ns {
@ -990,6 +961,23 @@ impl LinkCollector<'_, '_> {
parent_node
};
let module_id = if let Some(id) = base_node {
id
} else {
debug!("attempting to resolve item without parent module: {}", path_str);
let err_kind = ResolutionFailure::NoParentItem.into();
resolution_failure(
self,
&item,
path_str,
disambiguator,
dox,
link_range,
smallvec![err_kind],
);
return;
};
// replace `Self` with suitable item's parent name
if path_str.starts_with("Self::") {
if let Some(ref name) = parent_name {
@ -1004,7 +992,7 @@ impl LinkCollector<'_, '_> {
dox,
path_str,
current_item,
base_node,
module_id,
extra_fragment,
&ori_link,
link_range.clone(),
@ -1132,7 +1120,7 @@ impl LinkCollector<'_, '_> {
dox: &str,
path_str: &str,
current_item: &Option<String>,
base_node: Option<DefId>,
base_node: DefId,
extra_fragment: Option<String>,
ori_link: &str,
link_range: Option<Range<usize>>,
@ -1580,13 +1568,9 @@ fn resolution_failure(
break;
}
};
if let Some(res) = collector.check_full_res(
TypeNS,
&current,
Some(*module_id),
&None,
&None,
) {
if let Some(res) =
collector.check_full_res(TypeNS, &current, *module_id, &None, &None)
{
failure = ResolutionFailure::NoAssocItem(res, Symbol::intern(current));
break;
}