resolve: refactor away PRIVATE_VARIANT and ensure that restricted

reexports of private variants are handled correctly.

Author: jseyfried

src/librustc_resolve/build_reduced_graph.rs

@@ -291,14 +291,9 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
                 let module = self.new_module(parent_link, Some(def), false, vis);
                 self.define(parent, name, TypeNS, (module, sp));
 
-                let variant_modifiers = match vis {
-                    ty::Visibility::Public => DefModifiers::empty(),
-                    _ => DefModifiers::PRIVATE_VARIANT,
-                };
                 for variant in &(*enum_definition).variants {
                     let item_def_id = self.ast_map.local_def_id(item.id);
-                    self.build_reduced_graph_for_variant(variant, item_def_id,
-                                                         module, variant_modifiers);
+                    self.build_reduced_graph_for_variant(variant, item_def_id, module);
                 }
             }
 
@@ -358,8 +353,7 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
     fn build_reduced_graph_for_variant(&mut self,
                                        variant: &Variant,
                                        item_id: DefId,
-                                       parent: Module<'b>,
-                                       variant_modifiers: DefModifiers) {
+                                       parent: Module<'b>) {
         let name = variant.node.name;
         if variant.node.data.is_struct() {
             // Not adding fields for variants as they are not accessed with a self receiver
@@ -369,12 +363,11 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
 
         // Variants are always treated as importable to allow them to be glob used.
         // All variants are defined in both type and value namespaces as future-proofing.
-        let modifiers = DefModifiers::IMPORTABLE | variant_modifiers;
+        let modifiers = DefModifiers::IMPORTABLE;
         let def = Def::Variant(item_id, self.ast_map.local_def_id(variant.node.data.id()));
-        let vis = ty::Visibility::Public;
 
-        self.define(parent, name, ValueNS, (def, variant.span, modifiers, vis));
-        self.define(parent, name, TypeNS, (def, variant.span, modifiers, vis));
+        self.define(parent, name, ValueNS, (def, variant.span, modifiers, parent.vis));
+        self.define(parent, name, TypeNS, (def, variant.span, modifiers, parent.vis));
     }
 
     /// Constructs the reduced graph for one foreign item.

src/librustc_resolve/lib.rs

@@ -919,9 +919,6 @@ bitflags! {
     #[derive(Debug)]
     flags DefModifiers: u8 {
         const IMPORTABLE = 1 << 1,
-        // Variants are considered `PUBLIC`, but some of them live in private enums.
-        // We need to track them to prohibit reexports like `pub use PrivEnum::Variant`.
-        const PRIVATE_VARIANT = 1 << 2,
         const GLOB_IMPORTED = 1 << 3,
     }
 }
@@ -932,8 +929,6 @@ pub struct NameBinding<'a> {
     modifiers: DefModifiers,
     kind: NameBindingKind<'a>,
     span: Option<Span>,
-    // Enum variants are always considered `PUBLIC`, this is needed for `use Enum::Variant`
-    // or `use Enum::*` to work on private enums.
     vis: ty::Visibility,
 }
 
@@ -982,8 +977,20 @@ impl<'a> NameBinding<'a> {
         self.modifiers.contains(modifiers)
     }
 
-    fn is_public(&self) -> bool {
-        self.vis == ty::Visibility::Public
+    fn is_pseudo_public(&self) -> bool {
+        self.pseudo_vis() == ty::Visibility::Public
+    }
+
+    // We sometimes need to treat variants as `pub` for backwards compatibility
+    fn pseudo_vis(&self) -> ty::Visibility {
+        if self.is_variant() { ty::Visibility::Public } else { self.vis }
+    }
+
+    fn is_variant(&self) -> bool {
+        match self.kind {
+            NameBindingKind::Def(Def::Variant(..)) => true,
+            _ => false,
+        }
     }
 
     fn is_extern_crate(&self) -> bool {
@@ -3301,7 +3308,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         // only if both the module is public and the entity is
                         // declared as public (due to pruning, we don't explore
                         // outside crate private modules => no need to check this)
-                        if !in_module_is_extern || name_binding.is_public() {
+                        if !in_module_is_extern || name_binding.vis == ty::Visibility::Public {
                             lookup_results.push(path);
                         }
                     }
@@ -3326,7 +3333,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         _ => bug!(),
                     };
 
-                    if !in_module_is_extern || name_binding.is_public() {
+                    if !in_module_is_extern || name_binding.vis == ty::Visibility::Public {
                         // add the module to the lookup
                         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

@@ -184,7 +184,7 @@ impl<'a> NameResolution<'a> {
                 // the name, and (3) no public glob has defined the name, the resolution depends
                 // on whether more globs can define the name.
                 if !allow_private_imports && directive.vis != ty::Visibility::Public &&
-                   !self.binding.map(NameBinding::is_public).unwrap_or(false) {
+                   !self.binding.map(NameBinding::is_pseudo_public).unwrap_or(false) {
                     return None;
                 }
 
@@ -242,7 +242,8 @@ impl<'a> ::ModuleS<'a> {
         if let Some(result) = resolution.try_result(ns, allow_private_imports) {
             // If the resolution doesn't depend on glob definability, check privacy and return.
             return result.and_then(|binding| {
-                let allowed = allow_private_imports || !binding.is_import() || binding.is_public();
+                let allowed = allow_private_imports || !binding.is_import() ||
+                                                       binding.is_pseudo_public();
                 if allowed { Success(binding) } else { Failed(None) }
             });
         }
@@ -336,7 +337,7 @@ impl<'a> ::ModuleS<'a> {
     }
 
     fn define_in_glob_importers(&self, name: Name, ns: Namespace, binding: &'a NameBinding<'a>) {
-        if !binding.defined_with(DefModifiers::IMPORTABLE) || !binding.is_public() { return }
+        if !binding.defined_with(DefModifiers::IMPORTABLE) || !binding.is_pseudo_public() { return }
         for &(importer, directive) in self.glob_importers.borrow_mut().iter() {
             let _ = importer.try_define_child(name, ns, directive.import(binding, None));
         }
@@ -569,7 +570,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
 
         let ast_map = self.resolver.ast_map;
         match (&value_result, &type_result) {
-            (&Success(binding), _) if !binding.vis.is_at_least(directive.vis, ast_map) &&
+            (&Success(binding), _) if !binding.pseudo_vis().is_at_least(directive.vis, ast_map) &&
                                       self.resolver.is_accessible(binding.vis) => {
                 let msg = format!("`{}` is private, and cannot be reexported", source);
                 let note_msg = format!("consider marking `{}` as `pub` in the imported module",
@@ -579,7 +580,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                     .emit();
             }
 
-            (_, &Success(binding)) if !binding.vis.is_at_least(directive.vis, ast_map) &&
+            (_, &Success(binding)) if !binding.pseudo_vis().is_at_least(directive.vis, ast_map) &&
                                       self.resolver.is_accessible(binding.vis) => {
                 if binding.is_extern_crate() {
                     let msg = format!("extern crate `{}` is private, and cannot be reexported \
@@ -661,7 +662,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
             resolution.borrow().binding().map(|binding| (*name, binding))
         }).collect::<Vec<_>>();
         for ((name, ns), binding) in bindings {
-            if binding.defined_with(DefModifiers::IMPORTABLE) && binding.is_public() {
+            if binding.defined_with(DefModifiers::IMPORTABLE) && binding.is_pseudo_public() {
                 let _ = module_.try_define_child(name, ns, directive.import(binding, None));
             }
         }
@@ -697,15 +698,16 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                 None => continue,
             };
 
-            if binding.is_public() && (binding.is_import() || binding.is_extern_crate()) {
+            if binding.vis == ty::Visibility::Public &&
+               (binding.is_import() || binding.is_extern_crate()) {
                 if let Some(def) = binding.def() {
                     reexports.push(Export { name: name, def_id: def.def_id() });
                 }
             }
 
             if let NameBindingKind::Import { binding: orig_binding, id, .. } = binding.kind {
-                if ns == TypeNS && binding.is_public() &&
-                   orig_binding.defined_with(DefModifiers::PRIVATE_VARIANT) {
+                if ns == TypeNS && orig_binding.is_variant() &&
+                   !orig_binding.vis.is_at_least(binding.vis, &self.resolver.ast_map) {
                     let msg = format!("variant `{}` is private, and cannot be reexported \
                                        (error E0364), consider declaring its enum as `pub`",
                                       name);