Skip to content
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

Fixes issue with generic trait impls using the same method names. #3676

Merged
merged 6 commits into from
Feb 3, 2023

Conversation

esdrubal
Copy link
Contributor

@esdrubal esdrubal commented Jan 3, 2023

When two generic trait implementation use the same method name with a different type parameter an error such as Duplicate definition for method "convert" for type "A" would occur.

This fix does remove the error when a method with diferent type parameters is added to another trait implementation with different type parameters.
The fix also ensures that when possible find_method_for_type returns a method whose parameters types matches the given arguments expressions return types.

Closes #3633.

@esdrubal esdrubal added bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged P: critical Should be looked at before anything else labels Jan 3, 2023
@esdrubal esdrubal self-assigned this Jan 3, 2023
@esdrubal esdrubal force-pushed the duplicate_def_3633 branch from 2cfa494 to 89a0a97 Compare January 3, 2023 08:15
@mohammadfawaz mohammadfawaz requested a review from a team January 3, 2023 10:22
@emilyaherbert
Copy link
Contributor

Upon further investigation, #3633 isn't a bug after all, but instead a matter of showing the user a bad error message. Apologies for adding this additional context after have submitted this PR 😭

I'm going to close this PR and remove your assignment from #3633 for bookkeeping, but please feel free to re-add your assignment to #3633 if you'd like to continue working on that GH issue.

@emilyaherbert
Copy link
Contributor

Hi @esdrubal, apologies for the confusion, I misread the example in #3633---this PR was closed in error. I'll reopen this PR and add you back to that issue to correct those changes.

@emilyaherbert emilyaherbert reopened this Jan 5, 2023
Copy link
Contributor

@emilyaherbert emilyaherbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks very close! Just a few comments left to address.

sway-core/src/semantic_analysis/namespace/trait_map.rs Outdated Show resolved Hide resolved
sway-core/src/semantic_analysis/namespace/trait_map.rs Outdated Show resolved Hide resolved
sway-core/src/semantic_analysis/namespace/namespace.rs Outdated Show resolved Hide resolved
@emilyaherbert emilyaherbert requested a review from a team January 13, 2023 17:27
When two generic trait implementation use the same method
implementation an error such as `Duplicate definition for method
"convert" for type "A"` would occur.

This fix does remove the error when a method with diferent type
parameters is added to another trait implementation with different type
parameters.
The fix also ensures that find_method_for_type returns a method whose
 parameters types matches the given arguments expressions return types.

Closes FuelLabs#3633.
nfurfaro
nfurfaro previously approved these changes Jan 26, 2023
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved based on the testing added to demonstrate the effectiveness of the fix, but needs a compiler team member for final review.

@emilyaherbert emilyaherbert requested a review from a team January 26, 2023 18:30
@emilyaherbert
Copy link
Contributor

(I wrote a review comment but it's showing up under the fold of a previous resolved comment. CC @esdrubal)

Copy link
Contributor

@emilyaherbert emilyaherbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good @esdrubal 🎉 I appreciate your persistence with this PR through the re-review process.

@emilyaherbert emilyaherbert requested review from nfurfaro and a team February 2, 2023 23:40
@mohammadfawaz mohammadfawaz enabled auto-merge (squash) February 3, 2023 11:11
@mohammadfawaz mohammadfawaz merged commit 5e67357 into FuelLabs:master Feb 3, 2023
eightfilms pushed a commit that referenced this pull request Feb 7, 2023
)

When two generic trait implementation use the same method name with a
different type parameter an error such as `Duplicate definition for
method "convert" for type "A"` would occur.

This fix does remove the error when a method with diferent type
parameters is added to another trait implementation with different type
parameters.
The fix also ensures that when possible find_method_for_type returns a
method whose parameters types matches the given arguments expressions
return types.

Closes #3633.

---------

Co-authored-by: Mohammad Fawaz <mohammadfawaz89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler General compiler. Should eventually become more specific as the issue is triaged P: critical Should be looked at before anything else
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug with duplicate impl blocks defined for the same type with different type parameters.
4 participants