Fixes multiple trait constraints for same method. #3818
Conversation
esdrubal
added
bug
There was a problem hiding this comment.
I think what I'm seeing here is that the diagnosis for this bug is that it is occurring because replace_decls is replacing methods defined on E with methods defined on T? And that this is because fromT: Eq and E: Eq, both of these Eq's share a common parent?
Hmm, this is tricky. And it's tempting to block this issue behind #1267, and solving that first would make solving this one much more approachable.
| continue; | ||
| } | ||
|
|
||
| let decl = engines.de().get(dest_decl_id.clone()); |
There was a problem hiding this comment.
Instead of adding these changes here, does it work to instead copy the code from lines 645-660 and add it to the definition of replace_decls on DeclId?
There was a problem hiding this comment.
This is inside a for loop, I could move the whole block from 620 to 669, but I think it looks better on the context it is now.
| @@ -191,7 +191,7 @@ impl TypeParameter { | |||
| let mut errors = vec![]; | |||
|
|
|||
| let mut original_method_ids: BTreeMap<Ident, DeclId> = BTreeMap::new(); | |||
| let mut impld_method_ids: BTreeMap<Ident, DeclId> = BTreeMap::new(); | |||
| let mut impld_method_ids: BTreeMap<IdentUnique, DeclId> = BTreeMap::new(); | |||
There was a problem hiding this comment.
The difference between Ident and IdentUnique is that it allows Span information to make idents unique when hashing and comparing them. We should not rely on span information in this part of the compiler to determine trait resolution. Among other reasons, Spans will not always be what the Sway compiler uses and may be upgraded in the future.
There was a problem hiding this comment.
I borrowed the concept of TyFunctionSig from #3878 and replaced IdentUnique usage with (Ident, TyFunctionSig).
When multiple trait constraints were used for same method but with different types an error would occur as the replace of the declaration would pick only one trait method implementation for both constraints. The solution was to use IdentUnique while collecting traits implemented methods. This allows to create `DeclMapping` mappings from one trait original method to multiple trait implementations. For instance eq method original method to both bool and u64 trait implementations. The other step of the solution was to verify while replacing decls if the current function application beeing replaced matches the argument types of one decl mapping specifically.
Replaces `IdentUnique` usage with `(Ident, TyFunctionSig)`. Borrowed TyFunctionSig from 3878.
esdrubal
force-pushed
the
esdrubal/3325_multiple_trait_constraints
branch
from
f01a14c
to
174438f
Compare
|
After some discussion offline, this ticket was found to be blocked behind #1267. While it is not intractable to solve this specific bug without #1267, this bug is a symptom of a deeper bug with the |
|
Already fixed in master. #5553 adds test case. |
## Description PR #3818 is no longer necessary and this PR proves that the test cases added on in already pass on master without any additional change. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers.
When multiple trait constraints were used for same method but with different types an error would occur as the replace of the declaration would pick only one trait method implementation for both constraints.
The solution was to use
IdentUniquewhile collecting traits implemented methods. This allows to createDeclMappingmappings from one trait original method to multiple trait implementations. For instance eq method original method to both bool and u64 trait implementations.The other step of the solution was to verify while replacing decls if the current function application being replaced matches the argument types of one decl mapping specifically.
The issue it solves is related with #3325
This is another subset of #3621