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

Zoxc · 2025-04-29T12:14:22Z

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.

cc @oli-obk

…iguatorState` type

rustbot · 2025-04-29T12:14:26Z

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-04-29T12:14:28Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

compiler/rustc_const_eval/src/const_eval/machine.rs

petrochenkov · 2025-04-29T12:52:05Z

compiler/rustc_hir/src/definitions.rs

+
+impl DisambiguatorState {
+    pub fn new() -> Self {
+        Self { next: Default::default() }


#[derive(Default)] can be used instead of this.

petrochenkov · 2025-04-29T12:55:12Z

compiler/rustc_hir/src/definitions.rs

@@ -288,7 +310,7 @@ pub enum DefPathData {
    /// Argument position `impl Trait` have a `TypeNs` with their pretty-printed name.
    OpaqueTy,
    /// An anonymous associated type from an RPITIT.
-    AnonAssocTy,
+    AnonAssocTy(Symbol),


Need a comment telling what the symbol is.

petrochenkov · 2025-04-29T12:56:41Z

compiler/rustc_hir/src/definitions.rs

@@ -411,7 +442,9 @@ impl DefPathData {
    pub fn get_opt_name(&self) -> Option<Symbol> {
        use self::DefPathData::*;
        match *self {
-            TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) => Some(name),
+            TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) | AnonAssocTy(name) => {


The AnonAssocTy case here doesn't seem right.

It does seem a little bit odd. I could duplicate get_opt_name to have a variant that's used for stable hashing.

compiler/rustc_middle/src/ty/context.rs

petrochenkov · 2025-04-29T13:00:51Z

compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

+        None,
+        DefKind::SyntheticCoroutineBody,
+        None,
+        &mut DisambiguatorState::new(),


This needs a comment explaining why the disambiguate is correct, the removed comment sort of did that.

petrochenkov · 2025-04-29T13:03:36Z

compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

+        tcx,
+        rbv: &mut rbv,
+        scope: &Scope::Root { opt_parent_item: None },
+        disambiguator: &mut DisambiguatorState::new(),


Probably also needs a comment telling what is the def parenting here and why the disambiguator is correct.

…Const`

compiler/rustc_const_eval/src/interpret/intern.rs

RalfJung · 2025-05-02T14:11:03Z

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -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();


This is just a number, right? So it's cheap to create even when we don't need it (like for non-static consts)?

it's a hash map, but that's also cheap if it never gets pushed to

Makes sense. A perf run would probably still be a good idea (once the PR is ready for that) given how this changes a bunch of stuff all over the compiler and some of that could easily be hot code.

oli-obk and others added 2 commits April 28, 2025 18:11

We always use the current item as parent, so no need to pass it

2ac60bc

Remove global next_disambiguator state and handle it with a `Disamb…

e561ec0

…iguatorState` type

rustbot assigned estebank Apr 29, 2025

rustbot added PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 29, 2025

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Apr 29, 2025

Update stable-mir test

df1f8d1

RalfJung reviewed Apr 29, 2025

View reviewed changes

compiler/rustc_const_eval/src/const_eval/machine.rs Outdated Show resolved Hide resolved

oli-obk assigned oli-obk and unassigned estebank Apr 29, 2025

petrochenkov reviewed Apr 29, 2025

View reviewed changes

petrochenkov removed their assignment Apr 29, 2025

Zoxc added 4 commits May 2, 2025 12:24

Rename parameter to override_def_path_data

ddff387

Add DefPathData::NestedStatic instead of reusing `DefPathData::Anon…

efc51ce

…Const`

Add comment about the symbol on AnonAssocTy

d16daf5

Split get_opt_name hashing use into hashed_symbol

4869e20

oli-obk reviewed May 2, 2025

View reviewed changes

compiler/rustc_const_eval/src/interpret/intern.rs Outdated Show resolved Hide resolved

oli-obk removed the PG-exploit-mitigations Project group: Exploit mitigations label May 2, 2025

Move DisambiguatorState into intern_const_alloc_recursive

6a48217

Zoxc force-pushed the next-disambiguator branch from d5a3b9c to 6a48217 Compare May 2, 2025 13:51

rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label May 2, 2025

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2025

RalfJung reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Zoxc commented Apr 29, 2025

rustbot commented Apr 29, 2025

rustbot commented Apr 29, 2025

This comment has been minimized.

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

Zoxc Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

RalfJung May 2, 2025

oli-obk May 2, 2025

RalfJung May 2, 2025 •

edited

Loading

Remove global next_disambiguator state and handle it with a DisambiguatorState type #140453

Are you sure you want to change the base?

Remove global next_disambiguator state and handle it with a DisambiguatorState type #140453

Conversation

Zoxc commented Apr 29, 2025

rustbot commented Apr 29, 2025

rustbot commented Apr 29, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 2, 2025 • edited Loading

Choose a reason for hiding this comment

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

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

RalfJung May 2, 2025 •

edited

Loading