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

[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]

@rustbot ready

[petrochenkov]

r=me after squashing commits.
@rustbot author

[rustbot]

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

[petrochenkov]

@cjgillot @oli-obk
This PR effectively reverts #143361 and passes disambiguators from name resolution to AST lowering again (but now in a per-owner fashion).

I think the approach from #143361 is not very scalable, because it leaks internal details like desugaring something during AST lowering into the stable mangling scheme.

If we followed it in this PR, we'd need separate DefPathData and mangling symbols for delegated functions (as opposed to regular functions), but nobody, except perhaps diagnostics, should care whether the function was delegated or not after AST lowering. Other future desugarings may require even more new DefPathDatas and mangling symbols, I'd rather stop it now.
Unfortunately, definitions created even later, after AST lowering, like DefPathData::NestedStatic, still require changing the mangling scheme.

(I'll keep this PR open for a few days before approving.)

[aerooneqq]

@rustbot ready

[aerooneqq]

Removed duplicated test for #143498.