Skip to content

Commit

Permalink
Don't load all extern crates unconditionally
Browse files Browse the repository at this point in the history
Instead, only load the crates that are linked to with intra-doc links.

This doesn't help very much with any of rustdoc's fundamental issues
with freezing the resolver, but it at least fixes a stable-to-stable
regression, and makes the crate loading model somewhat more consistent
with rustc's.
  • Loading branch information
jyn514 committed Apr 1, 2021
1 parent a5029ac commit cc69c65
Show file tree
Hide file tree
Showing 10 changed files with 88 additions and 48 deletions.
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(
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
15 changes: 15 additions & 0 deletions src/test/rustdoc-ui/auxiliary/panic-item.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![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
File renamed without changes.
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

0 comments on commit cc69c65

Please sign in to comment.