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

rustdoc: Don't load all extern crates unconditionally #83738

Merged
merged 1 commit into from
Apr 3, 2021
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
86 changes: 55 additions & 31 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use rustc_ast as ast;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::sync::{self, Lrc};
use rustc_driver::abort_on_err;
Expand All @@ -22,7 +23,7 @@ use rustc_session::DiagnosticOutput;
use rustc_session::Session;
use rustc_span::source_map;
use rustc_span::symbol::sym;
use rustc_span::{Span, DUMMY_SP};
use rustc_span::Span;

use std::cell::RefCell;
use std::collections::hash_map::Entry;
Expand Down Expand Up @@ -348,42 +349,65 @@ crate fn create_config(
}

crate fn create_resolver<'a>(
externs: config::Externs,
queries: &Queries<'a>,
sess: &Session,
) -> Rc<RefCell<interface::BoxedResolver>> {
let extern_names: Vec<String> = externs
.iter()
.filter(|(_, entry)| entry.add_prelude)
.map(|(name, _)| name)
.cloned()
.collect();

let parts = abort_on_err(queries.expansion(), sess).peek();
let resolver = parts.1.borrow();

// Before we actually clone it, let's force all the extern'd crates to
// actually be loaded, just in case they're only referred to inside
// intra-doc links
resolver.borrow_mut().access(|resolver| {
sess.time("load_extern_crates", || {
for extern_name in &extern_names {
debug!("loading extern crate {}", extern_name);
if let Err(()) = resolver
.resolve_str_path_error(
DUMMY_SP,
extern_name,
TypeNS,
LocalDefId { local_def_index: CRATE_DEF_INDEX }.to_def_id(),
) {
warn!("unable to resolve external crate {} (do you have an unused `--extern` crate?)", extern_name)
}
let (krate, resolver, _) = &*parts;
let resolver = resolver.borrow().clone();

// Letting the resolver escape at the end of the function leads to inconsistencies between the
// crates the TyCtxt sees and the resolver sees (because the resolver could load more crates
// after escaping). Hopefully `IntraLinkCrateLoader` gets all the crates we need ...
struct IntraLinkCrateLoader {
current_mod: DefId,
resolver: Rc<RefCell<interface::BoxedResolver>>,
}
impl ast::visit::Visitor<'_> for IntraLinkCrateLoader {
fn visit_attribute(&mut self, attr: &ast::Attribute) {
use crate::html::markdown::{markdown_links, MarkdownLink};
use crate::passes::collect_intra_doc_links::Disambiguator;

if let Some(doc) = attr.doc_str() {
for MarkdownLink { link, .. } in markdown_links(&doc.as_str()) {
// FIXME: this misses a *lot* of the preprocessing done in collect_intra_doc_links
// I think most of it shouldn't be necessary since we only need the crate prefix?
let path_str = match Disambiguator::from_str(&link) {
Ok(x) => x.map_or(link.as_str(), |(_, p)| p),
Err(_) => continue,
};
self.resolver.borrow_mut().access(|resolver| {
let _ = resolver.resolve_str_path_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

If you save the resolution results for later instead of throwing them away, then you'll no longer need the resolver later on.
That's basically the idea behind fixing the resolver cloning/freezing issue.

attr.span,
path_str,
TypeNS,
self.current_mod,
);
});
}
}
});
});
ast::visit::walk_attribute(self, attr);
}

fn visit_item(&mut self, item: &ast::Item) {
use rustc_ast_lowering::ResolverAstLowering;

if let ast::ItemKind::Mod(..) = item.kind {
let new_mod =
self.resolver.borrow_mut().access(|resolver| resolver.local_def_id(item.id));
let old_mod = mem::replace(&mut self.current_mod, new_mod.to_def_id());
ast::visit::walk_item(self, item);
self.current_mod = old_mod;
} else {
ast::visit::walk_item(self, item);
}
}
}
let crate_id = LocalDefId { local_def_index: CRATE_DEF_INDEX }.to_def_id();
let mut loader = IntraLinkCrateLoader { current_mod: crate_id, resolver };
ast::visit::walk_crate(&mut loader, krate);

// Now we're good to clone the resolver because everything should be loaded
resolver.clone()
loader.resolver
}

crate fn run_global_ctxt(
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ extern crate tracing;
//
// Dependencies listed in Cargo.toml do not need `extern crate`.
extern crate rustc_ast;
extern crate rustc_ast_lowering;
extern crate rustc_ast_pretty;
extern crate rustc_attr;
extern crate rustc_data_structures;
Expand Down Expand Up @@ -637,7 +638,6 @@ fn main_options(options: config::Options) -> MainResult {
let default_passes = options.default_passes;
let output_format = options.output_format;
// FIXME: fix this clone (especially render_options)
let externs = options.externs.clone();
let manual_passes = options.manual_passes.clone();
let render_options = options.render_options.clone();
let config = core::create_config(options);
Expand All @@ -649,7 +649,7 @@ fn main_options(options: config::Options) -> MainResult {
// We need to hold on to the complete resolver, so we cause everything to be
// cloned for the analysis passes to use. Suboptimal, but necessary in the
// current architecture.
let resolver = core::create_resolver(externs, queries, &sess);
let resolver = core::create_resolver(queries, &sess);

if sess.has_errors() {
sess.fatal("Compilation failed, aborting rustdoc");
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1517,7 +1517,7 @@ fn range_between_backticks(ori_link: &MarkdownLink) -> Range<usize> {

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
/// Disambiguators for a link.
enum Disambiguator {
crate enum Disambiguator {
/// `prim@`
///
/// This is buggy, see <https://github.com/rust-lang/rust/pull/77875#discussion_r503583103>
Expand Down Expand Up @@ -1546,7 +1546,7 @@ impl Disambiguator {
/// This returns `Ok(Some(...))` if a disambiguator was found,
/// `Ok(None)` if no disambiguator was found, or `Err(...)`
/// if there was a problem with the disambiguator.
fn from_str(link: &str) -> Result<Option<(Self, &str)>, (String, Range<usize>)> {
crate fn from_str(link: &str) -> Result<Option<(Self, &str)>, (String, Range<usize>)> {
use Disambiguator::{Kind, Namespace as NS, Primitive};

if let Some(idx) = link.find('@') {
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ crate use self::unindent_comments::UNINDENT_COMMENTS;
mod propagate_doc_cfg;
crate use self::propagate_doc_cfg::PROPAGATE_DOC_CFG;

mod collect_intra_doc_links;
crate mod collect_intra_doc_links;
crate use self::collect_intra_doc_links::COLLECT_INTRA_DOC_LINKS;

mod doc_test_lints;
Expand Down
17 changes: 17 additions & 0 deletions src/test/rustdoc-ui/auxiliary/panic-item.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// no-prefer-dynamic
#![crate_type = "lib"]
#![no_std]
#![feature(lang_items)]

use core::panic::PanicInfo;
use core::sync::atomic::{self, Ordering};

#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
loop {
atomic::compiler_fence(Ordering::SeqCst);
}
}

#[lang = "eh_personality"]
fn foo() {}
3 changes: 3 additions & 0 deletions src/test/rustdoc-ui/unused-extern-crate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// check-pass
// aux-crate:panic_item=panic-item.rs
// @has unused_extern_crate/index.html
8 changes: 8 additions & 0 deletions src/test/rustdoc/intra-doc/extern-crate-only-used-in-link.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// aux-build:issue-66159-1.rs
// aux-crate:priv:issue_66159_1=issue-66159-1.rs
// build-aux-docs
// compile-flags:-Z unstable-options

// @has extern_crate_only_used_in_link/index.html
// @has - '//a[@href="../issue_66159_1/struct.Something.html"]' 'issue_66159_1::Something'
//! [issue_66159_1::Something]
10 changes: 0 additions & 10 deletions src/test/rustdoc/issue-66159.rs

This file was deleted.

4 changes: 2 additions & 2 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -708,8 +708,8 @@ impl Config {
self.parse_name_value_directive(line, "aux-crate").map(|r| {
let mut parts = r.trim().splitn(2, '=');
(
parts.next().expect("aux-crate name").to_string(),
parts.next().expect("aux-crate value").to_string(),
parts.next().expect("missing aux-crate name (e.g. log=log.rs)").to_string(),
parts.next().expect("missing aux-crate value (e.g. log=log.rs)").to_string(),
)
})
}
Expand Down