Auto merge of rust-lang#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

@@ -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_hir_analysis/src/lib.rs

@@ -201,6 +201,15 @@ pub fn check_crate(tcx: TyCtxt<'_>) {
         }
     });
 
+    // Make sure we actually have a value for static items, as they aren't cached in incremental.
+    // While we could just wait for codegen to invoke this, the definitions freeze below will cause
+    // that to ICE, because evaluating statics can create more items.
+    tcx.hir().par_body_owners(|item_def_id| {
+        if let DefKind::Static { .. } = tcx.def_kind(item_def_id) {
+            let _ = tcx.eval_static_initializer(item_def_id);
+        }
+    });
+
     // Freeze definitions as we don't add new ones at this point. This improves performance by
     // allowing lock-free access to them.
     tcx.untracked().definitions.freeze();

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_macros/src/query.rs

@@ -314,6 +314,17 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
     let mut query_description_stream = quote! {};
     let mut query_cached_stream = quote! {};
     let mut feedable_queries = quote! {};
+    let mut errors = quote! {};
+
+    macro_rules! assert {
+        ( $cond:expr, $span:expr, $( $tt:tt )+ ) => {
+            if !$cond {
+                errors.extend(
+                    Error::new($span, format!($($tt)*)).into_compile_error(),
+                );
+            }
+        }
+    }
 
     for query in queries.0 {
         let Query { name, arg, modifiers, .. } = &query;
@@ -369,10 +380,15 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
             [#attribute_stream] fn #name(#arg) #result,
         });
 
-        if modifiers.feedable.is_some() {
-            assert!(modifiers.anon.is_none(), "Query {name} cannot be both `feedable` and `anon`.");
+        if let Some(feedable) = &modifiers.feedable {
+            assert!(
+                modifiers.anon.is_none(),
+                feedable.span(),
+                "Query {name} cannot be both `feedable` and `anon`."
+            );
             assert!(
                 modifiers.eval_always.is_none(),
+                feedable.span(),
                 "Query {name} cannot be both `feedable` and `eval_always`."
             );
             feedable_queries.extend(quote! {
@@ -407,5 +423,6 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
             use super::*;
             #query_cached_stream
         }
+        #errors
     })
 }

compiler/rustc_middle/src/query/mod.rs

@@ -1065,7 +1065,6 @@ rustc_queries! {
             "evaluating initializer of static `{}`",
             tcx.def_path_str(key)
         }
-        cache_on_disk_if { key.is_local() }
         separate_provide_extern
         feedable
     }
@@ -1711,6 +1710,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

@@ -4,6 +4,7 @@
 
 pub mod tls;
 
+use rustc_query_system::query::DefIdInfo;
 pub use rustc_type_ir::lift::Lift;
 
 use crate::arena::Arena;
@@ -57,7 +58,7 @@ use rustc_hir::lang_items::LangItem;
 use rustc_hir::{HirId, Node, TraitCandidate};
 use rustc_index::IndexVec;
 use rustc_macros::{HashStable, TyDecodable, TyEncodable};
-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;
@@ -72,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;
@@ -1332,34 +1333,71 @@ 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.
         //
         // 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:
         // - 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.
-        // 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 => {
+                    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 => {
+                    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(_) => {
+                    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];
+                    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
+                }
+            }
+        });
 
         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

@@ -6,7 +6,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::bug;
 use rustc_middle::dep_graph::dep_kinds;
@@ -22,16 +21,15 @@ 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::{
-    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> {
@@ -125,7 +123,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
@@ -140,7 +138,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,
             };
@@ -172,6 +170,21 @@ impl QueryContext for QueryCtxt<'_> {
             crate_name: self.crate_name(LOCAL_CRATE),
         });
     }
+
+    #[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;
+
+        for diagnostic in diagnostics {
+            dcx.emit_diagnostic(diagnostic);
+        }
+
+        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);
+        }
+    }
 }
 
 pub(super) fn try_mark_green<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &dep_graph::DepNode) -> bool {