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

Author: bors

compiler/rustc_hir/src/definitions.rs

@@ -10,8 +10,8 @@ use std::hash::Hash;
 use rustc_data_structures::stable_hasher::StableHasher;
 use rustc_data_structures::unord::UnordMap;
 use rustc_hashes::Hash64;
-use rustc_index::IndexVec;
-use rustc_macros::{Decodable, Encodable};
+use rustc_index::{IndexVec, static_assert_size};
+use rustc_macros::{Decodable, Encodable, HashStable_Generic};
 use rustc_span::{Symbol, kw, sym};
 use tracing::{debug, instrument};
 
@@ -67,7 +67,8 @@ impl DefPathTable {
             // being used.
             //
             // See the documentation for DefPathHash for more information.
-            panic!(
+            assert_eq!(
+                def_path1, def_path2,
                 "found DefPathHash collision between {def_path1:#?} and {def_path2:#?}. \
                     Compilation cannot continue."
             );
@@ -114,6 +115,13 @@ impl DisambiguatorState {
         this.next.insert((def_id, data), index);
         this
     }
+
+    pub fn next(&mut self, parent: LocalDefId, data: DefPathData) -> u32 {
+        let next_disamb = self.next.entry((parent, data)).or_insert(0);
+        let disambiguator = *next_disamb;
+        *next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow");
+        disambiguator
+    }
 }
 
 /// The definition table containing node definitions.
@@ -207,7 +215,7 @@ impl fmt::Display for DisambiguatedDefPathData {
     }
 }
 
-#[derive(Clone, Debug, Encodable, Decodable)]
+#[derive(Clone, Debug, Encodable, Decodable, PartialEq)]
 pub struct DefPath {
     /// The path leading from the crate root to the item.
     pub data: Vec<DisambiguatedDefPathData>,
@@ -274,7 +282,7 @@ impl DefPath {
 }
 
 /// New variants should only be added in synchronization with `enum DefKind`.
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable, HashStable_Generic)]
 pub enum DefPathData {
     // Root: these should only be used for the root nodes, because
     // they are treated specially by the `def_path` function.
@@ -320,6 +328,8 @@ pub enum DefPathData {
     NestedStatic,
 }
 
+static_assert_size!(DefPathData, 8);
+
 impl Definitions {
     pub fn def_path_table(&self) -> &DefPathTable {
         &self.table
@@ -381,25 +391,18 @@ impl Definitions {
         &mut self,
         parent: LocalDefId,
         data: DefPathData,
-        disambiguator: &mut DisambiguatorState,
-    ) -> LocalDefId {
+        disambiguator: u32,
+    ) -> (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!(
-            "create_def(parent={}, data={data:?})",
+            "create_def(parent={}, data={data:?}, disambiguator: {disambiguator})",
             self.def_path(parent).to_string_no_crate_verbose(),
         );
 
         // The root node must be created in `new()`.
         assert!(data != DefPathData::CrateRoot);
 
-        // Find the next free disambiguator for this key.
-        let disambiguator = {
-            let next_disamb = disambiguator.next.entry((parent, data)).or_insert(0);
-            let disambiguator = *next_disamb;
-            *next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow");
-            disambiguator
-        };
         let key = DefKey {
             parent: Some(parent.local_def_index),
             disambiguated_data: DisambiguatedDefPathData { data, disambiguator },
@@ -411,7 +414,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_interface/src/callbacks.rs

@@ -14,9 +14,11 @@ use std::fmt;
 use rustc_errors::{DiagInner, TRACK_DIAGNOSTIC};
 use rustc_middle::dep_graph::{DepNodeExt, TaskDepsRef};
 use rustc_middle::ty::tls;
+use rustc_middle::util::Providers;
 use rustc_query_impl::QueryCtxt;
 use rustc_query_system::dep_graph::dep_node::default_dep_kind_debug;
 use rustc_query_system::dep_graph::{DepContext, DepKind, DepNode};
+use rustc_query_system::query::DefIdInfo;
 
 fn track_span_parent(def_id: rustc_span::def_id::LocalDefId) {
     tls::with_context_opt(|icx| {
@@ -113,3 +115,14 @@ pub fn setup_callbacks() {
         .swap(&(dep_node_debug as fn(_, &mut fmt::Formatter<'_>) -> _));
     TRACK_DIAGNOSTIC.swap(&(track_diagnostic as _));
 }
+
+pub fn provide(providers: &mut Providers) {
+    providers.create_def_raw = |tcx, (parent, data, disambiguator)| {
+        let (def_id, hash) =
+            tcx.untracked().definitions.write().create_def(parent, data, disambiguator);
+
+        tcx.dep_graph
+            .record_def(QueryCtxt::new(tcx), DefIdInfo { parent, data, hash, disambiguator });
+        def_id
+    }
+}

compiler/rustc_interface/src/passes.rs

@@ -799,6 +799,7 @@ pub static DEFAULT_QUERY_PROVIDERS: LazyLock<Providers> = LazyLock::new(|| {
     rustc_lint::provide(providers);
     rustc_symbol_mangling::provide(providers);
     rustc_codegen_ssa::provide(providers);
+    crate::callbacks::provide(providers);
     *providers
 });
 

compiler/rustc_middle/src/dep_graph/mod.rs

@@ -1,5 +1,6 @@
 use rustc_data_structures::profiling::SelfProfilerRef;
 use rustc_query_system::ich::StableHashingContext;
+use rustc_query_system::query::DefIdInfo;
 use rustc_session::Session;
 
 use crate::ty::{self, TyCtxt};
@@ -80,6 +81,11 @@ impl<'tcx> DepContext for TyCtxt<'tcx> {
         self.sess
     }
 
+    fn create_def(&self, &DefIdInfo { parent, data, disambiguator: index, hash }: &DefIdInfo) {
+        let (_, h) = self.untracked().definitions.write().create_def(parent, data, index);
+        assert_eq!(hash, h);
+    }
+
     #[inline]
     fn dep_kind_info(&self, dk: DepKind) -> &DepKindStruct<'tcx> {
         &self.query_kinds[dk.as_usize()]

compiler/rustc_middle/src/query/keys.rs

@@ -3,6 +3,7 @@
 use std::ffi::OsStr;
 
 use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE, LocalDefId, LocalModDefId, ModDefId};
+use rustc_hir::definitions::DefPathData;
 use rustc_hir::hir_id::{HirId, OwnerId};
 use rustc_query_system::dep_graph::DepNodeIndex;
 use rustc_query_system::query::{DefIdCache, DefaultCache, SingleCache, VecCache};
@@ -265,6 +266,14 @@ impl Key for (LocalDefId, LocalDefId) {
     }
 }
 
+impl Key for (LocalDefId, DefPathData, u32) {
+    type Cache<V> = DefaultCache<Self, V>;
+
+    fn default_span(&self, tcx: TyCtxt<'_>) -> Span {
+        self.0.default_span(tcx)
+    }
+}
+
 impl Key for (DefId, Ident) {
     type Cache<V> = DefaultCache<Self, V>;
 

compiler/rustc_middle/src/query/mod.rs

@@ -159,6 +159,11 @@ rustc_queries! {
         desc { "getting the source span" }
     }
 
+    /// Used to handle incremental replays of [`TyCtxt::create_def`] invocations from tracked queries.
+    query create_def_raw(key: (LocalDefId, rustc_hir::definitions::DefPathData, u32)) -> LocalDefId {
+        desc { "generating a new def id" }
+    }
+
     /// Represents crate as a whole (as distinct from the top-level crate module).
     ///
     /// If you call `tcx.hir_crate(())` we will have to assume that any change
@@ -2054,6 +2059,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

@@ -41,7 +41,7 @@ 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_serialize::opaque::{FileEncodeResult, FileEncoder};
 use rustc_session::config::CrateType;
@@ -1992,6 +1992,7 @@ 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,
@@ -2001,22 +2002,26 @@ impl<'tcx> TyCtxt<'tcx> {
         disambiguator: &mut DisambiguatorState,
     ) -> TyCtxtFeed<'tcx, LocalDefId> {
         let data = override_def_path_data.unwrap_or_else(|| def_kind.def_path_data(name));
-        // The following call has 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.
-        //
-        // 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 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.
-        let def_id = self.untracked.definitions.write().create_def(parent, data, disambiguator);
-
-        // 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);
+
+        let def_id = 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 => {
+                    // The following call has 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.
+                    self.untracked.definitions.write().create_def(parent, data, disambiguator.next(parent, data)).0
+                }
+                TaskDepsRef::Forbid => bug!(
+                    "cannot create definition {parent:?} {data:?} without being able to register task dependencies"
+                ),
+                TaskDepsRef::Allow(_) => {
+                    self.create_def_raw((parent, data, disambiguator.next(parent, data)))
+                }
+            }
+        });
 
         let feed = TyCtxtFeed { tcx: self, key: def_id };
         feed.def_kind(def_kind);

compiler/rustc_query_impl/src/plumbing.rs

@@ -871,7 +871,7 @@ macro_rules! define_queries {
                     is_eval_always: false,
                     fingerprint_style: FingerprintStyle::Unit,
                     force_from_dep_node: Some(|tcx, _, prev_index| {
-                        tcx.dep_graph.force_diagnostic_node(QueryCtxt::new(tcx), prev_index);
+                        tcx.dep_graph.force_side_effect_node(QueryCtxt::new(tcx), prev_index);
                         true
                     }),
                     try_load_from_on_disk_cache: None,