Skip to content

Stop passing resolver disambiguator state to AST lowering.#143361

Merged
bors merged 1 commit intorust-lang:masterfrom
cjgillot:split-disambig
Oct 25, 2025
Merged

Stop passing resolver disambiguator state to AST lowering.#143361
bors merged 1 commit intorust-lang:masterfrom
cjgillot:split-disambig

Conversation

@cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jul 3, 2025

AST->HIR lowering can use a disjoint set of DefPathData as the resolver, so we don't need to pass the disambiguator state.

r? @oli-obk

@rustbot 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 Jul 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2025

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

Some changes occurred in tests/codegen/sanitizer

cc @rcvalle

cjgillot
cjgillot commented Jul 3, 2025
View reviewed changes
compiler/rustc_symbol_mangling/src/v0.rs
DefPathData::Ctor => 'c',
DefPathData::AnonConst => 'k',
DefPathData::AnonConst => 'K',
DefPathData::LateAnonConst => 'k',
Copy link
Contributor Author

@cjgillot cjgillot Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain changing mangling is the best idea. Is there some update to a demangler to be made before this PR is accepted?

Copy link
Contributor

@oli-obk oli-obk Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised you're not getting the demangler roundtrip ICE, but most likely this code path isn't hit in codegen tests with v0 mangling.

I don't think it's possible without changing mangling

Copy link
Contributor Author

@cjgillot cjgillot Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find where in the demangler code this is used. IIUC, there are only two categories that are checked, 'C' and 'S', the latter not being produced any more. Should I look somewhere else?

https://github.com/rust-lang/rustc-demangle/blob/c5688cfec32d2bd00701836f12beb3560ee015b8/src/v0.rs#L834-L838

Copy link
Contributor

@oli-obk oli-obk Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is unused, the demangler will just fail to demangle. Which is fine, you'd just get a weird symbol. We do have a roundtrip assertion checking that all symbols emitted in tests can be demangled back to the original symbol. I'm not quite certain why this is not causing issues, but maybe paths are just handled well enough together that it all works out.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2025

@rustbot author

Please add a codegen test (which will probably ICE if the compiler is built with debug assertions as the demangler can't support this new syntax yet).

@rustbot rustbot 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 Jul 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@bors
Copy link
Collaborator

bors commented Jul 22, 2025

☔ The latest upstream changes (presumably #144249) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Oct 20, 2025

Some changes occurred in tests/codegen-llvm/sanitizer

cc @rcvalle

@rustbot

This comment has been minimized.

Sign in to view
@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 24, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 25, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 25, 2025

📌 Commit b67453f has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 25, 2025
Merged
bors added a commit that referenced this pull request Oct 25, 2025
@bors
Auto merge of #148100 - matthiaskrgr:rollup-vrd4wy8, r=matthiaskrgr
868b2e0 
Rollup of 4 pull requests

Successful merges:

 - #143361 (Stop passing resolver disambiguator state to AST lowering.)
 - #148000 (Improvements to attribute suggestions)
 - #148007 (chore: Update to the latest annotate-snippets)
 - #148088 (compiletest: Simplify passing arguments to spawned test threads)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Oct 25, 2025
@bors
Auto merge of #148100 - matthiaskrgr:rollup-vrd4wy8, r=matthiaskrgr
34f954f 
Rollup of 4 pull requests

Successful merges:

 - #143361 (Stop passing resolver disambiguator state to AST lowering.)
 - #148000 (Improvements to attribute suggestions)
 - #148007 (chore: Update to the latest annotate-snippets)
 - #148088 (compiletest: Simplify passing arguments to spawned test threads)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cee562c into rust-lang:master Oct 25, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Oct 25, 2025
rust-timer added a commit that referenced this pull request Oct 25, 2025
@rust-timer
Unrolled build for #143361
d78d83a 
Rollup merge of #143361 - cjgillot:split-disambig, r=oli-obk

Stop passing resolver disambiguator state to AST lowering.

AST->HIR lowering can use a disjoint set of `DefPathData` as the resolver, so we don't need to pass the disambiguator state.

r? `@oli-obk`
@cjgillot cjgillot deleted the split-disambig branch October 26, 2025 01:49
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Oct 27, 2025
@bors
Auto merge of #148100 - matthiaskrgr:rollup-vrd4wy8, r=matthiaskrgr
36538ec 
Rollup of 4 pull requests

Successful merges:

 - rust-lang/rust#143361 (Stop passing resolver disambiguator state to AST lowering.)
 - rust-lang/rust#148000 (Improvements to attribute suggestions)
 - rust-lang/rust#148007 (chore: Update to the latest annotate-snippets)
 - rust-lang/rust#148088 (compiletest: Simplify passing arguments to spawned test threads)

r? `@ghost`
`@rustbot` modify labels: rollup
@github-actions github-actions bot mentioned this pull request Oct 30, 2025
Draft
@dawidl022
Copy link
Contributor

dawidl022 commented Dec 27, 2025

@cjgillot What was the rationale behind this change?

I spent a few hours figuring out why the disambiguator failed to resolve collisions in the DefPathData. I'd like to document this design choice on DefPathData to make it clear that nodes created during AST->HIR lowering should use a disjoint set of enum variants, prefixed with Late, but I also want to understand why this change was made in the first place.

@cjgillot
Copy link
Contributor Author

cjgillot commented Dec 27, 2025

The high level objective is to stop having to carry the disambiguator state between compiler passes. The disambiguator state, as a pseudo-global mutable state is incompatible with incremental compilation.

1 similar comment
@cjgillot
Copy link
Contributor Author

cjgillot commented Dec 27, 2025

The high level objective is to stop having to carry the disambiguator state between compiler passes. The disambiguator state, as a pseudo-global mutable state is incompatible with incremental compilation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels

PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.93.0

Development

Successfully merging this pull request may close these issues.

5 participants

@cjgillot @rustbot @oli-obk @bors @dawidl022