Rollup merge of rust-lang#31362 - jseyfried:fix_extern_crate_visibility, r=nikomatsakis

Manishearth · Manishearth · commit a48f95879d4a · 2016-02-25T11:41:01.000+05:30
This PR changes the visibility of extern crate declarations to match that of items (fixes rust-lang#26775). To avoid breakage, the PR makes it a `public_in_private` lint to reexport a private extern crate, and it adds the lint `inaccessible_extern_crate` for uses of an inaccessible extern crate. The lints can be avoided by making the appropriate `extern crate` declaration public.
diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs
@@ -51,7 +51,7 @@ extern crate getopts;
 extern crate graphviz;
 extern crate libc;
 extern crate rbml;
-extern crate rustc_llvm;
+pub extern crate rustc_llvm as llvm;
 extern crate rustc_back;
 extern crate rustc_front;
 extern crate rustc_data_structures;
@@ -66,8 +66,6 @@ extern crate serialize as rustc_serialize; // used by deriving
 #[cfg(test)]
 extern crate test;
 
-pub use rustc_llvm as llvm;
-
 #[macro_use]
 mod macros;
 
diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs
@@ -124,6 +124,12 @@ declare_lint! {
     "detect private items in public interfaces not caught by the old implementation"
 }
 
+declare_lint! {
+    pub INACCESSIBLE_EXTERN_CRATE,
+    Warn,
+    "use of inaccessible extern crate erroneously allowed"
+}
+
 declare_lint! {
     pub INVALID_TYPE_PARAM_DEFAULT,
     Warn,
@@ -167,6 +173,7 @@ impl LintPass for HardwiredLints {
             TRIVIAL_CASTS,
             TRIVIAL_NUMERIC_CASTS,
             PRIVATE_IN_PUBLIC,
+            INACCESSIBLE_EXTERN_CRATE,
             INVALID_TYPE_PARAM_DEFAULT,
             MATCH_OF_UNIT_VARIANT_VIA_PAREN_DOTDOT,
             CONST_ERR,
diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs
@@ -158,6 +158,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(PRIVATE_IN_PUBLIC),
             reference: "the explanation for E0446 (`--explain E0446`)",
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(INACCESSIBLE_EXTERN_CRATE),
+            reference: "PR 31362 <https://github.com/rust-lang/rust/pull/31362>",
+        },
         FutureIncompatibleInfo {
             id: LintId::of(INVALID_TYPE_PARAM_DEFAULT),
             reference: "PR 30742 <https://github.com/rust-lang/rust/pull/30724>",
diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs
@@ -743,6 +743,7 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
                      source_did: Option<DefId>,
                      msg: &str)
                      -> CheckResult {
+        use rustc_front::hir::Item_::ItemExternCrate;
         debug!("ensure_public(span={:?}, to_check={:?}, source_did={:?}, msg={:?})",
                span, to_check, source_did, msg);
         let def_privacy = self.def_privacy(to_check);
@@ -763,6 +764,21 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
         // be local.)
         let def_id = source_did.unwrap_or(to_check);
         let node_id = self.tcx.map.as_local_node_id(def_id);
+
+        // Warn when using a inaccessible extern crate.
+        if let Some(node_id) = self.tcx.map.as_local_node_id(to_check) {
+            match self.tcx.map.get(node_id) {
+                ast_map::Node::NodeItem(&hir::Item { node: ItemExternCrate(_), name, .. }) => {
+                    self.tcx.sess.add_lint(lint::builtin::INACCESSIBLE_EXTERN_CRATE,
+                                           node_id,
+                                           span,
+                                           format!("extern crate `{}` is private", name));
+                    return None;
+                }
+                _ => {}
+            }
+        }
+
         let (err_span, err_msg) = if Some(id) == node_id {
             return Some((span, format!("{} is private", msg), None));
         } else {
diff --git a/src/librustc_resolve/build_reduced_graph.rs b/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
diff --git a/src/librustc_resolve/lib.rs b/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));
                     }
                 }
diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs
@@ -394,7 +394,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                                            directive.is_public &&
                                            !name_binding.is_public() => {
                 let msg = format!("`{}` is private, and cannot be reexported", source);
-                let note_msg = format!("Consider marking `{}` as `pub` in the imported module",
+                let note_msg = format!("consider marking `{}` as `pub` in the imported module",
                                         source);
                 struct_span_err!(self.resolver.session, directive.span, E0364, "{}", &msg)
                     .span_note(directive.span, &note_msg)
@@ -403,12 +403,22 @@ 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() {
-                    let msg = format!("`{}` is private, and cannot be reexported", source);
-                    let note_msg =
-                        format!("Consider declaring type or module `{}` with `pub`", source);
-                    struct_span_err!(self.resolver.session, directive.span, E0365, "{}", &msg)
-                        .span_note(directive.span, &note_msg)
-                        .emit();
+                    if name_binding.is_extern_crate() {
+                        let msg = format!("extern crate `{}` is private, and cannot be reexported \
+                                           (error E0364), consider declaring with `pub`",
+                                           source);
+                        self.resolver.session.add_lint(lint::builtin::PRIVATE_IN_PUBLIC,
+                                                       directive.id,
+                                                       directive.span,
+                                                       msg);
+                    } else {
+                        let msg = format!("`{}` is private, and cannot be reexported", source);
+                        let note_msg =
+                            format!("consider declaring type or module `{}` with `pub`", source);
+                        struct_span_err!(self.resolver.session, directive.span, E0365, "{}", &msg)
+                            .span_note(directive.span, &note_msg)
+                            .emit();
+                    }
                 } else if name_binding.defined_with(DefModifiers::PRIVATE_VARIANT) {
                     let msg = format!("variant `{}` is private, and cannot be reexported \
                                        (error E0364), consider declaring its enum as `pub`",
@@ -441,9 +451,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 +503,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 &&
diff --git a/src/librustc_trans/lib.rs b/src/librustc_trans/lib.rs
@@ -46,7 +46,7 @@ extern crate rustc;
 extern crate rustc_back;
 extern crate rustc_data_structures;
 extern crate rustc_front;
-extern crate rustc_llvm as llvm;
+pub extern crate rustc_llvm as llvm;
 extern crate rustc_mir;
 extern crate rustc_platform_intrinsics as intrinsics;
 extern crate serialize;
diff --git a/src/libstd/sys/unix/net.rs b/src/libstd/sys/unix/net.rs
@@ -21,7 +21,7 @@ use sys_common::net::{getsockopt, setsockopt};
 use time::Duration;
 
 pub use sys::{cvt, cvt_r};
-pub use libc as netc;
+pub extern crate libc as netc;
 
 pub type wrlen_t = size_t;
 
diff --git a/src/libsyntax/parse/parser.rs b/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,
diff --git a/src/test/auxiliary/privacy_reexport.rs b/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 {
diff --git a/src/test/compile-fail/extern-crate-visibility.rs b/src/test/compile-fail/extern-crate-visibility.rs
@@ -0,0 +1,34 @@
+// 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.
+
+#![feature(rustc_attrs)]
+#![allow(dead_code)]
+#![allow(unused_imports)]
+
+mod foo {
+    extern crate core;
+}
+
+// Check that private crates can be used from outside their modules, albeit with warnings
+use foo::core; //~ WARN extern crate `core` is private
+//~^ WARN this was previously accepted by the compiler but is being phased out
+use foo::core::cell; //~ WARN extern crate `core` is private
+//~^ WARN this was previously accepted by the compiler but is being phased out
+
+fn f() {
+    foo::core::cell::Cell::new(0); //~ WARN extern crate `core` is private
+    //~^ WARN this was previously accepted by the compiler but is being phased out
+
+    use foo::*;
+    mod core {} // Check that private crates are not glob imported
+}
+
+#[rustc_error]
+fn main() {} //~ ERROR compilation successful
diff --git a/src/test/compile-fail/no-extern-crate-in-glob-import.rs b/src/test/compile-fail/no-extern-crate-in-glob-import.rs
diff --git a/src/test/compile-fail/private-variant-and-crate-reexport.rs b/src/test/compile-fail/private-variant-and-crate-reexport.rs
@@ -11,6 +11,10 @@
 #![feature(rustc_attrs)]
 #![allow(dead_code)]
 
+extern crate core;
+pub use core as reexported_core; //~ WARN extern crate `core` is private, and cannot be reexported
+//~^ WARNING hard error
+
 mod m1 {
     pub use ::E::V; //~ WARN variant `V` is private, and cannot be reexported
     //~^ WARNING hard error
diff --git a/src/test/compile-fail/warn-pub-extern-crate.rs b/src/test/compile-fail/warn-pub-extern-crate.rs
diff --git a/src/test/run-pass/privacy-reexport.rs b/src/test/run-pass/privacy-reexport.rs
