From b107a4e4f669454b74dcc438c29b691c0bae086e Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 28 Jul 2016 02:34:01 +0000 Subject: [PATCH] Refactor `module.try_define_child(..)` -> `resolver.try_define(module, ..)`. --- src/librustc_resolve/build_reduced_graph.rs | 35 +++---- src/librustc_resolve/lib.rs | 10 ++ src/librustc_resolve/resolve_imports.rs | 101 ++++++++++---------- 3 files changed, 76 insertions(+), 70 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 5867e48c7ca2..116c1b7a6d06 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -16,7 +16,7 @@ use resolve_imports::ImportDirectiveSubclass::{self, GlobImport}; use Module; use Namespace::{self, TypeNS, ValueNS}; -use {NameBinding, NameBindingKind}; +use {NameBinding, NameBindingKind, ToNameBinding}; use ParentLink::{ModuleParentLink, BlockParentLink}; use Resolver; use {resolve_error, resolve_struct_error, ResolutionError}; @@ -39,10 +39,6 @@ use syntax::visit::{self, Visitor}; use syntax_pos::{Span, DUMMY_SP}; -trait ToNameBinding<'a> { - fn to_name_binding(self) -> NameBinding<'a>; -} - impl<'a> ToNameBinding<'a> for (Module<'a>, Span, ty::Visibility) { fn to_name_binding(self) -> NameBinding<'a> { NameBinding { kind: NameBindingKind::Module(self.0), span: self.1, vis: self.2 } @@ -68,18 +64,13 @@ impl<'b> Resolver<'b> { visit::walk_crate(&mut visitor, krate); } - /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined. - fn try_define(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) - where T: ToNameBinding<'b> - { - let _ = parent.try_define_child(name, ns, def.to_name_binding()); - } - /// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined; /// otherwise, reports an error. - fn define>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) { + fn define(&mut self, parent: Module<'b>, name: Name, ns: Namespace, def: T) + where T: ToNameBinding<'b>, + { let binding = def.to_name_binding(); - if let Err(old_binding) = parent.try_define_child(name, ns, binding.clone()) { + if let Err(old_binding) = self.try_define(parent, name, ns, binding.clone()) { self.report_conflict(parent, name, ns, old_binding, &binding); } } @@ -399,14 +390,14 @@ impl<'b> Resolver<'b> { name, vis); let parent_link = ModuleParentLink(parent, name); let module = self.new_module(parent_link, Some(def), true); - self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::Variant(_, variant_id) => { debug!("(building reduced graph for external crate) building variant {}", name); // Variants are always treated as importable to allow them to be glob used. // All variants are defined in both type and value namespaces as future-proofing. - self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); - self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); if self.session.cstore.variant_kind(variant_id) == Some(VariantKind::Struct) { // Not adding fields for variants as they are not accessed with a self receiver self.structs.insert(variant_id, Vec::new()); @@ -419,7 +410,7 @@ impl<'b> Resolver<'b> { Def::Method(..) => { debug!("(building reduced graph for external crate) building value (fn/static) {}", name); - self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); } Def::Trait(def_id) => { debug!("(building reduced graph for external crate) building type {}", name); @@ -441,20 +432,20 @@ impl<'b> Resolver<'b> { let parent_link = ModuleParentLink(parent, name); let module = self.new_module(parent_link, Some(def), true); - self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis)); } Def::TyAlias(..) | Def::AssociatedTy(..) => { debug!("(building reduced graph for external crate) building type {}", name); - self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); } Def::Struct(def_id) if self.session.cstore.tuple_struct_definition_if_ctor(def_id).is_none() => { debug!("(building reduced graph for external crate) building type and value for {}", name); - self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis)); if let Some(ctor_def_id) = self.session.cstore.struct_ctor_def_id(def_id) { let def = Def::Struct(ctor_def_id); - self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); + let _ = self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis)); } // Record the def ID and fields of this struct. diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 8c8cf1da4673..56b92d169b09 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -818,6 +818,16 @@ pub struct NameBinding<'a> { vis: ty::Visibility, } +pub trait ToNameBinding<'a> { + fn to_name_binding(self) -> NameBinding<'a>; +} + +impl<'a> ToNameBinding<'a> for NameBinding<'a> { + fn to_name_binding(self) -> NameBinding<'a> { + self + } +} + #[derive(Clone, Debug)] enum NameBindingKind<'a> { Def(Def), diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 8e2c21f880f1..feec81b9fc9b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -12,7 +12,7 @@ use self::ImportDirectiveSubclass::*; use Module; use Namespace::{self, TypeNS, ValueNS}; -use {NameBinding, NameBindingKind, PrivacyError}; +use {NameBinding, NameBindingKind, PrivacyError, ToNameBinding}; use ResolveResult; use ResolveResult::*; use Resolver; @@ -226,28 +226,6 @@ impl<'a> ::ModuleS<'a> { Failed(None) } - // Define the name or return the existing binding if there is a collision. - pub fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>) - -> Result<(), &'a NameBinding<'a>> { - let binding = self.arenas.alloc_name_binding(binding); - self.update_resolution(name, ns, |resolution| { - if let Some(old_binding) = resolution.binding { - if binding.is_glob_import() { - resolution.duplicate_globs.push(binding); - } else if old_binding.is_glob_import() { - resolution.duplicate_globs.push(old_binding); - resolution.binding = Some(binding); - } else { - return Err(old_binding); - } - } else { - resolution.binding = Some(binding); - } - - Ok(()) - }) - } - pub fn add_import_directive(&self, module_path: Vec, subclass: ImportDirectiveSubclass, @@ -277,19 +255,45 @@ impl<'a> ::ModuleS<'a> { GlobImport { .. } => self.globs.borrow_mut().push(directive), } } +} - // Use `update` to mutate the resolution for the name. - // If the resolution becomes a success, define it in the module's glob importers. - fn update_resolution(&self, name: Name, ns: Namespace, update: F) -> T - where F: FnOnce(&mut NameResolution<'a>) -> T +impl<'a> Resolver<'a> { + // Define the name or return the existing binding if there is a collision. + pub fn try_define(&mut self, module: Module<'a>, name: Name, ns: Namespace, binding: T) + -> Result<(), &'a NameBinding<'a>> + where T: ToNameBinding<'a> { - // Ensure that `resolution` isn't borrowed during `define_in_glob_importers`, - // where it might end up getting re-defined via a glob cycle. + let binding = self.arenas.alloc_name_binding(binding.to_name_binding()); + self.update_resolution(module, name, ns, |_, resolution| { + if let Some(old_binding) = resolution.binding { + if binding.is_glob_import() { + resolution.duplicate_globs.push(binding); + } else if old_binding.is_glob_import() { + resolution.duplicate_globs.push(old_binding); + resolution.binding = Some(binding); + } else { + return Err(old_binding); + } + } else { + resolution.binding = Some(binding); + } + + Ok(()) + }) + } + + // Use `f` to mutate the resolution of the name in the module. + // If the resolution becomes a success, define it in the module's glob importers. + fn update_resolution(&mut self, module: Module<'a>, name: Name, ns: Namespace, f: F) -> T + where F: FnOnce(&mut Resolver<'a>, &mut NameResolution<'a>) -> T + { + // Ensure that `resolution` isn't borrowed when defining in the module's glob importers, + // during which the resolution might end up getting re-defined via a glob cycle. let (new_binding, t) = { - let mut resolution = &mut *self.resolution(name, ns).borrow_mut(); + let mut resolution = &mut *module.resolution(name, ns).borrow_mut(); let was_known = resolution.binding().is_some(); - let t = update(resolution); + let t = f(self, resolution); if was_known { return t; } match resolution.binding() { @@ -298,15 +302,14 @@ impl<'a> ::ModuleS<'a> { } }; - self.define_in_glob_importers(name, ns, new_binding); - t - } - - fn define_in_glob_importers(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) { - if !binding.is_importable() || !binding.is_pseudo_public() { return } - for &(importer, directive) in self.glob_importers.borrow_mut().iter() { - let _ = importer.try_define_child(name, ns, directive.import(binding)); + // Define `new_binding` in `module`s glob importers. + if new_binding.is_importable() && new_binding.is_pseudo_public() { + for &(importer, directive) in module.glob_importers.borrow_mut().iter() { + let _ = self.try_define(importer, name, ns, directive.import(new_binding)); + } } + + t } } @@ -396,7 +399,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // Define a "dummy" resolution containing a Def::Err as a placeholder for a // failed resolution - fn import_dummy_binding(&self, source_module: Module<'b>, directive: &'b ImportDirective<'b>) { + fn import_dummy_binding(&mut self, + source_module: Module<'b>, + directive: &'b ImportDirective<'b>) { if let SingleImport { target, .. } = directive.subclass { let dummy_binding = self.arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Def(Def::Err), @@ -405,14 +410,14 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { }); let dummy_binding = directive.import(dummy_binding); - let _ = source_module.try_define_child(target, ValueNS, dummy_binding.clone()); - let _ = source_module.try_define_child(target, TypeNS, dummy_binding); + let _ = self.try_define(source_module, target, ValueNS, dummy_binding.clone()); + let _ = self.try_define(source_module, target, TypeNS, dummy_binding); } } /// Resolves an `ImportResolvingError` into the correct enum discriminant /// and passes that on to `resolve_error`. - fn import_resolving_error(&self, e: ImportResolvingError<'b>) { + fn import_resolving_error(&mut self, e: ImportResolvingError<'b>) { // If the error is a single failed import then create a "fake" import // resolution for it so that later resolve stages won't complain. self.import_dummy_binding(e.source_module, e.import_directive); @@ -492,7 +497,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { match *result { Failed(..) if !determined.get() => { determined.set(true); - module_.update_resolution(target, ns, |resolution| { + self.update_resolution(module_, target, ns, |_, resolution| { resolution.single_imports.directive_failed() }); } @@ -508,7 +513,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { Success(binding) if !determined.get() => { determined.set(true); let imported_binding = directive.import(binding); - let conflict = module_.try_define_child(target, ns, imported_binding); + let conflict = self.try_define(module_, target, ns, imported_binding); if let Err(old_binding) = conflict { let binding = &directive.import(binding); self.report_conflict(module_, target, ns, binding, old_binding); @@ -551,7 +556,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { for &(ns, result) in &[(ValueNS, &value_result), (TypeNS, &type_result)] { let binding = match *result { Success(binding) => binding, _ => continue }; self.privacy_errors.push(PrivacyError(directive.span, source, binding)); - let _ = module_.try_define_child(target, ns, directive.import(binding)); + let _ = self.try_define(module_, target, ns, directive.import(binding)); } } @@ -626,14 +631,14 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // Add to target_module's glob_importers target_module.glob_importers.borrow_mut().push((module_, directive)); - // Ensure that `resolutions` isn't borrowed during `try_define_child`, + // Ensure that `resolutions` isn't borrowed during `try_define`, // since it might get updated via a glob cycle. let bindings = target_module.resolutions.borrow().iter().filter_map(|(name, resolution)| { resolution.borrow().binding().map(|binding| (*name, binding)) }).collect::>(); for ((name, ns), binding) in bindings { if binding.is_importable() && binding.is_pseudo_public() { - let _ = module_.try_define_child(name, ns, directive.import(binding)); + let _ = self.try_define(module_, name, ns, directive.import(binding)); } }