Fix the visibility of extern crate declarations and stop warning on pub extern crate

Author: jseyfried

src/librustc_resolve/build_reduced_graph.rs

@@ -293,9 +293,19 @@ 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_extern_crate_module(parent_link, def);
+                    let local_def_id = self.ast_map.local_def_id(item.id);
+                    let external_module =
+                        self.new_extern_crate_module(parent_link, def, is_public, local_def_id);
                     self.define(parent, name, TypeNS, (external_module, sp));
 
+                    if is_public {
+                        let export = Export { name: name, def_id: def_id };
+                        if let Some(def_id) = parent.def_id() {
+                            let node_id = self.resolver.ast_map.as_local_node_id(def_id).unwrap();
+                            self.export_map.entry(node_id).or_insert(Vec::new()).push(export);
+                        }
+                    }
+
                     self.build_reduced_graph_for_external_crate(external_module);
                 }
                 parent

src/librustc_resolve/lib.rs

@@ -806,7 +806,10 @@ pub struct ModuleS<'a> {
     parent_link: ParentLink<'a>,
     def: Option<Def>,
     is_public: bool,
-    is_extern_crate: bool,
+
+    // If the module is an extern crate, `def` is root of the external crate and `extern_crate_did`
+    // is the DefId of the local `extern crate` item (otherwise, `extern_crate_did` is None).
+    extern_crate_did: Option<DefId>,
 
     resolutions: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
     unresolved_imports: RefCell<Vec<ImportDirective>>,
@@ -853,7 +856,7 @@ impl<'a> ModuleS<'a> {
             parent_link: parent_link,
             def: def,
             is_public: is_public,
-            is_extern_crate: false,
+            extern_crate_did: None,
             resolutions: RefCell::new(HashMap::new()),
             unresolved_imports: RefCell::new(Vec::new()),
             module_children: RefCell::new(NodeMap()),
@@ -917,6 +920,16 @@ impl<'a> ModuleS<'a> {
         self.def.as_ref().map(Def::def_id)
     }
 
+    // This returns the DefId of the crate local item that controls this module's visibility.
+    // It is only used to compute `LastPrivate` data, and it differs from `def_id` only for extern
+    // crates, whose `def_id` is the external crate's root, not the local `extern crate` item.
+    fn local_def_id(&self) -> Option<DefId> {
+        match self.extern_crate_did {
+            Some(def_id) => Some(def_id),
+            None => self.def_id(),
+        }
+    }
+
     fn is_normal(&self) -> bool {
         match self.def {
             Some(Def::Mod(_)) | Some(Def::ForeignMod(_)) => true,
@@ -1027,6 +1040,14 @@ impl<'a> NameBinding<'a> {
         }
     }
 
+    fn local_def_id(&self) -> Option<DefId> {
+        match self.kind {
+            NameBindingKind::Def(def) => Some(def.def_id()),
+            NameBindingKind::Module(ref module) => module.local_def_id(),
+            NameBindingKind::Import { binding, .. } => binding.local_def_id(),
+        }
+    }
+
     fn defined_with(&self, modifiers: DefModifiers) -> bool {
         self.modifiers.contains(modifiers)
     }
@@ -1038,11 +1059,12 @@ impl<'a> NameBinding<'a> {
     fn def_and_lp(&self) -> (Def, LastPrivate) {
         let def = self.def().unwrap();
         if let Def::Err = def { return (def, LastMod(AllPublic)) }
-        (def, LastMod(if self.is_public() { AllPublic } else { DependsOn(def.def_id()) }))
+        let lp = if self.is_public() { AllPublic } else { DependsOn(self.local_def_id().unwrap()) };
+        (def, LastMod(lp))
     }
 
     fn is_extern_crate(&self) -> bool {
-        self.module().map(|module| module.is_extern_crate).unwrap_or(false)
+        self.module().and_then(|module| module.extern_crate_did).is_some()
     }
 
     fn is_import(&self) -> bool {
@@ -1236,9 +1258,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         self.arenas.name_bindings.alloc(name_binding)
     }
 
-    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;
+    fn new_extern_crate_module(&self,
+                               parent_link: ParentLink<'a>,
+                               def: Def,
+                               is_public: bool,
+                               local_def: DefId)
+                               -> Module<'a> {
+        let mut module = ModuleS::new(parent_link, Some(def), false, is_public);
+        module.extern_crate_did = Some(local_def);
         self.arenas.modules.alloc(module)
     }
 
@@ -1357,7 +1384,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         // Keep track of the closest private module used
                         // when resolving this import chain.
                         if !binding.is_public() {
-                            if let Some(did) = search_module.def_id() {
+                            if let Some(did) = search_module.local_def_id() {
                                 closest_private = LastMod(DependsOn(did));
                             }
                         }
@@ -1462,7 +1489,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             Success(PrefixFound(ref containing_module, index)) => {
                 search_module = containing_module;
                 start_index = index;
-                last_private = LastMod(DependsOn(containing_module.def_id()
+                last_private = LastMod(DependsOn(containing_module.local_def_id()
                                                                   .unwrap()));
             }
         }
@@ -3571,7 +3598,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
                     if !in_module_is_extern || name_binding.is_public() {
                         // add the module to the lookup
-                        let is_extern = in_module_is_extern || module.is_extern_crate;
+                        let is_extern = in_module_is_extern || name_binding.is_extern_crate();
                         worklist.push((module, path_segments, is_extern));
                     }
                 }

src/librustc_resolve/resolve_imports.rs

@@ -402,7 +402,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
             }
 
             (_, &Success(name_binding)) if !name_binding.is_import() && directive.is_public => {
-                if !name_binding.is_public() {
+                // Disallow reexporting private items, excepting extern crates.
+                if !name_binding.is_public() && !name_binding.is_extern_crate() {
                     let msg = format!("`{}` is private, and cannot be reexported", source);
                     let note_msg =
                         format!("Consider declaring type or module `{}` with `pub`", source);
@@ -441,9 +442,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         module_.decrement_outstanding_references_for(target, TypeNS);
 
         let def_and_priv = |binding: &NameBinding| {
-            let def = binding.def().unwrap();
-            let last_private = if binding.is_public() { lp } else { DependsOn(def.def_id()) };
-            (def, last_private)
+            let last_private =
+                if binding.is_public() { lp } else { DependsOn(binding.local_def_id().unwrap()) };
+            (binding.def().unwrap(), last_private)
         };
         let value_def_and_priv = value_result.success().map(&def_and_priv);
         let type_def_and_priv = type_result.success().map(&def_and_priv);
@@ -493,7 +494,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
         build_reduced_graph::populate_module_if_necessary(self.resolver, target_module);
         target_module.for_each_child(|name, ns, binding| {
             if !binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { return }
-            if binding.is_extern_crate() { return }
             self.define(module_, name, ns, directive.import(binding));
 
             if ns == TypeNS && directive.is_public &&

src/libsyntax/parse/parser.rs

@@ -5487,13 +5487,6 @@ impl<'a> Parser<'a> {
         try!(self.expect(&token::Semi));
 
         let last_span = self.last_span;
-
-        if visibility == ast::Visibility::Public {
-            self.span_warn(mk_sp(lo, last_span.hi),
-                           "`pub extern crate` does not work as expected and should not be used. \
-                            Likely to become an error. Prefer `extern crate` and `pub use`.");
-        }
-
         Ok(self.mk_item(lo,
                         last_span.hi,
                         ident,

src/test/auxiliary/privacy_reexport.rs

@@ -8,6 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+pub extern crate core;
 pub use foo as bar;
 
 pub mod foo {

src/test/compile-fail/extern-crate-visibility.rs

@@ -0,0 +1,23 @@
+// 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 {
+    extern crate core;
+    pub use self::core as reexported_core; // Check that private extern crates can be reexported
+}
+
+// Check that private crates cannot be used from outside their modules
+use foo::core; //~ ERROR module `core` is inaccessible
+use foo::core::cell; //~ ERROR
+
+fn main() {
+    use foo::*;
+    mod core {} // Check that private crates are not glob imported
+}

src/test/compile-fail/no-extern-crate-in-glob-import.rs

@@ -15,5 +15,8 @@
 extern crate privacy_reexport;
 
 pub fn main() {
+    // Check that public extern crates are visible to outside crates
+    privacy_reexport::core::cell::Cell::new(0);
+
     privacy_reexport::bar::frob();
 }