resolve: More detailed effective visibility tracking for imports

Also drop `extern` blocks from the effective visibility table, they are nominally private and it doesn't make sense to keep them there.

compiler/rustc_middle/src/middle/privacy.rs

@@ -7,6 +7,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_macros::HashStable;
 use rustc_query_system::ich::StableHashingContext;
 use rustc_span::def_id::LocalDefId;
+use std::hash::Hash;
 
 /// Represents the levels of effective visibility an item can have.
 ///
@@ -74,9 +75,9 @@ impl EffectiveVisibility {
 }
 
 /// Holds a map of effective visibilities for reachable HIR nodes.
-#[derive(Default, Clone, Debug)]
-pub struct EffectiveVisibilities {
-    map: FxHashMap<LocalDefId, EffectiveVisibility>,
+#[derive(Clone, Debug)]
+pub struct EffectiveVisibilities<Id = LocalDefId> {
+    map: FxHashMap<Id, EffectiveVisibility>,
 }
 
 impl EffectiveVisibilities {
@@ -111,10 +112,6 @@ impl EffectiveVisibilities {
         })
     }
 
-    pub fn effective_vis(&self, id: LocalDefId) -> Option<&EffectiveVisibility> {
-        self.map.get(&id)
-    }
-
     pub fn iter(&self) -> impl Iterator<Item = (&LocalDefId, &EffectiveVisibility)> {
         self.map.iter()
     }
@@ -136,27 +133,31 @@ impl EffectiveVisibilities {
         }
         self.map.insert(id, effective_vis);
     }
+}
+
+impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
+    pub fn effective_vis(&self, id: Id) -> Option<&EffectiveVisibility> {
+        self.map.get(&id)
+    }
 
     // `parent_id` is not necessarily a parent in source code tree,
     // it is the node from which the maximum effective visibility is inherited.
     pub fn update(
         &mut self,
-        id: LocalDefId,
+        id: Id,
         nominal_vis: Visibility,
-        default_vis: impl FnOnce() -> Visibility,
-        parent_id: LocalDefId,
+        default_vis: Visibility,
+        inherited_eff_vis: Option<EffectiveVisibility>,
         level: Level,
         tree: impl DefIdTree,
     ) -> bool {
         let mut changed = false;
-        let mut current_effective_vis = self.effective_vis(id).copied().unwrap_or_else(|| {
-            if id.is_top_level_module() {
-                EffectiveVisibility::from_vis(Visibility::Public)
-            } else {
-                EffectiveVisibility::from_vis(default_vis())
-            }
-        });
-        if let Some(inherited_effective_vis) = self.effective_vis(parent_id) {
+        let mut current_effective_vis = self
+            .map
+            .get(&id)
+            .copied()
+            .unwrap_or_else(|| EffectiveVisibility::from_vis(default_vis));
+        if let Some(inherited_effective_vis) = inherited_eff_vis {
             let mut inherited_effective_vis_at_prev_level =
                 *inherited_effective_vis.at_level(level);
             let mut calculated_effective_vis = inherited_effective_vis_at_prev_level;
@@ -194,6 +195,12 @@ impl EffectiveVisibilities {
     }
 }
 
+impl<Id> Default for EffectiveVisibilities<Id> {
+    fn default() -> Self {
+        EffectiveVisibilities { map: Default::default() }
+    }
+}
+
 impl<'a> HashStable<StableHashingContext<'a>> for EffectiveVisibilities {
     fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
         let EffectiveVisibilities { ref map } = *self;

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -1,16 +1,38 @@
-use crate::{ImportKind, NameBindingKind, Resolver};
+use crate::{ImportKind, NameBinding, NameBindingKind, Resolver, ResolverTree};
 use rustc_ast::ast;
 use rustc_ast::visit;
 use rustc_ast::visit::Visitor;
 use rustc_ast::Crate;
 use rustc_ast::EnumDef;
+use rustc_data_structures::intern::Interned;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::def_id::CRATE_DEF_ID;
-use rustc_middle::middle::privacy::Level;
-use rustc_middle::ty::{DefIdTree, Visibility};
+use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility, Level};
+use rustc_middle::ty::Visibility;
+
+type ImportId<'a> = Interned<'a, NameBinding<'a>>;
+
+#[derive(Clone, Copy)]
+enum ParentId<'a> {
+    Def(LocalDefId),
+    Import(ImportId<'a>),
+}
+
+impl ParentId<'_> {
+    fn level(self) -> Level {
+        match self {
+            ParentId::Def(_) => Level::Direct,
+            ParentId::Import(_) => Level::Reexported,
+        }
+    }
+}
 
 pub struct EffectiveVisibilitiesVisitor<'r, 'a> {
     r: &'r mut Resolver<'a>,
+    /// While walking import chains we need to track effective visibilities per-binding, and def id
+    /// keys in `Resolver::effective_visibilities` are not enough for that, because multiple
+    /// bindings can correspond to a single def id in imports. So we keep a separate table.
+    import_effective_visibilities: EffectiveVisibilities<ImportId<'a>>,
     changed: bool,
 }
 
@@ -19,21 +41,25 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
     /// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we
     /// need access to a TyCtxt for that.
     pub fn compute_effective_visibilities<'c>(r: &'r mut Resolver<'a>, krate: &'c Crate) {
-        let mut visitor = EffectiveVisibilitiesVisitor { r, changed: false };
+        let mut visitor = EffectiveVisibilitiesVisitor {
+            r,
+            import_effective_visibilities: Default::default(),
+            changed: false,
+        };
 
-        visitor.update(CRATE_DEF_ID, Visibility::Public, CRATE_DEF_ID, Level::Direct);
+        visitor.update(CRATE_DEF_ID, CRATE_DEF_ID);
         visitor.set_bindings_effective_visibilities(CRATE_DEF_ID);
 
         while visitor.changed {
-            visitor.reset();
+            visitor.changed = false;
             visit::walk_crate(&mut visitor, krate);
         }
 
         info!("resolve::effective_visibilities: {:#?}", r.effective_visibilities);
     }
 
-    fn reset(&mut self) {
-        self.changed = false;
+    fn nearest_normal_mod(&mut self, def_id: LocalDefId) -> LocalDefId {
+        self.r.get_nearest_non_block_module(def_id.to_def_id()).nearest_parent_mod().expect_local()
     }
 
     /// Update effective visibilities of bindings in the given module,
@@ -48,92 +74,114 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
                 // Set the given effective visibility level to `Level::Direct` and
                 // sets the rest of the `use` chain to `Level::Reexported` until
                 // we hit the actual exported item.
+                let mut parent_id = ParentId::Def(module_id);
+                while let NameBindingKind::Import { binding: nested_binding, import, .. } =
+                    binding.kind
+                {
+                    let binding_id = ImportId::new_unchecked(binding);
+                    self.update_import(binding_id, parent_id);
 
-                // FIXME: tag and is_public() condition should be removed, but assertions occur.
-                let tag = if binding.is_import() { Level::Reexported } else { Level::Direct };
-                if binding.vis.is_public() {
-                    let mut prev_parent_id = module_id;
-                    let mut level = Level::Direct;
-                    while let NameBindingKind::Import { binding: nested_binding, import, .. } =
-                        binding.kind
-                    {
+                    // Update visibilities for import ids. These are not used during this pass,
+                    // because we have more detailed binding-based information, but are used by
+                    // later passes. Effective visibility of an import def id is the maximum value
+                    // among visibilities of bindings corresponding to that def id.
+                    if let Some(node_id) = import.id() {
                         let mut update = |node_id| {
-                            self.update(
+                            self.update_def(
                                 self.r.local_def_id(node_id),
                                 binding.vis.expect_local(),
-                                prev_parent_id,
-                                level,
+                                parent_id,
                             )
                         };
-                        match import.kind {
-                            ImportKind::Single { id, additional_ids, .. } => {
-                                // In theory all the import IDs have individual visibilities and
-                                // effective visibilities, but in practice these IDs go straigth to
-                                // HIR where all their few uses assume that their (effective)
-                                // visibility applies to the whole syntactic `use` item. So we
-                                // update them all to the maximum value among the potential
-                                // individual effective visibilities. Maybe HIR for imports
-                                // shouldn't use three IDs at all.
-                                update(id);
-                                update(additional_ids.0);
-                                update(additional_ids.1);
-                                prev_parent_id = self.r.local_def_id(id);
+                        update(node_id);
+                        if let ImportKind::Single { additional_ids: (id1, id2), .. } = import.kind {
+                            // In theory all the single import IDs have individual visibilities and
+                            // effective visibilities, but in practice these IDs go straigth to HIR
+                            // where all their few uses assume that their (effective) visibility
+                            // applies to the whole syntactic `use` item. So they all get the same
+                            // value which is the maximum of all bindings. Maybe HIR for imports
+                            // shouldn't use three IDs at all.
+                            if id1 != ast::DUMMY_NODE_ID {
+                                update(id1);
                             }
-                            ImportKind::Glob { id, .. } | ImportKind::ExternCrate { id, .. } => {
-                                update(id);
-                                prev_parent_id = self.r.local_def_id(id);
-                            }
-                            ImportKind::MacroUse => {
-                                // In theory we should reset the parent id to something private
-                                // here, but `macro_use` imports always refer to external items,
-                                // so it doesn't matter and we can just do nothing.
-                            }
-                            ImportKind::MacroExport => {
-                                // In theory we should reset the parent id to something public
-                                // here, but it has the same effect as leaving the previous parent,
-                                // so we can just do nothing.
+                            if id2 != ast::DUMMY_NODE_ID {
+                                update(id2);
                             }
                         }
-
-                        level = Level::Reexported;
-                        binding = nested_binding;
                     }
+
+                    parent_id = ParentId::Import(binding_id);
+                    binding = nested_binding;
                 }
 
                 if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
-                    self.update(def_id, binding.vis.expect_local(), module_id, tag);
+                    self.update_def(def_id, binding.vis.expect_local(), parent_id);
                 }
             }
         }
     }
 
-    fn update(
-        &mut self,
-        def_id: LocalDefId,
+    fn effective_vis(&self, parent_id: ParentId<'a>) -> Option<EffectiveVisibility> {
+        match parent_id {
+            ParentId::Def(def_id) => self.r.effective_visibilities.effective_vis(def_id),
+            ParentId::Import(binding) => self.import_effective_visibilities.effective_vis(binding),
+        }
+        .copied()
+    }
+
+    /// The update is guaranteed to not change the table and we can skip it.
+    fn is_noop_update(
+        &self,
+        parent_id: ParentId<'a>,
         nominal_vis: Visibility,
-        parent_id: LocalDefId,
-        tag: Level,
-    ) {
-        let module_id = self
-            .r
-            .get_nearest_non_block_module(def_id.to_def_id())
-            .nearest_parent_mod()
-            .expect_local();
-        if nominal_vis == Visibility::Restricted(module_id)
-            || self.r.visibilities[&parent_id] == Visibility::Restricted(module_id)
-        {
+        default_vis: Visibility,
+    ) -> bool {
+        nominal_vis == default_vis
+            || match parent_id {
+                ParentId::Def(def_id) => self.r.visibilities[&def_id],
+                ParentId::Import(binding) => binding.vis.expect_local(),
+            } == default_vis
+    }
+
+    fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) {
+        let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
+        let nominal_vis = binding.vis.expect_local();
+        let default_vis = Visibility::Restricted(
+            import
+                .id()
+                .map(|id| self.nearest_normal_mod(self.r.local_def_id(id)))
+                .unwrap_or(CRATE_DEF_ID),
+        );
+        if self.is_noop_update(parent_id, nominal_vis, default_vis) {
             return;
         }
-        let mut effective_visibilities = std::mem::take(&mut self.r.effective_visibilities);
-        self.changed |= effective_visibilities.update(
+        self.changed |= self.import_effective_visibilities.update(
+            binding,
+            nominal_vis,
+            default_vis,
+            self.effective_vis(parent_id),
+            parent_id.level(),
+            ResolverTree(&self.r.definitions, &self.r.crate_loader),
+        );
+    }
+
+    fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) {
+        let default_vis = Visibility::Restricted(self.nearest_normal_mod(def_id));
+        if self.is_noop_update(parent_id, nominal_vis, default_vis) {
+            return;
+        }
+        self.changed |= self.r.effective_visibilities.update(
             def_id,
             nominal_vis,
-            || Visibility::Restricted(module_id),
-            parent_id,
-            tag,
-            &*self.r,
+            if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis },
+            self.effective_vis(parent_id),
+            parent_id.level(),
+            ResolverTree(&self.r.definitions, &self.r.crate_loader),
         );
-        self.r.effective_visibilities = effective_visibilities;
+    }
+
+    fn update(&mut self, def_id: LocalDefId, parent_id: LocalDefId) {
+        self.update_def(def_id, self.r.visibilities[&def_id], ParentId::Def(parent_id));
     }
 }
 
@@ -151,12 +199,6 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> {
                 "ast::ItemKind::MacCall encountered, this should not anymore appear at this stage"
             ),
 
-            // Foreign modules inherit level from parents.
-            ast::ItemKind::ForeignMod(..) => {
-                let parent_id = self.r.local_parent(def_id);
-                self.update(def_id, Visibility::Public, parent_id, Level::Direct);
-            }
-
             ast::ItemKind::Mod(..) => {
                 self.set_bindings_effective_visibilities(def_id);
                 visit::walk_item(self, item);
@@ -167,18 +209,14 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> {
                 for variant in variants {
                     let variant_def_id = self.r.local_def_id(variant.id);
                     for field in variant.data.fields() {
-                        let field_def_id = self.r.local_def_id(field.id);
-                        let vis = self.r.visibilities[&field_def_id];
-                        self.update(field_def_id, vis, variant_def_id, Level::Direct);
+                        self.update(self.r.local_def_id(field.id), variant_def_id);
                     }
                 }
             }
 
             ast::ItemKind::Struct(ref def, _) | ast::ItemKind::Union(ref def, _) => {
                 for field in def.fields() {
-                    let field_def_id = self.r.local_def_id(field.id);
-                    let vis = self.r.visibilities[&field_def_id];
-                    self.update(field_def_id, vis, def_id, Level::Direct);
+                    self.update(self.r.local_def_id(field.id), def_id);
                 }
             }
 
@@ -194,6 +232,7 @@ impl<'r, 'ast> Visitor<'ast> for EffectiveVisibilitiesVisitor<'ast, 'r> {
             | ast::ItemKind::TyAlias(..)
             | ast::ItemKind::TraitAlias(..)
             | ast::ItemKind::MacroDef(..)
+            | ast::ItemKind::ForeignMod(..)
             | ast::ItemKind::Fn(..) => return,
         }
     }

compiler/rustc_resolve/src/lib.rs

@@ -1106,14 +1106,27 @@ impl<'a> AsMut<Resolver<'a>> for Resolver<'a> {
     }
 }
 
+/// A minimal subset of resolver that can implemenent `DefIdTree`, sometimes
+/// required to satisfy borrow checker by avoiding borrowing the whole resolver.
+#[derive(Clone, Copy)]
+struct ResolverTree<'a, 'b>(&'a Definitions, &'a CrateLoader<'b>);
+
+impl DefIdTree for ResolverTree<'_, '_> {
+    #[inline]
+    fn opt_parent(self, id: DefId) -> Option<DefId> {
+        let ResolverTree(definitions, crate_loader) = self;
+        match id.as_local() {
+            Some(id) => definitions.def_key(id).parent,
+            None => crate_loader.cstore().def_key(id).parent,
+        }
+        .map(|index| DefId { index, ..id })
+    }
+}
+
 impl<'a, 'b> DefIdTree for &'a Resolver<'b> {
     #[inline]
     fn opt_parent(self, id: DefId) -> Option<DefId> {
-        match id.as_local() {
-            Some(id) => self.definitions.def_key(id).parent,
-            None => self.cstore().def_key(id).parent,
-        }
-        .map(|index| DefId { index, ..id })
+        ResolverTree(&self.definitions, &self.crate_loader).opt_parent(id)
     }
 }
 

src/test/ui/privacy/effective_visibilities.rs

@@ -6,7 +6,7 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub
     pub mod inner1 { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
 
         #[rustc_effective_visibility]
-        extern "C" {} //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+        extern "C" {} //~ ERROR not in the table
 
         #[rustc_effective_visibility]
         pub trait PubTrait { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub

src/test/ui/privacy/effective_visibilities.stderr

@@ -10,7 +10,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
 LL |     pub mod inner1 {
    |     ^^^^^^^^^^^^^^
 
-error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+error: not in the table
   --> $DIR/effective_visibilities.rs:9:9
    |
 LL |         extern "C" {}

src/test/ui/privacy/effective_visibilities_glob.rs

@@ -0,0 +1,21 @@
+// Effective visibility tracking for imports is fine-grained, so `S2` is not fully exported
+// even if its parent import (`m::*`) is fully exported as a `use` item.
+
+#![feature(rustc_attrs)]
+
+mod m {
+    #[rustc_effective_visibility]
+    pub struct S1 {} //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+    #[rustc_effective_visibility]
+    pub struct S2 {} //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate)
+}
+
+mod glob {
+    #[rustc_effective_visibility]
+    pub use crate::m::*; //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+}
+
+#[rustc_effective_visibility]
+pub use glob::S1; //~ ERROR Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+
+fn main() {}

src/test/ui/privacy/effective_visibilities_glob.stderr

@@ -0,0 +1,26 @@
+error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+  --> $DIR/effective_visibilities_glob.rs:8:5
+   |
+LL |     pub struct S1 {}
+   |     ^^^^^^^^^^^^^
+
+error: Direct: pub(crate), Reexported: pub(crate), Reachable: pub(crate), ReachableThroughImplTrait: pub(crate)
+  --> $DIR/effective_visibilities_glob.rs:10:5
+   |
+LL |     pub struct S2 {}
+   |     ^^^^^^^^^^^^^
+
+error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+  --> $DIR/effective_visibilities_glob.rs:15:13
+   |
+LL |     pub use crate::m::*;
+   |             ^^^^^^^^
+
+error: Direct: pub, Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
+  --> $DIR/effective_visibilities_glob.rs:19:9
+   |
+LL | pub use glob::S1;
+   |         ^^^^^^^^
+
+error: aborting due to 4 previous errors
+