Skip to content

Fix const enum compilation failure #1120

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

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

DuncanUszkay1
Copy link
Contributor

Issue: #574

If you tried to use a const enum with multiple members, you would receive a compilation failure.

This was because the compiler relied upon each enum value to be registered as a global value in order to determine what the next enum value was. As a result, when we inlined the enum values and did not register that global value, compilation would fail.

To fix this, I've changed this method around a bit to rely upon the last expression value used (held in initExpr) rather than trying to load from the global store.

I'm not sure why it was implemented the way it was originally- it's possible that there is some tradeoff involved here I'm unaware of which causes reading from the global store to be faster/more efficient than simply reusing the last expression statement used in the loop. This certainly seems to work from a functional perspective 🤷‍♂

@dcodeIO
Copy link
Member

dcodeIO commented Feb 20, 2020

I believe the underlying idea was that if one has an enum like

enum A {
  A
}

that the value of A would be 0 since it's the first value (no initializer, no previousValue) and if one has an enum like

enum A {
  B = 2;
  C;
}

that the value of C would become B + 1.

Regarding the PR specifically, it is not possible to reuse the same expression as a member of two different expressions, so if B is initialized with initExpr, that same initExpr cannot be used again to initialize C, but one must global.get the value of A.B (use a new expression).

Looks like there should instead be a case where if the enum is const, it would make a new expression of the inlined value, and if it is mutable, use a global.get as it did.

@DuncanUszkay1
Copy link
Contributor Author

DuncanUszkay1 commented Feb 20, 2020

You say it isn't possible, but the code seems to work 🤔 here's the example that wasn't previously working:

// foo.ts
export const enum Foo {
  Boo1, Boo2, Boo3, Boo4
}

// main.ts
import { Foo } from './foo';

export function test(): i32 {
  return Foo.Boo1 + Foo.Boo2 + Foo.Boo3 + Foo.Boo4);
}

With this change it compiles and outputs: 6. Seems like tests pass as well

What's the scenario where this fails? Not sure I understand

@dcodeIO
Copy link
Member

dcodeIO commented Feb 20, 2020

Not reusing the same expression multiple times is a Binaryen convention. From their README:

Notes when working with Binaryen IR:

As mentioned above, Binaryen IR has a tree structure. As a result, each expression should have exactly one parent - you should not "reuse" a node by having it appear more than once in the tree. The motivation for this limitation is that when we optimize we modify nodes, so if they appear more than once in the tree, a change in one place can appear in another incorrectly.

@DuncanUszkay1
Copy link
Contributor Author

Ah I see. Back to the drawing board, I'll look into what you suggested above. Thanks for the review

@DuncanUszkay1 DuncanUszkay1 changed the title Fix const enum compilation failure [WIP]Fix const enum compilation failure Feb 20, 2020
@DuncanUszkay1 DuncanUszkay1 force-pushed the const-enums-fix branch 2 times, most recently from 48803d1 to 46e30b1 Compare February 24, 2020 21:04
@DuncanUszkay1 DuncanUszkay1 changed the title [WIP]Fix const enum compilation failure Fix const enum compilation failure Feb 25, 2020
@DuncanUszkay1
Copy link
Contributor Author

This should be ready for review again

@DuncanUszkay1 DuncanUszkay1 force-pushed the const-enums-fix branch 2 times, most recently from 479b944 to cd36d28 Compare February 25, 2020 23:33
@dcodeIO dcodeIO merged commit df1f6f9 into AssemblyScript:master Feb 26, 2020
@dcodeIO
Copy link
Member

dcodeIO commented Feb 26, 2020

Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants