Skip to content

Commit

Permalink
Rollup merge of rust-lang#63149 - petrochenkov:lazypop2, r=eddyb
Browse files Browse the repository at this point in the history 
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb
  • Loading branch information
@Mark-Simulacrum
Mark-Simulacrum authored Aug 11, 2019
2 parents 6381937 + a087df6 commit 9b835f3
Show file tree
Hide file tree
Showing 9 changed files with 149 additions and 159 deletions.
122 changes: 41 additions & 81 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,19 +157,6 @@ impl<'a> Resolver<'a> {
self.macro_map.insert(def_id, ext.clone());
Some(ext)
}

/// Ensures that the reduced graph rooted at the given external module
/// is built, building it if it is not.
pub fn populate_module_if_necessary(&mut self, module: Module<'a>) {
if module.populated.get() { return }
let def_id = module.def_id().unwrap();
for child in self.cstore.item_children_untracked(def_id, self.session) {
let child = child.map_id(|_| panic!("unexpected id"));
BuildReducedGraphVisitor { parent_scope: self.dummy_parent_scope(), r: self }
.build_reduced_graph_for_external_crate_res(module, child);
}
module.populated.set(true)
}
}

pub struct BuildReducedGraphVisitor<'a, 'b> {
Expand Down Expand Up @@ -595,7 +582,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
self.r.get_module(DefId { krate: crate_id, index: CRATE_DEF_INDEX })
};

self.r.populate_module_if_necessary(module);
if let Some(name) = self.r.session.parse_sess.injected_crate_name.try_get() {
if name.as_str() == ident.name.as_str() {
self.r.injected_crate = Some(module);
Expand Down Expand Up @@ -868,7 +854,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
}

/// Builds the reduced graph for a single item in an external crate.
fn build_reduced_graph_for_external_crate_res(
crate fn build_reduced_graph_for_external_crate_res(
&mut self,
parent: Module<'a>,
child: Export<ast::NodeId>,
Expand All @@ -879,88 +865,62 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
// This is only a guess, two equivalent idents may incorrectly get different gensyms here.
let ident = ident.gensym_if_underscore();
let expansion = ExpnId::root(); // FIXME(jseyfried) intercrate hygiene
// Record primary definitions.
match res {
Res::Def(kind @ DefKind::Mod, def_id)
| Res::Def(kind @ DefKind::Enum, def_id) => {
| Res::Def(kind @ DefKind::Enum, def_id)
| Res::Def(kind @ DefKind::Trait, def_id) => {
let module = self.r.new_module(parent,
ModuleKind::Def(kind, def_id, ident.name),
def_id,
expansion,
span);
self.r.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, expansion));
self.r.define(parent, ident, TypeNS, (module, vis, span, expansion));
}
Res::Def(DefKind::Variant, _)
Res::Def(DefKind::Struct, _)
| Res::Def(DefKind::Union, _)
| Res::Def(DefKind::Variant, _)
| Res::Def(DefKind::TyAlias, _)
| Res::Def(DefKind::ForeignTy, _)
| Res::Def(DefKind::OpaqueTy, _)
| Res::Def(DefKind::TraitAlias, _)
| Res::Def(DefKind::AssocTy, _)
| Res::Def(DefKind::AssocOpaqueTy, _)
| Res::PrimTy(..)
| Res::ToolMod => {
self.r.define(parent, ident, TypeNS, (res, vis, DUMMY_SP, expansion));
}
| Res::ToolMod =>
self.r.define(parent, ident, TypeNS, (res, vis, span, expansion)),
Res::Def(DefKind::Fn, _)
| Res::Def(DefKind::Method, _)
| Res::Def(DefKind::Static, _)
| Res::Def(DefKind::Const, _)
| Res::Def(DefKind::Ctor(CtorOf::Variant, ..), _) => {
self.r.define(parent, ident, ValueNS, (res, vis, DUMMY_SP, expansion));
}
Res::Def(DefKind::Ctor(CtorOf::Struct, ..), def_id) => {
self.r.define(parent, ident, ValueNS, (res, vis, DUMMY_SP, expansion));

if let Some(struct_def_id) =
self.r.cstore.def_key(def_id).parent
.map(|index| DefId { krate: def_id.krate, index: index }) {
self.r.struct_constructors.insert(struct_def_id, (res, vis));
}
}
Res::Def(DefKind::Trait, def_id) => {
let module_kind = ModuleKind::Def(DefKind::Trait, def_id, ident.name);
let module = self.r.new_module(parent,
module_kind,
parent.normal_ancestor_id,
expansion,
span);
self.r.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, expansion));

for child in self.r.cstore.item_children_untracked(def_id, self.r.session) {
let res = child.res.map_id(|_| panic!("unexpected id"));
let ns = if let Res::Def(DefKind::AssocTy, _) = res {
TypeNS
} else { ValueNS };
self.r.define(module, child.ident, ns,
(res, ty::Visibility::Public, DUMMY_SP, expansion));

if self.r.cstore.associated_item_cloned_untracked(child.res.def_id())
.method_has_self_argument {
self.r.has_self.insert(res.def_id());
}
}
module.populated.set(true);
}
| Res::Def(DefKind::AssocConst, _)
| Res::Def(DefKind::Ctor(..), _) =>
self.r.define(parent, ident, ValueNS, (res, vis, span, expansion)),
Res::Def(DefKind::Macro(..), _)
| Res::NonMacroAttr(..) =>
self.r.define(parent, ident, MacroNS, (res, vis, span, expansion)),
Res::Def(DefKind::TyParam, _) | Res::Def(DefKind::ConstParam, _)
| Res::Local(..) | Res::SelfTy(..) | Res::SelfCtor(..) | Res::Err =>
bug!("unexpected resolution: {:?}", res)
}
// Record some extra data for better diagnostics.
match res {
Res::Def(DefKind::Struct, def_id) | Res::Def(DefKind::Union, def_id) => {
self.r.define(parent, ident, TypeNS, (res, vis, DUMMY_SP, expansion));

// Record field names for error reporting.
let field_names = self.r.cstore.struct_field_names_untracked(def_id);
self.insert_field_names(def_id, field_names);
}
Res::Def(DefKind::Macro(..), _) | Res::NonMacroAttr(..) => {
self.r.define(parent, ident, MacroNS, (res, vis, DUMMY_SP, expansion));
Res::Def(DefKind::Method, def_id) => {
if self.r.cstore.associated_item_cloned_untracked(def_id).method_has_self_argument {
self.r.has_self.insert(def_id);
}
}
Res::Def(DefKind::Ctor(CtorOf::Struct, ..), def_id) => {
let parent = self.r.cstore.def_key(def_id).parent;
if let Some(struct_def_id) = parent.map(|index| DefId { index, ..def_id }) {
self.r.struct_constructors.insert(struct_def_id, (res, vis));
}
}
_ => bug!("unexpected resolution: {:?}", res)
}
}

fn legacy_import_macro(&mut self,
name: Name,
binding: &'a NameBinding<'a>,
span: Span,
allow_shadowing: bool) {
if self.r.macro_use_prelude.insert(name, binding).is_some() && !allow_shadowing {
let msg = format!("`{}` is already in scope", name);
let note =
"macro-expanded `#[macro_use]`s may not shadow existing macros (see RFC 1560)";
self.r.session.struct_span_err(span, &msg).note(note).emit();
_ => {}
}
}

Expand Down Expand Up @@ -1021,9 +981,9 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
if let Some(span) = import_all {
let directive = macro_use_directive(self, span);
self.r.potentially_unused_imports.push(directive);
module.for_each_child(|ident, ns, binding| if ns == MacroNS {
let imported_binding = self.r.import(binding, directive);
self.legacy_import_macro(ident.name, imported_binding, span, allow_shadowing);
self.r.for_each_child(module, |this, ident, ns, binding| if ns == MacroNS {
let imported_binding = this.import(binding, directive);
this.legacy_import_macro(ident.name, imported_binding, span, allow_shadowing);
});
} else {
for ident in single_imports.iter().cloned() {
Expand All @@ -1039,8 +999,8 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
let directive = macro_use_directive(self, ident.span);
self.r.potentially_unused_imports.push(directive);
let imported_binding = self.r.import(binding, directive);
self.legacy_import_macro(ident.name, imported_binding,
ident.span, allow_shadowing);
self.r.legacy_import_macro(ident.name, imported_binding,
ident.span, allow_shadowing);
} else {
span_err!(self.r.session, ident.span, E0469, "imported macro not found");
}
Expand Down
41 changes: 20 additions & 21 deletions src/librustc_resolve/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,23 @@ crate fn add_typo_suggestion(
false
}

crate fn add_module_candidates(
module: Module<'_>, names: &mut Vec<TypoSuggestion>, filter_fn: &impl Fn(Res) -> bool
) {
for (&(ident, _), resolution) in module.resolutions.borrow().iter() {
if let Some(binding) = resolution.borrow().binding {
let res = binding.res();
if filter_fn(res) {
names.push(TypoSuggestion::from_res(ident.name, res));
impl<'a> Resolver<'a> {
crate fn add_module_candidates(
&mut self,
module: Module<'a>,
names: &mut Vec<TypoSuggestion>,
filter_fn: &impl Fn(Res) -> bool,
) {
for (&(ident, _), resolution) in self.resolutions(module).borrow().iter() {
if let Some(binding) = resolution.borrow().binding {
let res = binding.res();
if filter_fn(res) {
names.push(TypoSuggestion::from_res(ident.name, res));
}
}
}
}
}

impl<'a> Resolver<'a> {
/// Combines an error with provided span and emits it.
///
/// This takes the error provided, combines it with the span and any additional spans inside the
Expand Down Expand Up @@ -391,10 +394,10 @@ impl<'a> Resolver<'a> {
Scope::CrateRoot => {
let root_ident = Ident::new(kw::PathRoot, ident.span);
let root_module = this.resolve_crate_root(root_ident);
add_module_candidates(root_module, &mut suggestions, filter_fn);
this.add_module_candidates(root_module, &mut suggestions, filter_fn);
}
Scope::Module(module) => {
add_module_candidates(module, &mut suggestions, filter_fn);
this.add_module_candidates(module, &mut suggestions, filter_fn);
}
Scope::MacroUsePrelude => {
suggestions.extend(this.macro_use_prelude.iter().filter_map(|(name, binding)| {
Expand Down Expand Up @@ -442,7 +445,7 @@ impl<'a> Resolver<'a> {
Scope::StdLibPrelude => {
if let Some(prelude) = this.prelude {
let mut tmp_suggestions = Vec::new();
add_module_candidates(prelude, &mut tmp_suggestions, filter_fn);
this.add_module_candidates(prelude, &mut tmp_suggestions, filter_fn);
suggestions.extend(tmp_suggestions.into_iter().filter(|s| {
use_prelude || this.is_builtin_macro(s.res.opt_def_id())
}));
Expand Down Expand Up @@ -498,11 +501,9 @@ impl<'a> Resolver<'a> {
while let Some((in_module,
path_segments,
in_module_is_extern)) = worklist.pop() {
self.populate_module_if_necessary(in_module);

// We have to visit module children in deterministic order to avoid
// instabilities in reported imports (#43552).
in_module.for_each_child_stable(|ident, ns, name_binding| {
self.for_each_child_stable(in_module, |this, ident, ns, name_binding| {
// avoid imports entirely
if name_binding.is_import() && !name_binding.is_extern_crate() { return; }
// avoid non-importable candidates as well
Expand Down Expand Up @@ -536,7 +537,7 @@ impl<'a> Resolver<'a> {
// outside crate private modules => no need to check this)
if !in_module_is_extern || name_binding.vis == ty::Visibility::Public {
let did = match res {
Res::Def(DefKind::Ctor(..), did) => self.parent(did),
Res::Def(DefKind::Ctor(..), did) => this.parent(did),
_ => res.opt_def_id(),
};
candidates.push(ImportSuggestion { did, path });
Expand Down Expand Up @@ -596,8 +597,6 @@ impl<'a> Resolver<'a> {
krate: crate_id,
index: CRATE_DEF_INDEX,
});
self.populate_module_if_necessary(&crate_root);

suggestions.extend(self.lookup_import_candidates_from_module(
lookup_ident, namespace, crate_root, ident, &filter_fn));
}
Expand Down Expand Up @@ -794,7 +793,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
/// at the root of the crate instead of the module where it is defined
/// ```
pub(crate) fn check_for_module_export_macro(
&self,
&mut self,
directive: &'b ImportDirective<'b>,
module: ModuleOrUniformRoot<'b>,
ident: Ident,
Expand All @@ -815,7 +814,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
return None;
}

let resolutions = crate_module.resolutions.borrow();
let resolutions = self.r.resolutions(crate_module).borrow();
let resolution = resolutions.get(&(ident, MacroNS))?;
let binding = resolution.borrow().binding()?;
if let Res::Def(DefKind::Macro(MacroKind::Bang), _) = binding.res() {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_resolve/late.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1934,7 +1934,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
let mut traits = module.traits.borrow_mut();
if traits.is_none() {
let mut collected_traits = Vec::new();
module.for_each_child(|name, ns, binding| {
self.r.for_each_child(module, |_, name, ns, binding| {
if ns != TypeNS { return }
match binding.res() {
Res::Def(DefKind::Trait, _) |
Expand Down
17 changes: 6 additions & 11 deletions src/librustc_resolve/late/diagnostics.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use crate::{CrateLint, Module, ModuleKind, ModuleOrUniformRoot};
use crate::{PathResult, PathSource, Segment};
use crate::path_names_to_string;
use crate::diagnostics::{add_typo_suggestion, add_module_candidates};
use crate::diagnostics::{ImportSuggestion, TypoSuggestion};
use crate::diagnostics::{add_typo_suggestion, ImportSuggestion, TypoSuggestion};
use crate::late::{LateResolutionVisitor, RibKind};

use errors::{Applicability, DiagnosticBuilder, DiagnosticId};
Expand Down Expand Up @@ -548,7 +547,7 @@ impl<'a> LateResolutionVisitor<'a, '_> {
// Items in scope
if let RibKind::ModuleRibKind(module) = rib.kind {
// Items from this module
add_module_candidates(module, &mut names, &filter_fn);
self.r.add_module_candidates(module, &mut names, &filter_fn);

if let ModuleKind::Block(..) = module.kind {
// We can see through blocks
Expand Down Expand Up @@ -577,7 +576,7 @@ impl<'a> LateResolutionVisitor<'a, '_> {
}));

if let Some(prelude) = self.r.prelude {
add_module_candidates(prelude, &mut names, &filter_fn);
self.r.add_module_candidates(prelude, &mut names, &filter_fn);
}
}
break;
Expand All @@ -599,7 +598,7 @@ impl<'a> LateResolutionVisitor<'a, '_> {
mod_path, Some(TypeNS), false, span, CrateLint::No
) {
if let ModuleOrUniformRoot::Module(module) = module {
add_module_candidates(module, &mut names, &filter_fn);
self.r.add_module_candidates(module, &mut names, &filter_fn);
}
}
}
Expand Down Expand Up @@ -717,9 +716,7 @@ impl<'a> LateResolutionVisitor<'a, '_> {
// abort if the module is already found
if result.is_some() { break; }

self.r.populate_module_if_necessary(in_module);

in_module.for_each_child_stable(|ident, _, name_binding| {
self.r.for_each_child_stable(in_module, |_, ident, _, name_binding| {
// abort if the module is already found or if name_binding is private external
if result.is_some() || !name_binding.vis.is_visible_locally() {
return
Expand Down Expand Up @@ -750,10 +747,8 @@ impl<'a> LateResolutionVisitor<'a, '_> {

fn collect_enum_variants(&mut self, def_id: DefId) -> Option<Vec<Path>> {
self.find_module(def_id).map(|(enum_module, enum_import_suggestion)| {
self.r.populate_module_if_necessary(enum_module);

let mut variants = Vec::new();
enum_module.for_each_child_stable(|ident, _, name_binding| {
self.r.for_each_child_stable(enum_module, |_, ident, _, name_binding| {
if let Res::Def(DefKind::Variant, _) = name_binding.res() {
let mut segms = enum_import_suggestion.path.segments.clone();
segms.push(ast::PathSegment::from_ident(ident));
Expand Down
Loading

0 comments on commit 9b835f3

Please sign in to comment.