Auto merge of #32328 - jseyfried:coherence, r=nikomatsakis

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 #32119.

r? @nikomatsakis

Author: bors

src/librustc_resolve/build_reduced_graph.rs

@@ -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};
@@ -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};
@@ -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,
@@ -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(),
@@ -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,
@@ -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>) {

src/librustc_resolve/lib.rs

@@ -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
@@ -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
@@ -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));
         }
     }
 
@@ -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> {
@@ -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> {
@@ -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)]
@@ -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(),
         }
     }