From d933edd5c6ee149d53bff4558c3fef8dc68b8766 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 21 Aug 2021 20:15:20 +0000 Subject: [PATCH 1/2] Revert "Revert "Don't load all extern crates unconditionally"" This reverts commit 5f0c54db4e595a6a77048f2b0605138ffa49a326. --- src/librustdoc/core.rs | 41 +++--------- src/librustdoc/lib.rs | 4 +- .../passes/collect_intra_doc_links.rs | 3 + .../passes/collect_intra_doc_links/early.rs | 63 +++++++++++++++++++ src/librustdoc/passes/mod.rs | 2 +- src/test/rustdoc-ui/auxiliary/panic-item.rs | 17 +++++ src/test/rustdoc-ui/unused-extern-crate.rs | 3 + src/test/rustdoc/auxiliary/issue-66159-1.rs | 2 - .../rustdoc/{ => intra-doc}/issue-66159.rs | 4 +- 9 files changed, 99 insertions(+), 40 deletions(-) create mode 100644 src/librustdoc/passes/collect_intra_doc_links/early.rs create mode 100644 src/test/rustdoc-ui/auxiliary/panic-item.rs create mode 100644 src/test/rustdoc-ui/unused-extern-crate.rs delete mode 100644 src/test/rustdoc/auxiliary/issue-66159-1.rs rename src/test/rustdoc/{ => intra-doc}/issue-66159.rs (80%) diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 512c4ed2d3c..249febb72bc 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -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, CRATE_DEF_INDEX}; +use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_hir::HirId; use rustc_hir::{ intravisit::{self, NestedVisitorMap, Visitor}, @@ -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, DUMMY_SP}; +use rustc_span::Span; use std::cell::RefCell; use std::mem; @@ -301,41 +301,16 @@ crate fn create_config( } crate fn create_resolver<'a>( - externs: config::Externs, queries: &Queries<'a>, sess: &Session, ) -> Rc> { - let extern_names: Vec = externs - .iter() - .filter(|(_, entry)| entry.add_prelude) - .map(|(name, _)| name) - .cloned() - .collect(); + let (krate, resolver, _) = &*abort_on_err(queries.expansion(), sess).peek(); + let resolver = resolver.clone(); - let (_, resolver, _) = &*abort_on_err(queries.expansion(), sess).peek(); + let mut loader = crate::passes::collect_intra_doc_links::IntraLinkCrateLoader::new(resolver); + ast::visit::walk_crate(&mut loader, krate); - // 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) - } - } - }); - }); - - // Now we're good to clone the resolver because everything should be loaded - resolver.clone() + loader.resolver } crate fn run_global_ctxt( diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index e02d92b11b8..40b1f243a05 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -30,6 +30,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; @@ -724,7 +725,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); @@ -742,7 +742,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"); diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 807872ae4fd..b909f6b2695 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -37,6 +37,9 @@ 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, diff --git a/src/librustdoc/passes/collect_intra_doc_links/early.rs b/src/librustdoc/passes/collect_intra_doc_links/early.rs new file mode 100644 index 00000000000..7cba2523d1a --- /dev/null +++ b/src/librustdoc/passes/collect_intra_doc_links/early.rs @@ -0,0 +1,63 @@ +use rustc_ast as ast; +use rustc_hir::def::Namespace::TypeNS; +use rustc_hir::def_id::{DefId, LocalDefId, CRATE_DEF_INDEX}; +use rustc_interface::interface; + +use std::cell::RefCell; +use std::mem; +use std::rc::Rc; + +// 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 ... +crate struct IntraLinkCrateLoader { + current_mod: DefId, + crate resolver: Rc>, +} + +impl IntraLinkCrateLoader { + crate fn new(resolver: Rc>) -> Self { + let crate_id = LocalDefId { local_def_index: CRATE_DEF_INDEX }.to_def_id(); + Self { current_mod: crate_id, resolver } + } +} + +impl ast::visit::Visitor<'_> for IntraLinkCrateLoader { + fn visit_attribute(&mut self, attr: &ast::Attribute) { + use crate::html::markdown::markdown_links; + use crate::passes::collect_intra_doc_links::preprocess_link; + + if let Some(doc) = attr.doc_str() { + for link in markdown_links(&doc.as_str()) { + let path_str = if let Some(Ok(x)) = preprocess_link(&link) { + x.path_str + } else { + 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); + } + } +} diff --git a/src/librustdoc/passes/mod.rs b/src/librustdoc/passes/mod.rs index 390ab1694a0..0e86fe45640 100644 --- a/src/librustdoc/passes/mod.rs +++ b/src/librustdoc/passes/mod.rs @@ -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; diff --git a/src/test/rustdoc-ui/auxiliary/panic-item.rs b/src/test/rustdoc-ui/auxiliary/panic-item.rs new file mode 100644 index 00000000000..17b26850d4d --- /dev/null +++ b/src/test/rustdoc-ui/auxiliary/panic-item.rs @@ -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() {} diff --git a/src/test/rustdoc-ui/unused-extern-crate.rs b/src/test/rustdoc-ui/unused-extern-crate.rs new file mode 100644 index 00000000000..f703a183790 --- /dev/null +++ b/src/test/rustdoc-ui/unused-extern-crate.rs @@ -0,0 +1,3 @@ +// check-pass +// aux-crate:panic_item=panic-item.rs +// @has unused_extern_crate/index.html diff --git a/src/test/rustdoc/auxiliary/issue-66159-1.rs b/src/test/rustdoc/auxiliary/issue-66159-1.rs deleted file mode 100644 index 2f3d069bd51..00000000000 --- a/src/test/rustdoc/auxiliary/issue-66159-1.rs +++ /dev/null @@ -1,2 +0,0 @@ -/// This will be referred to by the test docstring -pub struct Something; diff --git a/src/test/rustdoc/issue-66159.rs b/src/test/rustdoc/intra-doc/issue-66159.rs similarity index 80% rename from src/test/rustdoc/issue-66159.rs rename to src/test/rustdoc/intra-doc/issue-66159.rs index 003d079a470..56742b39790 100644 --- a/src/test/rustdoc/issue-66159.rs +++ b/src/test/rustdoc/intra-doc/issue-66159.rs @@ -1,4 +1,4 @@ -// aux-crate:priv:issue_66159_1=issue-66159-1.rs +// aux-crate:priv:pub_struct=pub-struct.rs // compile-flags:-Z unstable-options // The issue was an ICE which meant that we never actually generated the docs @@ -7,4 +7,4 @@ // verify that the struct is linked correctly. // @has issue_66159/index.html -//! [issue_66159_1::Something] +//! [pub_struct::SomeStruct] From c60a370dac4821d0a1a4d55943e28c2da2220dc7 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 21 Aug 2021 20:14:56 +0000 Subject: [PATCH 2/2] Fix the bugs and add a regression test - All attributes for an item need to be considered at once, they can't be considered a line at a time. - The top-level crate was not being visited. This bug was caught by `extern-crate-used-only-in-link`, which I'm very glad I added. - Make the loader private to the module, so that only one function is exposed. --- src/librustdoc/core.rs | 6 +-- .../passes/collect_intra_doc_links.rs | 2 +- .../passes/collect_intra_doc_links/early.rs | 46 ++++++++++++------- .../rustdoc/intra-doc/auxiliary/pub-struct.rs | 1 + .../intra-doc/extern-reference-link.rs | 7 +++ 5 files changed, 39 insertions(+), 23 deletions(-) create mode 100644 src/test/rustdoc/intra-doc/auxiliary/pub-struct.rs create mode 100644 src/test/rustdoc/intra-doc/extern-reference-link.rs diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 249febb72bc..4d498e6dbdb 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -1,4 +1,3 @@ -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; @@ -307,10 +306,7 @@ crate fn create_resolver<'a>( 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); - - loader.resolver + crate::passes::collect_intra_doc_links::load_intra_link_crates(resolver, krate) } crate fn run_global_ctxt( diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index b909f6b2695..178c8c15a15 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -38,7 +38,7 @@ use crate::lint::{BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS}; use crate::passes::Pass; mod early; -crate use early::IntraLinkCrateLoader; +crate use early::load_intra_link_crates; crate const COLLECT_INTRA_DOC_LINKS: Pass = Pass { name: "collect-intra-doc-links", diff --git a/src/librustdoc/passes/collect_intra_doc_links/early.rs b/src/librustdoc/passes/collect_intra_doc_links/early.rs index 7cba2523d1a..cd90528ab9c 100644 --- a/src/librustdoc/passes/collect_intra_doc_links/early.rs +++ b/src/librustdoc/passes/collect_intra_doc_links/early.rs @@ -1,34 +1,41 @@ use rustc_ast as ast; use rustc_hir::def::Namespace::TypeNS; -use rustc_hir::def_id::{DefId, LocalDefId, CRATE_DEF_INDEX}; +use rustc_hir::def_id::{LocalDefId, CRATE_DEF_ID}; use rustc_interface::interface; +use rustc_span::Span; use std::cell::RefCell; use std::mem; use std::rc::Rc; +type Resolver = Rc>; // 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 ... -crate struct IntraLinkCrateLoader { - current_mod: DefId, - crate resolver: Rc>, +crate fn load_intra_link_crates(resolver: Resolver, krate: &ast::Crate) -> Resolver { + let mut loader = IntraLinkCrateLoader { current_mod: CRATE_DEF_ID, resolver }; + // `walk_crate` doesn't visit the crate itself for some reason. + loader.load_links_in_attrs(&krate.attrs, krate.span); + ast::visit::walk_crate(&mut loader, krate); + loader.resolver +} + +struct IntraLinkCrateLoader { + current_mod: LocalDefId, + resolver: Rc>, } impl IntraLinkCrateLoader { - crate fn new(resolver: Rc>) -> Self { - let crate_id = LocalDefId { local_def_index: CRATE_DEF_INDEX }.to_def_id(); - Self { current_mod: crate_id, resolver } - } -} - -impl ast::visit::Visitor<'_> for IntraLinkCrateLoader { - fn visit_attribute(&mut self, attr: &ast::Attribute) { + fn load_links_in_attrs(&mut self, attrs: &[ast::Attribute], span: Span) { use crate::html::markdown::markdown_links; use crate::passes::collect_intra_doc_links::preprocess_link; - if let Some(doc) = attr.doc_str() { + // FIXME: this probably needs to consider inlining + let attrs = crate::clean::Attributes::from_ast(attrs, None); + for (parent_module, doc) in attrs.collapsed_doc_value_by_module_level() { + debug!(?doc); for link in markdown_links(&doc.as_str()) { + debug!(?link.link); let path_str = if let Some(Ok(x)) = preprocess_link(&link) { x.path_str } else { @@ -36,27 +43,32 @@ impl ast::visit::Visitor<'_> for IntraLinkCrateLoader { }; self.resolver.borrow_mut().access(|resolver| { let _ = resolver.resolve_str_path_error( - attr.span, + span, &path_str, TypeNS, - self.current_mod, + parent_module.unwrap_or(self.current_mod.to_def_id()), ); }); } } - ast::visit::walk_attribute(self, attr); } +} +impl ast::visit::Visitor<'_> for IntraLinkCrateLoader { 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()); + let old_mod = mem::replace(&mut self.current_mod, new_mod); + + self.load_links_in_attrs(&item.attrs, item.span); ast::visit::walk_item(self, item); + self.current_mod = old_mod; } else { + self.load_links_in_attrs(&item.attrs, item.span); ast::visit::walk_item(self, item); } } diff --git a/src/test/rustdoc/intra-doc/auxiliary/pub-struct.rs b/src/test/rustdoc/intra-doc/auxiliary/pub-struct.rs new file mode 100644 index 00000000000..75d4289321c --- /dev/null +++ b/src/test/rustdoc/intra-doc/auxiliary/pub-struct.rs @@ -0,0 +1 @@ +pub struct SomeStruct; diff --git a/src/test/rustdoc/intra-doc/extern-reference-link.rs b/src/test/rustdoc/intra-doc/extern-reference-link.rs new file mode 100644 index 00000000000..bad6ec75579 --- /dev/null +++ b/src/test/rustdoc/intra-doc/extern-reference-link.rs @@ -0,0 +1,7 @@ +// compile-flags: --extern pub_struct +// aux-build:pub-struct.rs + +/// [SomeStruct] +/// +/// [SomeStruct]: pub_struct::SomeStruct +pub fn foo() {}