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

Implement Array Destructuring [WIP] #1788

Closed
wants to merge 50 commits into from

Conversation

bnbarak
Copy link
Contributor

@bnbarak bnbarak commented Apr 6, 2021

Per #1473, Im stated to implement destructuring assignment.
At this step, I will implement the array destructuring.

Please feel free to comment on the progress.

  • I've read the contributing guidelines

@@ -0,0 +1,5 @@
const arr = [1, 2];
const a = arr[0], b = arr[1];
const x = [3, 4][0], y = [3, 4][1];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love to hear some suggestion how to implement a "dummy" variable to avoid the double invocation

Copy link
Member

Choose a reason for hiding this comment

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

We typically compile the Statements directly, i.e. not trying to "transpile" on the parser level, so we not only get a 1:1 roundtrip when serializing the AST again, but also do not have to deal with temporary AST nodes like the one that would be necessary here. Here, the compiler would typically implement a compileDestructuringAssignment method directly compiling to Binaryen IR, which may then use a temporary Wasm local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. From what I hear I need to set the NodeKind to DESTRUCTED_VARIABLE. And then handle it in compileStatement() switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or add a flag to the statement and keep the type to VARIABLE

@bnbarak
Copy link
Contributor Author

bnbarak commented Apr 12, 2021

@dcodeIO May I ask for your help with passing a test here. The same wat file array-destructuring.untouched.wat is passing on my local machine but fail the diff in the CI. I run Node version 14.16.1 on a macOS 10.13.6.

@bnbarak bnbarak marked this pull request as draft April 12, 2021 20:56
@bnbarak bnbarak marked this pull request as ready for review April 13, 2021 03:26
src/parser.ts Outdated
Comment on lines 930 to 931
var localCallableVarDeclaration: VariableDeclaration | undefined;
var localCallableIdentifier: IdentifierExpression | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

AssemblyScript doesn't support undefined. Only null. And due to compiler support self-bootstrapping you should never use undefined even in compiler's codebase.

@bnbarak
Copy link
Contributor Author

bnbarak commented Apr 19, 2021

@MaxGraey @dcodeIO took me a while to get the tests to pass (needed to rebase - I guess the tests runs agains master merge not my current branch dah...).

I know you asked to use a local wasm variable to cache the callable expresionRef. Which I did, but during my experiments I implemented a local JS variable const _func = func(), a = _func[0]similar to how TS transpiles to ES5.

I can revert back to use a local wasm variable:

// part of "compileElementAccessExpression()" method in compilers.ts
let thisArg = this.compileExpressionMaybeCached(targetExpression, thisType, destructingKey); // Try to hit a cache result

Where compileExpressionMaybeCached uses scopedDummyLocal.

The only issues is that I got an error from the optimization test because "the Binaryen IR tree is not flat".

Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Nov 26, 2023
Copy link

github-actions bot commented Dec 4, 2023

This PR has been automatically closed due to lack of recent activity, but feel free to reopen it as long as you merge in the main branch afterwards.

@github-actions github-actions bot closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants