From 69d6c3b2e6daa58e24ccec31961fe9d59fa332bb Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 5 Apr 2022 23:46:44 +0300 Subject: [PATCH] rustdoc: Early doc link resolution fixes and refactorings --- compiler/rustc_metadata/src/rmeta/decoder.rs | 20 +- .../src/rmeta/decoder/cstore_impl.rs | 4 +- .../rustc_resolve/src/build_reduced_graph.rs | 2 +- compiler/rustc_resolve/src/lib.rs | 9 +- src/librustdoc/core.rs | 4 +- src/librustdoc/lib.rs | 1 + .../passes/collect_intra_doc_links.rs | 47 +++-- .../passes/collect_intra_doc_links/early.rs | 176 ++++++++++-------- .../intra-doc/assoc-mod-inner-outer.rs | 19 ++ .../auxiliary/assoc-mod-inner-outer-dep.rs | 11 ++ 10 files changed, 180 insertions(+), 113 deletions(-) create mode 100644 src/test/rustdoc-ui/intra-doc/assoc-mod-inner-outer.rs create mode 100644 src/test/rustdoc-ui/intra-doc/auxiliary/assoc-mod-inner-outer-dep.rs diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 046322a42d85b..3402acccf3f92 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -1169,14 +1169,18 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { } } - fn get_associated_item_def_ids(self, tcx: TyCtxt<'tcx>, id: DefIndex) -> &'tcx [DefId] { - if let Some(children) = self.root.tables.children.get(self, id) { - tcx.arena.alloc_from_iter( - children.decode((self, tcx.sess)).map(|child_index| self.local_def_id(child_index)), - ) - } else { - &[] - } + fn get_associated_item_def_ids( + self, + id: DefIndex, + sess: &'a Session, + ) -> impl Iterator + 'a { + self.root + .tables + .children + .get(self, id) + .unwrap_or_else(Lazy::empty) + .decode((self, sess)) + .map(move |child_index| self.local_def_id(child_index)) } fn get_associated_item(self, id: DefIndex) -> ty::AssocItem { diff --git a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs index cd3a1d72d41d2..63bf929fb8639 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs @@ -160,7 +160,9 @@ provide! { <'tcx> tcx, def_id, other, cdata, let _ = cdata; tcx.calculate_dtor(def_id, |_,_| Ok(())) } - associated_item_def_ids => { cdata.get_associated_item_def_ids(tcx, def_id.index) } + associated_item_def_ids => { + tcx.arena.alloc_from_iter(cdata.get_associated_item_def_ids(def_id.index, tcx.sess)) + } associated_item => { cdata.get_associated_item(def_id.index) } inherent_impls => { cdata.get_inherent_implementations_for_type(tcx, def_id.index) } is_foreign_item => { cdata.is_foreign_item(def_id.index) } diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index b706547f7fcb5..07d261da8132f 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -1248,7 +1248,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { }; let binding = (res, vis, span, expansion).to_name_binding(self.r.arenas); self.r.set_binding_parent_module(binding, parent_scope.module); - self.r.all_macros.insert(ident.name, res); + self.r.all_macro_rules.insert(ident.name, res); if is_macro_export { let module = self.r.graph_root; self.r.define(module, ident, MacroNS, (res, vis, span, expansion, IsMacroExport)); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 0c7d2f7b4e5ed..a09a225a2b5d7 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -1003,7 +1003,8 @@ pub struct Resolver<'a> { registered_attrs: FxHashSet, registered_tools: RegisteredTools, macro_use_prelude: FxHashMap>, - all_macros: FxHashMap, + /// FIXME: The only user of this is a doc link resolution hack for rustdoc. + all_macro_rules: FxHashMap, macro_map: FxHashMap>, dummy_ext_bang: Lrc, dummy_ext_derive: Lrc, @@ -1385,7 +1386,7 @@ impl<'a> Resolver<'a> { registered_attrs, registered_tools, macro_use_prelude: FxHashMap::default(), - all_macros: FxHashMap::default(), + all_macro_rules: Default::default(), macro_map: FxHashMap::default(), dummy_ext_bang: Lrc::new(SyntaxExtension::dummy_bang(session.edition())), dummy_ext_derive: Lrc::new(SyntaxExtension::dummy_derive(session.edition())), @@ -3311,8 +3312,8 @@ impl<'a> Resolver<'a> { } // For rustdoc. - pub fn all_macros(&self) -> &FxHashMap { - &self.all_macros + pub fn take_all_macro_rules(&mut self) -> FxHashMap { + mem::take(&mut self.all_macro_rules) } /// For rustdoc. diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index a32b9caa30fde..9d5ae68b3e46f 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -1,3 +1,4 @@ +use rustc_ast::NodeId; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::sync::{self, Lrc}; use rustc_errors::emitter::{Emitter, EmitterWriter}; @@ -17,7 +18,7 @@ use rustc_session::lint; use rustc_session::DiagnosticOutput; use rustc_session::Session; use rustc_span::symbol::sym; -use rustc_span::{source_map, Span}; +use rustc_span::{source_map, Span, Symbol}; use std::cell::RefCell; use std::lazy::SyncLazy; @@ -38,6 +39,7 @@ crate struct ResolverCaches { crate traits_in_scope: DefIdMap>, crate all_traits: Option>, crate all_trait_impls: Option>, + crate all_macro_rules: FxHashMap>, } crate struct DocContext<'tcx> { diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 1d7a790bdb786..0fcfabba4c0a1 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -10,6 +10,7 @@ #![feature(control_flow_enum)] #![feature(box_syntax)] #![feature(drain_filter)] +#![feature(let_chains)] #![feature(let_else)] #![feature(nll)] #![feature(test)] diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index e19920cc2ceb6..c1b1139ad8cf9 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -2,13 +2,11 @@ //! //! [RFC 1946]: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md +use pulldown_cmark::LinkType; use rustc_data_structures::{fx::FxHashMap, intern::Interned, stable_set::FxHashSet}; use rustc_errors::{Applicability, Diagnostic}; -use rustc_hir::def::{ - DefKind, - Namespace::{self, *}, - PerNS, -}; +use rustc_hir::def::Namespace::*; +use rustc_hir::def::{DefKind, Namespace, PerNS}; use rustc_hir::def_id::{DefId, CRATE_DEF_ID}; use rustc_hir::Mutability; use rustc_middle::ty::{DefIdTree, Ty, TyCtxt}; @@ -19,10 +17,7 @@ use rustc_span::symbol::{sym, Ident, Symbol}; use rustc_span::{BytePos, DUMMY_SP}; use smallvec::{smallvec, SmallVec}; -use pulldown_cmark::LinkType; - use std::borrow::Cow; -use std::convert::{TryFrom, TryInto}; use std::fmt::Write; use std::mem; use std::ops::Range; @@ -487,25 +482,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { item_id: ItemId, module_id: DefId, ) -> Result> { - self.cx.enter_resolver(|resolver| { - // NOTE: this needs 2 separate lookups because `resolve_rustdoc_path` doesn't take - // lexical scope into account (it ignores all macros not defined at the mod-level) - debug!("resolving {} as a macro in the module {:?}", path_str, module_id); - if let Some(res) = resolver.resolve_rustdoc_path(path_str, MacroNS, module_id) { - // don't resolve builtins like `#[derive]` - if let Ok(res) = res.try_into() { - return Ok(res); - } - } - if let Some(&res) = resolver.all_macros().get(&Symbol::intern(path_str)) { - return Ok(res.try_into().unwrap()); - } - Err(ResolutionFailure::NotResolved { + self.resolve_path(path_str, MacroNS, item_id, module_id).ok_or_else(|| { + ResolutionFailure::NotResolved { item_id, module_id, partial_res: None, unresolved: path_str.into(), - }) + } }) } @@ -539,6 +522,21 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }) } + /// HACK: Try to search the macro name in the list of all `macro_rules` items in the crate. + /// Used when nothing else works, may often give an incorrect result. + fn resolve_macro_rules(&self, path_str: &str, ns: Namespace) -> Option { + if ns != MacroNS { + return None; + } + + self.cx + .resolver_caches + .all_macro_rules + .get(&Symbol::intern(path_str)) + .copied() + .and_then(|res| res.try_into().ok()) + } + /// Convenience wrapper around `resolve_rustdoc_path`. /// /// This also handles resolving `true` and `false` as booleans. @@ -560,7 +558,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .cx .enter_resolver(|resolver| resolver.resolve_rustdoc_path(path_str, ns, module_id)) .and_then(|res| res.try_into().ok()) - .or_else(|| resolve_primitive(path_str, ns)); + .or_else(|| resolve_primitive(path_str, ns)) + .or_else(|| self.resolve_macro_rules(path_str, ns)); debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns); result } diff --git a/src/librustdoc/passes/collect_intra_doc_links/early.rs b/src/librustdoc/passes/collect_intra_doc_links/early.rs index 44bf86b082ad1..dffceff045d0b 100644 --- a/src/librustdoc/passes/collect_intra_doc_links/early.rs +++ b/src/librustdoc/passes/collect_intra_doc_links/early.rs @@ -1,4 +1,4 @@ -use crate::clean; +use crate::clean::Attributes; use crate::core::ResolverCaches; use crate::html::markdown::markdown_links; use crate::passes::collect_intra_doc_links::preprocess_link; @@ -24,7 +24,7 @@ crate fn early_resolve_intra_doc_links( externs: Externs, document_private_items: bool, ) -> ResolverCaches { - let mut loader = IntraLinkCrateLoader { + let mut link_resolver = EarlyDocLinkResolver { resolver, current_mod: CRATE_DEF_ID, visited_mods: Default::default(), @@ -36,27 +36,28 @@ crate fn early_resolve_intra_doc_links( // Overridden `visit_item` below doesn't apply to the crate root, // so we have to visit its attributes and reexports separately. - loader.load_links_in_attrs(&krate.attrs); - loader.process_module_children_or_reexports(CRATE_DEF_ID.to_def_id()); - visit::walk_crate(&mut loader, krate); - loader.add_foreign_traits_in_scope(); + link_resolver.load_links_in_attrs(&krate.attrs); + link_resolver.process_module_children_or_reexports(CRATE_DEF_ID.to_def_id()); + visit::walk_crate(&mut link_resolver, krate); + link_resolver.process_extern_impls(); // FIXME: somehow rustdoc is still missing crates even though we loaded all // the known necessary crates. Load them all unconditionally until we find a way to fix this. // DO NOT REMOVE THIS without first testing on the reproducer in // https://github.com/jyn514/objr/commit/edcee7b8124abf0e4c63873e8422ff81beb11ebb for (extern_name, _) in externs.iter().filter(|(_, entry)| entry.add_prelude) { - loader.resolver.resolve_rustdoc_path(extern_name, TypeNS, CRATE_DEF_ID.to_def_id()); + link_resolver.resolver.resolve_rustdoc_path(extern_name, TypeNS, CRATE_DEF_ID.to_def_id()); } ResolverCaches { - traits_in_scope: loader.traits_in_scope, - all_traits: Some(loader.all_traits), - all_trait_impls: Some(loader.all_trait_impls), + traits_in_scope: link_resolver.traits_in_scope, + all_traits: Some(link_resolver.all_traits), + all_trait_impls: Some(link_resolver.all_trait_impls), + all_macro_rules: link_resolver.resolver.take_all_macro_rules(), } } -struct IntraLinkCrateLoader<'r, 'ra> { +struct EarlyDocLinkResolver<'r, 'ra> { resolver: &'r mut Resolver<'ra>, current_mod: LocalDefId, visited_mods: DefIdSet, @@ -66,7 +67,7 @@ struct IntraLinkCrateLoader<'r, 'ra> { document_private_items: bool, } -impl IntraLinkCrateLoader<'_, '_> { +impl EarlyDocLinkResolver<'_, '_> { fn add_traits_in_scope(&mut self, def_id: DefId) { // Calls to `traits_in_scope` are expensive, so try to avoid them if only possible. // Keys in the `traits_in_scope` cache are always module IDs. @@ -101,64 +102,83 @@ impl IntraLinkCrateLoader<'_, '_> { /// That pass filters impls using type-based information, but we don't yet have such /// information here, so we just conservatively calculate traits in scope for *all* modules /// having impls in them. - fn add_foreign_traits_in_scope(&mut self) { - for cnum in Vec::from_iter(self.resolver.cstore().crates_untracked()) { - let all_traits = Vec::from_iter(self.resolver.cstore().traits_in_crate_untracked(cnum)); - let all_trait_impls = - Vec::from_iter(self.resolver.cstore().trait_impls_in_crate_untracked(cnum)); - let all_inherent_impls = - Vec::from_iter(self.resolver.cstore().inherent_impls_in_crate_untracked(cnum)); - let all_incoherent_impls = - Vec::from_iter(self.resolver.cstore().incoherent_impls_in_crate_untracked(cnum)); - - // Querying traits in scope is expensive so we try to prune the impl and traits lists - // using privacy, private traits and impls from other crates are never documented in - // the current crate, and links in their doc comments are not resolved. - for &def_id in &all_traits { - if self.resolver.cstore().visibility_untracked(def_id) == Visibility::Public { - self.add_traits_in_parent_scope(def_id); + fn process_extern_impls(&mut self) { + // FIXME: Need to resolve doc links on all these impl and trait items below. + // Resolving links in already existing crates may trigger loading of new crates. + let mut start_cnum = 0; + loop { + let crates = Vec::from_iter(self.resolver.cstore().crates_untracked()); + for &cnum in &crates[start_cnum..] { + let all_traits = + Vec::from_iter(self.resolver.cstore().traits_in_crate_untracked(cnum)); + let all_trait_impls = + Vec::from_iter(self.resolver.cstore().trait_impls_in_crate_untracked(cnum)); + let all_inherent_impls = + Vec::from_iter(self.resolver.cstore().inherent_impls_in_crate_untracked(cnum)); + let all_incoherent_impls = Vec::from_iter( + self.resolver.cstore().incoherent_impls_in_crate_untracked(cnum), + ); + + // Querying traits in scope is expensive so we try to prune the impl and traits lists + // using privacy, private traits and impls from other crates are never documented in + // the current crate, and links in their doc comments are not resolved. + for &def_id in &all_traits { + if self.resolver.cstore().visibility_untracked(def_id) == Visibility::Public { + self.add_traits_in_parent_scope(def_id); + } } - } - for &(trait_def_id, impl_def_id, simplified_self_ty) in &all_trait_impls { - if self.resolver.cstore().visibility_untracked(trait_def_id) == Visibility::Public - && simplified_self_ty.and_then(|ty| ty.def()).map_or(true, |ty_def_id| { - self.resolver.cstore().visibility_untracked(ty_def_id) == Visibility::Public - }) - { - self.add_traits_in_parent_scope(impl_def_id); + for &(trait_def_id, impl_def_id, simplified_self_ty) in &all_trait_impls { + if self.resolver.cstore().visibility_untracked(trait_def_id) + == Visibility::Public + && simplified_self_ty.and_then(|ty| ty.def()).map_or(true, |ty_def_id| { + self.resolver.cstore().visibility_untracked(ty_def_id) + == Visibility::Public + }) + { + self.add_traits_in_parent_scope(impl_def_id); + } } - } - for (ty_def_id, impl_def_id) in all_inherent_impls { - if self.resolver.cstore().visibility_untracked(ty_def_id) == Visibility::Public { - self.add_traits_in_parent_scope(impl_def_id); + for (ty_def_id, impl_def_id) in all_inherent_impls { + if self.resolver.cstore().visibility_untracked(ty_def_id) == Visibility::Public + { + self.add_traits_in_parent_scope(impl_def_id); + } } - } - for def_id in all_incoherent_impls { - self.add_traits_in_parent_scope(def_id); + for def_id in all_incoherent_impls { + self.add_traits_in_parent_scope(def_id); + } + + self.all_traits.extend(all_traits); + self.all_trait_impls + .extend(all_trait_impls.into_iter().map(|(_, def_id, _)| def_id)); } - self.all_traits.extend(all_traits); - self.all_trait_impls.extend(all_trait_impls.into_iter().map(|(_, def_id, _)| def_id)); + if crates.len() > start_cnum { + start_cnum = crates.len(); + } else { + break; + } } } fn load_links_in_attrs(&mut self, attrs: &[ast::Attribute]) { - // FIXME: this needs to consider reexport inlining. - let attrs = clean::Attributes::from_ast(attrs, None); - for (parent_module, doc) in attrs.collapsed_doc_value_by_module_level() { - let module_id = parent_module.unwrap_or(self.current_mod.to_def_id()); - - self.add_traits_in_scope(module_id); - + let module_id = self.current_mod.to_def_id(); + let mut need_traits_in_scope = false; + for (doc_module, doc) in + Attributes::from_ast(attrs, None).collapsed_doc_value_by_module_level() + { + assert_eq!(doc_module, None); 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.resolve_rustdoc_path(&path_str, TypeNS, module_id); + if let Some(Ok(pinfo)) = preprocess_link(&link) { + self.resolver.resolve_rustdoc_path(&pinfo.path_str, TypeNS, module_id); + need_traits_in_scope = true; + } } } + + if need_traits_in_scope { + self.add_traits_in_scope(module_id); + } } /// When reexports are inlined, they are replaced with item which they refer to, those items @@ -170,32 +190,41 @@ impl IntraLinkCrateLoader<'_, '_> { } for child in self.resolver.module_children_or_reexports(module_id) { - if child.vis == Visibility::Public || self.document_private_items { - if let Some(def_id) = child.res.opt_def_id() { - self.add_traits_in_parent_scope(def_id); - } - if let Res::Def(DefKind::Mod, module_id) = child.res { - self.process_module_children_or_reexports(module_id); + // This condition should give a superset of `denied` from `fn clean_use_statement`. + if child.vis == Visibility::Public + || self.document_private_items + && child.vis != Visibility::Restricted(module_id) + && module_id.is_local() + { + if let Some(def_id) = child.res.opt_def_id() && !def_id.is_local() { + // FIXME: Need to resolve doc links on all these extern items + // reached through reexports. + let scope_id = match child.res { + Res::Def(DefKind::Variant, ..) => self.resolver.parent(def_id).unwrap(), + _ => def_id, + }; + self.add_traits_in_parent_scope(scope_id); // Outer attribute scope + if let Res::Def(DefKind::Mod, ..) = child.res { + self.add_traits_in_scope(def_id); // Inner attribute scope + } + // Traits are processed in `add_extern_traits_in_scope`. + if let Res::Def(DefKind::Mod | DefKind::Enum, ..) = child.res { + self.process_module_children_or_reexports(def_id); + } } } } } } -impl Visitor<'_> for IntraLinkCrateLoader<'_, '_> { +impl Visitor<'_> for EarlyDocLinkResolver<'_, '_> { fn visit_item(&mut self, item: &ast::Item) { + self.load_links_in_attrs(&item.attrs); // Outer attribute scope if let ItemKind::Mod(..) = item.kind { let old_mod = mem::replace(&mut self.current_mod, self.resolver.local_def_id(item.id)); - - // A module written with a outline doc comments will resolve traits relative - // to the parent module. Make sure the parent module's traits-in-scope are - // loaded, even if the module itself has no doc comments. - self.add_traits_in_parent_scope(self.current_mod.to_def_id()); - - self.load_links_in_attrs(&item.attrs); + self.load_links_in_attrs(&item.attrs); // Inner attribute scope self.process_module_children_or_reexports(self.current_mod.to_def_id()); visit::walk_item(self, item); - self.current_mod = old_mod; } else { match item.kind { @@ -207,7 +236,6 @@ impl Visitor<'_> for IntraLinkCrateLoader<'_, '_> { } _ => {} } - self.load_links_in_attrs(&item.attrs); visit::walk_item(self, item); } } diff --git a/src/test/rustdoc-ui/intra-doc/assoc-mod-inner-outer.rs b/src/test/rustdoc-ui/intra-doc/assoc-mod-inner-outer.rs new file mode 100644 index 0000000000000..b4ce3443ccd18 --- /dev/null +++ b/src/test/rustdoc-ui/intra-doc/assoc-mod-inner-outer.rs @@ -0,0 +1,19 @@ +// Traits in scope are collected for doc links in both outer and inner module attributes. + +// check-pass +// aux-build: assoc-mod-inner-outer-dep.rs + +extern crate assoc_mod_inner_outer_dep; +pub use assoc_mod_inner_outer_dep::*; + +#[derive(Clone)] +pub struct Struct; + +pub mod outer1 { + /// [crate::Struct::clone] + pub mod inner {} +} + +pub mod outer2 { + //! [crate::Struct::clone] +} diff --git a/src/test/rustdoc-ui/intra-doc/auxiliary/assoc-mod-inner-outer-dep.rs b/src/test/rustdoc-ui/intra-doc/auxiliary/assoc-mod-inner-outer-dep.rs new file mode 100644 index 0000000000000..7a11a165723ff --- /dev/null +++ b/src/test/rustdoc-ui/intra-doc/auxiliary/assoc-mod-inner-outer-dep.rs @@ -0,0 +1,11 @@ +#[derive(Clone)] +pub struct Struct; + +pub mod dep_outer1 { + /// [crate::Struct::clone] + pub mod inner {} +} + +pub mod dep_outer2 { + //! [crate::Struct::clone] +}