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.

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

Resolve items for cross-crate imports relative to the original module #73101

Merged
merged 19 commits into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 11 additions & 6 deletions src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,17 @@ impl<'a> Resolver<'a> {
(self.cstore().crate_name_untracked(def_id.krate), None)
} else {
let def_key = self.cstore().def_key(def_id);
(
// This unwrap is safe: crates must always have a name
def_key.disambiguated_data.data.get_opt_name().unwrap(),
// This unwrap is safe since we know this isn't the root
Some(self.get_module(DefId { index: def_key.parent.unwrap(), ..def_id })),
)
let name = def_key
.disambiguated_data
.data
.get_opt_name()
.expect("given a DefId that wasn't a module");
// This unwrap is safe since we know this isn't the root
let parent = Some(self.get_module(DefId {
index: def_key.parent.expect("failed to get parent for module"),
..def_id
}));
(name, parent)
};

// Allocate and return a new module with the information we found
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2978,7 +2978,7 @@ impl<'a> Resolver<'a> {
span: Span,
path_str: &str,
ns: Namespace,
module_id: LocalDefId,
module_id: DefId,
) -> Result<(ast::Path, Res), ()> {
let path = if path_str.starts_with("::") {
ast::Path {
Expand All @@ -2998,7 +2998,7 @@ impl<'a> Resolver<'a> {
.collect(),
}
};
let module = self.module_map.get(&module_id).copied().unwrap_or(self.graph_root);
let module = self.get_module(module_id);
let parent_scope = &ParentScope::module(module);
let res = self.resolve_ast_path(&path, ns, parent_scope).map_err(|_| ())?;
Ok((path, res))
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt
DUMMY_SP,
extern_name,
TypeNS,
LocalDefId { local_def_index: CRATE_DEF_INDEX },
LocalDefId { local_def_index: CRATE_DEF_INDEX }.to_def_id(),
)
.unwrap_or_else(|()| {
panic!("Unable to resolve external crate {}", extern_name)
Expand Down
83 changes: 46 additions & 37 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_hir::def::{
Namespace::{self, *},
PerNS, Res,
};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::def_id::DefId;
use rustc_middle::ty;
use rustc_resolve::ParentScope;
use rustc_session::lint;
Expand Down Expand Up @@ -50,7 +50,8 @@ enum ErrorKind {

struct LinkCollector<'a, 'tcx> {
cx: &'a DocContext<'tcx>,
mod_ids: Vec<hir::HirId>,
// NOTE: this may not necessarily be a module in the current crate
mod_ids: Vec<DefId>,
}

impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Expand All @@ -62,7 +63,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
&self,
path_str: &str,
current_item: &Option<String>,
module_id: LocalDefId,
module_id: DefId,
) -> Result<(Res, Option<String>), ErrorKind> {
let cx = self.cx;

Expand Down Expand Up @@ -124,7 +125,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}

/// Resolves a string as a macro.
fn macro_resolve(&self, path_str: &str, parent_id: Option<hir::HirId>) -> Option<Res> {
fn macro_resolve(&self, path_str: &str, parent_id: Option<DefId>) -> Option<Res> {
let cx = self.cx;
let path = ast::Path::from_ident(Ident::from_str(path_str));
cx.enter_resolver(|resolver| {
Expand All @@ -142,8 +143,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
if let Some(res) = resolver.all_macros().get(&Symbol::intern(path_str)) {
return Some(res.map_id(|_| panic!("unexpected id")));
}
if let Some(module_id) = parent_id.or(self.mod_ids.last().cloned()) {
let module_id = cx.tcx.hir().local_def_id(module_id);
if let Some(module_id) = parent_id {
if let Ok((_, res)) =
resolver.resolve_str_path_error(DUMMY_SP, path_str, MacroNS, module_id)
{
Expand All @@ -167,15 +167,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
disambiguator: Option<&str>,
ns: Namespace,
current_item: &Option<String>,
parent_id: Option<hir::HirId>,
parent_id: Option<DefId>,
extra_fragment: &Option<String>,
item_opt: Option<&Item>,
) -> Result<(Res, Option<String>), ErrorKind> {
let cx = self.cx;

// In case we're in a module, try to resolve the relative path.
if let Some(module_id) = parent_id.or(self.mod_ids.last().cloned()) {
let module_id = cx.tcx.hir().local_def_id(module_id);
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)
});
Expand Down Expand Up @@ -445,40 +444,40 @@ fn is_derive_trait_collision<T>(ns: &PerNS<Option<(Res, T)>>) -> bool {

impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
fn fold_item(&mut self, mut item: Item) -> Option<Item> {
let item_hir_id = if item.is_mod() {
if let Some(def_id) = item.def_id.as_local() {
Some(self.cx.tcx.hir().as_local_hir_id(def_id))
} else {
debug!("attempting to fold on a non-local item: {:?}", item);
return self.fold_item_recur(item);
}
} else {
None
};
use rustc_middle::ty::DefIdTree;
jyn514 marked this conversation as resolved.
Show resolved Hide resolved

// FIXME: get the resolver to work with non-local resolve scopes.
let parent_node = self.cx.as_local_hir_id(item.def_id).and_then(|hir_id| {
// FIXME: this fails hard for impls in non-module scope, but is necessary for the
// current `resolve()` implementation.
match self.cx.as_local_hir_id(self.cx.tcx.parent_module(hir_id).to_def_id()).unwrap() {
id if id != hir_id => Some(id),
_ => None,
let parent_node = if item.is_fake() {
// FIXME: is this correct?
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we'll never hit this case, it's only for auto traits and blanket impls, which don't get docs resolved on the local scope anyway.

What happens if you panic here? Especially if you try and document a blanket impl that uses intra doc links?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rustdoc panics if I don't check this (with or without an explicit panic):

thread 'rustc' panicked at 'index out of bounds: the len is 38899 but the index is 38905', /home/joshua/src/rust/src/librustc_hir/definitions.rs:53:9
  18: rustc_hir::definitions::DefPathTable::def_key
             at /home/joshua/src/rust/src/librustc_hir/definitions.rs:53
  19: rustc_metadata::rmeta::decoder::<impl rustc_metadata::creader::CrateMetadataRef>::def_key
             at src/librustc_metadata/rmeta/decoder.rs:1411
  20: rustc_metadata::rmeta::decoder::cstore_impl::<impl rustc_middle::middle::cstore::CrateStore for rustc_metadata::creader::CStore>::def_key
             at src/librustc_metadata/rmeta/decoder/cstore_impl.rs:484
  21: rustc_middle::ty::context::TyCtxt::def_key
             at src/librustc_middle/ty/context.rs:1188
  22: <rustc_middle::ty::context::TyCtxt as rustc_middle::ty::DefIdTree>::parent
             at src/librustc_middle/ty/mod.rs:358
  23: <rustdoc::passes::collect_intra_doc_links::LinkCollector as rustdoc::fold::DocFolder>::fold_item
             at src/librustdoc/passes/collect_intra_doc_links.rs:482

This was the motivation for #73098.

Copy link
Member Author

Choose a reason for hiding this comment

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

And actually it will panic on any file, including empty files.

None
} else {
let mut current = item.def_id;
// The immediate parent might not always be a module.
// Find the first parent which is.
loop {
if let Some(parent) = self.cx.tcx.parent(current) {
if self.cx.tcx.def_kind(parent) == DefKind::Mod {
break Some(parent);
}
current = parent;
} else {
break None;
}
}
});
};

if parent_node.is_some() {
debug!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.def_id);
trace!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.def_id);
}

let current_item = match item.inner {
ModuleItem(..) => {
if item.attrs.inner_docs {
if item_hir_id.unwrap() != hir::CRATE_HIR_ID { item.name.clone() } else { None }
if item.def_id.is_top_level_module() { item.name.clone() } else { None }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bug, there should be a ! in front.

Copy link
Member Author

Choose a reason for hiding this comment

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

This bit is getting removed altogether in #76467, so I don't think there's much point in fixing it.

} else {
match parent_node.or(self.mod_ids.last().cloned()) {
Some(parent) if parent != hir::CRATE_HIR_ID => {
match parent_node.or(self.mod_ids.last().copied()) {
Some(parent) if !parent.is_top_level_module() => {
// FIXME: can we pull the parent module's name from elsewhere?
Some(self.cx.tcx.hir().name(parent).to_string())
Some(self.cx.tcx.item_name(parent).to_string())
}
_ => None,
}
Expand All @@ -488,18 +487,22 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
for_.def_id().map(|did| self.cx.tcx.item_name(did).to_string())
}
// we don't display docs on `extern crate` items anyway, so don't process them.
ExternCrateItem(..) => return self.fold_item_recur(item),
ExternCrateItem(..) => {
debug!("ignoring extern crate item {:?}", item.def_id);
return self.fold_item_recur(item);
}
ImportItem(Import::Simple(ref name, ..)) => Some(name.clone()),
MacroItem(..) => None,
_ => item.name.clone(),
};

if item.is_mod() && item.attrs.inner_docs {
self.mod_ids.push(item_hir_id.unwrap());
self.mod_ids.push(item.def_id);
}

let cx = self.cx;
let dox = item.attrs.collapsed_doc_value().unwrap_or_else(String::new);
trace!("got documentation '{}'", dox);

look_for_tests(&cx, &dox, &item, true);

Expand Down Expand Up @@ -541,6 +544,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
});

for (ori_link, link_range) in markdown_links(&dox) {
trace!("considering link '{}'", ori_link);

// Bail early for real links.
if ori_link.contains('/') {
continue;
Expand Down Expand Up @@ -641,8 +646,11 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
// we've already pushed this node onto the resolution stack but
// for outer comments we explicitly try and resolve against the
// parent_node first.
let base_node =
if item.is_mod() && item.attrs.inner_docs { None } else { parent_node };
let base_node = if item.is_mod() && item.attrs.inner_docs {
self.mod_ids.last().copied()
} else {
parent_node
};

// replace `Self` with suitable item's parent name
if path_str.starts_with("Self::") {
Expand Down Expand Up @@ -826,7 +834,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
}

if item.is_mod() && !item.attrs.inner_docs {
self.mod_ids.push(item_hir_id.unwrap());
self.mod_ids.push(item.def_id);
}

if item.is_mod() {
Expand Down Expand Up @@ -864,6 +872,7 @@ fn build_diagnostic(
Some(hir_id) => hir_id,
None => {
// If non-local, no need to check anything.
info!("ignoring warning from parent crate: {}", err_msg);
return;
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/test/rustdoc-ui/intra-links-private.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// check-pass
// revisions: public private
// [private]compile-flags: --document-private-items
#![cfg_attr(private, deny(intra_doc_resolution_failure))]
#![cfg_attr(private, deny(intra_doc_link_resolution_failure))]

/// docs [DontDocMe]
//[public]~^ WARNING `[DontDocMe]` public documentation for `DocMe` links to a private item
Expand Down
10 changes: 10 additions & 0 deletions src/test/rustdoc/intra-doc-crate/additional_doc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// aux-build:additional_doc.rs
// build-aux-docs
#![deny(intra_doc_link_resolution_failure)]

extern crate my_rand;

// @has 'additional_doc/trait.Rng.html' '//a[@href="../additional_doc/trait.Rng.html"]' 'Rng'
// @has 'additional_doc/trait.Rng.html' '//a[@href="../my_rand/trait.RngCore.html"]' 'RngCore'
/// This is an [`Rng`].
pub use my_rand::Rng;
6 changes: 6 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/additional_doc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#![crate_name = "my_rand"]
#![deny(intra_doc_link_resolution_failure)]

pub trait RngCore {}
/// Rng extends [`RngCore`].
pub trait Rng: RngCore {}
7 changes: 7 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/intra-doc-basic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![crate_name = "a"]
#![deny(intra_doc_link_resolution_failure)]

pub struct Foo;

/// Link to [Foo]
pub struct Bar;
10 changes: 10 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/macro_inner.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![crate_name = "macro_inner"]
#![deny(intra_doc_link_resolution_failure)]

pub struct Foo;

/// See also [`Foo`]
#[macro_export]
macro_rules! my_macro {
() => {}
}
7 changes: 7 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/module.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![crate_name = "module_inner"]
#![deny(intra_doc_link_resolution_failure)]
/// [SomeType] links to [bar]
pub struct SomeType;
pub trait SomeTrait {}
/// [bar] links to [SomeTrait] and also [SomeType]
pub mod bar {}
20 changes: 20 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/proc_macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// force-host
// no-prefer-dynamic
// compile-flags: --crate-type proc-macro
#![crate_type="proc-macro"]
#![crate_name="proc_macro_inner"]

extern crate proc_macro;

use proc_macro::TokenStream;

/// Links to [`OtherDerive`]
#[proc_macro_derive(DeriveA)]
pub fn a_derive(input: TokenStream) -> TokenStream {
input
}

#[proc_macro_derive(OtherDerive)]
pub fn other_derive(input: TokenStream) -> TokenStream {
input
}
12 changes: 12 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/submodule-inner.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#![crate_name = "a"]
#![deny(intra_doc_link_resolution_failure)]

pub mod bar {
pub struct Bar;
}

pub mod foo {
use crate::bar;
/// link to [bar::Bar]
pub struct Foo;
}
13 changes: 13 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/submodule-outer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![crate_name = "bar"]
#![deny(intra_doc_link_resolution_failure)]

pub trait Foo {
/// [`Bar`] [`Baz`]
fn foo();
}

pub trait Bar {
}

pub trait Baz {
}
16 changes: 16 additions & 0 deletions src/test/rustdoc/intra-doc-crate/auxiliary/traits.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#![crate_name = "inner"]
/// this is a trait
pub trait SomeTrait {
/// this is a method for [a trait][SomeTrait]
fn foo();
}

pub mod bar {
use super::SomeTrait;

pub struct BarStruct;

impl SomeTrait for BarStruct {
fn foo() {}
}
}
9 changes: 9 additions & 0 deletions src/test/rustdoc/intra-doc-crate/basic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// aux-build:intra-doc-basic.rs
// build-aux-docs
#![deny(intra_doc_link_resolution_failure)]

// from https://github.com/rust-lang/rust/issues/65983
extern crate a;

// @has 'basic/struct.Bar.html' '//a[@href="../a/struct.Foo.html"]' 'Foo'
pub use a::Bar;
12 changes: 12 additions & 0 deletions src/test/rustdoc/intra-doc-crate/macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// ignore-tidy-linelength
// aux-build:macro_inner.rs
// aux-build:proc_macro.rs
// build-aux-docs
#![deny(intra_doc_link_resolution_failure)]
extern crate macro_inner;
extern crate proc_macro_inner;

// @has 'macro/macro.my_macro.html' '//a[@href="../macro_inner/struct.Foo.html"]' 'Foo'
pub use macro_inner::my_macro;
// @has 'macro/derive.DeriveA.html' '//a[@href="../proc_macro_inner/derive.OtherDerive.html"]' 'OtherDerive'
pub use proc_macro_inner::DeriveA;
8 changes: 8 additions & 0 deletions src/test/rustdoc/intra-doc-crate/module.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// outer.rs
// aux-build: module.rs
// build-aux-docs
#![deny(intra_doc_link_resolution_failure)]
extern crate module_inner;
// @has 'module/bar/index.html' '//a[@href="../../module_inner/trait.SomeTrait.html"]' 'SomeTrait'
// @has 'module/bar/index.html' '//a[@href="../../module_inner/struct.SomeType.html"]' 'SomeType'
pub use module_inner::bar;
Loading