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

Use real Spans when resolving intra-doc links #78797

Closed
wants to merge 4 commits into from
Closed

Use real Spans when resolving intra-doc links #78797

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
804d8a3
Fix intra-doc links scoping by using real Spans
pitaj Nov 6, 2020
6e3d809
provide Idents with real spans
pitaj Nov 20, 2020
600b71a
import rustc_span::Span, first param is span
pitaj Nov 20, 2020
ae931c2
try real spans for idents in resolve_str_path_error
pitaj Nov 20, 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
110 changes: 60 additions & 50 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 @@ -12,7 +12,7 @@ use rustc_hir::def::{
Namespace::{self, *},
PerNS, Res,
};
use rustc_hir::def_id::{CrateNum, DefId};
use rustc_hir::def_id::DefId;
use rustc_middle::ty;
use rustc_resolve::ParentScope;
use rustc_session::lint::{
Expand All @@ -23,7 +23,6 @@ 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;
use smallvec::{smallvec, SmallVec};

use std::borrow::Cow;
Expand Down Expand Up @@ -195,6 +194,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn variant_field(
&self,
path_str: &'path str,
span: rustc_span::Span,
current_item: &Option<String>,
module_id: DefId,
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
Expand Down Expand Up @@ -231,7 +231,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
.ok_or_else(no_res)?;
let ty_res = cx
.enter_resolver(|resolver| {
resolver.resolve_str_path_error(DUMMY_SP, &path, TypeNS, module_id)
resolver.resolve_str_path_error(span, &path, TypeNS, module_id)
})
.map(|(_, res)| res)
.unwrap_or(Res::Err);
Expand Down Expand Up @@ -294,6 +294,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
module_id: DefId,
item_name: Symbol,
item_str: &'path str,
span: rustc_span::Span,
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let cx = self.cx;

Expand All @@ -303,12 +304,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
.find_map(|&impl_| {
cx.tcx
.associated_items(impl_)
.find_by_name_and_namespace(
cx.tcx,
Ident::with_dummy_span(item_name),
ns,
impl_,
)
.find_by_name_and_namespace(cx.tcx, Ident::new(item_name, span), ns, impl_)
.map(|item| match item.kind {
ty::AssocKind::Fn => "method",
ty::AssocKind::Const => "associatedconstant",
Expand Down Expand Up @@ -343,10 +339,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn resolve_macro(
&self,
path_str: &'a str,
span: rustc_span::Span,
module_id: DefId,
) -> Result<Res, ResolutionFailure<'a>> {
let cx = self.cx;
let path = ast::Path::from_ident(Ident::from_str(path_str));
let path = ast::Path::from_ident(Ident::from_str_and_span(path_str, span));
cx.enter_resolver(|resolver| {
// FIXME(jynelson): does this really need 3 separate lookups?
if let Ok((Some(ext), res)) = resolver.resolve_macro_path(
Expand All @@ -365,7 +362,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}
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)
resolver.resolve_str_path_error(span, path_str, MacroNS, module_id)
{
// don't resolve builtins like `#[derive]`
if let Res::Def(..) = res {
Expand All @@ -386,9 +383,15 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
/// This also handles resolving `true` and `false` as booleans.
/// NOTE: `resolve_str_path_error` knows only about paths, not about types.
/// Associated items will never be resolved by this function.
fn resolve_path(&self, path_str: &str, ns: Namespace, module_id: DefId) -> Option<Res> {
fn resolve_path(
&self,
path_str: &str,
span: rustc_span::Span,
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)
resolver.resolve_str_path_error(span, &path_str, ns, module_id)
});
debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns);
match result.map(|(_, res)| res) {
Expand All @@ -404,6 +407,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn resolve<'path>(
&self,
path_str: &'path str,
span: rustc_span::Span,
ns: Namespace,
// FIXME(#76467): This is for `Self`, and it's wrong.
current_item: &Option<String>,
Expand All @@ -412,7 +416,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
) -> Result<(Res, Option<String>), ErrorKind<'path>> {
let cx = self.cx;

if let Some(res) = self.resolve_path(path_str, ns, module_id) {
if let Some(res) = self.resolve_path(path_str, span, ns, module_id) {
match res {
// FIXME(#76467): make this fallthrough to lookup the associated
// item a separate function.
Expand Down Expand Up @@ -471,13 +475,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// FIXME: are these both necessary?
let ty_res = if let Some(ty_res) = resolve_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
.or_else(|| self.resolve_path(&path_root, span, 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, current_item, module_id)
self.variant_field(path_str, span, current_item, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
Expand All @@ -489,9 +493,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
};

let res = match ty_res {
Res::PrimTy(prim) => Some(
self.resolve_primitive_associated_item(prim, ns, module_id, item_name, item_str),
),
Res::PrimTy(prim) => {
Some(self.resolve_primitive_associated_item(
prim, ns, module_id, item_name, item_str, span,
))
}
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`
Expand All @@ -502,7 +508,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
.flat_map(|&imp| {
cx.tcx.associated_items(imp).find_by_name_and_namespace(
cx.tcx,
Ident::with_dummy_span(item_name),
Ident::new(item_name, span),
ns,
imp,
)
Expand All @@ -516,8 +522,9 @@ 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, span, ns, &self.cx,
);
debug!("got associated item kind {:?}", kind);
kind
});
Expand Down Expand Up @@ -585,7 +592,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Res::Def(DefKind::Trait, did) => cx
.tcx
.associated_items(did)
.find_by_name_and_namespace(cx.tcx, Ident::with_dummy_span(item_name), ns, did)
.find_by_name_and_namespace(cx.tcx, Ident::new(item_name, span), ns, did)
.map(|item| {
let kind = match item.kind {
ty::AssocKind::Const => "associatedconstant",
Expand All @@ -610,7 +617,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
};
res.unwrap_or_else(|| {
if ns == Namespace::ValueNS {
self.variant_field(path_str, current_item, module_id)
self.variant_field(path_str, span, current_item, module_id)
} else {
Err(ResolutionFailure::NotResolved {
module_id,
Expand All @@ -632,15 +639,18 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
&self,
ns: Namespace,
path_str: &str,
span: rustc_span::Span,
module_id: DefId,
current_item: &Option<String>,
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::MacroNS => {
self.resolve_macro(path_str, span, module_id).map_err(ErrorKind::from)
}
Namespace::TypeNS | Namespace::ValueNS => self
.resolve(path_str, ns, current_item, module_id, extra_fragment)
.resolve(path_str, span, ns, current_item, module_id, extra_fragment)
.map(|(res, _)| res),
};

Expand All @@ -663,6 +673,7 @@ fn resolve_associated_trait_item(
did: DefId,
module: DefId,
item_name: Symbol,
span: rustc_span::Span,
ns: Namespace,
cx: &DocContext<'_>,
) -> Option<(ty::AssocKind, DefId)> {
Expand Down Expand Up @@ -711,7 +722,7 @@ fn resolve_associated_trait_item(
.associated_items(trait_)
.find_by_name_and_namespace(
cx.tcx,
Ident::with_dummy_span(item_name),
Ident::new(item_name, span),
ns,
trait_,
)
Expand All @@ -731,7 +742,7 @@ fn resolve_associated_trait_item(
candidates.extend(traits.iter().filter_map(|&trait_| {
cx.tcx
.associated_items(trait_)
.find_by_name_and_namespace(cx.tcx, Ident::with_dummy_span(item_name), ns, trait_)
.find_by_name_and_namespace(cx.tcx, Ident::new(item_name, span), ns, trait_)
.map(|assoc| (assoc.kind, assoc.def_id))
}));
}
Expand Down Expand Up @@ -925,12 +936,12 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
}
debug!("combined_docs={}", combined_docs);

let (krate, parent_node) = if let Some(id) = attr.parent_module {
let parent_node = if let Some(id) = attr.parent_module {
trace!("docs {:?} came from {:?}", attr.doc, id);
(id.krate, Some(id))
Some(id)
} else {
trace!("no parent found for {:?}", attr.doc);
(item.def_id.krate, parent_node)
parent_node
};
// NOTE: if there are links that start in one crate and end in another, this will not resolve them.
// This is a degenerate case and it's not supported by rustdoc.
Expand All @@ -941,7 +952,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
&current_item,
parent_node,
&parent_name,
krate,
ori_link,
link_range,
);
Expand Down Expand Up @@ -976,7 +986,6 @@ impl LinkCollector<'_, '_> {
current_item: &Option<String>,
parent_node: Option<DefId>,
parent_name: &Option<String>,
krate: CrateNum,
ori_link: String,
link_range: Option<Range<usize>>,
) -> Option<ItemLink> {
Expand Down Expand Up @@ -1041,7 +1050,7 @@ impl LinkCollector<'_, '_> {
parent_node
};

let mut module_id = if let Some(id) = base_node {
let module_id = if let Some(id) = base_node {
id
} else {
// This is a bug.
Expand All @@ -1063,21 +1072,14 @@ impl LinkCollector<'_, '_> {
// replace `Self` with suitable item's parent name
if path_str.starts_with("Self::") {
if let Some(ref name) = parent_name {
resolved_self = format!("{}::{}", name, &path_str[6..]);
resolved_self = format!("{}::{}", name, &path_str["Self::".len()..]);
path_str = &resolved_self;
}
} else if path_str.starts_with("crate::") {
use rustc_span::def_id::CRATE_DEF_INDEX;

// HACK(jynelson): rustc_resolve thinks that `crate` is the crate currently being documented.
// But rustdoc wants it to mean the crate this item was originally present in.
// To work around this, remove it and resolve relative to the crate root instead.
// HACK(jynelson)(2): If we just strip `crate::` then suddenly primitives become ambiguous
// (consider `crate::char`). Instead, change it to `self::`. This works because 'self' is now the crate root.
// FIXME(#78696): This doesn't always work.
resolved_self = format!("self::{}", &path_str["crate::".len()..]);
} else if path_str.starts_with("crate::") || path_str == "crate" {
// Resolve `crate` relative to the original scope of the item, not the current scope.
// Note that this depends on giving `rustc_resolve` the correct span for the item.
resolved_self = format!("${}", path_str); // Changes `crate::` to `$crate::`
path_str = &resolved_self;
module_id = DefId { krate, index: CRATE_DEF_INDEX };
}

// Strip generics from the path.
Expand Down Expand Up @@ -1242,9 +1244,11 @@ impl LinkCollector<'_, '_> {
ori_link: &str,
link_range: Option<Range<usize>>,
) -> Option<(Res, Option<String>)> {
let span = item.source.span();

match disambiguator.map(Disambiguator::ns) {
Some(ns @ (ValueNS | TypeNS)) => {
match self.resolve(path_str, ns, &current_item, base_node, &extra_fragment) {
match self.resolve(path_str, span, ns, &current_item, 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 @@ -1256,6 +1260,7 @@ impl LinkCollector<'_, '_> {
if let Some(res) = self.check_full_res(
new_ns,
path_str,
span,
base_node,
&current_item,
&extra_fragment,
Expand Down Expand Up @@ -1289,10 +1294,11 @@ impl LinkCollector<'_, '_> {
// Try everything!
let mut candidates = PerNS {
macro_ns: self
.resolve_macro(path_str, base_node)
.resolve_macro(path_str, span, base_node)
.map(|res| (res, extra_fragment.clone())),
type_ns: match self.resolve(
path_str,
span,
TypeNS,
&current_item,
base_node,
Expand All @@ -1310,6 +1316,7 @@ impl LinkCollector<'_, '_> {
},
value_ns: match self.resolve(
path_str,
span,
ValueNS,
&current_item,
base_node,
Expand Down Expand Up @@ -1377,14 +1384,15 @@ impl LinkCollector<'_, '_> {
}
}
Some(MacroNS) => {
match self.resolve_macro(path_str, base_node) {
match self.resolve_macro(path_str, span, base_node) {
Ok(res) => Some((res, extra_fragment)),
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,
span,
base_node,
&current_item,
&extra_fragment,
Expand Down Expand Up @@ -1664,6 +1672,8 @@ fn resolution_failure(
link_range: Option<Range<usize>>,
kinds: SmallVec<[ResolutionFailure<'_>; 3]>,
) {
let span = item.source.span();

report_diagnostic(
collector.cx,
BROKEN_INTRA_DOC_LINKS,
Expand Down Expand Up @@ -1727,7 +1737,7 @@ fn resolution_failure(
name = start;
for &ns in &[TypeNS, ValueNS, MacroNS] {
if let Some(res) =
collector.check_full_res(ns, &start, module_id, &None, &None)
collector.check_full_res(ns, &start, span, module_id, &None, &None)
{
debug!("found partial_res={:?}", res);
*partial_res = Some(res);
Expand Down