Skip to content

Commit

Permalink
Auto merge of #118657 - petrochenkov:feedvis, r=<try>
Browse files Browse the repository at this point in the history 
resolve: Replace visibility table in resolver outputs with query feeding

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, so let's check.
  • Loading branch information
@bors
bors committed Dec 5, 2023
2 parents 56278a6 + 1f539c6 commit 526830d
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 66 deletions.
4 changes: 0 additions & 4 deletions compiler/rustc_middle/src/hir/map/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1077,8 +1077,6 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {

let upstream_crates = upstream_crates(tcx);

let resolutions = tcx.resolutions(());

// We hash the final, remapped names of all local source files so we
// don't have to include the path prefix remapping commandline args.
// If we included the full mapping in the SVH, we could only have
Expand Down Expand Up @@ -1133,8 +1131,6 @@ 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);
stable_hasher.finish()
});

Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/ty/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ pub struct ResolverOutputs {

#[derive(Debug)]
pub struct ResolverGlobalCtxt {
pub visibilities: FxHashMap<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,
Expand Down
71 changes: 33 additions & 38 deletions compiler/rustc_privacy/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1801,48 +1801,43 @@ fn visibility(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Visibility<DefId> {
}

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().get(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(..),
let hir_id = tcx.local_def_id_to_hir_id(def_id);
match tcx.hir().get(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().get_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), .. }),
..
}) => 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().get_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,
}) => 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,
),
}
}

Expand Down
24 changes: 12 additions & 12 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,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(
Expand Down Expand Up @@ -437,7 +437,7 @@ 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);
self.r.feed_visibility(self.r.local_def_id(id), vis);

if nested {
// Correctly handle `self`
Expand Down Expand Up @@ -552,7 +552,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.r.feed_visibility(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) => {
Expand Down Expand Up @@ -629,7 +629,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
let local_def_id = self.r.local_def_id(item.id);
let def_id = local_def_id.to_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) => {
Expand Down Expand Up @@ -753,7 +753,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
let ctor_res =
Res::Def(DefKind::Ctor(CtorOf::Struct, ctor_kind), ctor_def_id.to_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);

Expand Down Expand Up @@ -899,7 +899,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
let vis = self.resolve_visibility(&item.vis);
let res = Res::Def(def_kind, def_id);
self.r.define(parent, item.ident, ns, (res, 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) {
Expand Down Expand Up @@ -1233,7 +1233,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,
Expand All @@ -1257,7 +1257,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
}
}
Expand Down Expand Up @@ -1359,7 +1359,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 {
Expand Down Expand Up @@ -1438,7 +1438,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);
}
}
Expand All @@ -1460,7 +1460,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
let res = Res::Def(DefKind::Variant, def_id.to_def_id());
let vis = self.resolve_visibility(&variant.vis);
self.r.define(parent, ident, TypeNS, (res, 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 =
Expand All @@ -1476,7 +1476,7 @@ impl<'a, 'b, 'tcx> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
let ctor_res =
Res::Def(DefKind::Ctor(CtorOf::Variant, ctor_kind), ctor_def_id.to_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.
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_resolve/src/effective_visibilities.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand Down Expand Up @@ -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));
}
}

Expand Down
14 changes: 5 additions & 9 deletions compiler/rustc_resolve/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ mod imports;
mod late;
mod macros;
pub mod rustdoc;

rustc_fluent_macro::fluent_messages! { "../messages.ftl" }

#[derive(Debug)]
Expand Down Expand Up @@ -1007,8 +1006,6 @@ 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>,
used_imports: FxHashSet<NodeId>,
maybe_unused_trait_imports: FxIndexSet<LocalDefId>,

Expand Down Expand Up @@ -1295,9 +1292,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();
Expand Down Expand Up @@ -1363,7 +1357,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ast_transform_scopes: FxHashMap::default(),

glob_map: Default::default(),
visibilities,
used_imports: FxHashSet::default(),
maybe_unused_trait_imports: Default::default(),

Expand Down Expand Up @@ -1450,6 +1443,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
}
Expand Down Expand Up @@ -1497,10 +1491,13 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
Default::default()
}

fn feed_visibility(&self, def_id: LocalDefId, vis: ty::Visibility) {
self.tcx.feed_local_def_id(def_id).visibility(vis.to_def_id());
}

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;
Expand All @@ -1517,7 +1514,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {

let global_ctxt = ResolverGlobalCtxt {
expn_that_defined,
visibilities,
effective_visibilities,
extern_crate_map,
module_children: self.module_children,
Expand Down

0 comments on commit 526830d

Please sign in to comment.