[es6] Spread in array literals

[es6] Spread in array literals

This allows you to put iterables into your array literals
and the will get spread into the array.

  let x = [0, ...range(1, 3)];  // [0, 1, 2]

This is done by treating the array literal up to the first
spread element as usual, including using a boiler plate
array, and then appending the remaining expressions and rest
expressions.

BUG=v8:3018
LOG=N

Committed: https://crrev.com/9502e91adbed48a546463aa848656d07619d14e6
Cr-Commit-Position: refs/heads/master@{#28534}

[arv (Not doing code reviews), 2015-05-19 23:30:57]

Use SetElementsLength

[arv (Not doing code reviews), 2015-05-20 01:00:50]

arv@chromium.org changed reviewers:
+ adamk@chromium.org, dslomov@chromium.org, mstarzinger@chromium.org

[arv (Not doing code reviews), 2015-05-20 01:00:51]

Dmitry. I wanted to unblock you from spread patterns but this still needs a
little bit more work.

The approach is similar to computed property names in object literals. We let
the array elements up to the first spread element be created like before and
after that we append elements to the array.

One alternative design would be to desugar it in the parser but the problem with
that is that we would have to go back and change the AST when we finally know if
the array literal is going to end up as an array literal and not as an array
pattern.

It is not clear to me why turbofan is trying to optimize this even when I call 
DisableOptimization in ast-numbering.cc.

TODO:

- Classifier
- More tests
- Ports
- TF (in a follow up CL)
- Crankshaft?

[Dmitry Lomov (no reviews), 2015-05-20 07:06:46]

Here is a design I came up with yesterday evening (nothing done yet): for
   [a, b, c, ...d] 
parser builds an ast:
   %SpreadArrayLiteral([a,b,c], d)
where builtin:
   function SpreadArrayLiteral(lit, rest) {
       let index = lit.length;
       let done = false;
       while (!done) {
           let result = IteratorNext(rest);
           done = result.done;
           if (!done) lit[index+] = result.value;
       }
   }
Codegen will generate the code as-is, whereas PatternRewriter will reinterpret
this as a pattern.

[Michael Starzinger, 2015-05-20 07:59:17]

On 2015/05/20 07:06:46, Dmitry Lomov (chromium) wrote:
> Here is a design I came up with yesterday evening (nothing done yet): for
>    [a, b, c, ...d] 
> parser builds an ast:
>    %SpreadArrayLiteral([a,b,c], d)

This will have similar issues with evaluation order as the spread calls.
Concretely, the following:
   [a, ...b, c, ...d]
Will have evaluation order:
   "a, b, c, d, b.next(), d.next()",
Instead of:
   "a, b, b.next(), c, d, d.next()"

I didn't check with the spec draft, so I don't actually know which of these
evaluation orders is the expected one. But I have the feeling the later is the
intended order. Please correct me if I am wrong.

> where builtin:
>    function SpreadArrayLiteral(lit, rest) {
>        let index = lit.length;
>        let done = false;
>        while (!done) {
>            let result = IteratorNext(rest);
>            done = result.done;
>            if (!done) lit[index+] = result.value;
>        }
>    }
> Codegen will generate the code as-is, whereas PatternRewriter will reinterpret
> this as a pattern.

[Dmitry Lomov (no reviews), 2015-05-20 08:15:14]

On 2015/05/20 07:59:17, Michael Starzinger wrote:
> On 2015/05/20 07:06:46, Dmitry Lomov (chromium) wrote:
> > Here is a design I came up with yesterday evening (nothing done yet): for
> >    [a, b, c, ...d] 
> > parser builds an ast:
> >    %SpreadArrayLiteral([a,b,c], d)
> 
> This will have similar issues with evaluation order as the spread calls.
> Concretely, the following:
>    [a, ...b, c, ...d]
> Will have evaluation order:
>    "a, b, c, d, b.next(), d.next()",
> Instead of:
>    "a, b, b.next(), c, d, d.next()"
> 
> I didn't check with the spec draft, so I don't actually know which of these
> evaluation orders is the expected one. But I have the feeling the later is the
> intended order. Please correct me if I am wrong.

Ah I see, you are right, I forgot that in expressions '...d' can got in the
middle as well.
In patterns, the only allowed form is [a, b, c, ...d] that does not have this
issue.
For now, I'll probably just parse ...d in array literals in patterns only.

> 
> > where builtin:
> >    function SpreadArrayLiteral(lit, rest) {
> >        let index = lit.length;
> >        let done = false;
> >        while (!done) {
> >            let result = IteratorNext(rest);
> >            done = result.done;
> >            if (!done) lit[index+] = result.value;
> >        }
> >    }
> > Codegen will generate the code as-is, whereas PatternRewriter will
reinterpret
> > this as a pattern.

[Dmitry Lomov (no reviews), 2015-05-20 08:37:32]

(so re CL I am not blocked on this for destructuring in any way - take your
time)

[Dmitry Lomov (no reviews), 2015-05-20 11:57:41]

CL looks good to me (not sure about why TF still tries to optimize though)

(I stole some bits for https://codereview.chromium.org/1151503002/ :))

[Michael Starzinger, 2015-05-20 13:52:49]

On 2015/05/20 01:00:51, arv wrote:
> Dmitry. I wanted to unblock you from spread patterns but this still needs a
> little bit more work.
> 
> The approach is similar to computed property names in object literals. We let
> the array elements up to the first spread element be created like before and
> after that we append elements to the array.
> 
> One alternative design would be to desugar it in the parser but the problem
with
> that is that we would have to go back and change the AST when we finally know
if
> the array literal is going to end up as an array literal and not as an array
> pattern.
> 
> It is not clear to me why turbofan is trying to optimize this even when I call

> DisableOptimization in ast-numbering.cc.

This is working as intended. The AstNumberingVisitor can only disable
optimization with Crankshaft. TurboFan is supposed to support the full language
and hence disabling optimization with TurboFan entirely is not supported,
--always-opt will always trigger TurboFan on purpose. I propose to port to
TurboFan right away. If there is a strong reason to do it separately then one
can cause a stack-overflow in the AstGraphBuilder and hence perform an emergency
bailout while building the graph.

> 
> TODO:
> 
> - Classifier
> - More tests
> - Ports
> - TF (in a follow up CL)
> - Crankshaft?

[arv (Not doing code reviews), 2015-05-20 14:54:49]

More test... classifier... rename flag

[arv (Not doing code reviews), 2015-05-20 16:57:41]

TF mostly works

[arv (Not doing code reviews), 2015-05-20 17:53:27]

Fix TF and deopts

[arv (Not doing code reviews), 2015-05-20 18:46:27]

Full codegen all ports

[arv (Not doing code reviews), 2015-05-20 19:07:58]

PTAL

This is now ready.

[Dmitry Lomov (no reviews), 2015-05-20 20:36:33]

Neat! lgtm % TF (looks good to me too but I am no expert)

https://codereview.chromium.org/1125183008/diff/100001/src/ia32/full-codegen-...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1125183008/diff/100001/src/ia32/full-codegen-...
src/ia32/full-codegen-ia32.cc:1863: __ add(esp, Immediate(kPointerSize));  //
literal index
__ Drop(1) should work too AFAIR

https://codereview.chromium.org/1125183008/diff/100001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1125183008/diff/100001/src/runtime.js#newcode950
src/runtime.js:950: function ConcatIterableToArray(target, iterable) {
Duplicate function

[arv (Not doing code reviews), 2015-05-20 20:56:48]

cleanup

[arv (Not doing code reviews), 2015-05-20 20:57:44]

I'll wait for Michael to take a final look. Michael, if everything looks good
feel free to CQ.

https://codereview.chromium.org/1125183008/diff/100001/src/ia32/full-codegen-...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1125183008/diff/100001/src/ia32/full-codegen-...
src/ia32/full-codegen-ia32.cc:1863: __ add(esp, Immediate(kPointerSize));  //
literal index
On 2015/05/20 20:36:33, Dmitry Lomov (chromium) wrote:
> __ Drop(1) should work too AFAIR

Done.

Also fixed the old usage and x64.

https://codereview.chromium.org/1125183008/diff/100001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1125183008/diff/100001/src/runtime.js#newcode950
src/runtime.js:950: function ConcatIterableToArray(target, iterable) {
On 2015/05/20 20:36:33, Dmitry Lomov (chromium) wrote:
> Duplicate function

Done.

Merge failure.

[Michael Starzinger, 2015-05-21 08:06:12]

LGTM on the TurboFan part. I'll CQ it.

[Michael Starzinger, 2015-05-21 08:07:12]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2015-05-21 08:07:12]

The patchset sent to the CQ was uploaded after l-g-t-m from dslomov@chromium.org
Link to the patchset: https://codereview.chromium.org/1125183008/#ps120001
(title: "cleanup")

[commit-bot: I haz the power, 2015-05-21 08:07:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125183008/120001

[commit-bot: I haz the power, 2015-05-21 08:09:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-21 08:09:37]

Patchset 7 (id:??) landed as
https://crrev.com/9502e91adbed48a546463aa848656d07619d14e6
Cr-Commit-Position: refs/heads/master@{#28534}

[arv (Not doing code reviews), 2015-05-21 12:46:30]

On 2015/05/21 08:06:12, Michael Starzinger wrote:
> LGTM on the TurboFan part. I'll CQ it.

Thanks.

[arv (Not doing code reviews), 2015-05-29 14:05:30]

On 2015/05/20 01:00:51, arv wrote:

> - Crankshaft?

I looked into Crankshaft and decided not to do it for the following reasons:

- for-of is not supported in Crankshaft
- Calling builtins is not supported Crankshaft