fix(elaborator): Disambiguate associated types from ordered generics

[aakoshh]

Description

Problem

Resolves #10864

Summary

Changes the name assigned to NamedGeneric to be Self::{type} or <{object} as {trait}>::{type} for associated types, to disambiguate this generic from non-associated generics of the trait.

Additional Context

This does not solve #10858, the following is still rejected:

pub trait Foo {
    type Bar;
    fn foo_bar() -> <Self as Foo>::Bar;
}
fn main() {}

Because of this such types currently get the name Self::Bar. I'll try to solve that as a follow up.

I also found that the current fix doesn't work across different traits that share the same associated type name, even if I uncomment the special casing for Self and render them as <Self as Foo>::Bar: #11347 Since there is no mixing of ordered and named generics there, I think we can treat it as a follow up.(It turns out names don't even matter there).

User Documentation

Check one:

• No user documentation needed.
• Changes in docs/ included in this PR.
• [For Experimental Features] Changes in docs/ to be submitted in a separate PR.

PR Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[jfecher]

Resolves #10864

I'd honestly be fine rejecting this code outright as well.

Seems like in general though we're using these names too often. An invariant of Type::Generic is that you should only ever have 1 set of generics in scope. You shouldn't ever have to match generics from one context against another. In situations where that may arise the types always need to be instantiated, such as when we instantiate variables referring to generic functions. Maybe we're missing an instantiation somewhere when doing these trait checks or some other check we have is hacky and bypassing them.

I do think it would be easier rejecting the code in the original issue and would lead to slightly simpler compiler code (it just seems confusing to me to allow type parameters & associated types of the same name in 1 trait), but if we want to allow it then we'd need to rename one of them internally as you've done here. My paragraph above still stands in regards to having multiple trait constraints though.

but even if I go back to render the type as ::Item it doesn't reject it, so there is some other reason

It may be using unbound type variables for these types under the hood.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 104d1c8	Previous: f00ead9	Ratio
test_report_zkpassport_noir_rsa_	1 s	0 s	+∞

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 104d1c8	Previous: f00ead9	Ratio
rollup-block-root-single-tx	0.003 s	0.002 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[aakoshh]

@jfecher thank you for the insights into how it should work. I'm not sure where we might be missing something, but I found an instance where the NamedGeneric was already assigned this <{object} as {trait}>::{name} label instead of just name:

noir/compiler/noirc_frontend/src/elaborator/traits.rs

Line 392 in f00ead9

let name = format!("<{object} as {trait_name}>::{}", associated_type.name);

I tried to make sure it's more difficult to miss these by adding them to them method.

I do think it would be easier rejecting the code in the original issue and would lead to slightly simpler compiler code

I think that might be an illusion of simplicity, since we'd still be using that pattern, but it would be (at least for me) less clear where it needs to be applied and where it doesn't. With this change hopefully it's a bit simpler to think about it: if you deal with associated types, please pass the trait on, otherwise don't.

The error messages are also more akin to Rust's this way, always putting associated types into context.

It may be using unbound type variables for these types under the hood.

I had a look: #11347 (comment)

It turns out that the generics were added implicitly, which is why it doesn't go into this rule but stop at the equality check.

I am not sure exactly what's so special about these being implicit, but it's a soundness bug: it allows me to pass invalid data, and if it satisfies SSA then it goes along with it.

[aakoshh]

@jfecher I opened #11351 to fix that other issue, but it's also based on this change. If we didn't add more context to the names, then it would need a different solution as the names in the example are the same, but both are just associated types, no mixing with ordered.

[jfecher]

I'm not sure where we might be missing something, but I found an instance where the NamedGeneric was already assigned this <{object} as {trait}>::{name} label instead of just name:

I think that was originally added so that when the type showed up in error messages it was more clear where it originated from.

I opened #11351 to fix that other issue, but it's also based on this change. If we didn't add more context to the names, then it would need a different solution as the names in the example are the same, but both are just associated types, no mixing with ordered.

I'm fine with going through with this PR, just a bit worried since it exposes just how stringly-typed our current system is. E.g. maybe we should only compare by their typevariableid

[aakoshh]

@jfecher I agree with the stringiness being a problem. I actually started this PR by adding an optional TraitId to the NamedGeneric and comparing that in b3c4e32

It worked, but the error message was still saying "expected Bar but got Bar", so I went down the route of following the <{object} as {trait>::{name} example, thinking that you really wanted unification to be based on stuffing the name with more information. Thanks for sharing that it was meant for debug purposes.

I created another ticket where the same trait and type appears across different modules, to show that comparing names the way I do in this PR is still insufficient:
#11358

I'll open another one tomorrow to add back the TraitId and then the name can be info only.

[jfecher]

@aakoshh I'd rather an approach based on the typevariableids that are already there. As is done in e.g. #11354. I think with #11358 specifically (I tested it with that PR) there must be unbound type variables that are being bound to each other first. This would be consistent with your note that these types are set equal to each other first, then tested.

[aakoshh]

@jfecher that sounds good. Just to confirm you are seeing that sometimes the type variables are different, but they should still unify, right?

(Actually with the TraitId it wouldn't cover the same item from different objects, so it would need that information as well).

[jfecher]

@aakoshh I'm not 100% sure what you mean but I always expect the same named generic to have the same type variable id and for different ones to have different ids.

For the type variable comment I was speculating that we may allow unification of associated types when they're only implicitly specified. E.g we treat

fn foo<T: Iterator>(x: T) {}

as

fn foo<T: Iterator<Item = _>>(x: T) {}

or at least that was my theory but #7026 shows we've always added them as actual, but implicit NamedGenerics. The behavior we're seeing does look like type variables being bound over though so perhaps that is still happening somewhere.