Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Resolve: refactor away the field Module::external_module_children #31317

Merged
merged 1 commit into from
Feb 1, 2016
Merged

Resolve: refactor away the field Module::external_module_children #31317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,20 +107,37 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
/// Defines `name` in namespace `ns` of module `parent` to be `def` if it is not yet defined;
/// otherwise, reports an error.
fn define<T: ToNameBinding<'b>>(&self, parent: Module<'b>, name: Name, ns: Namespace, def: T) {
let name_binding = def.to_name_binding();
let span = name_binding.span.unwrap_or(DUMMY_SP);
self.check_for_conflicts_between_external_crates_and_items(&parent, name, span);
if !parent.try_define_child(name, ns, name_binding) {
let binding = def.to_name_binding();
let old_binding = match parent.try_define_child(name, ns, binding.clone()) {
Some(old_binding) => old_binding,
None => return,
};

let span = binding.span.unwrap_or(DUMMY_SP);
if !old_binding.is_extern_crate() && !binding.is_extern_crate() {
// Record an error here by looking up the namespace that had the duplicate
let ns_str = match ns { TypeNS => "type or module", ValueNS => "value" };
let resolution_error = ResolutionError::DuplicateDefinition(ns_str, name);
let mut err = resolve_struct_error(self, span, resolution_error);

if let Some(sp) = parent.children.borrow().get(&(name, ns)).unwrap().span {
if let Some(sp) = old_binding.span {
let note = format!("first definition of {} `{}` here", ns_str, name);
err.span_note(sp, &note);
}
err.emit();
} else if old_binding.is_extern_crate() && binding.is_extern_crate() {
span_err!(self.session,
span,
E0259,
"an external crate named `{}` has already been imported into this module",
name);
} else {
span_err!(self.session,
span,
E0260,
"the name `{}` conflicts with an external crate \
that has been imported into this module",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, if a non-crate is defined first and a crate is defined second, the error message will be incorrect.

I think both theelse cases above would be better moved from here and handled by resolve_struct_error and ResolutionError::DuplicateDefinition.
The error messages will have to be tweaked though, something along the lines:
"definition of {kind} {name} conflicts with existing definition of {kind} {name}" where {kind} is a precise item kind (including "external crate") and not just "type or module"/"value".
multispans can be used to report spans of the both conflicting items at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'm planning on cleaning up the duplicate errors in a separate PR so that it can be reviewed and discussed independently of this refactoring (unless you would like do that instead).
Thanks for the pointer to multispans -- this looks like a good use case for them.

name);
}
}

Expand Down Expand Up @@ -289,14 +306,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
self.external_exports.insert(def_id);
let parent_link = ModuleParentLink(parent, name);
let def = Def::Mod(def_id);
let external_module = self.new_module(parent_link, Some(def), false, true);

debug!("(build reduced graph for item) found extern `{}`",
module_to_string(&*external_module));
self.check_for_conflicts_for_external_crate(parent, name, sp);
parent.external_module_children
.borrow_mut()
.insert(name, external_module);
let external_module = self.new_extern_crate_module(parent_link, def);
self.define(parent, name, TypeNS, (external_module, sp));

self.build_reduced_graph_for_external_crate(&external_module);
}
parent
Expand Down
104 changes: 32 additions & 72 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@ enum SuggestionType {
}

pub enum ResolutionError<'a> {
/// error E0260: name conflicts with an extern crate
NameConflictsWithExternCrate(Name),
/// error E0401: can't use type parameters from outer function
TypeParametersFromOuterFunction,
/// error E0402: cannot use an outer type parameter in this context
Expand Down Expand Up @@ -228,14 +226,6 @@ fn resolve_struct_error<'b, 'a: 'b, 'tcx: 'a>(resolver: &'b Resolver<'a, 'tcx>,
}

match resolution_error {
ResolutionError::NameConflictsWithExternCrate(name) => {
struct_span_err!(resolver.session,
span,
E0260,
"the name `{}` conflicts with an external crate \
that has been imported into this module",
name)
}
ResolutionError::TypeParametersFromOuterFunction => {
struct_span_err!(resolver.session,
span,
Expand Down Expand Up @@ -801,14 +791,11 @@ pub struct ModuleS<'a> {
parent_link: ParentLink<'a>,
def: Cell<Option<Def>>,
is_public: bool,
is_extern_crate: bool,

children: RefCell<HashMap<(Name, Namespace), NameBinding<'a>>>,
imports: RefCell<Vec<ImportDirective>>,

// The external module children of this node that were declared with
// `extern crate`.
external_module_children: RefCell<HashMap<Name, Module<'a>>>,

// The anonymous children of this node. Anonymous children are pseudo-
// modules that are implicitly created around items contained within
// blocks.
Expand Down Expand Up @@ -854,9 +841,9 @@ impl<'a> ModuleS<'a> {
parent_link: parent_link,
def: Cell::new(def),
is_public: is_public,
is_extern_crate: false,
children: RefCell::new(HashMap::new()),
imports: RefCell::new(Vec::new()),
external_module_children: RefCell::new(HashMap::new()),
anonymous_children: RefCell::new(NodeMap()),
import_resolutions: RefCell::new(HashMap::new()),
glob_count: Cell::new(0),
Expand All @@ -871,10 +858,21 @@ impl<'a> ModuleS<'a> {
self.children.borrow().get(&(name, ns)).cloned()
}

fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>) -> bool {
// If the name is not yet defined, define the name and return None.
// Otherwise, return the existing definition.
fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>)
-> Option<NameBinding<'a>> {
match self.children.borrow_mut().entry((name, ns)) {
hash_map::Entry::Vacant(entry) => { entry.insert(binding); true }
hash_map::Entry::Occupied(_) => false,
hash_map::Entry::Vacant(entry) => { entry.insert(binding); None }
hash_map::Entry::Occupied(entry) => { Some(entry.get().clone()) },
}
}

fn for_each_local_child<F: FnMut(Name, Namespace, &NameBinding<'a>)>(&self, mut f: F) {
for (&(name, ns), name_binding) in self.children.borrow().iter() {
if !name_binding.is_extern_crate() {
f(name, ns, name_binding)
}
}
}

Expand Down Expand Up @@ -1005,6 +1003,10 @@ impl<'a> NameBinding<'a> {
let def = self.def().unwrap();
(def, LastMod(if self.is_public() { AllPublic } else { DependsOn(def.def_id()) }))
}

fn is_extern_crate(&self) -> bool {
self.module().map(|module| module.is_extern_crate).unwrap_or(false)
}
}

/// Interns the names of the primitive types.
Expand Down Expand Up @@ -1184,6 +1186,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
self.arenas.modules.alloc(ModuleS::new(parent_link, def, external, is_public))
}

fn new_extern_crate_module(&self, parent_link: ParentLink<'a>, def: Def) -> Module<'a> {
let mut module = ModuleS::new(parent_link, Some(def), false, true);
module.is_extern_crate = true;
self.arenas.modules.alloc(module)
}

fn get_ribs<'b>(&'b mut self, ns: Namespace) -> &'b mut Vec<Rib<'a>> {
match ns { ValueNS => &mut self.value_ribs, TypeNS => &mut self.type_ribs }
}
Expand Down Expand Up @@ -1211,32 +1219,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}

/// Check that an external crate doesn't collide with items or other external crates.
fn check_for_conflicts_for_external_crate(&self, module: Module<'a>, name: Name, span: Span) {
if module.external_module_children.borrow().contains_key(&name) {
span_err!(self.session,
span,
E0259,
"an external crate named `{}` has already been imported into this module",
name);
}
if let Some(name_binding) = module.get_child(name, TypeNS) {
resolve_error(self,
name_binding.span.unwrap_or(codemap::DUMMY_SP),
ResolutionError::NameConflictsWithExternCrate(name));
}
}

/// Checks that the names of items don't collide with external crates.
fn check_for_conflicts_between_external_crates_and_items(&self,
module: Module<'a>,
name: Name,
span: Span) {
if module.external_module_children.borrow().contains_key(&name) {
resolve_error(self, span, ResolutionError::NameConflictsWithExternCrate(name));
}
}

/// Resolves the given module path from the given root `module_`.
fn resolve_module_path_from_root(&mut self,
module_: Module<'a>,
Expand All @@ -1245,11 +1227,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
span: Span,
lp: LastPrivate)
-> ResolveResult<(Module<'a>, LastPrivate)> {
fn search_parent_externals<'a>(needle: Name, module: Module<'a>)
-> Option<Module<'a>> {
match module.external_module_children.borrow().get(&needle) {
Some(_) => Some(module),
None => match module.parent_link {
fn search_parent_externals<'a>(needle: Name, module: Module<'a>) -> Option<Module<'a>> {
match module.get_child(needle, TypeNS) {
Some(ref binding) if binding.is_extern_crate() => Some(module),
_ => match module.parent_link {
ModuleParentLink(ref parent, _) => {
search_parent_externals(needle, parent)
}
Expand Down Expand Up @@ -1480,17 +1461,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}

// Search for external modules.
if namespace == TypeNS {
let children = module_.external_module_children.borrow();
if let Some(module) = children.get(&name) {
let name_binding = NameBinding::create_from_module(module, None);
debug!("lower name bindings succeeded");
return Success((Target::new(module_, name_binding, Shadowable::Never),
false));
}
}

// Finally, proceed up the scope chain looking for parent modules.
let mut search_module = module_;
loop {
Expand Down Expand Up @@ -1684,16 +1654,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
Some(..) | None => {} // Continue.
}

// Finally, search through external children.
if namespace == TypeNS {
let children = module_.external_module_children.borrow();
if let Some(module) = children.get(&name) {
let name_binding = NameBinding::create_from_module(module, None);
return Success((Target::new(module_, name_binding, Shadowable::Never),
false));
}
}

// We're out of luck.
debug!("(resolving name in module) failed to resolve `{}`", name);
return Failed(None);
Expand All @@ -1712,7 +1672,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
// Descend into children and anonymous children.
build_reduced_graph::populate_module_if_necessary(self, &module_);

for (_, child_node) in module_.children.borrow().iter() {
module_.for_each_local_child(|_, _, child_node| {
match child_node.module() {
None => {
// Continue.
Expand All @@ -1721,7 +1681,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
self.report_unresolved_imports(child_module);
}
}
}
});

for (_, module_) in module_.anonymous_children.borrow().iter() {
self.report_unresolved_imports(module_);
Expand Down
46 changes: 17 additions & 29 deletions src/librustc_resolve/resolve_imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
self.resolver.current_module = orig_module;

build_reduced_graph::populate_module_if_necessary(self.resolver, &module_);
for (_, child_node) in module_.children.borrow().iter() {
module_.for_each_local_child(|_, _, child_node| {
match child_node.module() {
None => {
// Nothing to do.
Expand All @@ -222,7 +222,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
errors.extend(self.resolve_imports_for_module_subtree(child_module));
}
}
}
});

for (_, child_module) in module_.anonymous_children.borrow().iter() {
errors.extend(self.resolve_imports_for_module_subtree(child_module));
Expand Down Expand Up @@ -386,18 +386,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
-> (ResolveResult<(Module<'b>, NameBinding<'b>)>, bool) {
build_reduced_graph::populate_module_if_necessary(self.resolver, module);
if let Some(name_binding) = module.get_child(name, ns) {
return (Success((module, name_binding)), false);
}

if ns == TypeNS {
if let Some(extern_crate) = module.external_module_children.borrow().get(&name) {
if name_binding.is_extern_crate() {
// track the extern crate as used.
if let Some(DefId{ krate: kid, .. }) = extern_crate.def_id() {
self.resolver.used_crates.insert(kid);
if let Some(DefId { krate, .. }) = name_binding.module().unwrap().def_id() {
self.resolver.used_crates.insert(krate);
}
let name_binding = NameBinding::create_from_module(extern_crate, None);
return (Success((module, name_binding)), false);
}
return (Success((module, name_binding)), false)
}

// If there is an unresolved glob at this point in the containing module, bail out.
Expand Down Expand Up @@ -725,13 +720,13 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
// Add all children from the containing module.
build_reduced_graph::populate_module_if_necessary(self.resolver, &target_module);

for (&name, name_binding) in target_module.children.borrow().iter() {
target_module.for_each_local_child(|name, ns, name_binding| {
self.merge_import_resolution(module_,
target_module,
import_directive,
name,
(name, ns),
name_binding.clone());
}
});

// Record the destination of this import
if let Some(did) = target_module.def_id() {
Expand Down Expand Up @@ -881,21 +876,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
import: &ImportResolution<'b>,
import_span: Span,
(name, ns): (Name, Namespace)) {
// First, check for conflicts between imports and `extern crate`s.
if ns == TypeNS {
if module.external_module_children.borrow().contains_key(&name) {
match import.target {
Some(ref target) if target.shadowable != Shadowable::Always => {
let msg = format!("import `{0}` conflicts with imported crate \
in this module (maybe you meant `use {0}::*`?)",
name);
span_err!(self.resolver.session, import_span, E0254, "{}", &msg[..]);
}
Some(_) | None => {}
}
}
}

// Check for item conflicts.
let name_binding = match module.get_child(name, ns) {
None => {
Expand Down Expand Up @@ -924,6 +904,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
} else {
match import.target {
Some(ref target) if target.shadowable != Shadowable::Always => {
if name_binding.is_extern_crate() {
let msg = format!("import `{0}` conflicts with imported crate \
in this module (maybe you meant `use {0}::*`?)",
name);
span_err!(self.resolver.session, import_span, E0254, "{}", &msg[..]);
return;
}

let (what, note) = match name_binding.module() {
Some(ref module) if module.is_normal() =>
("existing submodule", "note conflicting module here"),
Expand Down
3 changes: 2 additions & 1 deletion src/test/compile-fail/resolve-conflict-item-vs-extern-crate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn std() {} //~ ERROR the name `std` conflicts with an external crate
fn std() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does fn std no longer conflict? Seems like it should to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn std no longer conflicts since it defines std in the value namespace but the extern crate defines std in the type namespace.

Right now, fn std not does conflict with mod std { ... } for the same reason and I see no reason to treat extern crate std differently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, seems fine, thanks for the explanation

mod std {} //~ ERROR the name `std` conflicts with an external crate

fn main() {
}