-
Notifications
You must be signed in to change notification settings - Fork 370
fix(elaborator): Check that trait constraints aren't unifying unbound associated types from different traits #11383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 946ba40 | Previous: c695737 | 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ 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: 02f0421 | Previous: 413148b | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
1dad12f to
d8394f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ 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: d8394f3 | Previous: 413148b | Ratio |
|---|---|---|---|
rollup-checkpoint-merge |
0.002 s |
0.001 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
02f0421 to
c552525
Compare
NamedGeneric in bind_named_generics
Description
Problem
Resolves #11348
Summary
lookup_trait_implementation_helperto perform an extra check before trying to unify associated types, to avoid allowing an associated type in one trait to unify with an unbound type variable that refers to another associated type in another trait.Debugformat forLocationin the form of{file_id}/{start}:{end}to make debug formats less noisyDisplayformat ofType::NamedGenericto use the name if its type variable is bound to an unbound type variable, which would ultimately be rendered as_, hiding information. This is to avoid error messages likeexpected Self::Baz but got _instead ofexpected Self::Baz but got Self::Bar(I'll look at the ambiguity aspect of inheritance separately in #11401, I just included a test to show what's wrong with it).
Additional Context
In the unit test we have this code:
Rust rejects his, because
<Self as Foo>::Baris not the same type as<Self as Qux>::Baz, unless we add more trait constraints to say otherwise. In Noir, we accept this code, and only try to prove it works on concrete implementations.The way it works is that:
Baris aNamedGenericBar'1; when we introduce the constraint thatSelfisFoo, it is added generic as aTypeVariable, designated'6.foo, it starts with the typeforall '1 '2: Self::Bar'1 -> Self::Bar'1(withSelf'2being the other named generic), and gets got elaborated intofn('6) -> '6.x,'6gets bound toSelf::Baz'5Foo, a trait constraint to be resolved when the function context is popped.Self::Bar'1toSelf::Baz'5, which are different, but because it actually ends up trying to unifySelf::Bar'1against'5, which just binds'5. Instead of doing this, we now say that if'5is still unbound, then we cannot call these equivalent.User Documentation
Check one:
PR Checklist
cargo fmton default settings.