From a8017d6869510407f7f8cad786b81aeda26507f3 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 29 Jul 2019 21:19:50 +0300 Subject: [PATCH 1/3] resolve: Populate external modules in more automatic and lazy way The modules are now populated implicitly on the first access --- src/librustc_resolve/build_reduced_graph.rs | 41 ++++----------------- src/librustc_resolve/diagnostics.rs | 27 +++++++------- src/librustc_resolve/late.rs | 2 +- src/librustc_resolve/late/diagnostics.rs | 14 +++---- src/librustc_resolve/lib.rs | 34 +++++++++-------- src/librustc_resolve/macros.rs | 13 +++++++ src/librustc_resolve/resolve_imports.rs | 34 +++++++++++------ 7 files changed, 80 insertions(+), 85 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 6e5750e752e94..a63e2ec2a96bc 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -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> { @@ -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); @@ -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, @@ -922,6 +908,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { span); self.r.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, expansion)); + module.populate_on_access.set(false); 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 { @@ -935,7 +922,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { self.r.has_self.insert(res.def_id()); } } - module.populated.set(true); } Res::Def(DefKind::Struct, def_id) | Res::Def(DefKind::Union, def_id) => { self.r.define(parent, ident, TypeNS, (res, vis, DUMMY_SP, expansion)); @@ -951,19 +937,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { } } - 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(); - } - } - /// Returns `true` if we should consider the underlying `extern crate` to be used. fn process_legacy_macro_imports(&mut self, item: &Item, module: Module<'a>) -> bool { let mut import_all = None; @@ -1021,9 +994,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); + module.for_each_child(&mut self.r, |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() { @@ -1039,8 +1012,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"); } diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index 9e7e56f4a3a26..09046e4043ab7 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -73,10 +73,13 @@ crate fn add_typo_suggestion( false } -crate fn add_module_candidates( - module: Module<'_>, names: &mut Vec, filter_fn: &impl Fn(Res) -> bool +crate fn add_module_candidates<'a>( + resolver: &mut Resolver<'a>, + module: Module<'a>, + names: &mut Vec, + filter_fn: &impl Fn(Res) -> bool, ) { - for (&(ident, _), resolution) in module.resolutions.borrow().iter() { + for (&(ident, _), resolution) in resolver.resolutions(module).borrow().iter() { if let Some(binding) = resolution.borrow().binding { let res = binding.res(); if filter_fn(res) { @@ -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); + add_module_candidates(this, root_module, &mut suggestions, filter_fn); } Scope::Module(module) => { - add_module_candidates(module, &mut suggestions, filter_fn); + add_module_candidates(this, module, &mut suggestions, filter_fn); } Scope::MacroUsePrelude => { suggestions.extend(this.macro_use_prelude.iter().filter_map(|(name, binding)| { @@ -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); + add_module_candidates(this, 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()) })); @@ -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| { + in_module.for_each_child_stable(self, |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 @@ -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 }); @@ -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)); } @@ -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, @@ -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() { diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 7cb11195ee02b..5df38b8ade8f3 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -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| { + module.for_each_child(&mut self.r, |_, name, ns, binding| { if ns != TypeNS { return } match binding.res() { Res::Def(DefKind::Trait, _) | diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index 68f9c1684d6fb..cc40a41a241d9 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -548,7 +548,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); + add_module_candidates(&mut self.r, module, &mut names, &filter_fn); if let ModuleKind::Block(..) = module.kind { // We can see through blocks @@ -577,7 +577,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { })); if let Some(prelude) = self.r.prelude { - add_module_candidates(prelude, &mut names, &filter_fn); + add_module_candidates(&mut self.r, prelude, &mut names, &filter_fn); } } break; @@ -599,7 +599,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); + add_module_candidates(&mut self.r, module, &mut names, &filter_fn); } } } @@ -717,9 +717,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| { + in_module.for_each_child_stable(&mut self.r, |_, 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 @@ -750,10 +748,8 @@ impl<'a> LateResolutionVisitor<'a, '_> { fn collect_enum_variants(&mut self, def_id: DefId) -> Option> { 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| { + enum_module.for_each_child_stable(&mut self.r, |_, 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)); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 9fc3e11505c29..635d184b10a2e 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -408,6 +408,8 @@ impl ModuleKind { } } +type Resolutions<'a> = RefCell>>>; + /// One node in the tree of modules. pub struct ModuleData<'a> { parent: Option>, @@ -416,7 +418,11 @@ pub struct ModuleData<'a> { // The def id of the closest normal module (`mod`) ancestor (including this module). normal_ancestor_id: DefId, - resolutions: RefCell>>>, + // Mapping between names and their (possibly in-progress) resolutions in this module. + // Resolutions in modules from other crates are not populated until accessed. + lazy_resolutions: Resolutions<'a>, + // True if this is a module from other crate that needs to be populated on access. + populate_on_access: Cell, single_segment_macro_resolutions: RefCell, Option<&'a NameBinding<'a>>)>>, multi_segment_macro_resolutions: RefCell, Span, MacroKind, ParentScope<'a>, @@ -434,11 +440,6 @@ pub struct ModuleData<'a> { // Used to memoize the traits in this module for faster searches through all traits in scope. traits: RefCell)]>>>, - // Whether this module is populated. If not populated, any attempt to - // access the children must be preceded with a - // `populate_module_if_necessary` call. - populated: Cell, - /// Span of the module itself. Used for error reporting. span: Span, @@ -457,7 +458,8 @@ impl<'a> ModuleData<'a> { parent, kind, normal_ancestor_id, - resolutions: Default::default(), + lazy_resolutions: Default::default(), + populate_on_access: Cell::new(!normal_ancestor_id.is_local()), single_segment_macro_resolutions: RefCell::new(Vec::new()), multi_segment_macro_resolutions: RefCell::new(Vec::new()), builtin_attrs: RefCell::new(Vec::new()), @@ -466,24 +468,27 @@ impl<'a> ModuleData<'a> { glob_importers: RefCell::new(Vec::new()), globs: RefCell::new(Vec::new()), traits: RefCell::new(None), - populated: Cell::new(normal_ancestor_id.is_local()), span, expansion, } } - fn for_each_child)>(&self, mut f: F) { - for (&(ident, ns), name_resolution) in self.resolutions.borrow().iter() { - name_resolution.borrow().binding.map(|binding| f(ident, ns, binding)); + fn for_each_child(&'a self, resolver: &mut Resolver<'a>, mut f: F) + where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) + { + for (&(ident, ns), name_resolution) in resolver.resolutions(self).borrow().iter() { + name_resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding)); } } - fn for_each_child_stable)>(&self, mut f: F) { - let resolutions = self.resolutions.borrow(); + fn for_each_child_stable(&'a self, resolver: &mut Resolver<'a>, mut f: F) + where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) + { + let resolutions = resolver.resolutions(self).borrow(); let mut resolutions = resolutions.iter().collect::>(); resolutions.sort_by_cached_key(|&(&(ident, ns), _)| (ident.as_str(), ns)); for &(&(ident, ns), &resolution) in resolutions.iter() { - resolution.borrow().binding.map(|binding| f(ident, ns, binding)); + resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding)); } } @@ -2608,7 +2613,6 @@ impl<'a> Resolver<'a> { return None; }; let crate_root = self.get_module(DefId { krate: crate_id, index: CRATE_DEF_INDEX }); - self.populate_module_if_necessary(&crate_root); Some((crate_root, ty::Visibility::Public, DUMMY_SP, ExpnId::root()) .to_name_binding(self.arenas)) } diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 5af71a0170a7b..efcc14b4cd92e 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -869,4 +869,17 @@ impl<'a> Resolver<'a> { Lrc::new(result) } + + crate fn legacy_import_macro(&mut self, + name: ast::Name, + binding: &'a NameBinding<'a>, + span: Span, + allow_shadowing: bool) { + if self.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.session.struct_span_err(span, &msg).note(note).emit(); + } + } } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 00e89f0fdae0a..342ca7a34755d 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -5,9 +5,10 @@ use crate::{CrateLint, Module, ModuleOrUniformRoot, PerNS, ScopeSet, ParentScope use crate::Determinacy::{self, *}; use crate::Namespace::{self, TypeNS, MacroNS}; use crate::{NameBinding, NameBindingKind, ToNameBinding, PathResult, PrivacyError}; -use crate::{Resolver, ResolutionError, Segment}; +use crate::{Resolutions, Resolver, ResolutionError, Segment}; use crate::{names_to_string, module_to_string}; use crate::ModuleKind; +use crate::build_reduced_graph::BuildReducedGraphVisitor; use crate::diagnostics::Suggestion; use errors::Applicability; @@ -159,9 +160,22 @@ impl<'a> NameResolution<'a> { } impl<'a> Resolver<'a> { - crate fn resolution(&self, module: Module<'a>, ident: Ident, ns: Namespace) + crate fn resolutions(&mut self, module: Module<'a>) -> &'a Resolutions<'a> { + if module.populate_on_access.get() { + module.populate_on_access.set(false); + let def_id = module.def_id().expect("unpopulated module without a def-id"); + 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.lazy_resolutions + } + + crate fn resolution(&mut self, module: Module<'a>, ident: Ident, ns: Namespace) -> &'a RefCell> { - *module.resolutions.borrow_mut().entry((ident.modern(), ns)) + *self.resolutions(module).borrow_mut().entry((ident.modern(), ns)) .or_insert_with(|| self.arenas.alloc_name_resolution()) } @@ -240,8 +254,6 @@ impl<'a> Resolver<'a> { } }; - self.populate_module_if_necessary(module); - let resolution = self.resolution(module, ident, ns) .try_borrow_mut() .map_err(|_| (Determined, Weak::No))?; // This happens when there is a cycle of imports. @@ -1025,7 +1037,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { return if all_ns_failed { let resolutions = match module { - ModuleOrUniformRoot::Module(module) => Some(module.resolutions.borrow()), + ModuleOrUniformRoot::Module(module) => Some(self.r.resolutions(module).borrow()), _ => None, }; let resolutions = resolutions.as_ref().into_iter().flat_map(|r| r.iter()); @@ -1263,8 +1275,6 @@ impl<'a, 'b> ImportResolver<'a, 'b> { } }; - self.r.populate_module_if_necessary(module); - if module.is_trait() { self.r.session.span_err(directive.span, "items in traits are not importable."); return; @@ -1280,7 +1290,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { // Ensure that `resolutions` isn't borrowed during `try_define`, // since it might get updated via a glob cycle. - let bindings = module.resolutions.borrow().iter().filter_map(|(&ident, resolution)| { + let bindings = self.r.resolutions(module).borrow().iter().filter_map(|(&ident, resolution)| { resolution.borrow().binding().map(|binding| (ident, binding)) }).collect::>(); for ((mut ident, ns), binding) in bindings { @@ -1308,7 +1318,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { let mut reexports = Vec::new(); - for (&(ident, ns), resolution) in module.resolutions.borrow().iter() { + for (&(ident, ns), resolution) in self.r.resolutions(module).borrow().iter() { let resolution = &mut *resolution.borrow_mut(); let binding = match resolution.binding { Some(binding) => binding, @@ -1367,8 +1377,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> { Some(ModuleOrUniformRoot::Module(module)) => module, _ => bug!("module should exist"), }; - let resolutions = imported_module.parent.expect("parent should exist") - .resolutions.borrow(); + let parent_module = imported_module.parent.expect("parent should exist"); + let resolutions = self.r.resolutions(parent_module).borrow(); let enum_path_segment_index = directive.module_path.len() - 1; let enum_ident = directive.module_path[enum_path_segment_index].ident; From aee4313544ebb4dc18d793e5e56fdf3656f6f6cf Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 30 Jul 2019 23:07:18 +0300 Subject: [PATCH 2/3] resolve: Populate external traits lazily as well --- src/librustc_resolve/build_reduced_graph.rs | 31 ++++++--------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index a63e2ec2a96bc..8376cf7944702 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -867,7 +867,8 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { let expansion = ExpnId::root(); // FIXME(jseyfried) intercrate hygiene 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, @@ -880,6 +881,8 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { | 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)); @@ -887,6 +890,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { Res::Def(DefKind::Fn, _) | Res::Def(DefKind::Static, _) | Res::Def(DefKind::Const, _) + | Res::Def(DefKind::AssocConst, _) | Res::Def(DefKind::Ctor(CtorOf::Variant, ..), _) => { self.r.define(parent, ident, ValueNS, (res, vis, DUMMY_SP, expansion)); } @@ -899,28 +903,11 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { 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)); + Res::Def(DefKind::Method, def_id) => { + self.r.define(parent, ident, ValueNS, (res, vis, DUMMY_SP, expansion)); - module.populate_on_access.set(false); - 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()); - } + if self.r.cstore.associated_item_cloned_untracked(def_id).method_has_self_argument { + self.r.has_self.insert(def_id); } } Res::Def(DefKind::Struct, def_id) | Res::Def(DefKind::Union, def_id) => { From a087df62111d7805376608e5f1ac2f78cd7a9f3e Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 30 Jul 2019 23:30:54 +0300 Subject: [PATCH 3/3] resolve: Move some code around Avoid using dummy spans for some external items with available spans --- src/librustc_resolve/build_reduced_graph.rs | 58 +++++++++---------- src/librustc_resolve/diagnostics.rs | 34 +++++------ src/librustc_resolve/late.rs | 2 +- src/librustc_resolve/late/diagnostics.rs | 13 ++--- src/librustc_resolve/lib.rs | 58 +++++++++++++------ src/librustc_resolve/resolve_imports.rs | 32 ++-------- ...e-extern-crate-restricted-shadowing.stderr | 8 ++- src/test/ui/issues/issue-27033.stderr | 5 ++ 8 files changed, 109 insertions(+), 101 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 8376cf7944702..dae322e9b72f0 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -865,6 +865,7 @@ 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) @@ -874,9 +875,11 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { 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, _) @@ -884,43 +887,40 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { | 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::AssocConst, _) - | 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::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) => { + let field_names = self.r.cstore.struct_field_names_untracked(def_id); + self.insert_field_names(def_id, field_names); } Res::Def(DefKind::Method, def_id) => { - self.r.define(parent, ident, ValueNS, (res, vis, DUMMY_SP, expansion)); - if self.r.cstore.associated_item_cloned_untracked(def_id).method_has_self_argument { self.r.has_self.insert(def_id); } } - 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::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) + _ => {} } } @@ -981,7 +981,7 @@ 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(&mut self.r, |this, ident, ns, binding| if ns == MacroNS { + 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); }); diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index 09046e4043ab7..9ce513801ff6e 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -73,23 +73,23 @@ crate fn add_typo_suggestion( false } -crate fn add_module_candidates<'a>( - resolver: &mut Resolver<'a>, - module: Module<'a>, - names: &mut Vec, - filter_fn: &impl Fn(Res) -> bool, -) { - for (&(ident, _), resolution) in resolver.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> { + crate fn add_module_candidates( + &mut self, + module: Module<'a>, + names: &mut Vec, + 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 @@ -394,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(this, root_module, &mut suggestions, filter_fn); + this.add_module_candidates(root_module, &mut suggestions, filter_fn); } Scope::Module(module) => { - add_module_candidates(this, 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)| { @@ -445,7 +445,7 @@ impl<'a> Resolver<'a> { Scope::StdLibPrelude => { if let Some(prelude) = this.prelude { let mut tmp_suggestions = Vec::new(); - add_module_candidates(this, 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()) })); @@ -503,7 +503,7 @@ impl<'a> Resolver<'a> { in_module_is_extern)) = worklist.pop() { // We have to visit module children in deterministic order to avoid // instabilities in reported imports (#43552). - in_module.for_each_child_stable(self, |this, 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 diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 5df38b8ade8f3..30fb9dc5bbbe8 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -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(&mut self.r, |_, name, ns, binding| { + self.r.for_each_child(module, |_, name, ns, binding| { if ns != TypeNS { return } match binding.res() { Res::Def(DefKind::Trait, _) | diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index cc40a41a241d9..cd757c4677f17 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -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}; @@ -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(&mut self.r, 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 @@ -577,7 +576,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { })); if let Some(prelude) = self.r.prelude { - add_module_candidates(&mut self.r, prelude, &mut names, &filter_fn); + self.r.add_module_candidates(prelude, &mut names, &filter_fn); } } break; @@ -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(&mut self.r, module, &mut names, &filter_fn); + self.r.add_module_candidates(module, &mut names, &filter_fn); } } } @@ -717,7 +716,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { // abort if the module is already found if result.is_some() { break; } - in_module.for_each_child_stable(&mut self.r, |_, 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 @@ -749,7 +748,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { fn collect_enum_variants(&mut self, def_id: DefId) -> Option> { self.find_module(def_id).map(|(enum_module, enum_import_suggestion)| { let mut variants = Vec::new(); - enum_module.for_each_child_stable(&mut self.r, |_, 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)); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 635d184b10a2e..bb51a19584afb 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -50,6 +50,7 @@ use std::collections::BTreeSet; use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::Lrc; +use build_reduced_graph::BuildReducedGraphVisitor; use diagnostics::{Suggestion, ImportSuggestion}; use diagnostics::{find_span_of_binding_until_next_binding, extend_span_to_previous_binding}; use late::{PathSource, Rib, RibKind::*}; @@ -473,25 +474,6 @@ impl<'a> ModuleData<'a> { } } - fn for_each_child(&'a self, resolver: &mut Resolver<'a>, mut f: F) - where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) - { - for (&(ident, ns), name_resolution) in resolver.resolutions(self).borrow().iter() { - name_resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding)); - } - } - - fn for_each_child_stable(&'a self, resolver: &mut Resolver<'a>, mut f: F) - where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) - { - let resolutions = resolver.resolutions(self).borrow(); - let mut resolutions = resolutions.iter().collect::>(); - resolutions.sort_by_cached_key(|&(&(ident, ns), _)| (ident.as_str(), ns)); - for &(&(ident, ns), &resolution) in resolutions.iter() { - resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding)); - } - } - fn res(&self) -> Option { match self.kind { ModuleKind::Def(kind, def_id, _) => Some(Res::Def(kind, def_id)), @@ -1230,6 +1212,44 @@ impl<'a> Resolver<'a> { self.arenas.alloc_module(module) } + fn resolutions(&mut self, module: Module<'a>) -> &'a Resolutions<'a> { + if module.populate_on_access.get() { + module.populate_on_access.set(false); + let def_id = module.def_id().expect("unpopulated module without a def-id"); + 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.lazy_resolutions + } + + fn resolution(&mut self, module: Module<'a>, ident: Ident, ns: Namespace) + -> &'a RefCell> { + *self.resolutions(module).borrow_mut().entry((ident.modern(), ns)) + .or_insert_with(|| self.arenas.alloc_name_resolution()) + } + + fn for_each_child(&mut self, module: Module<'a>, mut f: F) + where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) + { + for (&(ident, ns), name_resolution) in self.resolutions(module).borrow().iter() { + name_resolution.borrow().binding.map(|binding| f(self, ident, ns, binding)); + } + } + + fn for_each_child_stable(&mut self, module: Module<'a>, mut f: F) + where F: FnMut(&mut Resolver<'a>, Ident, Namespace, &'a NameBinding<'a>) + { + let resolutions = self.resolutions(module).borrow(); + let mut resolutions = resolutions.iter().collect::>(); + resolutions.sort_by_cached_key(|&(&(ident, ns), _)| (ident.as_str(), ns)); + for &(&(ident, ns), &resolution) in resolutions.iter() { + resolution.borrow().binding.map(|binding| f(self, ident, ns, binding)); + } + } + fn record_use(&mut self, ident: Ident, ns: Namespace, used_binding: &'a NameBinding<'a>, is_lexical_scope: bool) { if let Some((b2, kind)) = used_binding.ambiguity { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 342ca7a34755d..fb973821be67a 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -5,10 +5,8 @@ use crate::{CrateLint, Module, ModuleOrUniformRoot, PerNS, ScopeSet, ParentScope use crate::Determinacy::{self, *}; use crate::Namespace::{self, TypeNS, MacroNS}; use crate::{NameBinding, NameBindingKind, ToNameBinding, PathResult, PrivacyError}; -use crate::{Resolutions, Resolver, ResolutionError, Segment}; +use crate::{Resolver, ResolutionError, Segment, ModuleKind}; use crate::{names_to_string, module_to_string}; -use crate::ModuleKind; -use crate::build_reduced_graph::BuildReducedGraphVisitor; use crate::diagnostics::Suggestion; use errors::Applicability; @@ -36,7 +34,7 @@ use syntax_pos::{MultiSpan, Span}; use log::*; -use std::cell::{Cell, RefCell}; +use std::cell::Cell; use std::{mem, ptr}; type Res = def::Res; @@ -160,25 +158,6 @@ impl<'a> NameResolution<'a> { } impl<'a> Resolver<'a> { - crate fn resolutions(&mut self, module: Module<'a>) -> &'a Resolutions<'a> { - if module.populate_on_access.get() { - module.populate_on_access.set(false); - let def_id = module.def_id().expect("unpopulated module without a def-id"); - 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.lazy_resolutions - } - - crate fn resolution(&mut self, module: Module<'a>, ident: Ident, ns: Namespace) - -> &'a RefCell> { - *self.resolutions(module).borrow_mut().entry((ident.modern(), ns)) - .or_insert_with(|| self.arenas.alloc_name_resolution()) - } - crate fn resolve_ident_in_module_unadjusted( &mut self, module: ModuleOrUniformRoot<'a>, @@ -1037,7 +1016,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> { return if all_ns_failed { let resolutions = match module { - ModuleOrUniformRoot::Module(module) => Some(self.r.resolutions(module).borrow()), + ModuleOrUniformRoot::Module(module) => + Some(self.r.resolutions(module).borrow()), _ => None, }; let resolutions = resolutions.as_ref().into_iter().flat_map(|r| r.iter()); @@ -1290,8 +1270,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> { // Ensure that `resolutions` isn't borrowed during `try_define`, // since it might get updated via a glob cycle. - let bindings = self.r.resolutions(module).borrow().iter().filter_map(|(&ident, resolution)| { - resolution.borrow().binding().map(|binding| (ident, binding)) + let bindings = self.r.resolutions(module).borrow().iter().filter_map(|(ident, resolution)| { + resolution.borrow().binding().map(|binding| (*ident, binding)) }).collect::>(); for ((mut ident, ns), binding) in bindings { let scope = match ident.span.reverse_glob_adjust(module.expansion, directive.span) { diff --git a/src/test/ui/imports/extern-prelude-extern-crate-restricted-shadowing.stderr b/src/test/ui/imports/extern-prelude-extern-crate-restricted-shadowing.stderr index e8dfd43b6767c..a0bb557bf27b0 100644 --- a/src/test/ui/imports/extern-prelude-extern-crate-restricted-shadowing.stderr +++ b/src/test/ui/imports/extern-prelude-extern-crate-restricted-shadowing.stderr @@ -13,8 +13,7 @@ error[E0659]: `Vec` is ambiguous (macro-expanded name vs less macro-expanded nam LL | Vec::panic!(); | ^^^ ambiguous name | - = note: `Vec` could refer to a struct from prelude -note: `Vec` could also refer to the crate imported here +note: `Vec` could refer to the crate imported here --> $DIR/extern-prelude-extern-crate-restricted-shadowing.rs:5:9 | LL | extern crate std as Vec; @@ -22,6 +21,11 @@ LL | extern crate std as Vec; ... LL | define_vec!(); | -------------- in this macro invocation +note: `Vec` could also refer to the struct defined here + --> $SRC_DIR/libstd/prelude/v1.rs:LL:COL + | +LL | pub use crate::vec::Vec; + | ^^^^^^^^^^^^^^^ error: aborting due to 2 previous errors diff --git a/src/test/ui/issues/issue-27033.stderr b/src/test/ui/issues/issue-27033.stderr index ab95433228037..b11298d4ab90b 100644 --- a/src/test/ui/issues/issue-27033.stderr +++ b/src/test/ui/issues/issue-27033.stderr @@ -3,6 +3,11 @@ error[E0530]: match bindings cannot shadow unit variants | LL | None @ _ => {} | ^^^^ cannot be named the same as a unit variant + | + ::: $SRC_DIR/libstd/prelude/v1.rs:LL:COL + | +LL | pub use crate::option::Option::{self, Some, None}; + | ---- the unit variant `None` is defined here error[E0530]: match bindings cannot shadow constants --> $DIR/issue-27033.rs:7:9