Auto merge of #120006 - cjgillot:no-hir-owner, r=wesleywiser

Get rid of the hir_owner query.

This query was meant as a firewall between `hir_owner_nodes` which is supposed to change often, and the queries that only depend on the item signature. That firewall was inefficient, leaking the contents of the HIR body through `HirId`s.

`hir_owner` incurs a significant cost, as we need to hash HIR twice in multiple modes. This PR proposes to remove it, and simplify the hashing scheme.

For the future, `def_kind`, `def_span`... are much more efficient for incremental decoupling, and should be preferred.

Author: bors

compiler/rustc_ast_lowering/src/lib.rs

@@ -662,9 +662,9 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         let (opt_hash_including_bodies, attrs_hash) = if self.tcx.needs_crate_hash() {
             self.tcx.with_stable_hashing_context(|mut hcx| {
                 let mut stable_hasher = StableHasher::new();
-                hcx.with_hir_bodies(node.def_id(), &bodies, |hcx| {
-                    node.hash_stable(hcx, &mut stable_hasher)
-                });
+                node.hash_stable(&mut hcx, &mut stable_hasher);
+                // Bodies are stored out of line, so we need to pull them explicitly in the hash.
+                bodies.hash_stable(&mut hcx, &mut stable_hasher);
                 let h1 = stable_hasher.finish();
 
                 let mut stable_hasher = StableHasher::new();

compiler/rustc_hir/src/hir.rs

@@ -841,7 +841,7 @@ pub struct OwnerNodes<'tcx> {
 }
 
 impl<'tcx> OwnerNodes<'tcx> {
-    fn node(&self) -> OwnerNode<'tcx> {
+    pub fn node(&self) -> OwnerNode<'tcx> {
         use rustc_index::Idx;
         let node = self.nodes[ItemLocalId::new(0)].as_ref().unwrap().node;
         let node = node.as_owner().unwrap(); // Indexing must ensure it is an OwnerNode.
@@ -1305,7 +1305,7 @@ pub enum UnsafeSource {
     UserProvided,
 }
 
-#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
+#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, HashStable_Generic)]
 pub struct BodyId {
     pub hir_id: HirId,
 }

compiler/rustc_hir/src/stable_hash_impls.rs

@@ -12,7 +12,6 @@ use rustc_span::def_id::DefPathHash;
 pub trait HashStableContext:
     rustc_ast::HashStableContext + rustc_target::HashStableContext
 {
-    fn hash_body_id(&mut self, _: BodyId, hasher: &mut StableHasher);
 }
 
 impl<HirCtx: crate::HashStableContext> ToStableHashKey<HirCtx> for HirId {
@@ -80,12 +79,6 @@ impl<HirCtx: crate::HashStableContext> ToStableHashKey<HirCtx> for ForeignItemId
     }
 }
 
-impl<HirCtx: crate::HashStableContext> HashStable<HirCtx> for BodyId {
-    fn hash_stable(&self, hcx: &mut HirCtx, hasher: &mut StableHasher) {
-        hcx.hash_body_id(*self, hasher)
-    }
-}
-
 // The following implementations of HashStable for `ItemId`, `TraitItemId`, and
 // `ImplItemId` deserve special attention. Normally we do not hash `NodeId`s within
 // the HIR, since they just signify a HIR nodes own path. But `ItemId` et al

compiler/rustc_incremental/src/assert_dep_graph.rs

@@ -128,9 +128,11 @@ impl<'tcx> IfThisChanged<'tcx> {
             if attr.has_name(sym::rustc_if_this_changed) {
                 let dep_node_interned = self.argument(attr);
                 let dep_node = match dep_node_interned {
-                    None => {
-                        DepNode::from_def_path_hash(self.tcx, def_path_hash, dep_kinds::hir_owner)
-                    }
+                    None => DepNode::from_def_path_hash(
+                        self.tcx,
+                        def_path_hash,
+                        dep_kinds::hir_owner_nodes,
+                    ),
                     Some(n) => {
                         match DepNode::from_label_string(self.tcx, n.as_str(), def_path_hash) {
                             Ok(n) => n,

compiler/rustc_incremental/src/persist/dirty_clean.rs

@@ -57,8 +57,7 @@ const BASE_FN: &[&str] = &[
 
 /// DepNodes for Hir, which is pretty much everything
 const BASE_HIR: &[&str] = &[
-    // hir_owner and hir_owner_nodes should be computed for all nodes
-    label_strs::hir_owner,
+    // hir_owner_nodes should be computed for all nodes
     label_strs::hir_owner_nodes,
 ];
 

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

@@ -1,4 +1,4 @@
-use crate::hir::{ModuleItems, Owner};
+use crate::hir::ModuleItems;
 use crate::middle::debugger_visualizer::DebuggerVisualizerFile;
 use crate::query::LocalCrate;
 use crate::ty::TyCtxt;
@@ -108,7 +108,7 @@ impl<'hir> Iterator for ParentOwnerIterator<'hir> {
         if self.current_id.local_id.index() != 0 {
             self.current_id.local_id = ItemLocalId::new(0);
             if let Some(node) = self.map.tcx.hir_owner(self.current_id.owner) {
-                return Some((self.current_id.owner, node.node));
+                return Some((self.current_id.owner, node));
             }
         }
         if self.current_id == CRATE_HIR_ID {
@@ -126,23 +126,23 @@ impl<'hir> Iterator for ParentOwnerIterator<'hir> {
 
             // If this `HirId` doesn't have an entry, skip it and look for its `parent_id`.
             if let Some(node) = self.map.tcx.hir_owner(self.current_id.owner) {
-                return Some((self.current_id.owner, node.node));
+                return Some((self.current_id.owner, node));
             }
         }
     }
 }
 
 impl<'tcx> TyCtxt<'tcx> {
+    #[inline]
+    fn hir_owner(self, owner: OwnerId) -> Option<OwnerNode<'tcx>> {
+        Some(self.hir_owner_nodes(owner).as_owner()?.node())
+    }
+
     /// Retrieves the `hir::Node` corresponding to `id`, returning `None` if cannot be found.
     pub fn opt_hir_node(self, id: HirId) -> Option<Node<'tcx>> {
-        if id.local_id == ItemLocalId::from_u32(0) {
-            let owner = self.hir_owner(id.owner)?;
-            Some(owner.node.into())
-        } else {
-            let owner = self.hir_owner_nodes(id.owner).as_owner()?;
-            let node = owner.nodes[id.local_id].as_ref()?;
-            Some(node.node)
-        }
+        let owner = self.hir_owner_nodes(id.owner).as_owner()?;
+        let node = owner.nodes[id.local_id].as_ref()?;
+        Some(node.node)
     }
 
     /// Retrieves the `hir::Node` corresponding to `id`, returning `None` if cannot be found.
@@ -174,7 +174,7 @@ impl<'hir> Map<'hir> {
 
     #[inline]
     pub fn root_module(self) -> &'hir Mod<'hir> {
-        match self.tcx.hir_owner(CRATE_OWNER_ID).map(|o| o.node) {
+        match self.tcx.hir_owner(CRATE_OWNER_ID) {
             Some(OwnerNode::Crate(item)) => item,
             _ => bug!(),
         }
@@ -242,27 +242,27 @@ impl<'hir> Map<'hir> {
 
     pub fn get_generics(self, id: LocalDefId) -> Option<&'hir Generics<'hir>> {
         let node = self.tcx.hir_owner(OwnerId { def_id: id })?;
-        node.node.generics()
+        node.generics()
     }
 
     pub fn owner(self, id: OwnerId) -> OwnerNode<'hir> {
-        self.tcx.hir_owner(id).unwrap_or_else(|| bug!("expected owner for {:?}", id)).node
+        self.tcx.hir_owner(id).unwrap_or_else(|| bug!("expected owner for {:?}", id))
     }
 
     pub fn item(self, id: ItemId) -> &'hir Item<'hir> {
-        self.tcx.hir_owner(id.owner_id).unwrap().node.expect_item()
+        self.tcx.hir_owner(id.owner_id).unwrap().expect_item()
     }
 
     pub fn trait_item(self, id: TraitItemId) -> &'hir TraitItem<'hir> {
-        self.tcx.hir_owner(id.owner_id).unwrap().node.expect_trait_item()
+        self.tcx.hir_owner(id.owner_id).unwrap().expect_trait_item()
     }
 
     pub fn impl_item(self, id: ImplItemId) -> &'hir ImplItem<'hir> {
-        self.tcx.hir_owner(id.owner_id).unwrap().node.expect_impl_item()
+        self.tcx.hir_owner(id.owner_id).unwrap().expect_impl_item()
     }
 
     pub fn foreign_item(self, id: ForeignItemId) -> &'hir ForeignItem<'hir> {
-        self.tcx.hir_owner(id.owner_id).unwrap().node.expect_foreign_item()
+        self.tcx.hir_owner(id.owner_id).unwrap().expect_foreign_item()
     }
 
     pub fn body(self, id: BodyId) -> &'hir Body<'hir> {
@@ -436,7 +436,7 @@ impl<'hir> Map<'hir> {
 
     pub fn get_module(self, module: LocalModDefId) -> (&'hir Mod<'hir>, Span, HirId) {
         let hir_id = HirId::make_owner(module.to_local_def_id());
-        match self.tcx.hir_owner(hir_id.owner).map(|o| o.node) {
+        match self.tcx.hir_owner(hir_id.owner) {
             Some(OwnerNode::Item(&Item { span, kind: ItemKind::Mod(m), .. })) => (m, span, hir_id),
             Some(OwnerNode::Crate(item)) => (item, item.spans.inner_span, hir_id),
             node => panic!("not a module: {node:?}"),
@@ -726,11 +726,10 @@ impl<'hir> Map<'hir> {
 
     pub fn get_foreign_abi(self, hir_id: HirId) -> Abi {
         let parent = self.get_parent_item(hir_id);
-        if let Some(node) = self.tcx.hir_owner(parent) {
-            if let OwnerNode::Item(Item { kind: ItemKind::ForeignMod { abi, .. }, .. }) = node.node
-            {
-                return *abi;
-            }
+        if let Some(node) = self.tcx.hir_owner(parent)
+            && let OwnerNode::Item(Item { kind: ItemKind::ForeignMod { abi, .. }, .. }) = node
+        {
+            return *abi;
         }
         bug!(
             "expected foreign mod or inlined parent, found {}",
@@ -742,33 +741,32 @@ impl<'hir> Map<'hir> {
         self.tcx
             .hir_owner(OwnerId { def_id })
             .unwrap_or_else(|| bug!("expected owner for {:?}", def_id))
-            .node
     }
 
     pub fn expect_item(self, id: LocalDefId) -> &'hir Item<'hir> {
         match self.tcx.hir_owner(OwnerId { def_id: id }) {
-            Some(Owner { node: OwnerNode::Item(item), .. }) => item,
+            Some(OwnerNode::Item(item)) => item,
             _ => bug!("expected item, found {}", self.node_to_string(HirId::make_owner(id))),
         }
     }
 
     pub fn expect_impl_item(self, id: LocalDefId) -> &'hir ImplItem<'hir> {
         match self.tcx.hir_owner(OwnerId { def_id: id }) {
-            Some(Owner { node: OwnerNode::ImplItem(item), .. }) => item,
+            Some(OwnerNode::ImplItem(item)) => item,
             _ => bug!("expected impl item, found {}", self.node_to_string(HirId::make_owner(id))),
         }
     }
 
     pub fn expect_trait_item(self, id: LocalDefId) -> &'hir TraitItem<'hir> {
         match self.tcx.hir_owner(OwnerId { def_id: id }) {
-            Some(Owner { node: OwnerNode::TraitItem(item), .. }) => item,
+            Some(OwnerNode::TraitItem(item)) => item,
             _ => bug!("expected trait item, found {}", self.node_to_string(HirId::make_owner(id))),
         }
     }
 
     pub fn get_fn_output(self, def_id: LocalDefId) -> Option<&'hir FnRetTy<'hir>> {
         match self.tcx.hir_owner(OwnerId { def_id }) {
-            Some(Owner { node, .. }) => node.fn_decl().map(|fn_decl| &fn_decl.output),
+            Some(node) => node.fn_decl().map(|fn_decl| &fn_decl.output),
             _ => None,
         }
     }
@@ -782,7 +780,7 @@ impl<'hir> Map<'hir> {
 
     pub fn expect_foreign_item(self, id: OwnerId) -> &'hir ForeignItem<'hir> {
         match self.tcx.hir_owner(id) {
-            Some(Owner { node: OwnerNode::ForeignItem(item), .. }) => item,
+            Some(OwnerNode::ForeignItem(item)) => item,
             _ => {
                 bug!(
                     "expected foreign item, found {}",

compiler/rustc_middle/src/hir/mod.rs

@@ -8,34 +8,12 @@ pub mod place;
 
 use crate::query::Providers;
 use crate::ty::{EarlyBinder, ImplSubject, TyCtxt};
-use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::sync::{try_par_for_each_in, DynSend, DynSync};
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId};
 use rustc_hir::*;
-use rustc_query_system::ich::StableHashingContext;
 use rustc_span::{ErrorGuaranteed, ExpnId, DUMMY_SP};
 
-/// Top-level HIR node for current owner. This only contains the node for which
-/// `HirId::local_id == 0`, and excludes bodies.
-///
-/// This struct exists to encapsulate all access to the hir_owner query in this module, and to
-/// implement HashStable without hashing bodies.
-#[derive(Copy, Clone, Debug)]
-pub struct Owner<'tcx> {
-    node: OwnerNode<'tcx>,
-}
-
-impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for Owner<'tcx> {
-    #[inline]
-    fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
-        // Perform a shallow hash instead using the deep hash saved in `OwnerNodes`. This lets us
-        // differentiate queries that depend on the full HIR tree from those that only depend on
-        // the item signature.
-        hcx.without_hir_bodies(|hcx| self.node.hash_stable(hcx, hasher));
-    }
-}
-
 /// Gather the LocalDefId for each item-like within a module, including items contained within
 /// bodies. The Ids are in visitor order. This is used to partition a pass between modules.
 #[derive(Debug, HashStable, Encodable, Decodable)]
@@ -149,11 +127,6 @@ pub fn provide(providers: &mut Providers) {
     providers.hir_crate_items = map::hir_crate_items;
     providers.crate_hash = map::crate_hash;
     providers.hir_module_items = map::hir_module_items;
-    providers.hir_owner = |tcx, id| {
-        let owner = tcx.hir_crate(()).owners.get(id.def_id)?.as_owner()?;
-        let node = owner.node();
-        Some(Owner { node })
-    };
     providers.opt_local_def_id_to_hir_id = |tcx, id| {
         let owner = tcx.hir_crate(()).owners[id].map(|_| ());
         Some(match owner {
@@ -162,7 +135,13 @@ pub fn provide(providers: &mut Providers) {
             MaybeOwner::NonOwner(hir_id) => hir_id,
         })
     };
-    providers.hir_owner_nodes = |tcx, id| tcx.hir_crate(()).owners[id.def_id].map(|i| &i.nodes);
+    providers.hir_owner_nodes = |tcx, id| {
+        if let Some(i) = tcx.hir_crate(()).owners.get(id.def_id) {
+            i.map(|i| &i.nodes)
+        } else {
+            MaybeOwner::Phantom
+        }
+    };
     providers.hir_owner_parent = |tcx, id| {
         // Accessing the local_parent is ok since its value is hashed as part of `id`'s DefPathHash.
         tcx.opt_local_parent(id.def_id).map_or(CRATE_HIR_ID, |parent| {

compiler/rustc_middle/src/query/erase.rs

@@ -164,10 +164,6 @@ impl<T> EraseType for Option<&'_ [T]> {
     type Result = [u8; size_of::<Option<&'static [()]>>()];
 }
 
-impl EraseType for Option<rustc_middle::hir::Owner<'_>> {
-    type Result = [u8; size_of::<Option<rustc_middle::hir::Owner<'static>>>()];
-}
-
 impl EraseType for Option<mir::DestructuredConstant<'_>> {
     type Result = [u8; size_of::<Option<mir::DestructuredConstant<'static>>>()];
 }
@@ -324,7 +320,6 @@ macro_rules! tcx_lifetime {
 }
 
 tcx_lifetime! {
-    rustc_middle::hir::Owner,
     rustc_middle::middle::exported_symbols::ExportedSymbol,
     rustc_middle::mir::Const,
     rustc_middle::mir::DestructuredConstant,

compiler/rustc_middle/src/query/mod.rs

@@ -174,14 +174,6 @@ rustc_queries! {
         cache_on_disk_if { true }
     }
 
-    /// Gives access to the HIR node for the HIR owner `key`.
-    ///
-    /// This can be conveniently accessed by methods on `tcx.hir()`.
-    /// Avoid calling this query directly.
-    query hir_owner(key: hir::OwnerId) -> Option<crate::hir::Owner<'tcx>> {
-        desc { |tcx| "getting HIR owner of `{}`", tcx.def_path_str(key) }
-    }
-
     /// Gives access to the HIR ID for the given `LocalDefId` owner `key` if any.
     ///
     /// Definitions that were generated with no HIR, would be fed to return `None`.