RFC 2145: Type privacy and private-in-public lints

compiler/rustc_middle/src/middle/privacy.rs

@@ -1,7 +1,7 @@
 //! A pass that checks to make sure private fields and methods aren't used
 //! outside their scopes. This pass will also generate a set of exported items
 //! which are available for use externally when compiled as a library.
-use crate::ty::Visibility;
+use crate::ty::{DefIdTree, Visibility};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_macros::HashStable;
@@ -58,6 +58,40 @@ impl EffectiveVisibility {
     pub fn is_public_at_level(&self, tag: AccessLevel) -> bool {
         self.get(tag).map_or(false, |vis| vis.is_public())
     }
+
+    pub fn nearest_available(&self, tag: AccessLevel) -> Option<Visibility> {
+        for level in [
+            AccessLevel::ReachableFromImplTrait,
+            AccessLevel::Reachable,
+            AccessLevel::Exported,
+            AccessLevel::Public,
+        ] {
+            if (level <= tag) && self.get(tag).is_some() {
+                return self.get(tag).cloned();
+            }
+        }
+        None
+    }
+
+    pub fn update<T: DefIdTree>(&mut self, vis: Visibility, tag: AccessLevel, tree: T) {
+        for level in [
+            AccessLevel::Public,
+            AccessLevel::Exported,
+            AccessLevel::Reachable,
+            AccessLevel::ReachableFromImplTrait,
+        ] {
+            if level <= tag {
+                let current_effective_vis = self.get_mut(level);
+                if let Some(current_effective_vis) = current_effective_vis {
+                    if vis.is_at_least(*current_effective_vis, tree) {
+                        *current_effective_vis = vis;
+                    }
+                } else {
+                    *current_effective_vis = Some(vis);
+                }
+            }
+        }
+    }
 }
 
 /// Holds a map of accessibility levels for reachable HIR nodes.
@@ -122,6 +156,10 @@ impl<Id: Hash + Eq + Copy> AccessLevels<Id> {
         self.map.get(&id)
     }
 
+    pub fn set_effective_vis(&mut self, id: Id, effective_vis: EffectiveVisibility) {
+        self.map.insert(id, effective_vis);
+    }
+
     pub fn iter(&self) -> impl Iterator<Item = (&Id, &EffectiveVisibility)> {
         self.map.iter()
     }

compiler/rustc_resolve/src/access_levels.rs

@@ -7,12 +7,10 @@ use rustc_ast::visit;
 use rustc_ast::visit::Visitor;
 use rustc_ast::Crate;
 use rustc_ast::EnumDef;
-use rustc_ast::NodeId;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::def_id::CRATE_DEF_ID;
 use rustc_middle::middle::privacy::AccessLevel;
-use rustc_middle::ty::DefIdTree;
-use rustc_span::sym;
+use rustc_middle::ty::{DefIdTree, Visibility};
 
 pub struct AccessLevelsVisitor<'r, 'a> {
     r: &'r mut Resolver<'a>,
@@ -46,25 +44,48 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {
     /// This will also follow `use` chains (see PrivacyVisitor::set_import_binding_access_level).
     fn set_bindings_access_level(&mut self, module_id: LocalDefId) {
         assert!(self.r.module_map.contains_key(&&module_id.to_def_id()));
-        let module_level = self.r.access_levels.get_access_level(module_id);
-        if !module_level.is_some() {
-            return;
-        }
+
         // Set the given binding access level to `AccessLevel::Public` and
         // sets the rest of the `use` chain to `AccessLevel::Exported` until
         // we hit the actual exported item.
         let set_import_binding_access_level =
-            |this: &mut Self, mut binding: &NameBinding<'a>, mut access_level| {
+            |this: &mut Self, mut binding: &NameBinding<'a>, mut parent_id| {
                 while let NameBindingKind::Import { binding: nested_binding, import, .. } =
                     binding.kind
                 {
-                    this.set_access_level(import.id, access_level);
-                    if let ImportKind::Single { additional_ids, .. } = import.kind {
-                        this.set_access_level(additional_ids.0, access_level);
-                        this.set_access_level(additional_ids.1, access_level);
+                    if this.r.opt_local_def_id(import.id).is_some() {
+                        let vis = match binding.vis {
+                            Visibility::Public => Visibility::Public,
+                            Visibility::Restricted(id) => Visibility::Restricted(id.expect_local()),
+                        };
+                        this.update_effective_vis(
+                            this.r.local_def_id(import.id),
+                            vis,
+                            parent_id,
+                            AccessLevel::Exported,
+                        );
+                        if let ImportKind::Single { additional_ids, .. } = import.kind {
+                            if let Some(id) = this.r.opt_local_def_id(additional_ids.0) {
+                                this.update_effective_vis(
+                                    id,
+                                    vis,
+                                    parent_id,
+                                    AccessLevel::Exported,
+                                );
+                            }
+
+                            if let Some(id) = this.r.opt_local_def_id(additional_ids.1) {
+                                this.update_effective_vis(
+                                    id,
+                                    vis,
+                                    parent_id,
+                                    AccessLevel::Exported,
+                                );
+                            }
+                        }
+
+                        parent_id = this.r.local_def_id(import.id);
                     }
-
-                    access_level = Some(AccessLevel::Exported);
                     binding = nested_binding;
                 }
             };
@@ -73,29 +94,66 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {
         let resolutions = self.r.resolutions(module);
 
         for (.., name_resolution) in resolutions.borrow().iter() {
-            if let Some(binding) = name_resolution.borrow().binding() && binding.vis.is_public() && !binding.is_ambiguity() {
-                let access_level = match binding.is_import() {
+            if let Some(binding) = name_resolution.borrow().binding() {
+                let tag = match binding.is_import() {
                     true => {
-                        set_import_binding_access_level(self, binding, module_level);
-                        Some(AccessLevel::Exported)
-                    },
-                    false => module_level,
+                        if !binding.is_ambiguity() {
+                            set_import_binding_access_level(self, binding, module_id);
+                        }
+                        AccessLevel::Exported
+                    }
+                    false => AccessLevel::Public,
                 };
-                if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
-                    self.set_access_level_def_id(def_id, access_level);
+
+                if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) && !binding.is_ambiguity(){
+                    let vis = match binding.vis {
+                        Visibility::Public => Visibility::Public,
+                        Visibility::Restricted(id) => Visibility::Restricted(id.expect_local())
+                    };
+                    self.update_effective_vis(def_id, vis, module_id, tag);
                 }
             }
         }
     }
 
-    /// Sets the access level of the `LocalDefId` corresponding to the given `NodeId`.
-    /// This function will panic if the `NodeId` does not have a `LocalDefId`
-    fn set_access_level(
+    fn update_effective_vis(
         &mut self,
-        node_id: NodeId,
-        access_level: Option<AccessLevel>,
-    ) -> Option<AccessLevel> {
-        self.set_access_level_def_id(self.r.local_def_id(node_id), access_level)
+        current_id: LocalDefId,
+        current_vis: Visibility,
+        module_id: LocalDefId,
+        tag: AccessLevel,
+    ) {
+        if let Some(inherited_effective_vis) = self.r.access_levels.get_effective_vis(module_id) {
+            let mut current_effective_vis =
+                self.r.access_levels.get_effective_vis(current_id).copied().unwrap_or_default();
+            let current_effective_vis_copy = current_effective_vis.clone();
+            for level in [
+                AccessLevel::Public,
+                AccessLevel::Exported,
+                AccessLevel::Reachable,
+                AccessLevel::ReachableFromImplTrait,
+            ] {
+                if level <= tag {
+                    let nearest_available_vis =
+                        inherited_effective_vis.nearest_available(level).unwrap();
+                    let calculated_effective_vis = match current_vis {
+                        Visibility::Public => nearest_available_vis,
+                        Visibility::Restricted(_) => {
+                            if current_vis.is_at_least(nearest_available_vis, &*self.r) {
+                                nearest_available_vis
+                            } else {
+                                current_vis
+                            }
+                        }
+                    };
+                    current_effective_vis.update(calculated_effective_vis, level, &*self.r);
+                }
+            }
+            if current_effective_vis_copy != current_effective_vis {
+                self.changed = true;
+                self.r.access_levels.set_effective_vis(current_id, current_effective_vis);
+            }
+        }
     }
 
     fn set_access_level_def_id(
@@ -130,16 +188,20 @@ impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> {
 
             // Foreign modules inherit level from parents.
             ast::ItemKind::ForeignMod(..) => {
-                let parent_level =
-                    self.r.access_levels.get_access_level(self.r.local_parent(def_id));
-                self.set_access_level(item.id, parent_level);
+                let parent_id = self.r.local_parent(def_id);
+                self.update_effective_vis(
+                    def_id,
+                    Visibility::Public,
+                    parent_id,
+                    AccessLevel::Public,
+                );
             }
 
             // Only exported `macro_rules!` items are public, but they always are
             ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => {
-                if item.attrs.iter().any(|attr| attr.has_name(sym::macro_export)) {
-                    self.set_access_level(item.id, Some(AccessLevel::Public));
-                }
+                let parent_id = self.r.local_parent(def_id);
+                let vis = self.r.visibilities.get(&def_id).unwrap().clone();
+                self.update_effective_vis(def_id, vis, parent_id, AccessLevel::Public);
             }
 
             ast::ItemKind::Mod(..) => {
@@ -151,19 +213,24 @@ impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> {
                 self.set_bindings_access_level(def_id);
                 for variant in variants {
                     let variant_def_id = self.r.local_def_id(variant.id);
-                    let variant_level = self.r.access_levels.get_access_level(variant_def_id);
                     for field in variant.data.fields() {
-                        self.set_access_level(field.id, variant_level);
+                        let field_def_id = self.r.local_def_id(field.id);
+                        let vis = self.r.visibilities.get(&field_def_id).unwrap().clone();
+                        self.update_effective_vis(
+                            field_def_id,
+                            vis,
+                            variant_def_id,
+                            AccessLevel::Public,
+                        );
                     }
                 }
             }
 
             ast::ItemKind::Struct(ref def, _) | ast::ItemKind::Union(ref def, _) => {
-                let inherited_level = self.r.access_levels.get_access_level(def_id);
                 for field in def.fields() {
-                    if field.vis.kind.is_pub() {
-                        self.set_access_level(field.id, inherited_level);
-                    }
+                    let field_def_id = self.r.local_def_id(field.id);
+                    let vis = self.r.visibilities.get(&field_def_id).unwrap();
+                    self.update_effective_vis(field_def_id, *vis, def_id, AccessLevel::Public);
                 }
             }
 

src/test/ui/privacy/access_levels.rs

@@ -1,57 +1,57 @@
 #![feature(rustc_attrs)]
 
 #[rustc_effective_visibility]
-mod outer { //~ ERROR Public: pub(self), Exported: pub(self), Reachable: pub(self), ReachableFromImplTrait: pub(self)
+mod outer { //~ ERROR Public: pub(access_levels), Exported: pub(access_levels), Reachable: pub(access_levels), ReachableFromImplTrait: pub(access_levels)
     #[rustc_effective_visibility]
-    pub mod inner1 { //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+    pub mod inner1 { //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
 
         #[rustc_effective_visibility]
-        extern "C" {} //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+        extern "C" {} //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
 
         #[rustc_effective_visibility]
-        pub trait PubTrait { //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+        pub trait PubTrait { //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
             #[rustc_effective_visibility]
-            const A: i32; //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+            const A: i32; //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
             #[rustc_effective_visibility]
-            type B; //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+            type B; //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
         }
 
         #[rustc_effective_visibility]
-        struct PrivStruct; //~ ERROR Public: pub(self), Exported: pub(self), Reachable: pub(self), ReachableFromImplTrait: pub(self)
+        struct PrivStruct; //~ ERROR Public: pub(inner1), Exported: pub(inner1), Reachable: pub(inner1), ReachableFromImplTrait: pub(inner1)
 
         #[rustc_effective_visibility]
-        pub union PubUnion { //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+        pub union PubUnion { //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
             #[rustc_effective_visibility]
-            a: u8, //~ ERROR Public: pub(self), Exported: pub(self), Reachable: pub(self), ReachableFromImplTrait: pub(self)
+            a: u8, //~ ERROR Public: pub(inner1), Exported: pub(inner1), Reachable: pub(inner1), ReachableFromImplTrait: pub(inner1)
             #[rustc_effective_visibility]
-            pub b: u8, //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+            pub b: u8, //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
         }
 
         #[rustc_effective_visibility]
-        pub enum Enum { //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+        pub enum Enum { //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
             #[rustc_effective_visibility]
-            A( //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+            A( //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
                 #[rustc_effective_visibility]
-                PubUnion,  //~ ERROR Public: pub(self), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+                PubUnion,   //~ ERROR Public: pub(access_levels), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
             ),
         }
     }
 
     #[rustc_effective_visibility]
-    macro_rules! none_macro { //~ Public: pub(self), Exported: pub(self), Reachable: pub(self), ReachableFromImplTrait: pub(self)
+    macro_rules! none_macro { //~ Public: pub(access_levels), Exported: pub(access_levels), Reachable: pub(access_levels), ReachableFromImplTrait: pub(access_levels)
         () => {};
     }
 
     #[macro_export]
     #[rustc_effective_visibility]
-    macro_rules! public_macro { //~ Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+    macro_rules! public_macro {  //~ Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
         () => {};
     }
 
     #[rustc_effective_visibility]
-    pub struct ReachableStruct { //~ ERROR Public: pub(self), Exported: pub(self), Reachable: pub, ReachableFromImplTrait: pub
+    pub struct ReachableStruct { //~ ERROR Public: pub(access_levels), Exported: pub(access_levels), Reachable: pub, ReachableFromImplTrait: pub
         #[rustc_effective_visibility]
-        pub a: u8, //~ ERROR Public: pub(self), Exported: pub(self), Reachable: pub, ReachableFromImplTrait: pub
+        pub a: u8, //~ ERROR Public: pub(access_levels), Exported: pub(access_levels), Reachable: pub, ReachableFromImplTrait: pub
     }
 }