Auto merge of #142711 - cjgillot:query-create-def, r=<try>

Make `create_def` a query

Creating a new definition using `create_def` currently behaves as a red node for incremental compilation: the caller query must be recomputed at the next run. This is suboptimal.

If a query's dependencies have not changed, we'd like the query engine to re-create the definitions and continue replaying the dependency graph and load cached data. This idea falls short with a subtle global state dependency: when creating definitions, we must ensure that `DefPathHash` are globally unique, and carry a disambiguator global state around. Because of this, a call to `create_def` may change its result `DefPathHash` due to global state, and force the caller query to be recomputed. If that happens, the caller query would need to be recomputed, but must not re-create the definitions that the query engine created for it.

This PR attempts to solve this issue by using a `create_def_raw` query. This query is `eval_always`, so it can safely read global state. The query engine sees its return value as the `DefPathHash`, so will detect if it unexpectedly changed. We benefit from the caching queries do.

However, we want multiple successive calls to `create_def` from the same queries to result in a new definition each time. For instance when creating anonymous definitions that are only identified by their parent and a disambiguator. To allow this, we add 2 extra parameters to `create_def_raw` that are unique to each call by the caller query: the query's `DepNode` and a counter of calls from within that query.

Consider this example. Some query A calls create_def twice, we generate calls to:
1. `create_def(CRATE_DEF_ID, Use) = create_def_raw(CRATE_DEF_ID, Use, A, 0)` This returns `::{use#0}`.
2. `create_def(CRATE_DEF_ID, Use) = create_def_raw(CRATE_DEF_ID, Use, A, 1)` The query has different arguments, so returns a brand new `::{use::1}`.
3. `create_def(CRATE_DEF_ID, Impl) = create_def_raw(CRATE_DEF_ID, Impl, A, 2)` This returns `::{impl#0}`.

When replaying, if nothing changed, the query engine will call `create_def_raw` thrice, and `A` will be green.

If another query created a definition with `(CRATE_DEF_ID, Impl)`, `::{impl#0}` is taken. The 3rd call to `create_def` needs to use another disambiguator, that call to `create_def_raw` returns `::{impl#1}` and be correctly red. `A` needs to be re-computed. The definitions for `use`s have already been created when walking the dep-graph. The re-computation calls use the results in `create_def_raw` cache, aka `::{use#0}`, `::{use#1}` and `::{impl#1}`, and we do not have duplicate definitions.

r? `@oli-obk`
cc `@Zoxc`

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->

Author: rust-bors[bot]

compiler/rustc_ast_lowering/src/lib.rs

@@ -532,11 +532,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             self.tcx.hir_def_key(self.local_def_id(node_id)),
         );
 
-        let def_id = self
-            .tcx
-            .at(span)
-            .create_def(parent, name, def_kind, None, &mut self.resolver.disambiguator)
-            .def_id();
+        let def_id = self.tcx.at(span).create_def(parent, name, def_kind, None).def_id();
 
         debug!("create_def: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id);
         self.resolver.node_id_to_def_id.insert(node_id, def_id);

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -17,7 +17,7 @@ use hir::def::DefKind;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_hir as hir;
-use rustc_hir::definitions::{DefPathData, DisambiguatorState};
+use rustc_hir::definitions::DefPathData;
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
 use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
 use rustc_middle::query::TyCtxtAt;
@@ -66,7 +66,6 @@ fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
     ecx: &mut InterpCx<'tcx, M>,
     alloc_id: AllocId,
     mutability: Mutability,
-    disambiguator: Option<&mut DisambiguatorState>,
 ) -> Result<impl Iterator<Item = CtfeProvenance> + 'tcx, ()> {
     trace!("intern_shallow {:?}", alloc_id);
     // remove allocation
@@ -89,13 +88,7 @@ fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
     // link the alloc id to the actual allocation
     let alloc = ecx.tcx.mk_const_alloc(alloc);
     if let Some(static_id) = ecx.machine.static_def_id() {
-        intern_as_new_static(
-            ecx.tcx,
-            static_id,
-            alloc_id,
-            alloc,
-            disambiguator.expect("disambiguator needed"),
-        );
+        intern_as_new_static(ecx.tcx, static_id, alloc_id, alloc);
     } else {
         ecx.tcx.set_alloc_id_memory(alloc_id, alloc);
     }
@@ -109,18 +102,16 @@ fn intern_as_new_static<'tcx>(
     static_id: LocalDefId,
     alloc_id: AllocId,
     alloc: ConstAllocation<'tcx>,
-    disambiguator: &mut DisambiguatorState,
 ) {
-    // `intern_const_alloc_recursive` is called once per static and it contains the `DisambiguatorState`.
-    //  The `<static_id>::{{nested}}` path is thus unique to `intern_const_alloc_recursive` and the
-    // `DisambiguatorState` ensures the generated path is unique for this call as we generate
-    // `<static_id>::{{nested#n}}` where `n` is the `n`th `intern_as_new_static` call.
+    // `intern_const_alloc_recursive` is called once per static. The `<static_id>::{{nested}}` path
+    // is thus unique to `intern_const_alloc_recursive`. If there are several calls to
+    // `intern_as_new_static`, `create_def` generates a path that is unique for this call as
+    // generate `<static_id>::{{nested#n}}` where `n` is the `n`th `intern_as_new_static` call.
     let feed = tcx.create_def(
         static_id,
         None,
         DefKind::Static { safety: hir::Safety::Safe, mutability: alloc.0.mutability, nested: true },
         Some(DefPathData::NestedStatic),
-        disambiguator,
     );
     tcx.set_nested_alloc_id_static(alloc_id, feed.def_id());
 
@@ -168,8 +159,6 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
     intern_kind: InternKind,
     ret: &MPlaceTy<'tcx>,
 ) -> Result<(), InternResult> {
-    let mut disambiguator = DisambiguatorState::new();
-
     // We are interning recursively, and for mutability we are distinguishing the "root" allocation
     // that we are starting in, and all other allocations that we are encountering recursively.
     let (base_mutability, inner_mutability, is_static) = match intern_kind {
@@ -213,9 +202,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
         alloc.1.mutability = base_mutability;
         alloc.1.provenance().ptrs().iter().map(|&(_, prov)| prov).collect()
     } else {
-        intern_shallow(ecx, base_alloc_id, base_mutability, Some(&mut disambiguator))
-            .unwrap()
-            .collect()
+        intern_shallow(ecx, base_alloc_id, base_mutability).unwrap().collect()
     };
     // We need to distinguish "has just been interned" from "was already in `tcx`",
     // so we track this in a separate set.
@@ -308,7 +295,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
         // okay with losing some potential for immutability here. This can anyway only affect
         // `static mut`.
         just_interned.insert(alloc_id);
-        match intern_shallow(ecx, alloc_id, inner_mutability, Some(&mut disambiguator)) {
+        match intern_shallow(ecx, alloc_id, inner_mutability) {
             Ok(nested) => todo.extend(nested),
             Err(()) => {
                 ecx.tcx.dcx().delayed_bug("found dangling pointer during const interning");
@@ -330,9 +317,8 @@ pub fn intern_const_alloc_for_constprop<'tcx, T, M: CompileTimeMachine<'tcx, T>>
         return interp_ok(());
     }
     // Move allocation to `tcx`.
-    if let Some(_) = intern_shallow(ecx, alloc_id, Mutability::Not, None)
-        .map_err(|()| err_ub!(DeadLocal))?
-        .next()
+    if let Some(_) =
+        intern_shallow(ecx, alloc_id, Mutability::Not).map_err(|()| err_ub!(DeadLocal))?.next()
     {
         // We are not doing recursive interning, so we don't currently support provenance.
         // (If this assertion ever triggers, we should just implement a
@@ -358,7 +344,7 @@ impl<'tcx> InterpCx<'tcx, DummyMachine> {
         let dest = self.allocate(layout, MemoryKind::Stack)?;
         f(self, &dest.clone().into())?;
         let alloc_id = dest.ptr().provenance.unwrap().alloc_id(); // this was just allocated, it must have provenance
-        for prov in intern_shallow(self, alloc_id, Mutability::Not, None).unwrap() {
+        for prov in intern_shallow(self, alloc_id, Mutability::Not).unwrap() {
             // We are not doing recursive interning, so we don't currently support provenance.
             // (If this assertion ever triggers, we should just implement a
             // proper recursive interning loop -- or just call `intern_const_alloc_recursive`.

compiler/rustc_hir/src/definitions.rs

@@ -11,7 +11,7 @@ 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_macros::{Decodable, Encodable, HashStable_Generic};
 use rustc_span::{Symbol, kw, sym};
 use tracing::{debug, instrument};
 
@@ -274,7 +274,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.

compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

@@ -14,7 +14,7 @@ use rustc_ast::visit::walk_list;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::definitions::{DefPathData, DisambiguatorState};
+use rustc_hir::definitions::DefPathData;
 use rustc_hir::intravisit::{self, InferKind, Visitor, VisitorExt};
 use rustc_hir::{
     self as hir, AmbigArg, GenericArg, GenericParam, GenericParamKind, HirId, LifetimeKind, Node,
@@ -64,7 +64,6 @@ impl ResolvedArg {
 struct BoundVarContext<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
     rbv: &'a mut ResolveBoundVars,
-    disambiguator: &'a mut DisambiguatorState,
     scope: ScopeRef<'a>,
 }
 
@@ -247,12 +246,8 @@ pub(crate) fn provide(providers: &mut Providers) {
 #[instrument(level = "debug", skip(tcx))]
 fn resolve_bound_vars(tcx: TyCtxt<'_>, local_def_id: hir::OwnerId) -> ResolveBoundVars {
     let mut rbv = ResolveBoundVars::default();
-    let mut visitor = BoundVarContext {
-        tcx,
-        rbv: &mut rbv,
-        scope: &Scope::Root { opt_parent_item: None },
-        disambiguator: &mut DisambiguatorState::new(),
-    };
+    let mut visitor =
+        BoundVarContext { tcx, rbv: &mut rbv, scope: &Scope::Root { opt_parent_item: None } };
     match tcx.hir_owner_node(local_def_id) {
         hir::OwnerNode::Item(item) => visitor.visit_item(item),
         hir::OwnerNode::ForeignItem(item) => visitor.visit_foreign_item(item),
@@ -1098,8 +1093,8 @@ impl<'a, 'tcx> BoundVarContext<'a, 'tcx> {
     where
         F: for<'b> FnOnce(&mut BoundVarContext<'b, 'tcx>),
     {
-        let BoundVarContext { tcx, rbv, disambiguator, .. } = self;
-        let mut this = BoundVarContext { tcx: *tcx, rbv, disambiguator, scope: &wrap_scope };
+        let BoundVarContext { tcx, rbv, .. } = self;
+        let mut this = BoundVarContext { tcx: *tcx, rbv, scope: &wrap_scope };
         let span = debug_span!("scope", scope = ?this.scope.debug_truncated());
         {
             let _enter = span.enter();
@@ -1472,13 +1467,12 @@ impl<'a, 'tcx> BoundVarContext<'a, 'tcx> {
                 // `opaque_def_id` is unique to the `BoundVarContext` pass which is executed once
                 // per `resolve_bound_vars` query. This is the only location that creates
                 // `OpaqueLifetime` paths. `<opaque_def_id>::OpaqueLifetime(..)` is thus unique
-                // to this query and duplicates within the query are handled by `self.disambiguator`.
+                // to this query.
                 let feed = self.tcx.create_def(
                     opaque_def_id,
                     None,
                     DefKind::LifetimeParam,
                     Some(DefPathData::OpaqueLifetime(ident.name)),
-                    &mut self.disambiguator,
                 );
                 feed.def_span(ident.span);
                 feed.def_ident_span(Some(ident.span));

compiler/rustc_interface/src/passes.rs

@@ -9,13 +9,12 @@ use rustc_ast as ast;
 use rustc_codegen_ssa::traits::CodegenBackend;
 use rustc_data_structures::jobserver::Proxy;
 use rustc_data_structures::steal::Steal;
-use rustc_data_structures::sync::{AppendOnlyIndexVec, FreezeLock, WorkerLocal};
+use rustc_data_structures::sync::WorkerLocal;
 use rustc_data_structures::{parallel, thousands};
 use rustc_expand::base::{ExtCtxt, LintStoreExpand};
 use rustc_feature::Features;
 use rustc_fs_util::try_canonicalize;
-use rustc_hir::def_id::{LOCAL_CRATE, StableCrateId, StableCrateIdMap};
-use rustc_hir::definitions::Definitions;
+use rustc_hir::def_id::{LOCAL_CRATE, StableCrateId};
 use rustc_incremental::setup_dep_graph;
 use rustc_lint::{BufferedEarlyLint, EarlyCheckNode, LintStore, unerased_lint_store};
 use rustc_metadata::EncodedMetadata;
@@ -30,7 +29,6 @@ use rustc_parse::{
 use rustc_passes::{abi_test, input_stats, layout_test};
 use rustc_resolve::Resolver;
 use rustc_session::config::{CrateType, Input, OutFileName, OutputFilenames, OutputType};
-use rustc_session::cstore::Untracked;
 use rustc_session::output::{collect_crate_types, filename_for_input};
 use rustc_session::parse::feature_err;
 use rustc_session::search_paths::PathKind;
@@ -904,13 +902,7 @@ pub fn create_and_enter_global_ctxt<T, F: for<'tcx> FnOnce(TyCtxt<'tcx>) -> T>(
     let dep_type = DepsType { dep_names: rustc_query_impl::dep_kind_names() };
     let dep_graph = setup_dep_graph(sess, crate_name, &dep_type);
 
-    let cstore =
-        FreezeLock::new(Box::new(CStore::new(compiler.codegen_backend.metadata_loader())) as _);
-    let definitions = FreezeLock::new(Definitions::new(stable_crate_id));
-
-    let stable_crate_ids = FreezeLock::new(StableCrateIdMap::default());
-    let untracked =
-        Untracked { cstore, source_span: AppendOnlyIndexVec::new(), definitions, stable_crate_ids };
+    let cstore = Box::new(CStore::new(compiler.codegen_backend.metadata_loader())) as _;
 
     // We're constructing the HIR here; we don't care what we will
     // read, since we haven't even constructed the *input* to
@@ -953,7 +945,7 @@ pub fn create_and_enter_global_ctxt<T, F: for<'tcx> FnOnce(TyCtxt<'tcx>) -> T>(
             stable_crate_id,
             arena,
             hir_arena,
-            untracked,
+            cstore,
             dep_graph,
             rustc_query_impl::query_callbacks(arena),
             rustc_query_impl::query_system(

compiler/rustc_middle/src/query/keys.rs

@@ -3,11 +3,13 @@
 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};
 use rustc_span::{DUMMY_SP, Ident, Span, Symbol};
 
+use crate::dep_graph::DepNode;
 use crate::infer::canonical::CanonicalQueryInput;
 use crate::mir::mono::CollectionMode;
 use crate::ty::fast_reject::SimplifiedType;
@@ -640,3 +642,11 @@ impl<'tcx> Key for (ty::Instance<'tcx>, CollectionMode) {
         self.0.default_span(tcx)
     }
 }
+
+impl Key for (LocalDefId, DefPathData, Option<DepNode>, usize) {
+    type Cache<V> = DefaultCache<Self, V>;
+
+    fn default_span(&self, _: TyCtxt<'_>) -> Span {
+        DUMMY_SP
+    }
+}

compiler/rustc_middle/src/query/mod.rs

@@ -81,6 +81,7 @@ use rustc_hir::def::{DefKind, DocLinkResMap};
 use rustc_hir::def_id::{
     CrateNum, DefId, DefIdMap, LocalDefId, LocalDefIdMap, LocalDefIdSet, LocalModDefId,
 };
+use rustc_hir::definitions::DefPathData;
 use rustc_hir::lang_items::{LangItem, LanguageItems};
 use rustc_hir::{Crate, ItemLocalId, ItemLocalMap, PreciseCapturingArgKind, TraitCandidate};
 use rustc_index::IndexVec;
@@ -102,6 +103,7 @@ use rustc_span::{DUMMY_SP, Span, Symbol};
 use rustc_target::spec::PanicStrategy;
 use {rustc_abi as abi, rustc_ast as ast, rustc_attr_data_structures as attr, rustc_hir as hir};
 
+use crate::dep_graph::DepNode;
 use crate::infer::canonical::{self, Canonical};
 use crate::lint::LintExpectation;
 use crate::metadata::ModChild;
@@ -215,6 +217,23 @@ rustc_queries! {
         desc { "getting the source span" }
     }
 
+    /// Create a new definition.
+    query create_def_raw(key: (
+        LocalDefId, // parent
+        DefPathData, // def_path_data
+        Option<DepNode>, // caller query
+        usize, // counter of calls to `create_def_raw` by the caller query
+    )) -> LocalDefId {
+        // Accesses untracked data
+        eval_always
+        desc { |tcx|
+            "create a new definition for `{}::{:?}`, {}th call",
+            tcx.def_path_str(key.0),
+            key.1,
+            key.3,
+        }
+    }
+
     /// 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