Auto merge of #140453 - Zoxc:next-disambiguator, r=oli-obk

Remove global `next_disambiguator` state and handle it with a `DisambiguatorState` type

This removes `Definitions.next_disambiguator` as it doesn't guarantee deterministic def paths when `create_def` is called in parallel. Instead a new `DisambiguatorState` type is passed as a mutable reference to `create_def` to help create unique def paths. `create_def` calls with distinct  `DisambiguatorState` instances must ensure that that the def paths are unique without its help.

Anon associated types did rely on this global state for uniqueness and are changed to use (method they're defined in + their position in the method return type) as the `DefPathData` to ensure uniqueness. This also means that the method they're defined in appears in error messages, which is nicer.

`DefPathData::NestedStatic` is added to use for nested data inside statics instead of reusing `DefPathData::AnonConst` to avoid conflicts with those.

cc `@oli-obk`

Author: bors

compiler/rustc_ast_lowering/src/expr.rs

@@ -492,9 +492,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
         let mut generic_args = ThinVec::new();
         for (idx, arg) in args.iter().cloned().enumerate() {
             if legacy_args_idx.contains(&idx) {
-                let parent_def_id = self.current_hir_id_owner.def_id;
                 let node_id = self.next_node_id();
-                self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, f.span);
+                self.create_def(node_id, None, DefKind::AnonConst, f.span);
                 let mut visitor = WillCreateDefIdsVisitor {};
                 let const_value = if let ControlFlow::Break(span) = visitor.visit_expr(&arg) {
                     AstP(Expr {

compiler/rustc_ast_lowering/src/lib.rs

@@ -500,12 +500,12 @@ enum GenericArgsMode {
 impl<'a, 'hir> LoweringContext<'a, 'hir> {
     fn create_def(
         &mut self,
-        parent: LocalDefId,
         node_id: ast::NodeId,
         name: Option<Symbol>,
         def_kind: DefKind,
         span: Span,
     ) -> LocalDefId {
+        let parent = self.current_hir_id_owner.def_id;
         debug_assert_ne!(node_id, ast::DUMMY_NODE_ID);
         assert!(
             self.opt_local_def_id(node_id).is_none(),
@@ -515,7 +515,11 @@ 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).def_id();
+        let def_id = self
+            .tcx
+            .at(span)
+            .create_def(parent, name, def_kind, None, &mut self.resolver.disambiguator)
+            .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);
@@ -787,7 +791,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             LifetimeRes::Fresh { param, kind, .. } => {
                 // Late resolution delegates to us the creation of the `LocalDefId`.
                 let _def_id = self.create_def(
-                    self.current_hir_id_owner.def_id,
                     param,
                     Some(kw::UnderscoreLifetime),
                     DefKind::LifetimeParam,
@@ -2113,16 +2116,14 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             hir::ConstArgKind::Path(qpath)
         } else {
             // Construct an AnonConst where the expr is the "ty"'s path.
-
-            let parent_def_id = self.current_hir_id_owner.def_id;
             let node_id = self.next_node_id();
             let span = self.lower_span(span);
 
             // Add a definition for the in-band const def.
             // We're lowering a const argument that was originally thought to be a type argument,
             // so the def collector didn't create the def ahead of time. That's why we have to do
             // it here.
-            let def_id = self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, span);
+            let def_id = self.create_def(node_id, None, DefKind::AnonConst, span);
             let hir_id = self.lower_node_id(node_id);
 
             let path_expr = Expr {

compiler/rustc_ast_lowering/src/pat.rs

@@ -522,14 +522,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         span: Span,
         base_type: Span,
     ) -> &'hir hir::ConstArg<'hir> {
-        let parent_def_id = self.current_hir_id_owner.def_id;
         let node_id = self.next_node_id();
 
         // Add a definition for the in-band const def.
         // We're generating a range end that didn't exist in the AST,
         // so the def collector didn't create the def ahead of time. That's why we have to do
         // it here.
-        let def_id = self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, span);
+        let def_id = self.create_def(node_id, None, DefKind::AnonConst, span);
         let hir_id = self.lower_node_id(node_id);
 
         let unstable_span = self.mark_span_with_reason(

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -17,13 +17,13 @@ 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_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
 use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
 use rustc_middle::query::TyCtxtAt;
 use rustc_middle::span_bug;
 use rustc_middle::ty::layout::TyAndLayout;
 use rustc_span::def_id::LocalDefId;
-use rustc_span::sym;
 use tracing::{instrument, trace};
 
 use super::{
@@ -66,6 +66,7 @@ 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
@@ -88,7 +89,13 @@ 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);
+        intern_as_new_static(
+            ecx.tcx,
+            static_id,
+            alloc_id,
+            alloc,
+            disambiguator.expect("disambiguator needed"),
+        );
     } else {
         ecx.tcx.set_alloc_id_memory(alloc_id, alloc);
     }
@@ -102,11 +109,18 @@ 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.
     let feed = tcx.create_def(
         static_id,
-        Some(sym::nested),
+        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());
 
@@ -154,6 +168,8 @@ 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 {
@@ -197,7 +213,9 @@ 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).unwrap().collect()
+        intern_shallow(ecx, base_alloc_id, base_mutability, Some(&mut disambiguator))
+            .unwrap()
+            .collect()
     };
     // We need to distinguish "has just been interned" from "was already in `tcx`",
     // so we track this in a separate set.
@@ -291,7 +309,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) {
+        match intern_shallow(ecx, alloc_id, inner_mutability, Some(&mut disambiguator)) {
             Ok(nested) => todo.extend(nested),
             Err(()) => {
                 ecx.tcx.dcx().delayed_bug("found dangling pointer during const interning");
@@ -313,8 +331,9 @@ 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).map_err(|()| err_ub!(DeadLocal))?).next()
+    if let Some(_) = intern_shallow(ecx, alloc_id, Mutability::Not, None)
+        .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
@@ -340,7 +359,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).unwrap() {
+        for prov in intern_shallow(self, alloc_id, Mutability::Not, None).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/def.rs

@@ -269,18 +269,10 @@ impl DefKind {
             | DefKind::TyParam
             | DefKind::ExternCrate => DefPathData::TypeNs(name.unwrap()),
 
-            // An associated type name will be missing for an RPITIT.
-            DefKind::AssocTy => {
-                if let Some(name) = name {
-                    DefPathData::TypeNs(name)
-                } else {
-                    DefPathData::AnonAssocTy
-                }
-            }
+            // An associated type name will be missing for an RPITIT (DefPathData::AnonAssocTy),
+            // but those provide their own DefPathData.
+            DefKind::AssocTy => DefPathData::TypeNs(name.unwrap()),
 
-            // It's not exactly an anon const, but wrt DefPathData, there
-            // is no difference.
-            DefKind::Static { nested: true, .. } => DefPathData::AnonConst,
             DefKind::Fn
             | DefKind::Const
             | DefKind::ConstParam

compiler/rustc_hir/src/definitions.rs

@@ -68,7 +68,7 @@ impl DefPathTable {
             //
             // See the documentation for DefPathHash for more information.
             panic!(
-                "found DefPathHash collision between {def_path1:?} and {def_path2:?}. \
+                "found DefPathHash collision between {def_path1:#?} and {def_path2:#?}. \
                     Compilation cannot continue."
             );
         }
@@ -97,13 +97,31 @@ impl DefPathTable {
     }
 }
 
+#[derive(Debug)]
+pub struct DisambiguatorState {
+    next: UnordMap<(LocalDefId, DefPathData), u32>,
+}
+
+impl DisambiguatorState {
+    pub fn new() -> Self {
+        Self { next: Default::default() }
+    }
+
+    /// Creates a `DisambiguatorState` where the next allocated `(LocalDefId, DefPathData)` pair
+    /// will have `index` as the disambiguator.
+    pub fn with(def_id: LocalDefId, data: DefPathData, index: u32) -> Self {
+        let mut this = Self::new();
+        this.next.insert((def_id, data), index);
+        this
+    }
+}
+
 /// The definition table containing node definitions.
 /// It holds the `DefPathTable` for `LocalDefId`s/`DefPath`s.
 /// It also stores mappings to convert `LocalDefId`s to/from `HirId`s.
 #[derive(Debug)]
 pub struct Definitions {
     table: DefPathTable,
-    next_disambiguator: UnordMap<(LocalDefId, DefPathData), u32>,
 }
 
 /// A unique identifier that we can use to lookup a definition
@@ -127,7 +145,7 @@ impl DefKey {
         let DisambiguatedDefPathData { ref data, disambiguator } = self.disambiguated_data;
 
         std::mem::discriminant(data).hash(&mut hasher);
-        if let Some(name) = data.get_opt_name() {
+        if let Some(name) = data.hashed_symbol() {
             // Get a stable hash by considering the symbol chars rather than
             // the symbol index.
             name.as_str().hash(&mut hasher);
@@ -173,7 +191,11 @@ impl DisambiguatedDefPathData {
                 }
             }
             DefPathDataName::Anon { namespace } => {
-                write!(writer, "{{{}#{}}}", namespace, self.disambiguator)
+                if let DefPathData::AnonAssocTy(method) = self.data {
+                    write!(writer, "{}::{{{}#{}}}", method, namespace, self.disambiguator)
+                } else {
+                    write!(writer, "{{{}#{}}}", namespace, self.disambiguator)
+                }
             }
         }
     }
@@ -287,10 +309,13 @@ pub enum DefPathData {
     /// An existential `impl Trait` type node.
     /// Argument position `impl Trait` have a `TypeNs` with their pretty-printed name.
     OpaqueTy,
-    /// An anonymous associated type from an RPITIT.
-    AnonAssocTy,
+    /// An anonymous associated type from an RPITIT. The symbol refers to the name of the method
+    /// that defined the type.
+    AnonAssocTy(Symbol),
     /// A synthetic body for a coroutine's by-move body.
     SyntheticCoroutineBody,
+    /// Additional static data referred to by a static.
+    NestedStatic,
 }
 
 impl Definitions {
@@ -342,24 +367,33 @@ impl Definitions {
         let root = LocalDefId { local_def_index: table.allocate(key, def_path_hash) };
         assert_eq!(root.local_def_index, CRATE_DEF_INDEX);
 
-        Definitions { table, next_disambiguator: Default::default() }
+        Definitions { table }
     }
 
-    /// Adds a definition with a parent definition.
-    pub fn create_def(&mut self, parent: LocalDefId, data: DefPathData) -> LocalDefId {
+    /// Creates a definition with a parent definition.
+    /// If there are multiple definitions with the same DefPathData and the same parent, use
+    /// `disambiguator` to differentiate them. Distinct `DisambiguatorState` instances are not
+    /// guaranteed to generate unique disambiguators and should instead ensure that the `parent`
+    /// and `data` pair is distinct from other instances.
+    pub fn create_def(
+        &mut self,
+        parent: LocalDefId,
+        data: DefPathData,
+        disambiguator: &mut DisambiguatorState,
+    ) -> LocalDefId {
         // 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:?})",
             self.def_path(parent).to_string_no_crate_verbose(),
         );
 
-        // The root node must be created with `create_root_def()`.
+        // 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 = self.next_disambiguator.entry((parent, data)).or_insert(0);
+            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
@@ -422,8 +456,30 @@ impl DefPathData {
             | Ctor
             | AnonConst
             | OpaqueTy
-            | AnonAssocTy
-            | SyntheticCoroutineBody => None,
+            | AnonAssocTy(..)
+            | SyntheticCoroutineBody
+            | NestedStatic => None,
+        }
+    }
+
+    fn hashed_symbol(&self) -> Option<Symbol> {
+        use self::DefPathData::*;
+        match *self {
+            TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) | AnonAssocTy(name) => {
+                Some(name)
+            }
+
+            Impl
+            | ForeignMod
+            | CrateRoot
+            | Use
+            | GlobalAsm
+            | Closure
+            | Ctor
+            | AnonConst
+            | OpaqueTy
+            | SyntheticCoroutineBody
+            | NestedStatic => None,
         }
     }
 
@@ -443,8 +499,9 @@ impl DefPathData {
             Ctor => DefPathDataName::Anon { namespace: sym::constructor },
             AnonConst => DefPathDataName::Anon { namespace: sym::constant },
             OpaqueTy => DefPathDataName::Anon { namespace: sym::opaque },
-            AnonAssocTy => DefPathDataName::Anon { namespace: sym::anon_assoc },
+            AnonAssocTy(..) => DefPathDataName::Anon { namespace: sym::anon_assoc },
             SyntheticCoroutineBody => DefPathDataName::Anon { namespace: sym::synthetic },
+            NestedStatic => DefPathDataName::Anon { namespace: sym::nested },
         }
     }
 }