Skip to content

Commit

Permalink
Auto merge of rust-lang#32328 - jseyfried:coherence, r=nikomatsakis
Browse files Browse the repository at this point in the history 
resolve: Improve import failure detection and lay groundwork for RFC 1422

This PR improves import failure detection and lays some groundwork for RFC 1422.
More specifically, it
 - Avoids recomputing the resolution of an import directive's module path.
 - Refactors code in `resolve_imports` that does not scale to the arbitrarily many levels of visibility that will be required by RFC 1422.
  - Replaces `ModuleS`'s fields `public_glob_count`, `private_glob_count`, and `resolved_globs` with a list of glob import directives `globs`.
  - Replaces `NameResolution`'s fields `pub_outstanding_references` and `outstanding_references` with a field `single_imports` of a newly defined type `SingleImports`.
 - Improves import failure detection by detecting cycles that include single imports (currently, only cycles of globs are detected). This fixes rust-lang#32119.

r? @nikomatsakis
  • Loading branch information
@bors
bors committed Apr 5, 2016
2 parents 600dc35 + 6f09dea commit 7fd331e
Show file tree
Hide file tree
Showing 4 changed files with 207 additions and 207 deletions.
50 changes: 8 additions & 42 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
//! any imports resolved.
use DefModifiers;
use resolve_imports::ImportDirective;
use resolve_imports::ImportDirectiveSubclass::{self, SingleImport, GlobImport};
use resolve_imports::ImportDirectiveSubclass::{self, GlobImport};
use Module;
use Namespace::{self, TypeNS, ValueNS};
use {NameBinding, NameBindingKind};
Expand All @@ -28,7 +27,7 @@ use rustc::middle::def::*;
use rustc::middle::def_id::{CRATE_DEF_INDEX, DefId};
use rustc::ty::VariantKind;

use syntax::ast::{Name, NodeId};
use syntax::ast::Name;
use syntax::attr::AttrMetaMethods;
use syntax::parse::token::special_idents;
use syntax::codemap::{Span, DUMMY_SP};
Expand Down Expand Up @@ -152,8 +151,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
}

let subclass = ImportDirectiveSubclass::single(binding, source_name);
self.build_import_directive(parent,
module_path,
self.unresolved_imports += 1;
parent.add_import_directive(module_path,
subclass,
view_path.span,
item.id,
Expand Down Expand Up @@ -203,8 +202,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
}
};
let subclass = ImportDirectiveSubclass::single(rename, name);
self.build_import_directive(parent,
module_path,
self.unresolved_imports += 1;
parent.add_import_directive(module_path,
subclass,
source_item.span,
source_item.node.id(),
Expand All @@ -213,8 +212,8 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
}
}
ViewPathGlob(_) => {
self.build_import_directive(parent,
module_path,
self.unresolved_imports += 1;
parent.add_import_directive(module_path,
GlobImport,
view_path.span,
item.id,
Expand Down Expand Up @@ -521,39 +520,6 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
}
}

/// Creates and adds an import directive to the given module.
fn build_import_directive(&mut self,
module_: Module<'b>,
module_path: Vec<Name>,
subclass: ImportDirectiveSubclass,
span: Span,
id: NodeId,
is_public: bool,
is_prelude: bool) {
// Bump the reference count on the name. Or, if this is a glob, set
// the appropriate flag.

match subclass {
SingleImport { target, .. } => {
module_.increment_outstanding_references_for(target, ValueNS, is_public);
module_.increment_outstanding_references_for(target, TypeNS, is_public);
}
GlobImport if !is_prelude => {
// Set the glob flag. This tells us that we don't know the
// module's exports ahead of time.
module_.inc_glob_count(is_public)
}
// Prelude imports are not included in the glob counts since they do not get added to
// `resolved_globs` -- they are handled separately in `resolve_imports`.
GlobImport => {}
}

let directive =
ImportDirective::new(module_path, subclass, span, id, is_public, is_prelude);
module_.add_import_directive(directive);
self.unresolved_imports += 1;
}

/// Ensures that the reduced graph rooted at the given external module
/// is built, building it if it is not.
pub fn populate_module_if_necessary(&mut self, module: Module<'b>) {
Expand Down
40 changes: 14 additions & 26 deletions src/librustc_resolve/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,8 +828,8 @@ pub struct ModuleS<'a> {
// is the NodeId of the local `extern crate` item (otherwise, `extern_crate_id` is None).
extern_crate_id: Option<NodeId>,

resolutions: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
unresolved_imports: RefCell<Vec<&'a ImportDirective>>,
resolutions: RefCell<HashMap<(Name, Namespace), &'a RefCell<NameResolution<'a>>>>,
unresolved_imports: RefCell<Vec<&'a ImportDirective<'a>>>,

// The module children of this node, including normal modules and anonymous modules.
// Anonymous children are pseudo-modules that are implicitly created around items
Expand All @@ -849,14 +849,8 @@ pub struct ModuleS<'a> {

prelude: RefCell<Option<Module<'a>>>,

glob_importers: RefCell<Vec<(Module<'a>, &'a ImportDirective)>>,
resolved_globs: RefCell<(Vec<Module<'a>> /* public */, Vec<Module<'a>> /* private */)>,

// The number of public glob imports in this module.
public_glob_count: Cell<usize>,

// The number of private glob imports in this module.
private_glob_count: Cell<usize>,
glob_importers: RefCell<Vec<(Module<'a>, &'a ImportDirective<'a>)>>,
globs: RefCell<Vec<&'a ImportDirective<'a>>>,

// Whether this module is populated. If not populated, any attempt to
// access the children must be preceded with a
Expand Down Expand Up @@ -884,22 +878,15 @@ impl<'a> ModuleS<'a> {
module_children: RefCell::new(NodeMap()),
prelude: RefCell::new(None),
glob_importers: RefCell::new(Vec::new()),
resolved_globs: RefCell::new((Vec::new(), Vec::new())),
public_glob_count: Cell::new(0),
private_glob_count: Cell::new(0),
globs: RefCell::new((Vec::new())),
populated: Cell::new(!external),
arenas: arenas
}
}

fn add_import_directive(&self, import_directive: ImportDirective) {
let import_directive = self.arenas.alloc_import_directive(import_directive);
self.unresolved_imports.borrow_mut().push(import_directive);
}

fn for_each_child<F: FnMut(Name, Namespace, &'a NameBinding<'a>)>(&self, mut f: F) {
for (&(name, ns), name_resolution) in self.resolutions.borrow().iter() {
name_resolution.binding.map(|binding| f(name, ns, binding));
name_resolution.borrow().binding.map(|binding| f(name, ns, binding));
}
}

Expand Down Expand Up @@ -929,11 +916,6 @@ impl<'a> ModuleS<'a> {
_ => false,
}
}

fn inc_glob_count(&self, is_public: bool) {
let glob_count = if is_public { &self.public_glob_count } else { &self.private_glob_count };
glob_count.set(glob_count.get() + 1);
}
}

impl<'a> fmt::Debug for ModuleS<'a> {
Expand Down Expand Up @@ -1135,7 +1117,8 @@ pub struct Resolver<'a, 'tcx: 'a> {
struct ResolverArenas<'a> {
modules: arena::TypedArena<ModuleS<'a>>,
name_bindings: arena::TypedArena<NameBinding<'a>>,
import_directives: arena::TypedArena<ImportDirective>,
import_directives: arena::TypedArena<ImportDirective<'a>>,
name_resolutions: arena::TypedArena<RefCell<NameResolution<'a>>>,
}

impl<'a> ResolverArenas<'a> {
Expand All @@ -1145,9 +1128,13 @@ impl<'a> ResolverArenas<'a> {
fn alloc_name_binding(&'a self, name_binding: NameBinding<'a>) -> &'a NameBinding<'a> {
self.name_bindings.alloc(name_binding)
}
fn alloc_import_directive(&'a self, import_directive: ImportDirective) -> &'a ImportDirective {
fn alloc_import_directive(&'a self, import_directive: ImportDirective<'a>)
-> &'a ImportDirective {
self.import_directives.alloc(import_directive)
}
fn alloc_name_resolution(&'a self) -> &'a RefCell<NameResolution<'a>> {
self.name_resolutions.alloc(Default::default())
}
}

#[derive(PartialEq)]
Expand Down Expand Up @@ -1216,6 +1203,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
modules: arena::TypedArena::new(),
name_bindings: arena::TypedArena::new(),
import_directives: arena::TypedArena::new(),
name_resolutions: arena::TypedArena::new(),
}
}

Expand Down
Loading

0 comments on commit 7fd331e

Please sign in to comment.