Also store the DefPathHash as part of a DefId creation side effect, so we can reliably recreate the same DefId over and over again

Author: oli-obk

compiler/rustc_hir/src/definitions.rs

@@ -344,7 +344,11 @@ impl Definitions {
     }
 
     /// Adds a definition with a parent definition.
-    pub fn create_def(&mut self, parent: LocalDefId, data: DefPathData) -> LocalDefId {
+    pub fn create_def(
+        &mut self,
+        parent: LocalDefId,
+        data: DefPathData,
+    ) -> (LocalDefId, DefPathHash) {
         // We can't use `Debug` implementation for `LocalDefId` here, since it tries to acquire a
         // reference to `Definitions` and we're already holding a mutable reference.
         debug!(
@@ -373,7 +377,7 @@ impl Definitions {
         debug!("create_def: after disambiguation, key = {:?}", key);
 
         // Create the definition.
-        LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }
+        (LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }, def_path_hash)
     }
 
     #[inline(always)]

compiler/rustc_middle/src/ty/context.rs

@@ -4,6 +4,7 @@
 
 pub mod tls;
 
+use rustc_query_system::query::DefIdInfo;
 pub use rustc_type_ir::lift::Lift;
 
 use crate::arena::Arena;
@@ -59,7 +60,6 @@ use rustc_index::IndexVec;
 use rustc_macros::{HashStable, TyDecodable, TyEncodable};
 use rustc_query_system::dep_graph::{DepNodeIndex, TaskDepsRef};
 use rustc_query_system::ich::StableHashingContext;
-use rustc_query_system::query::DefIdInfo;
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
 use rustc_session::config::CrateType;
 use rustc_session::cstore::{CrateStoreDyn, Untracked};
@@ -73,7 +73,7 @@ use rustc_target::spec::abi;
 use rustc_type_ir::TyKind::*;
 use rustc_type_ir::WithCachedTypeInfo;
 use rustc_type_ir::{CollectAndApply, Interner, TypeFlags};
-use tracing::{debug, instrument};
+use tracing::{debug, instrument, trace};
 
 use std::assert_matches::assert_matches;
 use std::borrow::Borrow;
@@ -1330,14 +1330,15 @@ impl<'tcx> TyCtxtAt<'tcx> {
 
 impl<'tcx> TyCtxt<'tcx> {
     /// `tcx`-dependent operations performed for every created definition.
+    #[instrument(level = "trace", skip(self))]
     pub fn create_def(
         self,
         parent: LocalDefId,
         name: Symbol,
         def_kind: DefKind,
     ) -> TyCtxtFeed<'tcx, LocalDefId> {
         let data = def_kind.def_path_data(name);
-        // The following call has the side effect of modifying the tables inside `definitions`.
+        // The following create_def calls have the side effect of modifying the tables inside `definitions`.
         // These very tables are relied on by the incr. comp. engine to decode DepNodes and to
         // decode the on-disk cache.
         //
@@ -1350,31 +1351,47 @@ impl<'tcx> TyCtxt<'tcx> {
         // This is fine because:
         // - those queries are `eval_always` so we won't miss their result changing;
         // - this write will have happened before these queries are called.
-        let def_id = self.untracked.definitions.write().create_def(parent, data);
-
-        // This function modifies `self.definitions` using a side-effect.
-        // We need to ensure that these side effects are re-run by the incr. comp. engine.
-        tls::with_context(|icx| {
+        let def_id = tls::with_context(|icx| {
             match icx.task_deps {
                 // Always gets rerun anyway, so nothing to replay
-                TaskDepsRef::EvalAlways => {}
+                TaskDepsRef::EvalAlways => {
+                    let def_id = self.untracked.definitions.write().create_def(parent, data).0;
+                    trace!(?def_id, "eval always");
+                    def_id
+                }
                 // Top-level queries like the resolver get rerun every time anyway
-                TaskDepsRef::Ignore => {}
+                TaskDepsRef::Ignore => {
+                    let def_id = self.untracked.definitions.write().create_def(parent, data).0;
+                    trace!(?def_id, "ignore");
+                    def_id
+                }
                 TaskDepsRef::Forbid => bug!(
                     "cannot create definition {parent:?}, {name:?}, {def_kind:?} without being able to register task dependencies"
                 ),
                 TaskDepsRef::Allow(_) => {
-                    icx.side_effects
-                        .as_ref()
-                        .unwrap()
-                        .lock()
-                        .definitions
-                        .push(DefIdInfo { parent, data });
+                    let (def_id, hash) =
+                        self.untracked.definitions.write().create_def(parent, data);
+                    trace!(?def_id, "record side effects");
+
+                    icx.side_effects.as_ref().unwrap().lock().definitions.push(DefIdInfo {
+                        parent,
+                        data,
+                        hash,
+                    });
+                    def_id
                 }
                 TaskDepsRef::Replay { prev_side_effects, created_def_ids } => {
+                    trace!(?created_def_ids, "replay side effects");
+                    trace!("num_defs : {}", prev_side_effects.definitions.len());
                     let index = created_def_ids.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
                     let prev_info = &prev_side_effects.definitions[index];
-                    assert_eq!(*prev_info, DefIdInfo { parent, data });
+                    let def_id = self.untracked.definitions.read().local_def_path_hash_to_def_id(
+                        prev_info.hash,
+                        &"should have already recreated def id in try_mark_green",
+                    );
+                    assert_eq!(prev_info.data, data);
+                    assert_eq!(prev_info.parent, parent);
+                    def_id
                 }
             }
         });

compiler/rustc_query_impl/src/plumbing.rs

@@ -171,7 +171,7 @@ impl QueryContext for QueryCtxt<'_> {
         });
     }
 
-    #[tracing::instrument(level = "trace", skip(self))]
+    #[tracing::instrument(level = "trace", skip(self, side_effects))]
     fn apply_side_effects(self, side_effects: QuerySideEffects) {
         let dcx = self.dep_context().sess().dcx();
         let QuerySideEffects { diagnostics, definitions } = side_effects;
@@ -180,8 +180,9 @@ impl QueryContext for QueryCtxt<'_> {
             dcx.emit_diagnostic(diagnostic);
         }
 
-        for DefIdInfo { parent, data } in definitions {
-            self.tcx.untracked().definitions.write().create_def(parent, data);
+        for DefIdInfo { parent, data, hash } in definitions {
+            let (_def_id, h) = self.tcx.untracked().definitions.write().create_def(parent, data);
+            debug_assert_eq!(h, hash);
         }
     }
 }

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -908,7 +908,7 @@ impl<D: Deps> DepGraphData<D> {
         Some(dep_node_index)
     }
 
-    /// Atomically emits some loaded diagnostics.
+    /// Atomically emits some loaded side effects.
     /// This may be called concurrently on multiple threads for the same dep node.
     #[cold]
     #[inline(never)]
@@ -924,7 +924,7 @@ impl<D: Deps> DepGraphData<D> {
             // We were the first to insert the node in the set so this thread
             // must process side effects
 
-            // Promote the previous diagnostics to the current session.
+            // Promote the previous side effects to the current session.
             qcx.store_side_effects(dep_node_index, side_effects.clone());
 
             qcx.apply_side_effects(side_effects);

compiler/rustc_query_system/src/query/mod.rs

@@ -20,6 +20,7 @@ use rustc_data_structures::stable_hasher::Hash64;
 use rustc_data_structures::sync::Lock;
 use rustc_errors::DiagInner;
 use rustc_hir::def::DefKind;
+use rustc_hir::def_id::DefPathHash;
 use rustc_macros::{Decodable, Encodable};
 use rustc_span::def_id::{DefId, LocalDefId};
 use rustc_span::Span;
@@ -97,6 +98,7 @@ pub struct QuerySideEffects {
 pub struct DefIdInfo {
     pub parent: LocalDefId,
     pub data: rustc_hir::definitions::DefPathData,
+    pub hash: DefPathHash,
 }
 
 impl QuerySideEffects {