From ceda8383c9956d1829f1e26315ad17d39323022c Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 16 Nov 2015 02:10:09 +0000 Subject: [PATCH 1/4] Replace `TypeNsDef` and `ValueNsDef` with a more general type `NsDef`. Define a newtype `NameBinding` for `Rc>>` and refactor `NameBindings` to be a `NameBinding` for each namespace. Replace uses of `NameBindings` with `NameBinding` where only one binding is being used (in `NamespaceResult`, `Target,` etc). Refactor away `resolve_definition_of_name_in_module` and `NameDefinition`. --- src/librustc_resolve/build_reduced_graph.rs | 62 +-- src/librustc_resolve/lib.rs | 546 ++++++-------------- src/librustc_resolve/record_exports.rs | 17 +- src/librustc_resolve/resolve_imports.rs | 89 ++-- 4 files changed, 236 insertions(+), 478 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 3481f1bfd5203..802f9a0d40f80 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -25,7 +25,6 @@ use {names_to_string, module_to_string}; use ParentLink::{self, ModuleParentLink, BlockParentLink}; use Resolver; use resolve_imports::Shadowable; -use TypeNsDef; use {resolve_error, ResolutionError}; use self::DuplicateCheckingMode::*; @@ -130,7 +129,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { duplicate_checking_mode: DuplicateCheckingMode, // For printing errors sp: Span) - -> Rc { + -> NameBindings { // If this is the immediate descendant of a module, then we add the // child name directly. Otherwise, we create or reuse an anonymous // module and add the child to that. @@ -141,7 +140,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let child = parent.children.borrow().get(&name).cloned(); match child { None => { - let child = Rc::new(NameBindings::new()); + let child = NameBindings::new(); parent.children.borrow_mut().insert(name, child.clone()); child } @@ -173,27 +172,27 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { Some(TypeNS) } ForbidDuplicateTypesAndModules => { - if child.defined_in_namespace(TypeNS) { + if child.type_ns.defined() { duplicate_type = TypeError; } Some(TypeNS) } ForbidDuplicateValues => { - if child.defined_in_namespace(ValueNS) { + if child.value_ns.defined() { duplicate_type = ValueError; } Some(ValueNS) } ForbidDuplicateTypesAndValues => { let mut n = None; - match child.def_for_namespace(TypeNS) { + match child.type_ns.def() { Some(DefMod(_)) | None => {} Some(_) => { n = Some(TypeNS); duplicate_type = TypeError; } } - if child.defined_in_namespace(ValueNS) { + if child.value_ns.defined() { duplicate_type = ValueError; n = Some(ValueNS); } @@ -213,7 +212,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { name) ); { - let r = child.span_for_namespace(ns); + let r = child[ns].span(); if let Some(sp) = r { self.session.span_note(sp, &format!("first definition of {} `{}` here", @@ -415,7 +414,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let child = parent.children.borrow().get(&name).cloned(); if let Some(child) = child { // check if there's struct of the same name already defined - if child.defined_in_namespace(TypeNS) && + if child.type_ns.defined() && child.get_module_if_available().is_none() { self.session.span_warn(sp, &format!("duplicate definition of {} `{}`. \ @@ -424,7 +423,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { namespace_error_to_string(TypeError), name)); { - let r = child.span_for_namespace(TypeNS); + let r = child.type_ns.span(); if let Some(sp) = r { self.session.span_note(sp, &format!("first definition of {} `{}` here", @@ -530,7 +529,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let child = parent.children.borrow().get(&name).cloned(); if let Some(child) = child { // check if theres a DefMod - if let Some(DefMod(_)) = child.def_for_namespace(TypeNS) { + if let Some(DefMod(_)) = child.type_ns.def() { self.session.span_warn(sp, &format!("duplicate definition of {} `{}`. \ Defining a module and a struct \ @@ -539,7 +538,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { namespace_error_to_string(TypeError), name)); { - let r = child.span_for_namespace(TypeNS); + let r = child.type_ns.span(); if let Some(sp) = r { self.session .span_note(sp, @@ -752,26 +751,21 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { DefForeignMod(def_id) | DefStruct(def_id) | DefTy(def_id, _) => { - let type_def = child_name_bindings.type_def.borrow().clone(); - match type_def { - Some(TypeNsDef { module_def: Some(module_def), .. }) => { - debug!("(building reduced graph for external crate) already created \ - module"); - module_def.def_id.set(Some(def_id)); - } - Some(_) | None => { - debug!("(building reduced graph for external crate) building module {} {}", - final_ident, - is_public); - let parent_link = self.get_parent_link(new_parent, name); - - child_name_bindings.define_module(parent_link, - Some(def_id), - kind, - true, - is_public, - DUMMY_SP); - } + if let Some(module_def) = child_name_bindings.type_ns.module() { + debug!("(building reduced graph for external crate) already created module"); + module_def.def_id.set(Some(def_id)); + } else { + debug!("(building reduced graph for external crate) building module {} {}", + final_ident, + is_public); + let parent_link = self.get_parent_link(new_parent, name); + + child_name_bindings.define_module(parent_link, + Some(def_id), + kind, + true, + is_public, + DUMMY_SP); } } _ => {} @@ -807,7 +801,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { final_ident); // impl methods have already been defined with the correct importability // modifier - let mut modifiers = match *child_name_bindings.value_def.borrow() { + let mut modifiers = match *child_name_bindings.value_ns.borrow() { Some(ref def) => (modifiers & !DefModifiers::IMPORTABLE) | (def.modifiers & DefModifiers::IMPORTABLE), None => modifiers, @@ -922,7 +916,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { self.handle_external_def(def, def_visibility, - &*child_name_bindings, + &child_name_bindings, &name.as_str(), name, root); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index a402d8310f96a..3f6f66cad7a21 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -39,7 +39,6 @@ extern crate rustc; use self::PatternBindingMode::*; use self::Namespace::*; use self::NamespaceResult::*; -use self::NameDefinition::*; use self::ResolveResult::*; use self::FallbackSuggestion::*; use self::TypeParameters::*; @@ -513,8 +512,8 @@ enum NamespaceResult { /// not bound in the namespace. UnboundResult, /// Means that resolve has determined that the name is bound in the Module - /// argument, and specified by the NameBindings argument. - BoundResult(Rc, Rc), + /// argument, and specified by the NameBinding argument. + BoundResult(Rc, NameBinding), } impl NamespaceResult { @@ -532,15 +531,6 @@ impl NamespaceResult { } } -enum NameDefinition { - // The name was unbound. - NoNameDefinition, - // The name identifies an immediate child. - ChildNameDefinition(Def, LastPrivate), - // The name identifies an import. - ImportNameDefinition(Def, LastPrivate), -} - impl<'a, 'v, 'tcx> Visitor<'v> for Resolver<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { execute_callback!(hir_map::Node::NodeItem(item), self); @@ -786,7 +776,7 @@ pub struct Module { kind: Cell, is_public: bool, - children: RefCell>>, + children: RefCell>, imports: RefCell>, // The external module children of this node that were declared with @@ -911,36 +901,103 @@ bitflags! { } } -// Records a possibly-private type definition. +// Records a possibly-private definition. #[derive(Clone,Debug)] -struct TypeNsDef { +struct NsDef { modifiers: DefModifiers, // see note in ImportResolution about how to use this - module_def: Option>, - type_def: Option, - type_span: Option, + def: Option, + module: Option>, + span: Option, } -// Records a possibly-private value definition. -#[derive(Clone, Copy, Debug)] -struct ValueNsDef { - modifiers: DefModifiers, // see note in ImportResolution about how to use this - def: Def, - value_span: Option, +impl NsDef { + fn def(&self) -> Option { + match (self.def, &self.module) { + (def @ Some(_), _) => def, + (_, &Some(ref module)) => module.def_id.get().map(|def_id| DefMod(def_id)), + _ => panic!("NsDef has neither a Def nor a Module"), + } + } +} + +// Records at most one definition that a name in a namespace is bound to +#[derive(Clone,Debug)] +pub struct NameBinding(Rc>>); + +impl NameBinding { + fn new() -> Self { + NameBinding(Rc::new(RefCell::new(None))) + } + + fn create_from_module(module: Rc) -> Self { + NameBinding(Rc::new(RefCell::new(Some(NsDef { + modifiers: DefModifiers::IMPORTABLE, + def: None, + module: Some(module), + span: None, + })))) + } + + fn set(&self, modifiers: DefModifiers, def: Option, mod_: Option>, sp: Span) { + *self.0.borrow_mut() = + Some(NsDef { modifiers: modifiers, def: def, module: mod_, span: Some(sp) }); + } + + fn and_then Option>(&self, f: F) -> Option { + self.borrow().as_ref().and_then(f) + } + + fn borrow(&self) -> ::std::cell::Ref> { self.0.borrow() } + + // Lifted versions of the NsDef fields and method + fn def(&self) -> Option { self.and_then(NsDef::def) } + fn span(&self) -> Option { self.and_then(|def| def.span) } + fn module(&self) -> Option> { self.and_then(|def| def.module.clone()) } + fn modifiers(&self) -> Option { self.and_then(|def| Some(def.modifiers)) } + + fn defined(&self) -> bool { self.borrow().is_some() } + + fn defined_with(&self, modifiers: DefModifiers) -> bool { + self.modifiers().map(|m| m.contains(modifiers)).unwrap_or(false) + } + + fn is_public(&self) -> bool { + self.defined_with(DefModifiers::PUBLIC) + } + + fn def_and_lp(&self) -> (Def, LastPrivate) { + let def = self.def().unwrap(); + (def, LastMod(if self.is_public() { AllPublic } else { DependsOn(def.def_id()) })) + } } // Records the definitions (at most one for each namespace) that a name is // bound to. -#[derive(Debug)] +#[derive(Clone,Debug)] pub struct NameBindings { - type_def: RefCell>, // < Meaning in type namespace. - value_def: RefCell>, // < Meaning in value namespace. + type_ns: NameBinding, // < Meaning in type namespace. + value_ns: NameBinding, // < Meaning in value namespace. +} + +impl ::std::ops::Index for NameBindings { + type Output = NameBinding; + fn index(&self, namespace: Namespace) -> &NameBinding { + match namespace { TypeNS => &self.type_ns, ValueNS => &self.value_ns } + } } impl NameBindings { fn new() -> NameBindings { NameBindings { - type_def: RefCell::new(None), - value_def: RefCell::new(None), + type_ns: NameBinding::new(), + value_ns: NameBinding::new(), + } + } + + fn create_from_module(module: Rc) -> NameBindings { + NameBindings { + type_ns: NameBinding::create_from_module(module), + value_ns: NameBinding::new(), } } @@ -958,26 +1015,9 @@ impl NameBindings { } else { DefModifiers::empty() } | DefModifiers::IMPORTABLE; + let module_ = Rc::new(Module::new(parent_link, def_id, kind, external, is_public)); - let type_def = self.type_def.borrow().clone(); - match type_def { - None => { - *self.type_def.borrow_mut() = Some(TypeNsDef { - modifiers: modifiers, - module_def: Some(module_), - type_def: None, - type_span: Some(sp), - }); - } - Some(type_def) => { - *self.type_def.borrow_mut() = Some(TypeNsDef { - modifiers: modifiers, - module_def: Some(module_), - type_span: Some(sp), - type_def: type_def.type_def, - }); - } - } + self.type_ns.set(modifiers, self.type_ns.def(), Some(module_), sp); } /// Sets the kind of the module, creating a new one if necessary. @@ -988,36 +1028,10 @@ impl NameBindings { external: bool, is_public: bool, _sp: Span) { - let modifiers = if is_public { - DefModifiers::PUBLIC + if let Some(module) = self.type_ns.module() { + module.kind.set(kind) } else { - DefModifiers::empty() - } | DefModifiers::IMPORTABLE; - let type_def = self.type_def.borrow().clone(); - match type_def { - None => { - let module = Module::new(parent_link, def_id, kind, external, is_public); - *self.type_def.borrow_mut() = Some(TypeNsDef { - modifiers: modifiers, - module_def: Some(Rc::new(module)), - type_def: None, - type_span: None, - }); - } - Some(type_def) => { - match type_def.module_def { - None => { - let module = Module::new(parent_link, def_id, kind, external, is_public); - *self.type_def.borrow_mut() = Some(TypeNsDef { - modifiers: modifiers, - module_def: Some(Rc::new(module)), - type_def: type_def.type_def, - type_span: None, - }); - } - Some(module_def) => module_def.kind.set(kind), - } - } + self.define_module(parent_link, def_id, kind, external, is_public, _sp) } } @@ -1027,46 +1041,17 @@ impl NameBindings { def, modifiers); // Merges the type with the existing type def or creates a new one. - let type_def = self.type_def.borrow().clone(); - match type_def { - None => { - *self.type_def.borrow_mut() = Some(TypeNsDef { - module_def: None, - type_def: Some(def), - type_span: Some(sp), - modifiers: modifiers, - }); - } - Some(type_def) => { - *self.type_def.borrow_mut() = Some(TypeNsDef { - module_def: type_def.module_def, - type_def: Some(def), - type_span: Some(sp), - modifiers: modifiers, - }); - } - } + self.type_ns.set(modifiers, Some(def), self.type_ns.module(), sp); } /// Records a value definition. fn define_value(&self, def: Def, sp: Span, modifiers: DefModifiers) { - debug!("defining value for def {:?} with modifiers {:?}", - def, - modifiers); - *self.value_def.borrow_mut() = Some(ValueNsDef { - def: def, - value_span: Some(sp), - modifiers: modifiers, - }); + debug!("defining value for def {:?} with modifiers {:?}", def, modifiers); + self.value_ns.set(modifiers, Some(def), None, sp); } /// Returns the module node if applicable. - fn get_module_if_available(&self) -> Option> { - match *self.type_def.borrow() { - Some(ref type_def) => type_def.module_def.clone(), - None => None, - } - } + fn get_module_if_available(&self) -> Option> { self.type_ns.module() } /// Returns the module node. Panics if this node does not have a module /// definition. @@ -1078,96 +1063,6 @@ impl NameBindings { Some(module_def) => module_def, } } - - fn defined_in_namespace(&self, namespace: Namespace) -> bool { - match namespace { - TypeNS => return self.type_def.borrow().is_some(), - ValueNS => return self.value_def.borrow().is_some(), - } - } - - fn defined_in_public_namespace(&self, namespace: Namespace) -> bool { - self.defined_in_namespace_with(namespace, DefModifiers::PUBLIC) - } - - fn defined_in_namespace_with(&self, namespace: Namespace, modifiers: DefModifiers) -> bool { - match namespace { - TypeNS => match *self.type_def.borrow() { - Some(ref def) => def.modifiers.contains(modifiers), - None => false, - }, - ValueNS => match *self.value_def.borrow() { - Some(ref def) => def.modifiers.contains(modifiers), - None => false, - }, - } - } - - fn def_for_namespace(&self, namespace: Namespace) -> Option { - match namespace { - TypeNS => { - match *self.type_def.borrow() { - None => None, - Some(ref type_def) => { - match type_def.type_def { - Some(type_def) => Some(type_def), - None => { - match type_def.module_def { - Some(ref module) => { - match module.def_id.get() { - Some(did) => Some(DefMod(did)), - None => None, - } - } - None => None, - } - } - } - } - } - } - ValueNS => { - match *self.value_def.borrow() { - None => None, - Some(value_def) => Some(value_def.def), - } - } - } - } - - fn span_for_namespace(&self, namespace: Namespace) -> Option { - if self.defined_in_namespace(namespace) { - match namespace { - TypeNS => { - match *self.type_def.borrow() { - None => None, - Some(ref type_def) => type_def.type_span, - } - } - ValueNS => { - match *self.value_def.borrow() { - None => None, - Some(ref value_def) => value_def.value_span, - } - } - } - } else { - None - } - } - - fn is_public(&self, namespace: Namespace) -> bool { - match namespace { - TypeNS => { - let type_def = self.type_def.borrow(); - type_def.as_ref().unwrap().modifiers.contains(DefModifiers::PUBLIC) - } - ValueNS => { - let value_def = self.value_def.borrow(); - value_def.as_ref().unwrap().modifiers.contains(DefModifiers::PUBLIC) - } - } - } } /// Interns the names of the primitive types. @@ -1356,18 +1251,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } } - fn create_name_bindings_from_module(module: Rc) -> NameBindings { - NameBindings { - type_def: RefCell::new(Some(TypeNsDef { - modifiers: DefModifiers::IMPORTABLE, - module_def: Some(module), - type_def: None, - type_span: None, - })), - value_def: RefCell::new(None), - } - } - /// Checks that the names of external crates don't collide with other /// external crates. fn check_for_conflicts_between_external_crates(&self, @@ -1473,38 +1356,24 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Success((target, used_proxy)) => { // Check to see whether there are type bindings, and, if // so, whether there is a module within. - match *target.bindings.type_def.borrow() { - Some(ref type_def) => { - match type_def.module_def { - None => { - let msg = format!("Not a module `{}`", name); + if let Some(module_def) = target.binding.module() { + // track extern crates for unused_extern_crate lint + if let Some(did) = module_def.def_id.get() { + self.used_crates.insert(did.krate); + } - return Failed(Some((span, msg))); - } - Some(ref module_def) => { - search_module = module_def.clone(); + search_module = module_def; - // track extern crates for unused_extern_crate lint - if let Some(did) = module_def.def_id.get() { - self.used_crates.insert(did.krate); - } - - // Keep track of the closest - // private module used when - // resolving this import chain. - if !used_proxy && !search_module.is_public { - if let Some(did) = search_module.def_id.get() { - closest_private = LastMod(DependsOn(did)); - } - } - } + // Keep track of the closest private module used + // when resolving this import chain. + if !used_proxy && !search_module.is_public { + if let Some(did) = search_module.def_id.get() { + closest_private = LastMod(DependsOn(did)); } } - None => { - // There are no type bindings at all. - let msg = format!("Not a module `{}`", name); - return Failed(Some((span, msg))); - } + } else { + let msg = format!("Not a module `{}`", name); + return Failed(Some((span, msg))); } } } @@ -1628,10 +1497,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { build_reduced_graph::populate_module_if_necessary(self, &module_); match module_.children.borrow().get(&name) { - Some(name_bindings) if name_bindings.defined_in_namespace(namespace) => { + Some(name_bindings) if name_bindings[namespace].defined() => { debug!("top name bindings succeeded"); return Success((Target::new(module_.clone(), - name_bindings.clone(), + name_bindings[namespace].clone(), Shadowable::Never), false)); } @@ -1671,9 +1540,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // FIXME (21114): In principle unclear `child` *has* to be lifted. let child = module_.external_module_children.borrow().get(&name).cloned(); if let Some(module) = child { - let name_bindings = Rc::new(Resolver::create_name_bindings_from_module(module)); + let name_binding = NameBinding::create_from_module(module); debug!("lower name bindings succeeded"); - return Success((Target::new(module_, name_bindings, Shadowable::Never), + return Success((Target::new(module_, name_binding, Shadowable::Never), false)); } } @@ -1745,26 +1614,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let resolve_result = self.resolve_item_in_lexical_scope(module_, name, TypeNS); match resolve_result { Success((target, _)) => { - let bindings = &*target.bindings; - match *bindings.type_def.borrow() { - Some(ref type_def) => { - match type_def.module_def { - None => { - debug!("!!! (resolving module in lexical scope) module wasn't \ - actually a module!"); - return Failed(None); - } - Some(ref module_def) => { - return Success(module_def.clone()); - } - } - } - None => { - debug!("!!! (resolving module in lexical scope) module - \ - wasn't actually a module!"); - return Failed(None); - } + if let Some(module_def) = target.binding.module() { + return Success(module_def) + } else { + debug!("!!! (resolving module in lexical scope) module \ + wasn't actually a module!"); + return Failed(None); } } Indeterminate => { @@ -1872,10 +1727,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { build_reduced_graph::populate_module_if_necessary(self, &module_); match module_.children.borrow().get(&name) { - Some(name_bindings) if name_bindings.defined_in_namespace(namespace) => { + Some(name_bindings) if name_bindings[namespace].defined() => { debug!("(resolving name in module) found node as child"); return Success((Target::new(module_.clone(), - name_bindings.clone(), + name_bindings[namespace].clone(), Shadowable::Never), false)); } @@ -1926,8 +1781,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // FIXME (21114): In principle unclear `child` *has* to be lifted. let child = module_.external_module_children.borrow().get(&name).cloned(); if let Some(module) = child { - let name_bindings = Rc::new(Resolver::create_name_bindings_from_module(module)); - return Success((Target::new(module_, name_bindings, Shadowable::Never), + let name_binding = NameBinding::create_from_module(module); + return Success((Target::new(module_, name_binding, Shadowable::Never), false)); } } @@ -2958,32 +2813,26 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Success((target, _)) => { debug!("(resolve bare identifier pattern) succeeded in finding {} at {:?}", name, - target.bindings.value_def.borrow()); - match *target.bindings.value_def.borrow() { + target.binding.borrow()); + match target.binding.def() { None => { panic!("resolved name in the value namespace to a set of name bindings \ with no def?!"); } - Some(def) => { - // For the two success cases, this lookup can be - // considered as not having a private component because - // the lookup happened only within the current module. - match def.def { - def @ DefVariant(..) | def @ DefStruct(..) => { - return FoundStructOrEnumVariant(def, LastMod(AllPublic)); - } - def @ DefConst(..) | def @ DefAssociatedConst(..) => { - return FoundConst(def, LastMod(AllPublic), name); - } - DefStatic(..) => { - resolve_error(self, span, ResolutionError::StaticVariableReference); - return BareIdentifierPatternUnresolved; - } - _ => { - return BareIdentifierPatternUnresolved; - } - } + // For the two success cases, this lookup can be + // considered as not having a private component because + // the lookup happened only within the current module. + Some(def @ DefVariant(..)) | Some(def @ DefStruct(..)) => { + return FoundStructOrEnumVariant(def, LastMod(AllPublic)); + } + Some(def @ DefConst(..)) | Some(def @ DefAssociatedConst(..)) => { + return FoundConst(def, LastMod(AllPublic), name); + } + Some(DefStatic(..)) => { + resolve_error(self, span, ResolutionError::StaticVariableReference); + return BareIdentifierPatternUnresolved; } + _ => return BareIdentifierPatternUnresolved } } @@ -3207,85 +3056,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { return Some(def); } - // FIXME #4952: Merge me with resolve_name_in_module? - fn resolve_definition_of_name_in_module(&mut self, - containing_module: Rc, - name: Name, - namespace: Namespace) - -> NameDefinition { - // First, search children. - build_reduced_graph::populate_module_if_necessary(self, &containing_module); - - match containing_module.children.borrow().get(&name) { - Some(child_name_bindings) => { - match child_name_bindings.def_for_namespace(namespace) { - Some(def) => { - // Found it. Stop the search here. - let p = child_name_bindings.defined_in_public_namespace(namespace); - let lp = if p { - LastMod(AllPublic) - } else { - LastMod(DependsOn(def.def_id())) - }; - return ChildNameDefinition(def, lp); - } - None => {} - } - } - None => {} - } - - // Next, search import resolutions. - match containing_module.import_resolutions.borrow().get(&name) { - Some(import_resolution) if import_resolution.is_public => { - if let Some(target) = (*import_resolution).target_for_namespace(namespace) { - match target.bindings.def_for_namespace(namespace) { - Some(def) => { - // Found it. - let id = import_resolution.id(namespace); - // track imports and extern crates as well - self.used_imports.insert((id, namespace)); - self.record_import_use(id, name); - match target.target_module.def_id.get() { - Some(DefId{krate: kid, ..}) => { - self.used_crates.insert(kid); - } - _ => {} - } - return ImportNameDefinition(def, LastMod(AllPublic)); - } - None => { - // This can happen with external impls, due to - // the imperfect way we read the metadata. - } - } - } - } - Some(..) | None => {} // Continue. - } - - // Finally, search through external children. - if namespace == TypeNS { - if let Some(module) = containing_module.external_module_children - .borrow() - .get(&name) - .cloned() { - if let Some(def_id) = module.def_id.get() { - // track used crates - self.used_crates.insert(def_id.krate); - let lp = if module.is_public { - LastMod(AllPublic) - } else { - LastMod(DependsOn(def_id)) - }; - return ChildNameDefinition(DefMod(def_id), lp); - } - } - } - - return NoNameDefinition; - } - // resolve a "module-relative" path, e.g. a::b::c fn resolve_module_relative_path(&mut self, span: Span, @@ -3328,16 +3098,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } let name = segments.last().unwrap().identifier.name; - let def = match self.resolve_definition_of_name_in_module(containing_module.clone(), - name, - namespace) { - NoNameDefinition => { - // We failed to resolve the name. Report an error. - return None; - } - ChildNameDefinition(def, lp) | ImportNameDefinition(def, lp) => { + let def = match self.resolve_name_in_module(containing_module.clone(), + name, + namespace, + NameSearchType::PathSearch, + false) { + Success((Target { binding, .. }, _)) => { + let (def, lp) = binding.def_and_lp(); (def, last_private.or(lp)) } + _ => return None, }; if let Some(DefId{krate: kid, ..}) = containing_module.def_id.get() { self.used_crates.insert(kid); @@ -3394,14 +3164,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } let name = segments.last().unwrap().identifier.name; - match self.resolve_definition_of_name_in_module(containing_module, name, namespace) { - NoNameDefinition => { - // We failed to resolve the name. Report an error. - return None; - } - ChildNameDefinition(def, lp) | ImportNameDefinition(def, lp) => { - return Some((def, last_private.or(lp))); + match self.resolve_name_in_module(containing_module, + name, + namespace, + NameSearchType::PathSearch, + false) { + Success((Target { binding, .. }, _)) => { + let (def, lp) = binding.def_and_lp(); + Some((def, last_private.or(lp))) } + _ => None, } } @@ -3449,7 +3221,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let module = self.current_module.clone(); match self.resolve_item_in_lexical_scope(module, name, namespace) { Success((target, _)) => { - match (*target.bindings).def_for_namespace(namespace) { + match target.binding.def() { None => { // This can happen if we were looking for a type and // found a module instead. Modules don't have defs. @@ -3586,7 +3358,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Look for a method in the current self type's impl module. if let Some(module) = get_module(self, path.span, &name_path) { if let Some(binding) = module.children.borrow().get(&name) { - if let Some(DefMethod(did)) = binding.def_for_namespace(ValueNS) { + if let Some(DefMethod(did)) = binding.value_ns.def() { if is_static_method(self, did) { return StaticMethod(path_names_to_string(&path, 0)); } @@ -3894,7 +3666,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { { for (_, child_names) in search_module.children.borrow().iter() { - let def = match child_names.def_for_namespace(TypeNS) { + let def = match child_names.type_ns.def() { Some(def) => def, None => continue, }; @@ -3914,7 +3686,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { None => continue, Some(target) => target, }; - let did = match target.bindings.def_for_namespace(TypeNS) { + let did = match target.binding.def() { Some(DefTrait(trait_def_id)) => trait_def_id, Some(..) | None => continue, }; diff --git a/src/librustc_resolve/record_exports.rs b/src/librustc_resolve/record_exports.rs index 96fad16536cb8..02ab3bd029509 100644 --- a/src/librustc_resolve/record_exports.rs +++ b/src/librustc_resolve/record_exports.rs @@ -18,8 +18,8 @@ // Then this operation can simply be performed as part of item (or import) // processing. -use {Module, NameBindings, Resolver}; -use Namespace::{self, TypeNS, ValueNS}; +use {Module, NameBinding, Resolver}; +use Namespace::{TypeNS, ValueNS}; use build_reduced_graph; use module_to_string; @@ -108,12 +108,11 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { } } - fn add_exports_of_namebindings(&mut self, - exports: &mut Vec, - name: ast::Name, - namebindings: &NameBindings, - ns: Namespace) { - match namebindings.def_for_namespace(ns) { + fn add_export_of_namebinding(&mut self, + exports: &mut Vec, + name: ast::Name, + namebinding: &NameBinding) { + match namebinding.def() { Some(d) => { debug!("(computing exports) YES: export '{}' => {:?}", name, @@ -139,7 +138,7 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { match import_resolution.target_for_namespace(ns) { Some(target) => { debug!("(computing exports) maybe export '{}'", name); - self.add_exports_of_namebindings(exports, *name, &*target.bindings, ns) + self.add_export_of_namebinding(exports, *name, &target.binding) } _ => (), } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 9a21ec86685c1..150c6ec965752 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -14,7 +14,7 @@ use DefModifiers; use Module; use ModuleKind; use Namespace::{self, TypeNS, ValueNS}; -use NameBindings; +use {NameBindings, NameBinding}; use NamespaceResult::{BoundResult, UnboundResult, UnknownResult}; use NamespaceResult; use NameSearchType; @@ -86,18 +86,18 @@ impl ImportDirective { #[derive(Clone,Debug)] pub struct Target { pub target_module: Rc, - pub bindings: Rc, + pub binding: NameBinding, pub shadowable: Shadowable, } impl Target { pub fn new(target_module: Rc, - bindings: Rc, + binding: NameBinding, shadowable: Shadowable) -> Target { Target { target_module: target_module, - bindings: bindings, + binding: binding, shadowable: shadowable, } } @@ -459,11 +459,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(ref child_name_bindings) => { // pub_err makes sure we don't give the same error twice. let mut pub_err = false; - if child_name_bindings.defined_in_namespace(ValueNS) { + if child_name_bindings.value_ns.defined() { debug!("(resolving single import) found value binding"); value_result = BoundResult(target_module.clone(), - (*child_name_bindings).clone()); - if directive.is_public && !child_name_bindings.is_public(ValueNS) { + child_name_bindings.value_ns.clone()); + if directive.is_public && !child_name_bindings.value_ns.is_public() { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = format!("Consider marking `{}` as `pub` in the imported \ module", @@ -473,11 +473,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { pub_err = true; } } - if child_name_bindings.defined_in_namespace(TypeNS) { + if child_name_bindings.type_ns.defined() { debug!("(resolving single import) found type binding"); type_result = BoundResult(target_module.clone(), - (*child_name_bindings).clone()); - if !pub_err && directive.is_public && !child_name_bindings.is_public(TypeNS) { + child_name_bindings.type_ns.clone()); + if !pub_err && directive.is_public && + !child_name_bindings.type_ns.is_public() { let msg = format!("`{}` is private, and cannot be reexported", source); let note_msg = format!("Consider declaring module `{}` as a `pub mod`", source); @@ -540,7 +541,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } Some(Target { target_module, - bindings, + binding, shadowable: _ }) => { debug!("(resolving single import) found import in ns {:?}", @@ -555,7 +556,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } _ => {} } - return BoundResult(target_module, bindings); + return BoundResult(target_module, binding); } } } @@ -630,9 +631,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } _ => {} } - let name_bindings = - Rc::new(Resolver::create_name_bindings_from_module(module)); - type_result = BoundResult(target_module.clone(), name_bindings); + let name_binding = NameBinding::create_from_module(module); + type_result = BoundResult(target_module.clone(), name_binding); type_used_public = true; } } @@ -651,26 +651,25 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }; match *result { - BoundResult(ref target_module, ref name_bindings) => { + BoundResult(ref target_module, ref name_binding) => { debug!("(resolving single import) found {:?} target: {:?}", namespace_name, - name_bindings.def_for_namespace(namespace)); + name_binding.def()); self.check_for_conflicting_import(&import_resolution, directive.span, target, namespace); - self.check_that_import_is_importable(&**name_bindings, + self.check_that_import_is_importable(&name_binding, directive.span, - target, - namespace); + target); let target = Some(Target::new(target_module.clone(), - name_bindings.clone(), + name_binding.clone(), directive.shadowable)); import_resolution.set_target_and_id(namespace, target, directive.id); import_resolution.is_public = directive.is_public; - *used_public = name_bindings.defined_in_public_namespace(namespace); + *used_public = name_binding.is_public(); } UnboundResult => { // Continue. @@ -705,7 +704,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // this may resolve to either a value or a type, but for documentation // purposes it's good enough to just favor one over the other. let value_def_and_priv = import_resolution.value_target.as_ref().map(|target| { - let def = target.bindings.def_for_namespace(ValueNS).unwrap(); + let def = target.binding.def().unwrap(); (def, if value_used_public { lp @@ -714,7 +713,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }) }); let type_def_and_priv = import_resolution.type_target.as_ref().map(|target| { - let def = target.bindings.def_for_namespace(TypeNS).unwrap(); + let def = target.binding.def().unwrap(); (def, if type_used_public { lp @@ -857,12 +856,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // Add external module children from the containing module. for (&name, module) in target_module.external_module_children.borrow().iter() { - let name_bindings = Rc::new(Resolver::create_name_bindings_from_module(module.clone())); self.merge_import_resolution(module_, target_module.clone(), import_directive, name, - name_bindings); + NameBindings::create_from_module(module.clone())); } // Record the destination of this import @@ -884,7 +882,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { containing_module: Rc, import_directive: &ImportDirective, name: Name, - name_bindings: Rc) { + name_bindings: NameBindings) { let id = import_directive.id; let is_public = import_directive.is_public; @@ -904,7 +902,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let mut merge_child_item = |namespace| { let modifier = DefModifiers::IMPORTABLE | DefModifiers::PUBLIC; - if name_bindings.defined_in_namespace_with(namespace, modifier) { + if name_bindings[namespace].defined_with(modifier) { let namespace_name = match namespace { TypeNS => "type", ValueNS => "value", @@ -922,7 +920,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { msg); } else { let target = Target::new(containing_module.clone(), - name_bindings.clone(), + name_bindings[namespace].clone(), import_directive.shadowable); dest_import_resolution.set_target_and_id(namespace, Some(target), id); } @@ -955,16 +953,12 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(ref target) if target.shadowable != Shadowable::Always => { let ns_word = match namespace { TypeNS => { - if let Some(ref ty_def) = *target.bindings.type_def.borrow() { - match ty_def.module_def { - Some(ref module) if module.kind.get() == - ModuleKind::NormalModuleKind => "module", - Some(ref module) if module.kind.get() == - ModuleKind::TraitModuleKind => "trait", - _ => "type", - } - } else { - "type" + match target.binding.module() { + Some(ref module) if module.kind.get() == + ModuleKind::NormalModuleKind => "module", + Some(ref module) if module.kind.get() == + ModuleKind::TraitModuleKind => "trait", + _ => "type", } } ValueNS => "value", @@ -989,11 +983,10 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Checks that an import is actually importable fn check_that_import_is_importable(&mut self, - name_bindings: &NameBindings, + name_binding: &NameBinding, import_span: Span, - name: Name, - namespace: Namespace) { - if !name_bindings.defined_in_namespace_with(namespace, DefModifiers::IMPORTABLE) { + name: Name) { + if !name_binding.defined_with(DefModifiers::IMPORTABLE) { let msg = format!("`{}` is not directly importable", name); span_err!(self.resolver.session, import_span, E0253, "{}", &msg[..]); } @@ -1032,13 +1025,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match import_resolution.value_target { Some(ref target) if target.shadowable != Shadowable::Always => { - if let Some(ref value) = *name_bindings.value_def.borrow() { + if let Some(ref value) = *name_bindings.value_ns.borrow() { span_err!(self.resolver.session, import_span, E0255, "import `{}` conflicts with value in this module", name); - if let Some(span) = value.value_span { + if let Some(span) = value.span { self.resolver.session.span_note(span, "conflicting value here"); } } @@ -1048,8 +1041,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match import_resolution.type_target { Some(ref target) if target.shadowable != Shadowable::Always => { - if let Some(ref ty) = *name_bindings.type_def.borrow() { - let (what, note) = match ty.module_def { + if let Some(ref ty) = *name_bindings.type_ns.borrow() { + let (what, note) = match ty.module.clone() { Some(ref module) if module.kind.get() == ModuleKind::NormalModuleKind => ("existing submodule", "note conflicting module here"), Some(ref module) if module.kind.get() == ModuleKind::TraitModuleKind => @@ -1062,7 +1055,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { "import `{}` conflicts with {}", name, what); - if let Some(span) = ty.type_span { + if let Some(span) = ty.span { self.resolver.session.span_note(span, note); } } From 8a6187fde1de7b209bbd2b49ea3c669006a0d154 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Mon, 16 Nov 2015 07:59:50 +0000 Subject: [PATCH 2/4] Refactor fields def_id and kind of Module into a single field def. Change build_reduced_graph.rs so the fields def and module of NsDef are never both Some unless the NsDef represents a duplicate definition (see issue 26421). --- src/librustc_resolve/build_reduced_graph.rs | 103 ++++------ src/librustc_resolve/lib.rs | 182 ++++++++---------- src/librustc_resolve/record_exports.rs | 4 +- src/librustc_resolve/resolve_imports.rs | 19 +- .../enum-and-module-in-same-scope.rs | 2 +- 5 files changed, 126 insertions(+), 184 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 802f9a0d40f80..b70349bfe94a1 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -18,7 +18,6 @@ use resolve_imports::ImportDirective; use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport}; use resolve_imports::ImportResolution; use Module; -use ModuleKind::*; use Namespace::{TypeNS, ValueNS}; use NameBindings; use {names_to_string, module_to_string}; @@ -395,8 +394,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { self.external_exports.insert(def_id); let parent_link = ModuleParentLink(Rc::downgrade(parent), name); let external_module = Rc::new(Module::new(parent_link, - Some(def_id), - NormalModuleKind, + Some(DefMod(def_id)), false, true)); debug!("(build reduced graph for item) found extern `{}`", @@ -436,13 +434,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let name_bindings = self.add_child(name, parent, ForbidDuplicateModules, sp); let parent_link = self.get_parent_link(parent, name); - let def_id = self.ast_map.local_def_id(item.id); - name_bindings.define_module(parent_link, - Some(def_id), - NormalModuleKind, - false, - is_public, - sp); + let def = DefMod(self.ast_map.local_def_id(item.id)); + name_bindings.define_module(parent_link, Some(def), false, is_public, sp); name_bindings.get_module() } @@ -479,17 +472,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ForbidDuplicateTypesAndModules, sp); - name_bindings.define_type(DefTy(self.ast_map.local_def_id(item.id), false), - sp, - modifiers); - let parent_link = self.get_parent_link(parent, name); - name_bindings.set_module_kind(parent_link, - Some(self.ast_map.local_def_id(item.id)), - TypeModuleKind, - false, - is_public, - sp); + let def = DefTy(self.ast_map.local_def_id(item.id), false); + name_bindings.define_module(parent_link, Some(def), false, is_public, sp); parent.clone() } @@ -499,17 +484,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ForbidDuplicateTypesAndModules, sp); - name_bindings.define_type(DefTy(self.ast_map.local_def_id(item.id), true), - sp, - modifiers); - let parent_link = self.get_parent_link(parent, name); - name_bindings.set_module_kind(parent_link, - Some(self.ast_map.local_def_id(item.id)), - EnumModuleKind, - false, - is_public, - sp); + let def = DefTy(self.ast_map.local_def_id(item.id), true); + name_bindings.define_module(parent_link, Some(def), false, is_public, sp); let module = name_bindings.get_module(); @@ -592,18 +569,14 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ForbidDuplicateTypesAndModules, sp); + let def_id = self.ast_map.local_def_id(item.id); + // Add all the items within to a new module. let parent_link = self.get_parent_link(parent, name); - name_bindings.define_module(parent_link, - Some(self.ast_map.local_def_id(item.id)), - TraitModuleKind, - false, - is_public, - sp); + let def = DefTrait(def_id); + name_bindings.define_module(parent_link, Some(def), false, is_public, sp); let module_parent = name_bindings.get_module(); - let def_id = self.ast_map.local_def_id(item.id); - // Add the names of all the items to the trait info. for trait_item in items { let name_bindings = self.add_child(trait_item.name, @@ -634,7 +607,6 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { self.trait_item_map.insert((trait_item.name, def_id), trait_item_def_id); } - name_bindings.define_type(DefTrait(def_id), sp, modifiers); parent.clone() } } @@ -705,7 +677,6 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let new_module = Rc::new(Module::new(BlockParentLink(Rc::downgrade(parent), block_id), None, - AnonymousModuleKind, false, false)); parent.anonymous_children.borrow_mut().insert(block_id, new_module.clone()); @@ -732,7 +703,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { DefModifiers::empty() } | DefModifiers::IMPORTABLE; let is_exported = is_public && - match new_parent.def_id.get() { + match new_parent.def_id() { None => true, Some(did) => self.external_exports.contains(&did), }; @@ -740,20 +711,14 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { self.external_exports.insert(def.def_id()); } - let kind = match def { - DefTy(_, true) => EnumModuleKind, - DefTy(_, false) | DefStruct(..) => TypeModuleKind, - _ => NormalModuleKind, - }; - match def { - DefMod(def_id) | - DefForeignMod(def_id) | - DefStruct(def_id) | - DefTy(def_id, _) => { + DefMod(_) | + DefForeignMod(_) | + DefStruct(_) | + DefTy(..) => { if let Some(module_def) = child_name_bindings.type_ns.module() { debug!("(building reduced graph for external crate) already created module"); - module_def.def_id.set(Some(def_id)); + module_def.def.set(Some(def)); } else { debug!("(building reduced graph for external crate) building module {} {}", final_ident, @@ -761,8 +726,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let parent_link = self.get_parent_link(new_parent, name); child_name_bindings.define_module(parent_link, - Some(def_id), - kind, + Some(def), true, is_public, DUMMY_SP); @@ -806,7 +770,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { (def.modifiers & DefModifiers::IMPORTABLE), None => modifiers, }; - if new_parent.kind.get() != NormalModuleKind { + if !new_parent.is_normal() { modifiers = modifiers & !DefModifiers::IMPORTABLE; } child_name_bindings.define_value(def, DUMMY_SP, modifiers); @@ -835,33 +799,33 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { } } - child_name_bindings.define_type(def, DUMMY_SP, modifiers); - // Define a module if necessary. let parent_link = self.get_parent_link(new_parent, name); - child_name_bindings.set_module_kind(parent_link, - Some(def_id), - TraitModuleKind, - true, - is_public, - DUMMY_SP) + child_name_bindings.define_module(parent_link, + Some(def), + true, + is_public, + DUMMY_SP) } DefTy(..) | DefAssociatedTy(..) => { debug!("(building reduced graph for external crate) building type {}", final_ident); - let modifiers = match new_parent.kind.get() { - NormalModuleKind => modifiers, + let modifiers = match new_parent.is_normal() { + true => modifiers, _ => modifiers & !DefModifiers::IMPORTABLE, }; - child_name_bindings.define_type(def, DUMMY_SP, modifiers); + if let DefTy(..) = def { + child_name_bindings.type_ns.set_modifiers(modifiers); + } else { + child_name_bindings.define_type(def, DUMMY_SP, modifiers); + } } DefStruct(def_id) => { debug!("(building reduced graph for external crate) building type and value for \ {}", final_ident); - child_name_bindings.define_type(def, DUMMY_SP, modifiers); let fields = csearch::get_struct_field_names(&self.session.cstore, def_id); if fields.is_empty() { @@ -937,7 +901,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { debug!("(populating external module) attempting to populate {}", module_to_string(&**module)); - let def_id = match module.def_id.get() { + let def_id = match module.def_id() { None => { debug!("(populating external module) ... no def ID!"); return; @@ -971,8 +935,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { /// crate. fn build_reduced_graph_for_external_crate(&mut self, root: &Rc) { csearch::each_top_level_item_of_crate(&self.session.cstore, - root.def_id - .get() + root.def_id() .unwrap() .krate, |def_like, name, visibility| { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 3f6f66cad7a21..18e2b66d3fb2e 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -49,7 +49,6 @@ use self::AssocItemResolveResult::*; use self::NameSearchType::*; use self::BareIdentifierPatternResolution::*; use self::ParentLink::*; -use self::ModuleKind::*; use self::FallbackChecks::*; use rustc::front::map as hir_map; @@ -759,21 +758,10 @@ enum ParentLink { BlockParentLink(Weak, NodeId), } -/// The type of module this is. -#[derive(Copy, Clone, PartialEq, Debug)] -enum ModuleKind { - NormalModuleKind, - TraitModuleKind, - EnumModuleKind, - TypeModuleKind, - AnonymousModuleKind, -} - /// One node in the tree of modules. pub struct Module { parent_link: ParentLink, - def_id: Cell>, - kind: Cell, + def: Cell>, is_public: bool, children: RefCell>, @@ -822,15 +810,13 @@ pub struct Module { impl Module { fn new(parent_link: ParentLink, - def_id: Option, - kind: ModuleKind, + def: Option, external: bool, is_public: bool) -> Module { Module { parent_link: parent_link, - def_id: Cell::new(def_id), - kind: Cell::new(kind), + def: Cell::new(def), is_public: is_public, children: RefCell::new(HashMap::new()), imports: RefCell::new(Vec::new()), @@ -845,6 +831,24 @@ impl Module { } } + fn def_id(&self) -> Option { + self.def.get().as_ref().map(Def::def_id) + } + + fn is_normal(&self) -> bool { + match self.def.get() { + Some(DefMod(_)) | Some(DefForeignMod(_)) => true, + _ => false, + } + } + + fn is_trait(&self) -> bool { + match self.def.get() { + Some(DefTrait(_)) => true, + _ => false, + } + } + fn all_imports_resolved(&self) -> bool { if self.imports.borrow_state() == ::std::cell::BorrowState::Writing { // it is currently being resolved ! so nope @@ -882,9 +886,8 @@ impl Module { impl fmt::Debug for Module { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, - "{:?}, kind: {:?}, {}", - self.def_id, - self.kind, + "{:?}, {}", + self.def, if self.is_public { "public" } else { @@ -902,7 +905,9 @@ bitflags! { } // Records a possibly-private definition. -#[derive(Clone,Debug)] +// FIXME once #21546 is resolved, the def and module fields will never both be Some, +// so they can be refactored into something like Result>. +#[derive(Debug)] struct NsDef { modifiers: DefModifiers, // see note in ImportResolution about how to use this def: Option, @@ -911,10 +916,20 @@ struct NsDef { } impl NsDef { + fn create_from_module(module: Rc, span: Option) -> Self { + let modifiers = if module.is_public { + DefModifiers::PUBLIC + } else { + DefModifiers::empty() + } | DefModifiers::IMPORTABLE; + + NsDef { modifiers: modifiers, def: None, module: Some(module), span: span } + } + fn def(&self) -> Option { match (self.def, &self.module) { (def @ Some(_), _) => def, - (_, &Some(ref module)) => module.def_id.get().map(|def_id| DefMod(def_id)), + (_, &Some(ref module)) => module.def.get(), _ => panic!("NsDef has neither a Def nor a Module"), } } @@ -930,17 +945,17 @@ impl NameBinding { } fn create_from_module(module: Rc) -> Self { - NameBinding(Rc::new(RefCell::new(Some(NsDef { - modifiers: DefModifiers::IMPORTABLE, - def: None, - module: Some(module), - span: None, - })))) + NameBinding(Rc::new(RefCell::new(Some(NsDef::create_from_module(module, None))))) + } + + fn set(&self, ns_def: NsDef) { + *self.0.borrow_mut() = Some(ns_def); } - fn set(&self, modifiers: DefModifiers, def: Option, mod_: Option>, sp: Span) { - *self.0.borrow_mut() = - Some(NsDef { modifiers: modifiers, def: def, module: mod_, span: Some(sp) }); + fn set_modifiers(&self, modifiers: DefModifiers) { + if let Some(ref mut ns_def) = *self.0.borrow_mut() { + ns_def.modifiers = modifiers + } } fn and_then Option>(&self, f: F) -> Option { @@ -1004,35 +1019,12 @@ impl NameBindings { /// Creates a new module in this set of name bindings. fn define_module(&self, parent_link: ParentLink, - def_id: Option, - kind: ModuleKind, + def: Option, external: bool, is_public: bool, sp: Span) { - // Merges the module with the existing type def or creates a new one. - let modifiers = if is_public { - DefModifiers::PUBLIC - } else { - DefModifiers::empty() - } | DefModifiers::IMPORTABLE; - - let module_ = Rc::new(Module::new(parent_link, def_id, kind, external, is_public)); - self.type_ns.set(modifiers, self.type_ns.def(), Some(module_), sp); - } - - /// Sets the kind of the module, creating a new one if necessary. - fn set_module_kind(&self, - parent_link: ParentLink, - def_id: Option, - kind: ModuleKind, - external: bool, - is_public: bool, - _sp: Span) { - if let Some(module) = self.type_ns.module() { - module.kind.set(kind) - } else { - self.define_module(parent_link, def_id, kind, external, is_public, _sp) - } + let module = Module::new(parent_link, def, external, is_public); + self.type_ns.set(NsDef::create_from_module(Rc::new(module), Some(sp))); } /// Records a type definition. @@ -1041,13 +1033,19 @@ impl NameBindings { def, modifiers); // Merges the type with the existing type def or creates a new one. - self.type_ns.set(modifiers, Some(def), self.type_ns.module(), sp); + self.type_ns.set(NsDef { + modifiers: modifiers, def: Some(def), module: self.type_ns.module(), span: Some(sp) + }); } /// Records a value definition. fn define_value(&self, def: Def, sp: Span, modifiers: DefModifiers) { - debug!("defining value for def {:?} with modifiers {:?}", def, modifiers); - self.value_ns.set(modifiers, Some(def), None, sp); + debug!("defining value for def {:?} with modifiers {:?}", + def, + modifiers); + self.value_ns.set(NsDef { + modifiers: modifiers, def: Some(def), module: None, span: Some(sp) + }); } /// Returns the module node if applicable. @@ -1178,8 +1176,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let root_def_id = ast_map.local_def_id(CRATE_NODE_ID); graph_root.define_module(NoParentLink, - Some(root_def_id), - NormalModuleKind, + Some(DefMod(root_def_id)), false, true, crate_span); @@ -1358,7 +1355,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // so, whether there is a module within. if let Some(module_def) = target.binding.module() { // track extern crates for unused_extern_crate lint - if let Some(did) = module_def.def_id.get() { + if let Some(did) = module_def.def_id() { self.used_crates.insert(did.krate); } @@ -1367,7 +1364,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Keep track of the closest private module used // when resolving this import chain. if !used_proxy && !search_module.is_public { - if let Some(did) = search_module.def_id.get() { + if let Some(did) = search_module.def_id() { closest_private = LastMod(DependsOn(did)); } } @@ -1466,8 +1463,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Success(PrefixFound(ref containing_module, index)) => { search_module = containing_module.clone(); start_index = index; - last_private = LastMod(DependsOn(containing_module.def_id - .get() + last_private = LastMod(DependsOn(containing_module.def_id() .unwrap())); } } @@ -1527,8 +1523,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let id = import_resolution.id(namespace); self.used_imports.insert((id, namespace)); self.record_import_use(id, name); - if let Some(DefId{krate: kid, ..}) = target.target_module.def_id.get() { - self.used_crates.insert(kid); + if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() { + self.used_crates.insert(kid); } return Success((target, false)); } @@ -1558,19 +1554,13 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { return Failed(None); } ModuleParentLink(parent_module_node, _) => { - match search_module.kind.get() { - NormalModuleKind => { - // We stop the search here. - debug!("(resolving item in lexical scope) unresolved module: not \ - searching through module parents"); + if search_module.is_normal() { + // We stop the search here. + debug!("(resolving item in lexical scope) unresolved module: not \ + searching through module parents"); return Failed(None); - } - TraitModuleKind | - EnumModuleKind | - TypeModuleKind | - AnonymousModuleKind => { - search_module = parent_module_node.upgrade().unwrap(); - } + } else { + search_module = parent_module_node.upgrade().unwrap(); } } BlockParentLink(ref parent_module_node, _) => { @@ -1642,13 +1632,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { ModuleParentLink(new_module, _) | BlockParentLink(new_module, _) => { let new_module = new_module.upgrade().unwrap(); - match new_module.kind.get() { - NormalModuleKind => return Some(new_module), - TraitModuleKind | - EnumModuleKind | - TypeModuleKind | - AnonymousModuleKind => module_ = new_module, + if new_module.is_normal() { + return Some(new_module); } + module_ = new_module; } } } @@ -1657,17 +1644,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { /// Returns the nearest normal module parent of the given module, or the /// module itself if it is a normal module. fn get_nearest_normal_module_parent_or_self(&mut self, module_: Rc) -> Rc { - match module_.kind.get() { - NormalModuleKind => return module_, - TraitModuleKind | - EnumModuleKind | - TypeModuleKind | - AnonymousModuleKind => { - match self.get_nearest_normal_module_parent(module_.clone()) { - None => module_, - Some(new_module) => new_module, - } - } + if module_.is_normal() { + return module_; + } + match self.get_nearest_normal_module_parent(module_.clone()) { + None => module_, + Some(new_module) => new_module, } } @@ -1766,7 +1748,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let id = import_resolution.id(namespace); self.used_imports.insert((id, namespace)); self.record_import_use(id, name); - if let Some(DefId{krate: kid, ..}) = target.target_module.def_id.get() { + if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() { self.used_crates.insert(kid); } return Success((target, true)); @@ -3109,7 +3091,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } _ => return None, }; - if let Some(DefId{krate: kid, ..}) = containing_module.def_id.get() { + if let Some(DefId{krate: kid, ..}) = containing_module.def_id() { self.used_crates.insert(kid); } return Some(def); @@ -3696,7 +3678,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.used_imports.insert((id, TypeNS)); let trait_name = self.get_trait_name(did); self.record_import_use(id, trait_name); - if let Some(DefId{krate: kid, ..}) = target.target_module.def_id.get() { + if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() { self.used_crates.insert(kid); } } diff --git a/src/librustc_resolve/record_exports.rs b/src/librustc_resolve/record_exports.rs index 02ab3bd029509..6e8d2ac4ca5c6 100644 --- a/src/librustc_resolve/record_exports.rs +++ b/src/librustc_resolve/record_exports.rs @@ -54,7 +54,7 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { // If this isn't a local krate, then bail out. We don't need to record // exports for nonlocal crates. - match module_.def_id.get() { + match module_.def_id() { Some(def_id) if def_id.is_local() => { // OK. Continue. debug!("(recording exports for module subtree) recording exports for local \ @@ -98,7 +98,7 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { let mut exports = Vec::new(); self.add_exports_for_module(&mut exports, module_); - match module_.def_id.get() { + match module_.def_id() { Some(def_id) => { let node_id = self.ast_map.as_local_node_id(def_id).unwrap(); self.export_map.insert(node_id, exports); diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 150c6ec965752..14f8287a5f875 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -12,7 +12,6 @@ use self::ImportDirectiveSubclass::*; use DefModifiers; use Module; -use ModuleKind; use Namespace::{self, TypeNS, ValueNS}; use {NameBindings, NameBinding}; use NamespaceResult::{BoundResult, UnboundResult, UnknownResult}; @@ -550,7 +549,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // track used imports and extern crates as well this.used_imports.insert((id, namespace)); this.record_import_use(id, source); - match target_module.def_id.get() { + match target_module.def_id() { Some(DefId{krate: kid, ..}) => { this.used_crates.insert(kid); } @@ -592,7 +591,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // In this case we continue as if we resolved the import and let the // check_for_conflicts_between_imports_and_items call below handle // the conflict - match (module_.def_id.get(), target_module.def_id.get()) { + match (module_.def_id(), target_module.def_id()) { (Some(id1), Some(id2)) if id1 == id2 => { if value_result.is_unknown() { value_result = UnboundResult; @@ -625,7 +624,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(module) => { debug!("(resolving single import) found external module"); // track the module as used. - match module.def_id.get() { + match module.def_id() { Some(DefId{krate: kid, ..}) => { self.resolver.used_crates.insert(kid); } @@ -864,7 +863,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { } // Record the destination of this import - if let Some(did) = target_module.def_id.get() { + if let Some(did) = target_module.def_id() { self.resolver.def_map.borrow_mut().insert(id, PathResolution { base_def: DefMod(did), @@ -954,10 +953,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { let ns_word = match namespace { TypeNS => { match target.binding.module() { - Some(ref module) if module.kind.get() == - ModuleKind::NormalModuleKind => "module", - Some(ref module) if module.kind.get() == - ModuleKind::TraitModuleKind => "trait", + Some(ref module) if module.is_normal() => "module", + Some(ref module) if module.is_trait() => "trait", _ => "type", } } @@ -1043,9 +1040,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { Some(ref target) if target.shadowable != Shadowable::Always => { if let Some(ref ty) = *name_bindings.type_ns.borrow() { let (what, note) = match ty.module.clone() { - Some(ref module) if module.kind.get() == ModuleKind::NormalModuleKind => + Some(ref module) if module.is_normal() => ("existing submodule", "note conflicting module here"), - Some(ref module) if module.kind.get() == ModuleKind::TraitModuleKind => + Some(ref module) if module.is_trait() => ("trait in this module", "note conflicting trait here"), _ => ("type in this module", "note conflicting type here"), }; diff --git a/src/test/compile-fail/enum-and-module-in-same-scope.rs b/src/test/compile-fail/enum-and-module-in-same-scope.rs index f3d8fcf31d76c..67969616ca3c9 100644 --- a/src/test/compile-fail/enum-and-module-in-same-scope.rs +++ b/src/test/compile-fail/enum-and-module-in-same-scope.rs @@ -13,7 +13,7 @@ mod Foo { } enum Foo { //~ ERROR duplicate definition of type or module `Foo` - X //~ ERROR duplicate definition of value `X` + X } fn main() {} From 572c2f3e07fd0927d6c50a6604b9c3f2fa3ebb86 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Tue, 17 Nov 2015 09:10:41 +0000 Subject: [PATCH 3/4] Fix issue #21546 and refactor NsDef --- src/librustc_resolve/build_reduced_graph.rs | 172 +++----------------- src/librustc_resolve/lib.rs | 56 ++++--- src/librustc_resolve/resolve_imports.rs | 2 +- src/test/compile-fail/issue-21546.rs | 10 +- 4 files changed, 62 insertions(+), 178 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index b70349bfe94a1..b519a72991863 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -27,7 +27,6 @@ use resolve_imports::Shadowable; use {resolve_error, ResolutionError}; use self::DuplicateCheckingMode::*; -use self::NamespaceError::*; use rustc::metadata::csearch; use rustc::metadata::decoder::{DefLike, DlDef, DlField, DlImpl}; @@ -60,29 +59,12 @@ use std::rc::Rc; // another item exists with the same name in some namespace. #[derive(Copy, Clone, PartialEq)] enum DuplicateCheckingMode { - ForbidDuplicateModules, - ForbidDuplicateTypesAndModules, + ForbidDuplicateTypes, ForbidDuplicateValues, ForbidDuplicateTypesAndValues, OverwriteDuplicates, } -#[derive(Copy, Clone, PartialEq)] -enum NamespaceError { - NoError, - ModuleError, - TypeError, - ValueError, -} - -fn namespace_error_to_string(ns: NamespaceError) -> &'static str { - match ns { - NoError => "", - ModuleError | TypeError => "type or module", - ValueError => "value", - } -} - struct GraphBuilder<'a, 'b: 'a, 'tcx: 'b> { resolver: &'a mut Resolver<'b, 'tcx>, } @@ -112,16 +94,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { visit::walk_crate(&mut visitor, krate); } - /// Adds a new child item to the module definition of the parent node and - /// returns its corresponding name bindings as well as the current parent. - /// Or, if we're inside a block, creates (or reuses) an anonymous module - /// corresponding to the innermost block ID and returns the name bindings - /// as well as the newly-created parent. - /// - /// # Panics - /// - /// Panics if this node does not have a module definition and we are not inside - /// a block. + /// Adds a new child item to the module definition of the parent node, + /// or if there is already a child, does duplicate checking on the child. + /// Returns the child's corresponding name bindings. fn add_child(&self, name: Name, parent: &Rc, @@ -129,10 +104,6 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { // For printing errors sp: Span) -> NameBindings { - // If this is the immediate descendant of a module, then we add the - // child name directly. Otherwise, we create or reuse an anonymous - // module and add the child to that. - self.check_for_conflicts_between_external_crates_and_items(&**parent, name, sp); // Add or reuse the child. @@ -146,79 +117,33 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { Some(child) => { // Enforce the duplicate checking mode: // - // * If we're requesting duplicate module checking, check that - // there isn't a module in the module with the same name. - // // * If we're requesting duplicate type checking, check that - // there isn't a type in the module with the same name. + // the name isn't defined in the type namespace. // // * If we're requesting duplicate value checking, check that - // there isn't a value in the module with the same name. + // the name isn't defined in the value namespace. // - // * If we're requesting duplicate type checking and duplicate - // value checking, check that there isn't a duplicate type - // and a duplicate value with the same name. + // * If we're requesting duplicate type and value checking, + // check that the name isn't defined in either namespace. // // * If no duplicate checking was requested at all, do // nothing. - let mut duplicate_type = NoError; let ns = match duplicate_checking_mode { - ForbidDuplicateModules => { - if child.get_module_if_available().is_some() { - duplicate_type = ModuleError; - } - Some(TypeNS) - } - ForbidDuplicateTypesAndModules => { - if child.type_ns.defined() { - duplicate_type = TypeError; - } - Some(TypeNS) - } - ForbidDuplicateValues => { - if child.value_ns.defined() { - duplicate_type = ValueError; - } - Some(ValueNS) - } - ForbidDuplicateTypesAndValues => { - let mut n = None; - match child.type_ns.def() { - Some(DefMod(_)) | None => {} - Some(_) => { - n = Some(TypeNS); - duplicate_type = TypeError; - } - } - if child.value_ns.defined() { - duplicate_type = ValueError; - n = Some(ValueNS); - } - n - } - OverwriteDuplicates => None, + ForbidDuplicateTypes if child.type_ns.defined() => TypeNS, + ForbidDuplicateValues if child.value_ns.defined() => ValueNS, + ForbidDuplicateTypesAndValues if child.type_ns.defined() => TypeNS, + ForbidDuplicateTypesAndValues if child.value_ns.defined() => ValueNS, + _ => return child, }; - if duplicate_type != NoError { - // Return an error here by looking up the namespace that - // had the duplicate. - let ns = ns.unwrap(); - resolve_error( - self, - sp, - ResolutionError::DuplicateDefinition( - namespace_error_to_string(duplicate_type), - name) - ); - { - let r = child[ns].span(); - if let Some(sp) = r { - self.session.span_note(sp, - &format!("first definition of {} `{}` here", - namespace_error_to_string(duplicate_type), - name)); - } - } + + // Record an error here by looking up the namespace that had the duplicate + let ns_str = match ns { TypeNS => "type or module", ValueNS => "value" }; + resolve_error(self, sp, ResolutionError::DuplicateDefinition(ns_str, name)); + + if let Some(sp) = child[ns].span() { + let note = format!("first definition of {} `{}` here", ns_str, name); + self.session.span_note(sp, ¬e); } child } @@ -409,29 +334,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { } ItemMod(..) => { - let child = parent.children.borrow().get(&name).cloned(); - if let Some(child) = child { - // check if there's struct of the same name already defined - if child.type_ns.defined() && - child.get_module_if_available().is_none() { - self.session.span_warn(sp, - &format!("duplicate definition of {} `{}`. \ - Defining a module and a struct with \ - the same name will be disallowed soon.", - namespace_error_to_string(TypeError), - name)); - { - let r = child.type_ns.span(); - if let Some(sp) = r { - self.session.span_note(sp, - &format!("first definition of {} `{}` here", - namespace_error_to_string(TypeError), - name)); - } - } - } - } - let name_bindings = self.add_child(name, parent, ForbidDuplicateModules, sp); + let name_bindings = self.add_child(name, parent, ForbidDuplicateTypes, sp); let parent_link = self.get_parent_link(parent, name); let def = DefMod(self.ast_map.local_def_id(item.id)); @@ -469,7 +372,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ItemTy(..) => { let name_bindings = self.add_child(name, parent, - ForbidDuplicateTypesAndModules, + ForbidDuplicateTypes, sp); let parent_link = self.get_parent_link(parent, name); @@ -481,7 +384,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ItemEnum(ref enum_definition, _) => { let name_bindings = self.add_child(name, parent, - ForbidDuplicateTypesAndModules, + ForbidDuplicateTypes, sp); let parent_link = self.get_parent_link(parent, name); @@ -501,31 +404,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ItemStruct(ref struct_def, _) => { // Adding to both Type and Value namespaces or just Type? let (forbid, ctor_id) = if struct_def.is_struct() { - (ForbidDuplicateTypesAndModules, None) + (ForbidDuplicateTypes, None) } else { - let child = parent.children.borrow().get(&name).cloned(); - if let Some(child) = child { - // check if theres a DefMod - if let Some(DefMod(_)) = child.type_ns.def() { - self.session.span_warn(sp, - &format!("duplicate definition of {} `{}`. \ - Defining a module and a struct \ - with the same name will be \ - disallowed soon.", - namespace_error_to_string(TypeError), - name)); - { - let r = child.type_ns.span(); - if let Some(sp) = r { - self.session - .span_note(sp, - &format!("first definition of {} `{}` here", - namespace_error_to_string(TypeError), - name)); - } - } - } - } (ForbidDuplicateTypesAndValues, Some(struct_def.id())) }; @@ -566,7 +446,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { ItemTrait(_, _, _, ref items) => { let name_bindings = self.add_child(name, parent, - ForbidDuplicateTypesAndModules, + ForbidDuplicateTypes, sp); let def_id = self.ast_map.local_def_id(item.id); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 18e2b66d3fb2e..6a49db8a14665 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -904,17 +904,20 @@ bitflags! { } } -// Records a possibly-private definition. -// FIXME once #21546 is resolved, the def and module fields will never both be Some, -// so they can be refactored into something like Result>. +// Records a possibly-private value, type, or module definition. #[derive(Debug)] struct NsDef { modifiers: DefModifiers, // see note in ImportResolution about how to use this - def: Option, - module: Option>, + def_or_module: DefOrModule, span: Option, } +#[derive(Debug)] +enum DefOrModule { + Def(Def), + Module(Rc), +} + impl NsDef { fn create_from_module(module: Rc, span: Option) -> Self { let modifiers = if module.is_public { @@ -923,14 +926,24 @@ impl NsDef { DefModifiers::empty() } | DefModifiers::IMPORTABLE; - NsDef { modifiers: modifiers, def: None, module: Some(module), span: span } + NsDef { modifiers: modifiers, def_or_module: DefOrModule::Module(module), span: span } + } + + fn create_from_def(def: Def, modifiers: DefModifiers, span: Option) -> Self { + NsDef { modifiers: modifiers, def_or_module: DefOrModule::Def(def), span: span } + } + + fn module(&self) -> Option> { + match self.def_or_module { + DefOrModule::Module(ref module) => Some(module.clone()), + DefOrModule::Def(_) => None, + } } fn def(&self) -> Option { - match (self.def, &self.module) { - (def @ Some(_), _) => def, - (_, &Some(ref module)) => module.def.get(), - _ => panic!("NsDef has neither a Def nor a Module"), + match self.def_or_module { + DefOrModule::Def(def) => Some(def), + DefOrModule::Module(ref module) => module.def.get(), } } } @@ -964,10 +977,10 @@ impl NameBinding { fn borrow(&self) -> ::std::cell::Ref> { self.0.borrow() } - // Lifted versions of the NsDef fields and method + // Lifted versions of the NsDef methods and fields fn def(&self) -> Option { self.and_then(NsDef::def) } + fn module(&self) -> Option> { self.and_then(NsDef::module) } fn span(&self) -> Option { self.and_then(|def| def.span) } - fn module(&self) -> Option> { self.and_then(|def| def.module.clone()) } fn modifiers(&self) -> Option { self.and_then(|def| Some(def.modifiers)) } fn defined(&self) -> bool { self.borrow().is_some() } @@ -1029,23 +1042,14 @@ impl NameBindings { /// Records a type definition. fn define_type(&self, def: Def, sp: Span, modifiers: DefModifiers) { - debug!("defining type for def {:?} with modifiers {:?}", - def, - modifiers); - // Merges the type with the existing type def or creates a new one. - self.type_ns.set(NsDef { - modifiers: modifiers, def: Some(def), module: self.type_ns.module(), span: Some(sp) - }); + debug!("defining type for def {:?} with modifiers {:?}", def, modifiers); + self.type_ns.set(NsDef::create_from_def(def, modifiers, Some(sp))); } /// Records a value definition. fn define_value(&self, def: Def, sp: Span, modifiers: DefModifiers) { - debug!("defining value for def {:?} with modifiers {:?}", - def, - modifiers); - self.value_ns.set(NsDef { - modifiers: modifiers, def: Some(def), module: None, span: Some(sp) - }); + debug!("defining value for def {:?} with modifiers {:?}", def, modifiers); + self.value_ns.set(NsDef::create_from_def(def, modifiers, Some(sp))); } /// Returns the module node if applicable. @@ -1524,7 +1528,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.used_imports.insert((id, namespace)); self.record_import_use(id, name); if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() { - self.used_crates.insert(kid); + self.used_crates.insert(kid); } return Success((target, false)); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 14f8287a5f875..24a21bcdfb1b0 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -1039,7 +1039,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match import_resolution.type_target { Some(ref target) if target.shadowable != Shadowable::Always => { if let Some(ref ty) = *name_bindings.type_ns.borrow() { - let (what, note) = match ty.module.clone() { + let (what, note) = match ty.module() { Some(ref module) if module.is_normal() => ("existing submodule", "note conflicting module here"), Some(ref module) if module.is_trait() => diff --git a/src/test/compile-fail/issue-21546.rs b/src/test/compile-fail/issue-21546.rs index bb1bcd4e8717d..535630e0824ca 100644 --- a/src/test/compile-fail/issue-21546.rs +++ b/src/test/compile-fail/issue-21546.rs @@ -16,7 +16,7 @@ mod Foo { } #[allow(dead_code)] struct Foo; -//~^ WARNING duplicate definition of type or module `Foo` +//~^ ERROR duplicate definition of type or module `Foo` #[allow(non_snake_case)] @@ -25,7 +25,7 @@ mod Bar { } #[allow(dead_code)] struct Bar(i32); -//~^ WARNING duplicate definition of type or module `Bar` +//~^ ERROR duplicate definition of type or module `Bar` #[allow(dead_code)] @@ -34,7 +34,7 @@ struct Baz(i32); #[allow(non_snake_case)] mod Baz { } -//~^ WARNING duplicate definition of type or module `Baz` +//~^ ERROR duplicate definition of type or module `Baz` #[allow(dead_code)] @@ -43,7 +43,7 @@ struct Qux { x: bool } #[allow(non_snake_case)] mod Qux { } -//~^ WARNING duplicate definition of type or module `Qux` +//~^ ERROR duplicate definition of type or module `Qux` #[allow(dead_code)] @@ -52,7 +52,7 @@ struct Quux; #[allow(non_snake_case)] mod Quux { } -//~^ WARNING duplicate definition of type or module `Quux` +//~^ ERROR duplicate definition of type or module `Quux` #[allow(dead_code)] From 6a6e1dba55388bdf252e79eec2f084a45e0e862f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 18 Nov 2015 01:22:32 +0000 Subject: [PATCH 4/4] Refactor away get_module_if_available and get_module and reformat one-liners --- src/librustc_resolve/build_reduced_graph.rs | 49 +++++------- src/librustc_resolve/lib.rs | 85 ++++++++------------- src/librustc_resolve/record_exports.rs | 4 +- src/librustc_resolve/resolve_imports.rs | 6 +- 4 files changed, 56 insertions(+), 88 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index b519a72991863..c669e847bf2dd 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -86,10 +86,9 @@ impl<'a, 'b:'a, 'tcx:'b> DerefMut for GraphBuilder<'a, 'b, 'tcx> { impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { /// Constructs the reduced graph for the entire crate. fn build_reduced_graph(self, krate: &hir::Crate) { - let parent = self.graph_root.get_module(); let mut visitor = BuildReducedGraphVisitor { + parent: self.graph_root.clone(), builder: self, - parent: parent, }; visit::walk_crate(&mut visitor, krate); } @@ -318,10 +317,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { }; self.external_exports.insert(def_id); let parent_link = ModuleParentLink(Rc::downgrade(parent), name); - let external_module = Rc::new(Module::new(parent_link, - Some(DefMod(def_id)), - false, - true)); + let def = DefMod(def_id); + let external_module = Module::new(parent_link, Some(def), false, true); + debug!("(build reduced graph for item) found extern `{}`", module_to_string(&*external_module)); self.check_for_conflicts_between_external_crates(&**parent, name, sp); @@ -338,9 +336,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let parent_link = self.get_parent_link(parent, name); let def = DefMod(self.ast_map.local_def_id(item.id)); - name_bindings.define_module(parent_link, Some(def), false, is_public, sp); - - name_bindings.get_module() + let module = Module::new(parent_link, Some(def), false, is_public); + name_bindings.define_module(module.clone(), sp); + module } ItemForeignMod(..) => parent.clone(), @@ -377,7 +375,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let parent_link = self.get_parent_link(parent, name); let def = DefTy(self.ast_map.local_def_id(item.id), false); - name_bindings.define_module(parent_link, Some(def), false, is_public, sp); + let module = Module::new(parent_link, Some(def), false, is_public); + name_bindings.define_module(module, sp); parent.clone() } @@ -389,9 +388,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { let parent_link = self.get_parent_link(parent, name); let def = DefTy(self.ast_map.local_def_id(item.id), true); - name_bindings.define_module(parent_link, Some(def), false, is_public, sp); - - let module = name_bindings.get_module(); + let module = Module::new(parent_link, Some(def), false, is_public); + name_bindings.define_module(module.clone(), sp); for variant in &(*enum_definition).variants { let item_def_id = self.ast_map.local_def_id(item.id); @@ -454,8 +452,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { // Add all the items within to a new module. let parent_link = self.get_parent_link(parent, name); let def = DefTrait(def_id); - name_bindings.define_module(parent_link, Some(def), false, is_public, sp); - let module_parent = name_bindings.get_module(); + let module_parent = Module::new(parent_link, Some(def), false, is_public); + name_bindings.define_module(module_parent.clone(), sp); // Add the names of all the items to the trait info. for trait_item in items { @@ -555,10 +553,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { {}", block_id); - let new_module = Rc::new(Module::new(BlockParentLink(Rc::downgrade(parent), block_id), - None, - false, - false)); + let parent_link = BlockParentLink(Rc::downgrade(parent), block_id); + let new_module = Module::new(parent_link, None, false, false); parent.anonymous_children.borrow_mut().insert(block_id, new_module.clone()); new_module } else { @@ -604,12 +600,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { final_ident, is_public); let parent_link = self.get_parent_link(new_parent, name); - - child_name_bindings.define_module(parent_link, - Some(def), - true, - is_public, - DUMMY_SP); + let module = Module::new(parent_link, Some(def), true, is_public); + child_name_bindings.define_module(module, DUMMY_SP); } } _ => {} @@ -681,11 +673,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { // Define a module if necessary. let parent_link = self.get_parent_link(new_parent, name); - child_name_bindings.define_module(parent_link, - Some(def), - true, - is_public, - DUMMY_SP) + let module = Module::new(parent_link, Some(def), true, is_public); + child_name_bindings.define_module(module, DUMMY_SP); } DefTy(..) | DefAssociatedTy(..) => { debug!("(building reduced graph for external crate) building type {}", diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 6a49db8a14665..e9d714c5524e0 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -813,8 +813,8 @@ impl Module { def: Option, external: bool, is_public: bool) - -> Module { - Module { + -> Rc { + Rc::new(Module { parent_link: parent_link, def: Cell::new(def), is_public: is_public, @@ -828,7 +828,7 @@ impl Module { pub_glob_count: Cell::new(0), resolved_import_count: Cell::new(0), populated: Cell::new(!external), - } + }) } fn def_id(&self) -> Option { @@ -971,19 +971,27 @@ impl NameBinding { } } - fn and_then Option>(&self, f: F) -> Option { - self.borrow().as_ref().and_then(f) + fn borrow(&self) -> ::std::cell::Ref> { + self.0.borrow() } - fn borrow(&self) -> ::std::cell::Ref> { self.0.borrow() } - // Lifted versions of the NsDef methods and fields - fn def(&self) -> Option { self.and_then(NsDef::def) } - fn module(&self) -> Option> { self.and_then(NsDef::module) } - fn span(&self) -> Option { self.and_then(|def| def.span) } - fn modifiers(&self) -> Option { self.and_then(|def| Some(def.modifiers)) } + fn def(&self) -> Option { + self.borrow().as_ref().and_then(NsDef::def) + } + fn module(&self) -> Option> { + self.borrow().as_ref().and_then(NsDef::module) + } + fn span(&self) -> Option { + self.borrow().as_ref().and_then(|def| def.span) + } + fn modifiers(&self) -> Option { + self.borrow().as_ref().and_then(|def| Some(def.modifiers)) + } - fn defined(&self) -> bool { self.borrow().is_some() } + fn defined(&self) -> bool { + self.borrow().is_some() + } fn defined_with(&self, modifiers: DefModifiers) -> bool { self.modifiers().map(|m| m.contains(modifiers)).unwrap_or(false) @@ -1030,14 +1038,8 @@ impl NameBindings { } /// Creates a new module in this set of name bindings. - fn define_module(&self, - parent_link: ParentLink, - def: Option, - external: bool, - is_public: bool, - sp: Span) { - let module = Module::new(parent_link, def, external, is_public); - self.type_ns.set(NsDef::create_from_module(Rc::new(module), Some(sp))); + fn define_module(&self, module: Rc, sp: Span) { + self.type_ns.set(NsDef::create_from_module(module, Some(sp))); } /// Records a type definition. @@ -1051,20 +1053,6 @@ impl NameBindings { debug!("defining value for def {:?} with modifiers {:?}", def, modifiers); self.value_ns.set(NsDef::create_from_def(def, modifiers, Some(sp))); } - - /// Returns the module node if applicable. - fn get_module_if_available(&self) -> Option> { self.type_ns.module() } - - /// Returns the module node. Panics if this node does not have a module - /// definition. - fn get_module(&self) -> Rc { - match self.get_module_if_available() { - None => { - panic!("get_module called on a node with no module definition!") - } - Some(module_def) => module_def, - } - } } /// Interns the names of the primitive types. @@ -1106,7 +1094,7 @@ pub struct Resolver<'a, 'tcx: 'a> { ast_map: &'a hir_map::Map<'tcx>, - graph_root: NameBindings, + graph_root: Rc, trait_item_map: FnvHashMap<(Name, DefId), DefId>, @@ -1173,19 +1161,10 @@ enum FallbackChecks { impl<'a, 'tcx> Resolver<'a, 'tcx> { fn new(session: &'a Session, ast_map: &'a hir_map::Map<'tcx>, - crate_span: Span, make_glob_map: MakeGlobMap) -> Resolver<'a, 'tcx> { - let graph_root = NameBindings::new(); - let root_def_id = ast_map.local_def_id(CRATE_NODE_ID); - graph_root.define_module(NoParentLink, - Some(DefMod(root_def_id)), - false, - true, - crate_span); - - let current_module = graph_root.get_module(); + let graph_root = Module::new(NoParentLink, Some(DefMod(root_def_id)), false, true); Resolver { session: session, @@ -1194,14 +1173,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // The outermost module has def ID 0; this is not reflected in the // AST. - graph_root: graph_root, + graph_root: graph_root.clone(), trait_item_map: FnvHashMap(), structs: FnvHashMap(), unresolved_imports: 0, - current_module: current_module, + current_module: graph_root, value_ribs: Vec::new(), type_ribs: Vec::new(), label_ribs: Vec::new(), @@ -1441,7 +1420,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { DontUseLexicalScope => { // This is a crate-relative path. We will start the // resolution process at index zero. - search_module = self.graph_root.get_module(); + search_module = self.graph_root.clone(); start_index = 0; last_private = LastMod(AllPublic); } @@ -1792,7 +1771,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { build_reduced_graph::populate_module_if_necessary(self, &module_); for (_, child_node) in module_.children.borrow().iter() { - match child_node.get_module_if_available() { + match child_node.type_ns.module() { None => { // Continue. } @@ -1845,7 +1824,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { module_to_string(&*orig_module)); } Some(name_bindings) => { - match (*name_bindings).get_module_if_available() { + match name_bindings.type_ns.module() { None => { debug!("!!! (with scope) didn't find module for `{}` in `{}`", name, @@ -3115,7 +3094,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { .map(|ps| ps.identifier.name) .collect::>(); - let root_module = self.graph_root.get_module(); + let root_module = self.graph_root.clone(); let containing_module; let last_private; @@ -3278,7 +3257,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Some(_) => None, None => { match this.current_module.children.borrow().get(last_name) { - Some(child) => child.get_module_if_available(), + Some(child) => child.type_ns.module(), None => None, } } @@ -3883,7 +3862,7 @@ pub fn create_resolver<'a, 'tcx>(session: &'a Session, make_glob_map: MakeGlobMap, callback: Option bool>>) -> Resolver<'a, 'tcx> { - let mut resolver = Resolver::new(session, ast_map, krate.span, make_glob_map); + let mut resolver = Resolver::new(session, ast_map, make_glob_map); resolver.callback = callback; diff --git a/src/librustc_resolve/record_exports.rs b/src/librustc_resolve/record_exports.rs index 6e8d2ac4ca5c6..3a6a5a031b6a1 100644 --- a/src/librustc_resolve/record_exports.rs +++ b/src/librustc_resolve/record_exports.rs @@ -79,7 +79,7 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { build_reduced_graph::populate_module_if_necessary(self.resolver, &module_); for (_, child_name_bindings) in module_.children.borrow().iter() { - match child_name_bindings.get_module_if_available() { + match child_name_bindings.type_ns.module() { None => { // Nothing to do. } @@ -149,6 +149,6 @@ impl<'a, 'b, 'tcx> ExportRecorder<'a, 'b, 'tcx> { pub fn record(resolver: &mut Resolver) { let mut recorder = ExportRecorder { resolver: resolver }; - let root_module = recorder.graph_root.get_module(); + let root_module = recorder.graph_root.clone(); recorder.record_exports_for_module_subtree(root_module); } diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 24a21bcdfb1b0..4f67d6e2f7e79 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -209,7 +209,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { i, self.resolver.unresolved_imports); - let module_root = self.resolver.graph_root.get_module(); + let module_root = self.resolver.graph_root.clone(); let errors = self.resolve_imports_for_module_subtree(module_root.clone()); if self.resolver.unresolved_imports == 0 { @@ -254,7 +254,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { build_reduced_graph::populate_module_if_necessary(self.resolver, &module_); for (_, child_node) in module_.children.borrow().iter() { - match child_node.get_module_if_available() { + match child_node.type_ns.module() { None => { // Nothing to do. } @@ -337,7 +337,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // First, resolve the module path for the directive, if necessary. let container = if module_path.is_empty() { // Use the crate root. - Some((self.resolver.graph_root.get_module(), LastMod(AllPublic))) + Some((self.resolver.graph_root.clone(), LastMod(AllPublic))) } else { match self.resolver.resolve_module_path(module_.clone(), &module_path[..],