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

CSE: spreading of arrays is not supported, propose new spread instruction #1650

Open
CZX123 opened this issue Apr 8, 2024 · 1 comment
Open
Labels
Bug Something isn't working Enhancement New feature or request Proposal Proposing a feature, please discuss

Comments

@CZX123
Copy link
Contributor

CZX123 commented Apr 8, 2024

The below code runs correctly in the REPL tab, but fails in the CSE machine tab:

const f = (...x) => x;
f(...[1, 2, 3]);

This is because the SpreadElement node is not handled inside the CSE machine interpreter.

Proposal

A SpreadElement node, when evaluated, should result in two new items inside the control:

  1. A spread instruction
  2. The expression on the right of the ellipsis.

Once the expression on the right is fully evaluated, there should be a runtime check that the result is indeed an array, and then the spread instruction should unpack the array in the stash, by removing it and adding the individual elements back into the stash.

This new spread instruction could also potentially allow spreading of array elements inside arrays, e.g. [...arr], instead of just being limited to the arguments of function calls.

Update

It appears that the call 1 instruction above the SpreadElement node would also be incorrect, and it should be call 3 instead. Unfortunately, the correct number of arguments can only be known after the array expression is evaluated, which is after the call instruction has already been pushed.

We also have to take note of more complex situations like below, where there can be multiple spread elements inside a function call:

const f = (...x) => x;
f(0, ...[1, 2], ...[3, 4]);

The simplest solution would be to increase the value in the call instruction by the array length - 1 every time a spread instruction is run, and keep the limitation where array spread can only be used inside function calls, so it is guaranteed that there will be a call instruction somewhere above the spread instruction.

@CZX123 CZX123 added Bug Something isn't working Enhancement New feature or request Proposal Proposing a feature, please discuss good first issue Easy issues to get your feet wet and removed good first issue Easy issues to get your feet wet labels Apr 8, 2024
@martin-henz
Copy link
Member

Yes, this looks like the correct specification for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement New feature or request Proposal Proposing a feature, please discuss
Projects
None yet
Development

No branches or pull requests

2 participants