Auto merge of #32814 - jseyfried:improve_duplicate_glob_detection, r=nikomatsakis

resolve: Improve duplicate glob detection

This fixes a bug introduced in #31726 in which we erroneously allow multiple imports of the same item under some circumstances.

More specifically, we erroneously allow a module that is in a cycle of glob re-exports to have other re-exports (besides the glob from the cycle).
For example,
```rust
pub fn f() {}
mod foo {
    pub use f; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This also defines `foo::f`, which should be a duplicate error but is currently allowed.
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}
```

A module in a glob re-export cycle can still have `pub` items after this PR. For example,
```rust
mod foo {
    pub fn f() {}; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This is not a duplicate error since items shadow glob-imported re-exports (cf #31337).
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}
```
r? @nikomatsakis

src/librustc_resolve/resolve_imports.rs

@@ -275,7 +275,6 @@ impl<'a> ::ModuleS<'a> {
     // Define the name or return the existing binding if there is a collision.
     pub fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>)
                             -> Result<(), &'a NameBinding<'a>> {
-        if self.resolutions.borrow_state() != ::std::cell::BorrowState::Unused { return Ok(()); }
         self.update_resolution(name, ns, |resolution| {
             resolution.try_define(self.arenas.alloc_name_binding(binding))
         })
@@ -318,15 +317,22 @@ impl<'a> ::ModuleS<'a> {
     fn update_resolution<T, F>(&self, name: Name, ns: Namespace, update: F) -> T
         where F: FnOnce(&mut NameResolution<'a>) -> T
     {
-        let mut resolution = &mut *self.resolution(name, ns).borrow_mut();
-        let was_known = resolution.binding().is_some();
+        // Ensure that `resolution` isn't borrowed during `define_in_glob_importers`,
+        // where it might end up getting re-defined via a glob cycle.
+        let (new_binding, t) = {
+            let mut resolution = &mut *self.resolution(name, ns).borrow_mut();
+            let was_known = resolution.binding().is_some();
 
-        let t = update(resolution);
-        if !was_known {
-            if let Some(binding) = resolution.binding() {
-                self.define_in_glob_importers(name, ns, binding);
+            let t = update(resolution);
+
+            if was_known { return t; }
+            match resolution.binding() {
+                Some(binding) => (binding, t),
+                None => return t,
             }
-        }
+        };
+
+        self.define_in_glob_importers(name, ns, new_binding);
         t
     }
 
@@ -650,11 +656,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         // Add to target_module's glob_importers
         target_module.glob_importers.borrow_mut().push((module_, directive));
 
-        for (&(name, ns), resolution) in target_module.resolutions.borrow().iter() {
-            if let Some(binding) = resolution.borrow().binding() {
-                if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) {
-                    let _ = module_.try_define_child(name, ns, directive.import(binding, None));
-                }
+        // Ensure that `resolutions` isn't borrowed during `try_define_child`,
+        // since it might get updated via a glob cycle.
+        let bindings = target_module.resolutions.borrow().iter().filter_map(|(name, resolution)| {
+            resolution.borrow().binding().map(|binding| (*name, binding))
+        }).collect::<Vec<_>>();
+        for ((name, ns), binding) in bindings {
+            if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) {
+                let _ = module_.try_define_child(name, ns, directive.import(binding, None));
             }
         }
 

src/test/compile-fail/glob-cycles.rs

@@ -0,0 +1,26 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+mod foo {
+    pub use bar::*;
+    pub use main as f; //~ ERROR has already been imported
+}
+
+mod bar {
+    pub use foo::*;
+}
+
+pub use foo::*;
+pub use baz::*; //~ ERROR has already been imported
+mod baz {
+    pub use super::*;
+}
+
+pub fn main() {}

src/test/compile-fail/issue-32797.rs

@@ -0,0 +1,21 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+pub use bar::*;
+mod bar {
+    pub use super::*;
+}
+
+pub use baz::*; //~ ERROR already been imported
+mod baz {
+    pub use main as f;
+}
+
+pub fn main() {}