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 regression caused by enum and struct callpaths. #4007

Merged

Conversation

esdrubal
Copy link
Contributor

@esdrubal esdrubal commented Feb 7, 2023

Description

The issue is that struct or enum can be imported with different callpaths depending the module they are imported from.

For instance we could have a struct with callpath
my_script::data_structures::SomeStruct in a module and in another module as my_script::eq_impls::data_structures::SomeStruct. This makes callpaths unreliable to compare declarations.

Closes #3998

Reopens #418

Disables tests multiple_enums_with_the_same_name and multiple_structs_with_the_same_name because they relied in callpath comparisons to work.

Checklist

  • I have linked to any relevant issues.
  • 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).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

The issue is that the module in which a struct or enum is can be
imported with different callpaths.

For instance we could have a struct with callpath
`my_script::data_structures::SomeStruct` in a module and in another
module as `my_script::eq_impls::data_structures::SomeStruct`. This makes
callpaths unreliable to compare declarations.

Closes #3998

Reopens #418

Disables tests multiple_enums_with_the_same_name and
multiple_structs_with_the_same_name.
@esdrubal esdrubal added bug Something isn't working P: critical Should be looked at before anything else compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Feb 7, 2023
@esdrubal esdrubal self-assigned this Feb 7, 2023
@mohammadfawaz
Copy link
Contributor

Thanks for the quick fix @esdrubal

Is the issue here that the call path used in type unification does not always refer to actual struct/enum declaration? I would have assumed that there is a unique path for the struct/enum declaration. Where is the other path coming from? Is it from the impl trait block?

@esdrubal esdrubal requested a review from a team February 7, 2023 14:31
@emilyaherbert
Copy link
Contributor

Hi @esdrubal, I think what you are saying is that even though two TypeInfo::Enums may point to the same type syntactically, relying on the call paths inside TypeInfo::Enum is unreliable as these can be relative call paths? Do I have this right?

If so, you might be interested in taking a look at #3744, among other things this PR changes TypeInfo::Enum and others to remove the call path field and instead use DeclIds directly. That would be a fix to this problem.

CC @mohammadfawaz

@mohammadfawaz
Copy link
Contributor

Hi @esdrubal, I think what you are saying is that even though two TypeInfo::Enums may point to the same type syntactically, relying on the call paths inside TypeInfo::Enum is unreliable as these can be relative call paths? Do I have this right?

If so, you might be interested in taking a look at #3744, among other things this PR changes TypeInfo::Enum and others to remove the call path field and instead use DeclIds directly. That would be a fix to this problem.

CC @mohammadfawaz

Oh excellent. In that case, I suggest we merge this PR to fix the regression until #3744 is complete.

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.

One of the temporary approaches we could take would be to add "unimplemented" compile errors for using types with a call path. i.e. the case defined here #1644. But if my hunch is right, then this unimplemented error will catch the case in #418, thereby disallowing any syntax like module0::Thing::new(), which is a breaking change and undesirable.

I can think of some temporary hacks to prevent #418, but I'm more inclined to block that bug behind #3744 instead. Approving this patch right now though.

@mohammadfawaz mohammadfawaz enabled auto-merge (squash) February 7, 2023 16:27
@esdrubal
Copy link
Contributor Author

esdrubal commented Feb 7, 2023

I think what you are saying is that even though two TypeInfo::Enums may point to the same type syntactically, relying on the call paths inside TypeInfo::Enum is unreliable as these can be relative call paths? Do I have this right?

Yes that's exactly the problem. If we have two modules A and B using an enum in module C, in module A we will have the enum declaration with call path my_script::A::C::MyEnum and in module B we will have the enum declaration with call path my_script::B::C::MyEnum. Then the unifier fails when using call paths.

@mohammadfawaz mohammadfawaz merged commit 7e861e4 into master Feb 7, 2023
@mohammadfawaz mohammadfawaz deleted the esdrubal/3998_regression_struct_enum_callpaths branch February 7, 2023 16:43
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: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen P: critical Should be looked at before anything else
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression after struct and enum changed to use call paths.
3 participants