From 572c2f3e07fd0927d6c50a6604b9c3f2fa3ebb86 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Tue, 17 Nov 2015 09:10:41 +0000 Subject: [PATCH] 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 b70349bfe94..b519a729918 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -27,7 +27,6 @@ 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 @@ // 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 @@ fn build_reduced_graph(self, krate: &hir::Crate) { 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 @@ fn add_child(&self, // 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 @@ fn add_child(&self, 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 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc) -> } 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 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc) -> 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 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc) -> 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 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc) -> 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 @@ fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc) -> 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 18e2b66d3fb..6a49db8a146 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -904,17 +904,20 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { } } -// 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 @@ fn create_from_module(module: Rc, span: Option) -> Self { 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 @@ fn and_then Option>(&self, f: F) -> Option { 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 @@ fn define_module(&self, /// 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 @@ fn resolve_item_in_lexical_scope(&mut self, 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 14f8287a5f8..24a21bcdfb1 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -1039,7 +1039,7 @@ fn check_for_conflicts_between_imports_and_items(&mut self, 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 bb1bcd4e871..535630e0824 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 @@ mod Bar { } #[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 @@ mod Qux { } #[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)] -- 2.44.0