Skip to content

Commit

Permalink
Rollup merge of rust-lang#85749 - GuillaumeGomez:revert-smart-extern-…
Browse files Browse the repository at this point in the history
…crate-load, r=jyn514

Revert "Don't load all extern crates unconditionally"

Fixes rust-lang#84738.

This reverts rust-lang#83738.

For the "smart" load of external crates, we need to be able to access their items in order to check their doc comments, which seems, if not impossible, quite complicated using only the AST.

For some context, I first tried to extend the `IntraLinkCrateLoader` visitor by adding `visit_foreign_item`. Unfortunately, it never enters into this call, so definitely not the right place...

I then added `visit_use_tree` to then check all the imports outside with something like this:

```rust
let mut loader = crate::passes::collect_intra_doc_links::IntraLinkCrateLoader::new(resolver);
    ast::visit::walk_crate(&mut loader, krate);

    let mut items = Vec::new();
    for import in &loader.imports_to_check {
        if let Some(item) = krate.items.iter().find(|i| i.id == *import) {
            items.push(item);
        }
    }
    for item in items {
        ast::visit::walk_item(&mut item);
        for attr in &item.attrs {
            loader.check_attribute(attr);
        }
    }
```

This was, of course, a failure. We find the items without problems, but we still can't go into the external crate to check its items' attributes.

Finally, `@jyn514` suggested to look into the [`CrateLoader`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CrateLoader.html), but it only seems to provide metadata (I went through [`CStore`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CStore.html) and [`CrateMetadata`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/rmeta/decoder/struct.CrateMetadata.html)).

I think we are too limited here (with AST only) to be able to determine the crates we actually need to import, but it's very likely that I missed something. Maybe `@petrochenkov` or `@Aaron1011` have an idea?

So until we find a way to make it work completely, we need to revert it to fix the ICE. Once merged, we'll need to re-open rust-lang#68427.

r? `@jyn514`
  • Loading branch information
GuillaumeGomez authored Jul 2, 2021
2 parents 1aa6c7c + 5f0c54d commit b7654a3
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 98 deletions.
43 changes: 34 additions & 9 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
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;
use rustc_errors::emitter::{Emitter, EmitterWriter};
use rustc_errors::json::JsonEmitter;
use rustc_feature::UnstableFeatures;
use rustc_hir::def::Namespace::TypeNS;
use rustc_hir::def::Res;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::def_id::{DefId, LocalDefId, CRATE_DEF_INDEX};
use rustc_hir::HirId;
use rustc_hir::{
intravisit::{self, NestedVisitorMap, Visitor},
Expand All @@ -23,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;
use rustc_span::{Span, DUMMY_SP};

use std::cell::RefCell;
use std::mem;
Expand Down Expand Up @@ -300,16 +300,41 @@ crate fn create_config(
}

crate fn create_resolver<'a>(
externs: config::Externs,
queries: &Queries<'a>,
sess: &Session,
) -> Rc<RefCell<interface::BoxedResolver>> {
let (krate, resolver, _) = &*abort_on_err(queries.expansion(), sess).peek();
let resolver = resolver.clone();

let mut loader = crate::passes::collect_intra_doc_links::IntraLinkCrateLoader::new(resolver);
ast::visit::walk_crate(&mut loader, krate);
let extern_names: Vec<String> = externs
.iter()
.filter(|(_, entry)| entry.add_prelude)
.map(|(name, _)| name)
.cloned()
.collect();

let (_, resolver, _) = &*abort_on_err(queries.expansion(), sess).peek();

// 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)
}
}
});
});

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

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,7 +31,6 @@ 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 @@ -714,6 +713,7 @@ 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 @@ -731,7 +731,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(queries, &sess);
let resolver = core::create_resolver(externs, queries, &sess);

if sess.has_errors() {
sess.fatal("Compilation failed, aborting rustdoc");
Expand Down
3 changes: 0 additions & 3 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ use crate::html::markdown::{markdown_links, MarkdownLink};
use crate::lint::{BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS};
use crate::passes::Pass;

mod early;
crate use early::IntraLinkCrateLoader;

crate const COLLECT_INTRA_DOC_LINKS: Pass = Pass {
name: "collect-intra-doc-links",
run: collect_intra_doc_links,
Expand Down
63 changes: 0 additions & 63 deletions src/librustdoc/passes/collect_intra_doc_links/early.rs

This file was deleted.

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;

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

mod doc_test_lints;
Expand Down
17 changes: 0 additions & 17 deletions src/test/rustdoc-ui/auxiliary/panic-item.rs

This file was deleted.

3 changes: 0 additions & 3 deletions src/test/rustdoc-ui/unused-extern-crate.rs

This file was deleted.

2 changes: 2 additions & 0 deletions src/test/rustdoc/auxiliary/issue-66159-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/// This will be referred to by the test docstring
pub struct Something;
10 changes: 10 additions & 0 deletions src/test/rustdoc/issue-66159.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// aux-crate:priv:issue_66159_1=issue-66159-1.rs
// compile-flags:-Z unstable-options

// The issue was an ICE which meant that we never actually generated the docs
// so if we have generated the docs, we're okay.
// Since we don't generate the docs for the auxiliary files, we can't actually
// verify that the struct is linked correctly.

// @has issue_66159/index.html
//! [issue_66159_1::Something]

0 comments on commit b7654a3

Please sign in to comment.