Recreate DefIds when a cached query gets replayed

Author: oli-obk

compiler/rustc_middle/src/query/mod.rs

@@ -1844,6 +1844,7 @@ rustc_queries! {
         desc { |tcx| "computing visibility of `{}`", tcx.def_path_str(def_id) }
         separate_provide_extern
         feedable
+        cache_on_disk_if { def_id.is_local() }
     }
 
     query inhabited_predicate_adt(key: DefId) -> ty::inhabitedness::InhabitedPredicate<'tcx> {

compiler/rustc_middle/src/ty/context.rs

@@ -39,8 +39,9 @@ use rustc_hir::{self as hir, Attribute, HirId, Node, TraitCandidate};
 use rustc_index::IndexVec;
 use rustc_macros::{HashStable, TyDecodable, TyEncodable};
 use rustc_query_system::cache::WithDepNode;
-use rustc_query_system::dep_graph::DepNodeIndex;
+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};
@@ -1807,10 +1808,9 @@ impl<'tcx> TyCtxt<'tcx> {
         // decode the on-disk cache.
         //
         // Any LocalDefId which is used within queries, either as key or result, either:
-        // - has been created before the construction of the TyCtxt;
+        // - has been created before the construction of the TyCtxt,
+        // - has been created when marking a query as green (recreating definitions it created in the actual run),
         // - has been created by this call to `create_def`.
-        // As a consequence, this LocalDefId is always re-created before it is needed by the incr.
-        // comp. engine itself.
         //
         // This call also writes to the value of `source_span` and `expn_that_defined` queries.
         // This is fine because:
@@ -1820,9 +1820,30 @@ impl<'tcx> TyCtxt<'tcx> {
 
         // 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.
-        // Depending on the forever-red node will tell the graph that the calling query
-        // needs to be re-evaluated.
-        self.dep_graph.read_index(DepNodeIndex::FOREVER_RED_NODE);
+        tls::with_context(|icx| {
+            match icx.task_deps {
+                // Always gets rerun anyway, so nothing to replay
+                TaskDepsRef::EvalAlways => {}
+                // Top-level queries like the resolver get rerun every time anyway
+                TaskDepsRef::Ignore => {}
+                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 });
+                }
+                TaskDepsRef::Replay { prev_side_effects, created_def_ids } => {
+                    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 feed = TyCtxtFeed { tcx: self, key: def_id };
         feed.def_kind(def_kind);

compiler/rustc_query_impl/src/plumbing.rs

@@ -23,8 +23,8 @@ use rustc_middle::ty::{self, TyCtxt, TyEncoder};
 use rustc_query_system::dep_graph::{DepNodeParams, HasDepContext};
 use rustc_query_system::ich::StableHashingContext;
 use rustc_query_system::query::{
-    QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects, QueryStackFrame,
-    force_query,
+    DefIdInfo, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects,
+    QueryStackFrame, force_query,
 };
 use rustc_query_system::{LayoutOfDepth, QueryOverflow};
 use rustc_serialize::{Decodable, Encodable};
@@ -176,11 +176,15 @@ impl QueryContext for QueryCtxt<'_> {
     #[tracing::instrument(level = "trace", skip(self))]
     fn apply_side_effects(self, side_effects: QuerySideEffects) {
         let dcx = self.dep_context().sess().dcx();
-        let QuerySideEffects { diagnostics } = side_effects;
+        let QuerySideEffects { diagnostics, definitions } = side_effects;
 
         for diagnostic in diagnostics {
             dcx.emit_diagnostic(diagnostic);
         }
+
+        for DefIdInfo { parent, data } in definitions {
+            self.tcx.untracked().definitions.write().create_def(parent, data);
+        }
     }
 }
 

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -11,7 +11,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::profiling::{QueryInvocationId, SelfProfilerRef};
 use rustc_data_structures::sharded::{self, Sharded};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
-use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock, Lrc};
+use rustc_data_structures::sync::{AtomicU32, AtomicU64, AtomicUsize, Lock, Lrc};
 use rustc_data_structures::unord::UnordMap;
 use rustc_index::IndexVec;
 use rustc_macros::{Decodable, Encodable};
@@ -224,6 +224,15 @@ impl<D: Deps> DepGraph<D> {
         D::with_deps(TaskDepsRef::Ignore, op)
     }
 
+    pub(crate) fn with_replay<R>(
+        &self,
+        prev_side_effects: &QuerySideEffects,
+        created_def_ids: &AtomicUsize,
+        op: impl FnOnce() -> R,
+    ) -> R {
+        D::with_deps(TaskDepsRef::Replay { prev_side_effects, created_def_ids }, op)
+    }
+
     /// Used to wrap the deserialization of a query result from disk,
     /// This method enforces that no new `DepNodes` are created during
     /// query result deserialization.
@@ -278,6 +287,7 @@ impl<D: Deps> DepGraph<D> {
     }
 
     #[inline(always)]
+    /// A helper for `codegen_cranelift`.
     pub fn with_task<Ctxt: HasDepContext<Deps = D>, A: Debug, R>(
         &self,
         key: DepNode,
@@ -477,6 +487,12 @@ impl<D: Deps> DepGraph<D> {
                         return;
                     }
                     TaskDepsRef::Ignore => return,
+                    // We don't need to record dependencies when rerunning a query
+                    // because we have no disk cache entry to load. The dependencies
+                    // are preserved.
+                    // FIXME: assert that the dependencies don't change instead of
+                    // recording them.
+                    TaskDepsRef::Replay { .. } => return,
                     TaskDepsRef::Forbid => {
                         // Reading is forbidden in this context. ICE with a useful error message.
                         panic_on_forbidden_read(data, dep_node_index)
@@ -583,6 +599,7 @@ impl<D: Deps> DepGraph<D> {
                     edges.push(DepNodeIndex::FOREVER_RED_NODE);
                 }
                 TaskDepsRef::Ignore => {}
+                TaskDepsRef::Replay { .. } => {}
                 TaskDepsRef::Forbid => {
                     panic!("Cannot summarize when dependencies are not recorded.")
                 }
@@ -1284,6 +1301,18 @@ pub enum TaskDepsRef<'a> {
     /// to ensure that the decoding process doesn't itself
     /// require the execution of any queries.
     Forbid,
+    /// Side effects from the previous run made available to
+    /// queries when they are reexecuted because their result was not
+    /// available in the cache. Whenever the query creates a new `DefId`,
+    /// it is checked against the entries in `QuerySideEffects::definitions`
+    /// to ensure that the new `DefId`s are the same as the ones that were
+    /// created the last time the query was executed.
+    Replay {
+        prev_side_effects: &'a QuerySideEffects,
+        /// Every new `DefId` is pushed here so we can check
+        /// that they match the cached ones.
+        created_def_ids: &'a AtomicUsize,
+    },
 }
 
 #[derive(Debug)]

compiler/rustc_query_system/src/query/mod.rs

@@ -17,7 +17,7 @@ use rustc_errors::DiagInner;
 use rustc_hir::def::DefKind;
 use rustc_macros::{Decodable, Encodable};
 use rustc_span::Span;
-use rustc_span::def_id::DefId;
+use rustc_span::def_id::{DefId, LocalDefId};
 use thin_vec::ThinVec;
 
 pub use self::config::{HashResult, QueryConfig};
@@ -76,20 +76,30 @@ pub struct QuerySideEffects {
     /// These diagnostics will be re-emitted if we mark
     /// the query as green.
     pub diagnostics: ThinVec<DiagInner>,
+    /// Stores any `DefId`s that were created during query execution.
+    /// These `DefId`s will be re-created when we mark the query as green.
+    pub definitions: ThinVec<DefIdInfo>,
+}
+
+#[derive(Debug, Clone, Encodable, Decodable, PartialEq)]
+pub struct DefIdInfo {
+    pub parent: LocalDefId,
+    pub data: rustc_hir::definitions::DefPathData,
 }
 
 impl QuerySideEffects {
     /// Returns true if there might be side effects.
     #[inline]
     pub fn maybe_any(&self) -> bool {
-        let QuerySideEffects { diagnostics } = self;
+        let QuerySideEffects { diagnostics, definitions } = self;
         // Use `has_capacity` so that the destructor for `self.diagnostics` can be skipped
         // if `maybe_any` is known to be false.
-        diagnostics.has_capacity()
+        diagnostics.has_capacity() || definitions.has_capacity()
     }
     pub fn append(&mut self, other: QuerySideEffects) {
-        let QuerySideEffects { diagnostics } = self;
+        let QuerySideEffects { diagnostics, definitions } = self;
         diagnostics.extend(other.diagnostics);
+        definitions.extend(other.definitions);
     }
 }
 

compiler/rustc_query_system/src/query/plumbing.rs

@@ -12,7 +12,7 @@ use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::sharded::Sharded;
 use rustc_data_structures::stack::ensure_sufficient_stack;
-use rustc_data_structures::sync::Lock;
+use rustc_data_structures::sync::{AtomicUsize, Lock};
 use rustc_data_structures::{outline, sync};
 use rustc_errors::{Diag, FatalError, StashKey};
 use rustc_span::{DUMMY_SP, Span};
@@ -504,7 +504,7 @@ where
         let dep_node =
             dep_node_opt.get_or_insert_with(|| query.construct_dep_node(*qcx.dep_context(), &key));
 
-        // The diagnostics for this query will be promoted to the current session during
+        // The side_effects for this query will be promoted to the current session during
         // `try_mark_green()`, so we can ignore them here.
         if let Some(ret) = qcx.start_query(job_id, false, None, || {
             try_load_from_disk_and_cache_in_memory(query, dep_graph_data, qcx, &key, dep_node)
@@ -624,8 +624,19 @@ where
     // recompute.
     let prof_timer = qcx.dep_context().profiler().query_provider();
 
+    let prev_side_effects = qcx.load_side_effects(prev_dep_node_index);
+    let created_def_ids = AtomicUsize::new(0);
     // The dep-graph for this computation is already in-place.
-    let result = qcx.dep_context().dep_graph().with_ignore(|| query.compute(qcx, *key));
+    let result =
+        qcx.dep_context()
+            .dep_graph()
+            .with_replay(&prev_side_effects, &created_def_ids, || query.compute(qcx, *key));
+
+    // We want to verify that the `DefId`s created by the call to `query.compute` are consistent with
+    // those from the previous compilation. We already checked at `DefId` creation time, that the
+    // created `DefId`s have the same parent and `DefPathData` as the cached ones.
+    // We check here that we have not missed any.
+    assert_eq!(created_def_ids.into_inner(), prev_side_effects.definitions.len());
 
     prof_timer.finish_with_query_invocation_id(dep_node_index.into());