Auto merge of rust-lang#118657 - petrochenkov:feedvis, r=cjgillot

resolve: Replace visibility table in resolver outputs with query feeding

Also feed missing visibilities for import stems and trait impl items, which were previously evaluated lazily.

I suspect that in general this approach should work for queries that are 1) executed for most keys and 2) have results that are cheap to hash (do not have spans, in particular).
Visibility query matches that description.

Author: bors

compiler/rustc_ast_lowering/src/lib.rs

@@ -59,10 +59,8 @@ use rustc_hir::def::{DefKind, LifetimeRes, Namespace, PartialRes, PerNS, Res};
 use rustc_hir::def_id::{LocalDefId, CRATE_DEF_ID, LOCAL_CRATE};
 use rustc_hir::{ConstArg, GenericArg, ItemLocalId, ParamName, TraitCandidate};
 use rustc_index::{Idx, IndexSlice, IndexVec};
-use rustc_middle::{
-    span_bug,
-    ty::{ResolverAstLowering, TyCtxt},
-};
+use rustc_middle::span_bug;
+use rustc_middle::ty::{ResolverAstLowering, TyCtxt, Visibility};
 use rustc_session::parse::{add_feature_diagnostics, feature_err};
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{DesugaringKind, Span, DUMMY_SP};
@@ -1653,6 +1651,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         );
         debug!(?opaque_ty_def_id);
 
+        // Meaningless, but provided so that all items have visibilities.
+        let parent_mod = self.tcx.parent_module_from_def_id(opaque_ty_def_id).to_def_id();
+        self.tcx.feed_local_def_id(opaque_ty_def_id).visibility(Visibility::Restricted(parent_mod));
+
         // Map from captured (old) lifetime to synthetic (new) lifetime.
         // Used to resolve lifetimes in the bounds of the opaque.
         let mut captured_to_synthesized_mapping = FxHashMap::default();

compiler/rustc_middle/src/hir/map/mod.rs

@@ -1142,7 +1142,10 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {
         tcx.sess.opts.dep_tracking_hash(true).hash_stable(&mut hcx, &mut stable_hasher);
         tcx.stable_crate_id(LOCAL_CRATE).hash_stable(&mut hcx, &mut stable_hasher);
         // Hash visibility information since it does not appear in HIR.
-        resolutions.visibilities.hash_stable(&mut hcx, &mut stable_hasher);
+        // FIXME: Figure out how to remove `visibilities_for_hashing` by hashing visibilities on
+        // the fly in the resolver, storing only their accumulated hash in `ResolverGlobalCtxt`,
+        // and combining it with other hashes here.
+        resolutions.visibilities_for_hashing.hash_stable(&mut hcx, &mut stable_hasher);
         stable_hasher.finish()
     });
 

compiler/rustc_middle/src/ty/mod.rs

@@ -152,7 +152,7 @@ pub struct ResolverOutputs {
 
 #[derive(Debug)]
 pub struct ResolverGlobalCtxt {
-    pub visibilities: FxHashMap<LocalDefId, Visibility>,
+    pub visibilities_for_hashing: Vec<(LocalDefId, Visibility)>,
     /// Item with a given `LocalDefId` was defined during macro expansion with ID `ExpnId`.
     pub expn_that_defined: FxHashMap<LocalDefId, ExpnId>,
     pub effective_visibilities: EffectiveVisibilities,

compiler/rustc_privacy/src/lib.rs

@@ -23,12 +23,11 @@ use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId, CRATE_DEF_ID};
 use rustc_hir::intravisit::{self, Visitor};
-use rustc_hir::{AssocItemKind, ForeignItemKind, ItemId, Node, PatKind};
+use rustc_hir::{AssocItemKind, ForeignItemKind, ItemId, PatKind};
 use rustc_middle::bug;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility, Level};
 use rustc_middle::query::Providers;
-use rustc_middle::span_bug;
 use rustc_middle::ty::GenericArgs;
 use rustc_middle::ty::{self, Const, GenericParamDefKind};
 use rustc_middle::ty::{TraitRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor};
@@ -1766,64 +1765,22 @@ impl<'tcx> PrivateItemsInPublicInterfacesChecker<'tcx, '_> {
 
 pub fn provide(providers: &mut Providers) {
     *providers = Providers {
-        visibility,
+        visibility: |tcx, def_id| {
+            // Unique types created for closures participate in type privacy checking.
+            // They have visibilities inherited from the module they are defined in.
+            // FIXME: Consider evaluating visibilities for closures eagerly, like for all
+            // other nodes. However, unlike for others, for closures it may cause a perf
+            // regression, because closure visibilities are not commonly queried.
+            assert_eq!(tcx.def_kind(def_id), DefKind::Closure);
+            ty::Visibility::Restricted(tcx.parent_module_from_def_id(def_id).to_def_id())
+        },
         effective_visibilities,
         check_private_in_public,
         check_mod_privacy,
         ..*providers
     };
 }
 
-fn visibility(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Visibility<DefId> {
-    local_visibility(tcx, def_id).to_def_id()
-}
-
-fn local_visibility(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Visibility {
-    match tcx.resolutions(()).visibilities.get(&def_id) {
-        Some(vis) => *vis,
-        None => {
-            let hir_id = tcx.local_def_id_to_hir_id(def_id);
-            match tcx.hir_node(hir_id) {
-                // Unique types created for closures participate in type privacy checking.
-                // They have visibilities inherited from the module they are defined in.
-                Node::Expr(hir::Expr { kind: hir::ExprKind::Closure{..}, .. })
-                // - AST lowering creates dummy `use` items which don't
-                //   get their entries in the resolver's visibility table.
-                // - AST lowering also creates opaque type items with inherited visibilities.
-                //   Visibility on them should have no effect, but to avoid the visibility
-                //   query failing on some items, we provide it for opaque types as well.
-                | Node::Item(hir::Item {
-                    kind: hir::ItemKind::Use(_, hir::UseKind::ListStem)
-                        | hir::ItemKind::OpaqueTy(..),
-                    ..
-                }) => ty::Visibility::Restricted(tcx.parent_module(hir_id).to_local_def_id()),
-                // Visibilities of trait impl items are inherited from their traits
-                // and are not filled in resolve.
-                Node::ImplItem(impl_item) => {
-                    match tcx.hir_node_by_def_id(tcx.hir().get_parent_item(hir_id).def_id) {
-                        Node::Item(hir::Item {
-                            kind: hir::ItemKind::Impl(hir::Impl { of_trait: Some(tr), .. }),
-                            ..
-                        }) => tr.path.res.opt_def_id().map_or_else(
-                            || {
-                                tcx.sess.span_delayed_bug(tr.path.span, "trait without a def-id");
-                                ty::Visibility::Public
-                            },
-                            |def_id| tcx.visibility(def_id).expect_local(),
-                        ),
-                        _ => span_bug!(impl_item.span, "the parent is not a trait impl"),
-                    }
-                }
-                _ => span_bug!(
-                    tcx.def_span(def_id),
-                    "visibility table unexpectedly missing a def-id: {:?}",
-                    def_id,
-                ),
-            }
-        }
-    }
-}
-
 fn check_mod_privacy(tcx: TyCtxt<'_>, module_def_id: LocalModDefId) {
     // Check privacy of names not checked in previous compilation stages.
     let mut visitor = NamePrivacyVisitor {

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -236,7 +236,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                     // (i.e. variants, fields, and trait items) inherits from the visibility
                     // of the enum or trait.
                     ModuleKind::Def(DefKind::Enum | DefKind::Trait, def_id, _) => {
-                        self.r.visibilities[&def_id.expect_local()]
+                        self.r.tcx.visibility(def_id).expect_local()
                     }
                     // Otherwise, the visibility is restricted to the nearest parent `mod` item.
                     _ => ty::Visibility::Restricted(
@@ -404,6 +404,10 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
             parent_prefix, use_tree, nested
         );
 
+        if nested {
+            self.r.feed_visibility(self.r.local_def_id(id), vis);
+        }
+
         let mut prefix_iter = parent_prefix
             .iter()
             .cloned()
@@ -442,8 +446,6 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                 let mut source = module_path.pop().unwrap();
                 let mut type_ns_only = false;
 
-                self.r.visibilities.insert(self.r.local_def_id(id), vis);
-
                 if nested {
                     // Correctly handle `self`
                     if source.ident.name == kw::SelfLower {
@@ -557,7 +559,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                     max_vis: Cell::new(None),
                     id,
                 };
-                self.r.visibilities.insert(self.r.local_def_id(id), vis);
+
                 self.add_import(prefix, kind, use_tree.span, item, root_span, item.id, vis);
             }
             ast::UseTreeKind::Nested(ref items) => {
@@ -636,7 +638,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
         let def_kind = self.r.tcx.def_kind(def_id);
         let res = Res::Def(def_kind, def_id);
 
-        self.r.visibilities.insert(local_def_id, vis);
+        self.r.feed_visibility(local_def_id, vis);
 
         match item.kind {
             ItemKind::Use(ref use_tree) => {
@@ -753,7 +755,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                     let ctor_def_id = self.r.local_def_id(ctor_node_id);
                     let ctor_res = self.res(ctor_def_id);
                     self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, sp, expansion));
-                    self.r.visibilities.insert(ctor_def_id, ctor_vis);
+                    self.r.feed_visibility(ctor_def_id, ctor_vis);
                     // We need the field visibility spans also for the constructor for E0603.
                     self.insert_field_visibilities_local(ctor_def_id.to_def_id(), vdata);
 
@@ -897,7 +899,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
         let expansion = self.parent_scope.expansion;
         let vis = self.resolve_visibility(&item.vis);
         self.r.define(parent, item.ident, ns, (self.res(def_id), vis, item.span, expansion));
-        self.r.visibilities.insert(local_def_id, vis);
+        self.r.feed_visibility(local_def_id, vis);
     }
 
     fn build_reduced_graph_for_block(&mut self, block: &Block) {
@@ -1228,7 +1230,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                 self.r.check_reserved_macro_name(ident, res);
                 self.insert_unused_macro(ident, def_id, item.id);
             }
-            self.r.visibilities.insert(def_id, vis);
+            self.r.feed_visibility(def_id, vis);
             let scope = self.r.arenas.alloc_macro_rules_scope(MacroRulesScope::Binding(
                 self.r.arenas.alloc_macro_rules_binding(MacroRulesBinding {
                     parent_macro_rules_scope: parent_scope.macro_rules,
@@ -1252,7 +1254,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
                 self.insert_unused_macro(ident, def_id, item.id);
             }
             self.r.define(module, ident, MacroNS, (res, vis, span, expansion));
-            self.r.visibilities.insert(def_id, vis);
+            self.r.feed_visibility(def_id, vis);
             self.parent_scope.macro_rules
         }
     }
@@ -1354,7 +1356,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
             // Trait impl item visibility is inherited from its trait when not specified
             // explicitly. In that case we cannot determine it here in early resolve,
             // so we leave a hole in the visibility table to be filled later.
-            self.r.visibilities.insert(local_def_id, vis);
+            self.r.feed_visibility(local_def_id, vis);
         }
 
         if ctxt == AssocCtxt::Trait {
@@ -1432,7 +1434,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
             self.visit_invoc(sf.id);
         } else {
             let vis = self.resolve_visibility(&sf.vis);
-            self.r.visibilities.insert(self.r.local_def_id(sf.id), vis);
+            self.r.feed_visibility(self.r.local_def_id(sf.id), vis);
             visit::walk_field_def(self, sf);
         }
     }
@@ -1453,7 +1455,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
         let def_id = self.r.local_def_id(variant.id);
         let vis = self.resolve_visibility(&variant.vis);
         self.r.define(parent, ident, TypeNS, (self.res(def_id), vis, variant.span, expn_id));
-        self.r.visibilities.insert(def_id, vis);
+        self.r.feed_visibility(def_id, vis);
 
         // If the variant is marked as non_exhaustive then lower the visibility to within the crate.
         let ctor_vis =
@@ -1468,7 +1470,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
             let ctor_def_id = self.r.local_def_id(ctor_node_id);
             let ctor_res = self.res(ctor_def_id);
             self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, variant.span, expn_id));
-            self.r.visibilities.insert(ctor_def_id, ctor_vis);
+            self.r.feed_visibility(ctor_def_id, ctor_vis);
         }
 
         // Record field names for error reporting.

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -186,7 +186,7 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
     ) -> Option<Option<Visibility>> {
         match parent_id {
             ParentId::Def(def_id) => (nominal_vis != self.current_private_vis
-                && self.r.visibilities[&def_id] != self.current_private_vis)
+                && self.r.tcx.local_visibility(def_id) != self.current_private_vis)
                 .then_some(Some(self.current_private_vis)),
             ParentId::Import(_) => Some(None),
         }
@@ -222,7 +222,7 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
     }
 
     fn update_field(&mut self, def_id: LocalDefId, parent_id: LocalDefId) {
-        self.update_def(def_id, self.r.visibilities[&def_id], ParentId::Def(parent_id));
+        self.update_def(def_id, self.r.tcx.local_visibility(def_id), ParentId::Def(parent_id));
     }
 }
 

compiler/rustc_resolve/src/late.rs

@@ -3112,6 +3112,8 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
             | (DefKind::AssocFn, AssocItemKind::Fn(..))
             | (DefKind::AssocConst, AssocItemKind::Const(..)) => {
                 self.r.record_partial_res(id, PartialRes::new(res));
+                let vis = self.r.tcx.visibility(id_in_trait).expect_local();
+                self.r.feed_visibility(self.r.local_def_id(id), vis);
                 return;
             }
             _ => {}

compiler/rustc_resolve/src/lib.rs

@@ -1007,8 +1007,7 @@ pub struct Resolver<'a, 'tcx> {
 
     /// Maps glob imports to the names of items actually imported.
     glob_map: FxHashMap<LocalDefId, FxHashSet<Symbol>>,
-    /// Visibilities in "lowered" form, for all entities that have them.
-    visibilities: FxHashMap<LocalDefId, ty::Visibility>,
+    visibilities_for_hashing: Vec<(LocalDefId, ty::Visibility)>,
     used_imports: FxHashSet<NodeId>,
     maybe_unused_trait_imports: FxIndexSet<LocalDefId>,
 
@@ -1295,9 +1294,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             &mut FxHashMap::default(),
         );
 
-        let mut visibilities = FxHashMap::default();
-        visibilities.insert(CRATE_DEF_ID, ty::Visibility::Public);
-
         let mut def_id_to_node_id = IndexVec::default();
         assert_eq!(def_id_to_node_id.push(CRATE_NODE_ID), CRATE_DEF_ID);
         let mut node_id_to_def_id = FxHashMap::default();
@@ -1363,7 +1359,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             ast_transform_scopes: FxHashMap::default(),
 
             glob_map: Default::default(),
-            visibilities,
+            visibilities_for_hashing: Default::default(),
             used_imports: FxHashSet::default(),
             maybe_unused_trait_imports: Default::default(),
 
@@ -1450,6 +1446,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
         let root_parent_scope = ParentScope::module(graph_root, &resolver);
         resolver.invocation_parent_scopes.insert(LocalExpnId::ROOT, root_parent_scope);
+        resolver.feed_visibility(CRATE_DEF_ID, ty::Visibility::Public);
 
         resolver
     }
@@ -1497,10 +1494,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         Default::default()
     }
 
+    fn feed_visibility(&mut self, def_id: LocalDefId, vis: ty::Visibility) {
+        self.tcx.feed_local_def_id(def_id).visibility(vis.to_def_id());
+        self.visibilities_for_hashing.push((def_id, vis));
+    }
+
     pub fn into_outputs(self) -> ResolverOutputs {
         let proc_macros = self.proc_macros.iter().map(|id| self.local_def_id(*id)).collect();
         let expn_that_defined = self.expn_that_defined;
-        let visibilities = self.visibilities;
         let extern_crate_map = self.extern_crate_map;
         let maybe_unused_trait_imports = self.maybe_unused_trait_imports;
         let glob_map = self.glob_map;
@@ -1517,7 +1518,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
         let global_ctxt = ResolverGlobalCtxt {
             expn_that_defined,
-            visibilities,
+            visibilities_for_hashing: self.visibilities_for_hashing,
             effective_visibilities,
             extern_crate_map,
             module_children: self.module_children,