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

RalfJung · 2025-04-29T12:49:10Z

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -63,7 +64,7 @@ pub struct CompileTimeMachine<'tcx> {
    /// If `Some`, we are evaluating the initializer of the static with the given `LocalDefId`,
    /// storing the result in the given `AllocId`.
    /// Used to prevent reads from a static's base allocation, as that may allow for self-initialization loops.


This doc comment should be updated to explain the role of this DisambiguatorState component.

Looking at the const_eval diff in this PR, I am completely clueless about what is happening.^^

Const eval is generating statics (not quite sure why, that's should probably be part of the comment) and DisambiguatorState is used to ensure these have unique names (def paths).

Const eval is generating statics (not quite sure why, that's should probably be part of the comment)

That part I can explain. :D Though it's way outside the scope for this comment I think -- this field is not directly related to generating those statics.

A static can require more than one allocation for its result, e.g. if you do static S = &Vec::new();. We generate anonymous statics for those inner allocations. (Note that this example does not involve any const promotion.)

It seems like what you are doing is funneling some state through to the allocation interner. That is the place where we should have comments explaining why and how we generate anonymous statics.

DisambiguatorState is used to ensure these have unique names (def paths).

Seems like ideally this would be managed by the interner. It's not relevant during const-eval, only after the end of const-eval, so ideally it should not be present during const-eval.

yea that one is my fault. It was just the minimal diff allowing this, doing it solely in the interner... I guess would work unconditionally, not even basing it off on whether we are interning a static item, but just generally. Most code won't use it, but initializing an empty map is not more expensive than wrapping it in an option

Which interner are you talking about? It could also just be a counter, if it's per-source static.

I also noticed it's using a nested symbol. That's should probably be a DefPathData variant to avoid collisions with source items.

Which interner are you talking about?

compiler/rustc_const_eval/src/interpret/intern.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.

petrochenkov · 2025-04-29T12:58:54Z

compiler/rustc_middle/src/ty/context.rs

@@ -1972,8 +1974,10 @@ impl<'tcx> TyCtxt<'tcx> {
        parent: LocalDefId,
        name: Option<Symbol>,
        def_kind: DefKind,
+        def_path_data: Option<DefPathData>,


Suggested change

def_path_data: Option<DefPathData>,

override_def_path_data: Option<DefPathData>,

or something like this.

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.

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

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

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.

RalfJung Apr 29, 2025

Zoxc Apr 29, 2025

RalfJung Apr 29, 2025

oli-obk Apr 29, 2025

Zoxc Apr 29, 2025

RalfJung Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

Zoxc Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

petrochenkov Apr 29, 2025

	def_path_data: Option<DefPathData>,
	override_def_path_data: Option<DefPathData>,

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

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

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