Skip to content

Commit 572c2f3

Browse files
jseyfriedjseyfried
committed
Fix issue rust-lang#21546 and refactor NsDef
1 parent 8a6187f commit 572c2f3

File tree

4 files changed

+62
-178
lines changed

4 files changed

+62
-178
lines changed

‎src/librustc_resolve/build_reduced_graph.rs

+26-146
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ use resolve_imports::Shadowable;
2727
use {resolve_error, ResolutionError};
2828

2929
use self::DuplicateCheckingMode::*;
30-
use self::NamespaceError::*;
3130

3231
use rustc::metadata::csearch;
3332
use rustc::metadata::decoder::{DefLike, DlDef, DlField, DlImpl};
@@ -60,29 +59,12 @@ use std::rc::Rc;
6059
// another item exists with the same name in some namespace.
6160
#[derive(Copy, Clone, PartialEq)]
6261
enum DuplicateCheckingMode {
63-
ForbidDuplicateModules,
64-
ForbidDuplicateTypesAndModules,
62+
ForbidDuplicateTypes,
6563
ForbidDuplicateValues,
6664
ForbidDuplicateTypesAndValues,
6765
OverwriteDuplicates,
6866
}
6967

70-
#[derive(Copy, Clone, PartialEq)]
71-
enum NamespaceError {
72-
NoError,
73-
ModuleError,
74-
TypeError,
75-
ValueError,
76-
}
77-
78-
fn namespace_error_to_string(ns: NamespaceError) -> &'static str {
79-
match ns {
80-
NoError => "",
81-
ModuleError | TypeError => "type or module",
82-
ValueError => "value",
83-
}
84-
}
85-
8668
struct GraphBuilder<'a, 'b: 'a, 'tcx: 'b> {
8769
resolver: &'a mut Resolver<'b, 'tcx>,
8870
}
@@ -112,27 +94,16 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
11294
visit::walk_crate(&mut visitor, krate);
11395
}
11496

115-
/// Adds a new child item to the module definition of the parent node and
116-
/// returns its corresponding name bindings as well as the current parent.
117-
/// Or, if we're inside a block, creates (or reuses) an anonymous module
118-
/// corresponding to the innermost block ID and returns the name bindings
119-
/// as well as the newly-created parent.
120-
///
121-
/// # Panics
122-
///
123-
/// Panics if this node does not have a module definition and we are not inside
124-
/// a block.
97+
/// Adds a new child item to the module definition of the parent node,
98+
/// or if there is already a child, does duplicate checking on the child.
99+
/// Returns the child's corresponding name bindings.
125100
fn add_child(&self,
126101
name: Name,
127102
parent: &Rc<Module>,
128103
duplicate_checking_mode: DuplicateCheckingMode,
129104
// For printing errors
130105
sp: Span)
131106
-> NameBindings {
132-
// If this is the immediate descendant of a module, then we add the
133-
// child name directly. Otherwise, we create or reuse an anonymous
134-
// module and add the child to that.
135-
136107
self.check_for_conflicts_between_external_crates_and_items(&**parent, name, sp);
137108

138109
// Add or reuse the child.
@@ -146,79 +117,33 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
146117
Some(child) => {
147118
// Enforce the duplicate checking mode:
148119
//
149-
// * If we're requesting duplicate module checking, check that
150-
// there isn't a module in the module with the same name.
151-
//
152120
// * If we're requesting duplicate type checking, check that
153-
// there isn't a type in the module with the same name.
121+
// the name isn't defined in the type namespace.
154122
//
155123
// * If we're requesting duplicate value checking, check that
156-
// there isn't a value in the module with the same name.
124+
// the name isn't defined in the value namespace.
157125
//
158-
// * If we're requesting duplicate type checking and duplicate
159-
// value checking, check that there isn't a duplicate type
160-
// and a duplicate value with the same name.
126+
// * If we're requesting duplicate type and value checking,
127+
// check that the name isn't defined in either namespace.
161128
//
162129
// * If no duplicate checking was requested at all, do
163130
// nothing.
164131

165-
let mut duplicate_type = NoError;
166132
let ns = match duplicate_checking_mode {
167-
ForbidDuplicateModules => {
168-
if child.get_module_if_available().is_some() {
169-
duplicate_type = ModuleError;
170-
}
171-
Some(TypeNS)
172-
}
173-
ForbidDuplicateTypesAndModules => {
174-
if child.type_ns.defined() {
175-
duplicate_type = TypeError;
176-
}
177-
Some(TypeNS)
178-
}
179-
ForbidDuplicateValues => {
180-
if child.value_ns.defined() {
181-
duplicate_type = ValueError;
182-
}
183-
Some(ValueNS)
184-
}
185-
ForbidDuplicateTypesAndValues => {
186-
let mut n = None;
187-
match child.type_ns.def() {
188-
Some(DefMod(_)) | None => {}
189-
Some(_) => {
190-
n = Some(TypeNS);
191-
duplicate_type = TypeError;
192-
}
193-
}
194-
if child.value_ns.defined() {
195-
duplicate_type = ValueError;
196-
n = Some(ValueNS);
197-
}
198-
n
199-
}
200-
OverwriteDuplicates => None,
133+
ForbidDuplicateTypes if child.type_ns.defined() => TypeNS,
134+
ForbidDuplicateValues if child.value_ns.defined() => ValueNS,
135+
ForbidDuplicateTypesAndValues if child.type_ns.defined() => TypeNS,
136+
ForbidDuplicateTypesAndValues if child.value_ns.defined() => ValueNS,
137+
_ => return child,
201138
};
202-
if duplicate_type != NoError {
203-
// Return an error here by looking up the namespace that
204-
// had the duplicate.
205-
let ns = ns.unwrap();
206-
resolve_error(
207-
self,
208-
sp,
209-
ResolutionError::DuplicateDefinition(
210-
namespace_error_to_string(duplicate_type),
211-
name)
212-
);
213-
{
214-
let r = child[ns].span();
215-
if let Some(sp) = r {
216-
self.session.span_note(sp,
217-
&format!("first definition of {} `{}` here",
218-
namespace_error_to_string(duplicate_type),
219-
name));
220-
}
221-
}
139+
140+
// Record an error here by looking up the namespace that had the duplicate
141+
let ns_str = match ns { TypeNS => "type or module", ValueNS => "value" };
142+
resolve_error(self, sp, ResolutionError::DuplicateDefinition(ns_str, name));
143+
144+
if let Some(sp) = child[ns].span() {
145+
let note = format!("first definition of {} `{}` here", ns_str, name);
146+
self.session.span_note(sp, &note);
222147
}
223148
child
224149
}
@@ -409,29 +334,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
409334
}
410335

411336
ItemMod(..) => {
412-
let child = parent.children.borrow().get(&name).cloned();
413-
if let Some(child) = child {
414-
// check if there's struct of the same name already defined
415-
if child.type_ns.defined() &&
416-
child.get_module_if_available().is_none() {
417-
self.session.span_warn(sp,
418-
&format!("duplicate definition of {} `{}`. \
419-
Defining a module and a struct with \
420-
the same name will be disallowed soon.",
421-
namespace_error_to_string(TypeError),
422-
name));
423-
{
424-
let r = child.type_ns.span();
425-
if let Some(sp) = r {
426-
self.session.span_note(sp,
427-
&format!("first definition of {} `{}` here",
428-
namespace_error_to_string(TypeError),
429-
name));
430-
}
431-
}
432-
}
433-
}
434-
let name_bindings = self.add_child(name, parent, ForbidDuplicateModules, sp);
337+
let name_bindings = self.add_child(name, parent, ForbidDuplicateTypes, sp);
435338

436339
let parent_link = self.get_parent_link(parent, name);
437340
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> {
469372
ItemTy(..) => {
470373
let name_bindings = self.add_child(name,
471374
parent,
472-
ForbidDuplicateTypesAndModules,
375+
ForbidDuplicateTypes,
473376
sp);
474377

475378
let parent_link = self.get_parent_link(parent, name);
@@ -481,7 +384,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
481384
ItemEnum(ref enum_definition, _) => {
482385
let name_bindings = self.add_child(name,
483386
parent,
484-
ForbidDuplicateTypesAndModules,
387+
ForbidDuplicateTypes,
485388
sp);
486389

487390
let parent_link = self.get_parent_link(parent, name);
@@ -501,31 +404,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
501404
ItemStruct(ref struct_def, _) => {
502405
// Adding to both Type and Value namespaces or just Type?
503406
let (forbid, ctor_id) = if struct_def.is_struct() {
504-
(ForbidDuplicateTypesAndModules, None)
407+
(ForbidDuplicateTypes, None)
505408
} else {
506-
let child = parent.children.borrow().get(&name).cloned();
507-
if let Some(child) = child {
508-
// check if theres a DefMod
509-
if let Some(DefMod(_)) = child.type_ns.def() {
510-
self.session.span_warn(sp,
511-
&format!("duplicate definition of {} `{}`. \
512-
Defining a module and a struct \
513-
with the same name will be \
514-
disallowed soon.",
515-
namespace_error_to_string(TypeError),
516-
name));
517-
{
518-
let r = child.type_ns.span();
519-
if let Some(sp) = r {
520-
self.session
521-
.span_note(sp,
522-
&format!("first definition of {} `{}` here",
523-
namespace_error_to_string(TypeError),
524-
name));
525-
}
526-
}
527-
}
528-
}
529409
(ForbidDuplicateTypesAndValues, Some(struct_def.id()))
530410
};
531411

@@ -566,7 +446,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
566446
ItemTrait(_, _, _, ref items) => {
567447
let name_bindings = self.add_child(name,
568448
parent,
569-
ForbidDuplicateTypesAndModules,
449+
ForbidDuplicateTypes,
570450
sp);
571451

572452
let def_id = self.ast_map.local_def_id(item.id);

‎src/librustc_resolve/lib.rs

+30-26
Original file line numberDiff line numberDiff line change
@@ -904,17 +904,20 @@ bitflags! {
904904
}
905905
}
906906

907-
// Records a possibly-private definition.
908-
// FIXME once #21546 is resolved, the def and module fields will never both be Some,
909-
// so they can be refactored into something like Result<Def, Rc<Module>>.
907+
// Records a possibly-private value, type, or module definition.
910908
#[derive(Debug)]
911909
struct NsDef {
912910
modifiers: DefModifiers, // see note in ImportResolution about how to use this
913-
def: Option<Def>,
914-
module: Option<Rc<Module>>,
911+
def_or_module: DefOrModule,
915912
span: Option<Span>,
916913
}
917914

915+
#[derive(Debug)]
916+
enum DefOrModule {
917+
Def(Def),
918+
Module(Rc<Module>),
919+
}
920+
918921
impl NsDef {
919922
fn create_from_module(module: Rc<Module>, span: Option<Span>) -> Self {
920923
let modifiers = if module.is_public {
@@ -923,14 +926,24 @@ impl NsDef {
923926
DefModifiers::empty()
924927
} | DefModifiers::IMPORTABLE;
925928

926-
NsDef { modifiers: modifiers, def: None, module: Some(module), span: span }
929+
NsDef { modifiers: modifiers, def_or_module: DefOrModule::Module(module), span: span }
930+
}
931+
932+
fn create_from_def(def: Def, modifiers: DefModifiers, span: Option<Span>) -> Self {
933+
NsDef { modifiers: modifiers, def_or_module: DefOrModule::Def(def), span: span }
934+
}
935+
936+
fn module(&self) -> Option<Rc<Module>> {
937+
match self.def_or_module {
938+
DefOrModule::Module(ref module) => Some(module.clone()),
939+
DefOrModule::Def(_) => None,
940+
}
927941
}
928942

929943
fn def(&self) -> Option<Def> {
930-
match (self.def, &self.module) {
931-
(def @ Some(_), _) => def,
932-
(_, &Some(ref module)) => module.def.get(),
933-
_ => panic!("NsDef has neither a Def nor a Module"),
944+
match self.def_or_module {
945+
DefOrModule::Def(def) => Some(def),
946+
DefOrModule::Module(ref module) => module.def.get(),
934947
}
935948
}
936949
}
@@ -964,10 +977,10 @@ impl NameBinding {
964977

965978
fn borrow(&self) -> ::std::cell::Ref<Option<NsDef>> { self.0.borrow() }
966979

967-
// Lifted versions of the NsDef fields and method
980+
// Lifted versions of the NsDef methods and fields
968981
fn def(&self) -> Option<Def> { self.and_then(NsDef::def) }
982+
fn module(&self) -> Option<Rc<Module>> { self.and_then(NsDef::module) }
969983
fn span(&self) -> Option<Span> { self.and_then(|def| def.span) }
970-
fn module(&self) -> Option<Rc<Module>> { self.and_then(|def| def.module.clone()) }
971984
fn modifiers(&self) -> Option<DefModifiers> { self.and_then(|def| Some(def.modifiers)) }
972985

973986
fn defined(&self) -> bool { self.borrow().is_some() }
@@ -1029,23 +1042,14 @@ impl NameBindings {
10291042

10301043
/// Records a type definition.
10311044
fn define_type(&self, def: Def, sp: Span, modifiers: DefModifiers) {
1032-
debug!("defining type for def {:?} with modifiers {:?}",
1033-
def,
1034-
modifiers);
1035-
// Merges the type with the existing type def or creates a new one.
1036-
self.type_ns.set(NsDef {
1037-
modifiers: modifiers, def: Some(def), module: self.type_ns.module(), span: Some(sp)
1038-
});
1045+
debug!("defining type for def {:?} with modifiers {:?}", def, modifiers);
1046+
self.type_ns.set(NsDef::create_from_def(def, modifiers, Some(sp)));
10391047
}
10401048

10411049
/// Records a value definition.
10421050
fn define_value(&self, def: Def, sp: Span, modifiers: DefModifiers) {
1043-
debug!("defining value for def {:?} with modifiers {:?}",
1044-
def,
1045-
modifiers);
1046-
self.value_ns.set(NsDef {
1047-
modifiers: modifiers, def: Some(def), module: None, span: Some(sp)
1048-
});
1051+
debug!("defining value for def {:?} with modifiers {:?}", def, modifiers);
1052+
self.value_ns.set(NsDef::create_from_def(def, modifiers, Some(sp)));
10491053
}
10501054

10511055
/// Returns the module node if applicable.
@@ -1524,7 +1528,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
15241528
self.used_imports.insert((id, namespace));
15251529
self.record_import_use(id, name);
15261530
if let Some(DefId{krate: kid, ..}) = target.target_module.def_id() {
1527-
self.used_crates.insert(kid);
1531+
self.used_crates.insert(kid);
15281532
}
15291533
return Success((target, false));
15301534
}

‎src/librustc_resolve/resolve_imports.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1039,7 +1039,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
10391039
match import_resolution.type_target {
10401040
Some(ref target) if target.shadowable != Shadowable::Always => {
10411041
if let Some(ref ty) = *name_bindings.type_ns.borrow() {
1042-
let (what, note) = match ty.module.clone() {
1042+
let (what, note) = match ty.module() {
10431043
Some(ref module) if module.is_normal() =>
10441044
("existing submodule", "note conflicting module here"),
10451045
Some(ref module) if module.is_trait() =>

0 commit comments

Comments
 (0)