delegation: fix def path hash collision, add per parent disambiguators

[aerooneqq]

View all comments

This PR addresses the following delegation issues:

• It fixes [ICE]: found DefPathHash collision between DefPath #153410 when generating new DefIds for generic parameters by saving DisambiguatorStates from resolve stage and using them at AST -> HIR lowering introducing per owner disambiguators and transferring them to AST -> HIR lowering stage
• Next it fixes the ICE which is connected to using DUMMY_SP in delegation code, which was found during previous fix
• Finally, after those fixes the ICE: delegation: DefId::expect_local DefId(..) isn't local #143498 is also fixed, only bugs with propagating synthetic generic params are left.

Fixes #153410. Part of #118212.

r? @petrochenkov

[petrochenkov]

This is all pretty awful.
A number of other def ids / def paths are also created during AST lowering, but they don't require keeping per-node disambiguator tables around. Why is delegation so special in this regard?

What exactly definitions create collisions like this? Generic parameters?

As I understand the small per-node disambiguator tables partially duplicate content from the big table.
What prevents us from looking up the current disambiguators in the big table instead?

[aerooneqq]

A number of other def ids / def paths are also created during AST lowering, but they don't require keeping per-node disambiguator tables around. Why is delegation so special in this regard?

We generate new DefIds for each delegation generic param, as creation of DefId requires a name, we want to use names that are defined in a function being reused, for example for proper error messages. It is possible to create DefIds with, for example, sym::dummy name, but in this case error messages will be terrible. If we prefer to use names of generic params of the delegee, then we use names that are created by user, so they can clash with already used names in delegation.

The difference between delegation and other DefIds creation is that latter don't use user-defined names, as other cases of DefIds creation are DefPathData::DesugaredAnonymousLifetime which uses Some(kw::UnderscoreLifetime) as name and DefPathData::LateAnonConst which uses None as name.

Maybe we can create our own DefPathData variant and use it during creation of DefIds, but it still will need to hold the name of the param, as it is required for example in hir_ty_param_name. I am not sure that this is a correct option.

What exactly definitions create collisions like this? Generic parameters?

From our side yes, when we create generic params, their def path data can be either DefPathData::TypeNs(symbol) for DefKind::TyParam, DefPathData::ValueNs(symbol) for DefKind::ConstParam or DefPathData::LifetimeNs(symbol) for DefKind::LifetimeParam, so this def path data can have a collision with anything that have same def path created in scope of delegation LocalDefId during resolve stage.

What prevents us from looking up the current disambiguators in the big table instead?

Big table which is Resolver::disambiguator is not passed anywhere, as DefIds which are created during AST -> HIR lowering before this PR do not need it. Passing the whole disambiguator to lowering stage seems to be not the best option as it contains many unneeded information and it is not passed from Resolver to ResolverAstLowering now.

Internally DisambiguatorState uses UnordMap<(LocalDefId, DefPathData), u32> which prevents from fast lookups by LocalDefId only and changing it (for example to UnordMap<LocalDefId, UnordMap<DefPathData, u32>>) seemed to be out of scope of this PR.

Given this underlying structure I also didn't want to iterate over all map to find delegation-related key-values, and I also didn't want to expose the possibility of traversal of whole DisambiguatorState.

This left me with an option that I implemented in this PR: for each delegation we create additional disambiguator which follows the main one and then stored in delegation_infos map, for this purpose I exposed next operation on DisambiguatorState, which is also not that good, beacuase it was inlined in create_def function, thus only create_def could update it, but in the given conditions I think it is more-or-less OK solution.

[aerooneqq]

@rustbot ready

To get feedback on the comments.

[petrochenkov]

This PR may be relevant:

• Stop passing resolver disambiguator state to AST lowering. #143361

It solved the collision problem by adding new DefPathData variants.

[petrochenkov]

Blocked on #154049.
Can try the approach with new DefPathData variants in the meantime.
@rustbot blocked

[rustbot]

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

[aerooneqq]

Removed duplicated test for #143498.

[petrochenkov]

r=me after rebase.
@bors delegate+

[rust-bors]

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

[rustbot]

This PR was rebased onto a different main 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.

[aerooneqq]

@bors r=petrochenkov

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-t-compiler.

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 699eb29 has been approved by petrochenkov

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: petrochenkov
Duration: 3h 23m 6s
Pushing 4dbafc3 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 27dbdb5 (parent) -> 4dbafc3 (this PR)

Test differences

Show 21 test diffs

Stage 1

• [ui] tests/ui/delegation/generics/def-path-hash-collision-ice-153410.rs: [missing] -> pass (J1)

Stage 2

• [ui] tests/ui/delegation/generics/def-path-hash-collision-ice-153410.rs: [missing] -> pass (J0)

Additionally, 19 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-21-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, optional-x86_64-gnu-parallel-frontend, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-21, x86_64-gnu-llvm-21-2, x86_64-gnu-llvm-22-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: x86_64-gnu-llvm-21-3, x86_64-gnu-llvm-22-3

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 4dbafc340b0f8e9ff9e95974d0ceef81ae2652c8 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. i686-gnu-nopt-2: 1h 44m -> 2h 14m (+28.5%)
2. dist-i686-msvc: 1h 51m -> 2h 18m (+24.2%)
3. x86_64-gnu-aux: 1h 55m -> 2h 21m (+22.8%)
4. dist-powerpc64-linux-gnu: 1h 33m -> 1h 14m (-19.8%)
5. dist-powerpc64le-linux-musl: 1h 34m -> 1h 17m (-18.9%)
6. tidy: 3m 8s -> 2m 40s (-14.7%)
7. x86_64-gnu-llvm-22-2: 1h 41m -> 1h 29m (-11.4%)
8. dist-x86_64-apple: 2h 5m -> 1h 52m (-10.0%)
9. armhf-gnu: 1h 38m -> 1h 28m (-9.8%)
10. x86_64-gnu-debug: 2h 3m -> 1h 52m (-9.4%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (4dbafc3): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	3
Regressions ❌ (secondary)	0.3%	[0.0%, 0.4%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	3

Max RSS (memory usage)

Results (primary 0.4%, secondary 0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.7%, 2.0%]	5
Regressions ❌ (secondary)	2.3%	[1.3%, 3.3%]	5
Improvements ✅ (primary)	-1.7%	[-2.0%, -1.4%]	2
Improvements ✅ (secondary)	-1.8%	[-3.1%, -1.0%]	3
All ❌✅ (primary)	0.4%	[-2.0%, 2.0%]	7

Cycles

Results (primary 2.3%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	5.6%	[3.4%, 9.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.1%]	2
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

Results (primary 0.1%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.3%]	52
Regressions ❌ (secondary)	0.2%	[0.0%, 0.9%]	34
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.3%]	52

Bootstrap: 490.301s -> 493.358s (0.62%)
Artifact size: 394.23 MiB -> 394.12 MiB (-0.03%)

[oli-obk]

I didn't see this. I don't like that this was done in a manner that didn't need input from us.

Disambiguators are not visible except in symbols. Why was this a problem?