Auto merge of #115613 - oli-obk:create_def_forever_red, r=<try>

Make create_def a side effect instead of marking the entire query as always red

Before this PR:

* query A creates def id D
* query A is marked as depending on the always-red node, meaning it will always get re-run
* in the next run of rustc: query A is not loaded from the incremental cache, but rerun

After this PR:

* query A creates def id D
* query system registers this a side effect (just like we collect diagnostics to re-emit them without running a query)
* in the next run of rustc: query A is loaded from the incremental cache and its side effect is run (thus re-creating def id D without running query A)

r? `@cjgillot`

TODO:

* [ ] need to make feeding queries a side effect, too? At least ones that aren't written to disk?
* [ ] need to re-feed the `def_span` query
* [ ] many more tests

compiler/rustc_interface/src/callbacks.rs

@@ -32,8 +32,9 @@ fn track_span_parent(def_id: rustc_span::def_id::LocalDefId) {
 fn track_diagnostic<R>(diagnostic: DiagInner, f: &mut dyn FnMut(DiagInner) -> R) -> R {
     tls::with_context_opt(|icx| {
         if let Some(icx) = icx {
-            if let Some(diagnostics) = icx.diagnostics {
-                diagnostics.lock().extend(Some(diagnostic.clone()));
+            if let Some(side_effects) = icx.side_effects {
+                let diagnostic = diagnostic.clone();
+                side_effects.lock().diagnostics.push(diagnostic);
             }
 
             // Diagnostics are tracked, we can ignore the dependency.

compiler/rustc_middle/src/query/mod.rs

@@ -1723,6 +1723,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

@@ -55,8 +55,9 @@ use rustc_hir::lang_items::LangItem;
 use rustc_hir::{HirId, Node, TraitCandidate};
 use rustc_index::IndexVec;
 use rustc_macros::HashStable;
-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};
@@ -1150,9 +1151,31 @@ 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 prev_info = &prev_side_effects.definitions
+                        [created_def_ids.load(std::sync::atomic::Ordering::Relaxed)];
+                    created_def_ids.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+                    assert_eq!(*prev_info, DefIdInfo { parent, data });
+                }
+            }
+        });
 
         let feed = TyCtxtFeed { tcx: self, key: def_id };
         feed.def_kind(def_kind);

compiler/rustc_middle/src/ty/context/tls.rs

@@ -3,12 +3,11 @@ use super::{GlobalCtxt, TyCtxt};
 use crate::dep_graph::TaskDepsRef;
 use crate::query::plumbing::QueryJobId;
 use rustc_data_structures::sync::{self, Lock};
-use rustc_errors::DiagInner;
+use rustc_query_system::query::QuerySideEffects;
 #[cfg(not(parallel_compiler))]
 use std::cell::Cell;
 use std::mem;
 use std::ptr;
-use thin_vec::ThinVec;
 
 /// This is the implicit state of rustc. It contains the current
 /// `TyCtxt` and query. It is updated when creating a local interner or
@@ -26,7 +25,7 @@ pub struct ImplicitCtxt<'a, 'tcx> {
 
     /// Where to store diagnostics for the current query job, if any.
     /// This is updated by `JobOwner::start` in `ty::query::plumbing` when executing a query.
-    pub diagnostics: Option<&'a Lock<ThinVec<DiagInner>>>,
+    pub side_effects: Option<&'a Lock<QuerySideEffects>>,
 
     /// Used to prevent queries from calling too deeply.
     pub query_depth: usize,
@@ -42,7 +41,7 @@ impl<'a, 'tcx> ImplicitCtxt<'a, 'tcx> {
         ImplicitCtxt {
             tcx,
             query: None,
-            diagnostics: None,
+            side_effects: None,
             query_depth: 0,
             task_deps: TaskDepsRef::Ignore,
         }

compiler/rustc_query_impl/src/plumbing.rs

@@ -8,7 +8,6 @@ use crate::QueryConfigRestored;
 use rustc_data_structures::stable_hasher::{Hash64, HashStable, StableHasher};
 use rustc_data_structures::sync::Lock;
 use rustc_data_structures::unord::UnordMap;
-use rustc_errors::DiagInner;
 
 use rustc_index::Idx;
 use rustc_middle::dep_graph::dep_kinds;
@@ -24,16 +23,15 @@ use rustc_middle::ty::{self, TyCtxt};
 use rustc_query_system::dep_graph::{DepNodeParams, HasDepContext};
 use rustc_query_system::ich::StableHashingContext;
 use rustc_query_system::query::{
-    force_query, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects,
-    QueryStackFrame,
+    force_query, DefIdInfo, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap,
+    QuerySideEffects, QueryStackFrame,
 };
 use rustc_query_system::{LayoutOfDepth, QueryOverflow};
 use rustc_serialize::Decodable;
 use rustc_serialize::Encodable;
 use rustc_session::Limit;
 use rustc_span::def_id::LOCAL_CRATE;
 use std::num::NonZero;
-use thin_vec::ThinVec;
 
 #[derive(Copy, Clone)]
 pub struct QueryCtxt<'tcx> {
@@ -127,7 +125,7 @@ impl QueryContext for QueryCtxt<'_> {
         self,
         token: QueryJobId,
         depth_limit: bool,
-        diagnostics: Option<&Lock<ThinVec<DiagInner>>>,
+        side_effects: Option<&Lock<QuerySideEffects>>,
         compute: impl FnOnce() -> R,
     ) -> R {
         // The `TyCtxt` stored in TLS has the same global interner lifetime
@@ -142,7 +140,7 @@ impl QueryContext for QueryCtxt<'_> {
             let new_icx = ImplicitCtxt {
                 tcx: self.tcx,
                 query: Some(token),
-                diagnostics,
+                side_effects,
                 query_depth: current_icx.query_depth + depth_limit as usize,
                 task_deps: current_icx.task_deps,
             };
@@ -174,6 +172,20 @@ impl QueryContext for QueryCtxt<'_> {
             crate_name: self.crate_name(LOCAL_CRATE),
         });
     }
+
+    #[tracing::instrument(level = "trace", skip(self))]
+    fn apply_side_effects(self, side_effects: QuerySideEffects) {
+        let dcx = self.dep_context().sess().dcx();
+        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);
+        }
+    }
 }
 
 pub(super) fn try_mark_green<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &dep_graph::DepNode) -> bool {

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -1,9 +1,15 @@
+use super::query::DepGraphQuery;
+use super::serialized::{GraphEncoder, SerializedDepGraph, SerializedDepNodeIndex};
+use super::{DepContext, DepKind, DepNode, Deps, HasDepContext, WorkProductId};
+use crate::dep_graph::edges::EdgesVec;
+use crate::ich::StableHashingContext;
+use crate::query::{QueryContext, QuerySideEffects};
 use rustc_data_structures::fingerprint::Fingerprint;
 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_serialize::opaque::{FileEncodeResult, FileEncoder};
@@ -14,13 +20,6 @@ use std::hash::Hash;
 use std::marker::PhantomData;
 use std::sync::atomic::Ordering;
 
-use super::query::DepGraphQuery;
-use super::serialized::{GraphEncoder, SerializedDepGraph, SerializedDepNodeIndex};
-use super::{DepContext, DepKind, DepNode, Deps, HasDepContext, WorkProductId};
-use crate::dep_graph::edges::EdgesVec;
-use crate::ich::StableHashingContext;
-use crate::query::{QueryContext, QuerySideEffects};
-
 #[cfg(debug_assertions)]
 use {super::debug::EdgeFilter, std::env};
 
@@ -56,7 +55,6 @@ pub struct MarkFrame<'a> {
     parent: Option<&'a MarkFrame<'a>>,
 }
 
-#[derive(PartialEq)]
 enum DepNodeColor {
     Red,
     Green(DepNodeIndex),
@@ -216,6 +214,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.
@@ -270,6 +277,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,
@@ -456,6 +464,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 => {
                         panic!("Illegal read of: {dep_node_index:?}")
                     }
@@ -561,6 +575,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.")
                 }
@@ -902,11 +917,7 @@ impl<D: Deps> DepGraphData<D> {
             // Promote the previous diagnostics to the current session.
             qcx.store_side_effects(dep_node_index, side_effects.clone());
 
-            let dcx = qcx.dep_context().sess().dcx();
-
-            for diagnostic in side_effects.diagnostics {
-                dcx.emit_diagnostic(diagnostic);
-            }
+            qcx.apply_side_effects(side_effects);
         }
     }
 }
@@ -915,7 +926,7 @@ impl<D: Deps> DepGraph<D> {
     /// Returns true if the given node has been marked as red during the
     /// current compilation session. Used in various assertions
     pub fn is_red(&self, dep_node: &DepNode) -> bool {
-        self.node_color(dep_node) == Some(DepNodeColor::Red)
+        matches!(self.node_color(dep_node), Some(DepNodeColor::Red))
     }
 
     /// Returns true if the given node has been marked as green during the
@@ -1284,6 +1295,16 @@ 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. The query removes entries from the
+    /// side effect table. The table must be empty
+    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

@@ -20,7 +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_span::def_id::DefId;
+use rustc_span::def_id::{DefId, LocalDefId};
 use rustc_span::Span;
 use thin_vec::ThinVec;
 
@@ -86,21 +86,31 @@ pub struct QuerySideEffects {
     /// Stores any diagnostics emitted during query execution.
     /// These diagnostics will be re-emitted if we mark
     /// the query as green.
-    pub(super) diagnostics: ThinVec<DiagInner>,
+    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);
     }
 }
 
@@ -118,6 +128,9 @@ pub trait QueryContext: HasDepContext {
     /// Register diagnostics for the given node, for use in next session.
     fn store_side_effects(self, dep_node_index: DepNodeIndex, side_effects: QuerySideEffects);
 
+    /// Actually execute the side effects
+    fn apply_side_effects(self, side_effects: QuerySideEffects);
+
     /// Register diagnostics for the given node, for use in next session.
     fn store_side_effects_for_anon_node(
         self,
@@ -132,7 +145,7 @@ pub trait QueryContext: HasDepContext {
         self,
         token: QueryJobId,
         depth_limit: bool,
-        diagnostics: Option<&Lock<ThinVec<DiagInner>>>,
+        side_effects: Option<&Lock<QuerySideEffects>>,
         compute: impl FnOnce() -> R,
     ) -> R;