Add spread rewriting

Add spread rewriting

In short, array literals containing spreads, when used as expressions,
are rewritten using do expressions.  E.g.

    [1, 2, 3, ...x, 4, ...y, 5]

is roughly rewritten as:

    do {
      $R = [1, 2, 3];
      for ($i of x) %AppendElement($R, $i);
      %AppendElement($R, 4);
      for ($j of y) %AppendElement($R, $j);
      %AppendElement($R, 5);
      $R
    }

where $R, $i and $j are fresh temporary variables.

R=rossberg@chromium.org
BUG=

Committed: https://crrev.com/07f1c36273e979d515bee81e9c847ca7dcf27355
Cr-Commit-Position: refs/heads/master@{#33307}

[nickie, 2016-01-07 16:58:11]

https://codereview.chromium.org/1564083002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1564083002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:6517: class SpreadRewriter : public AstExpressionRewriter
{
For the time being, this just rewrites spreads in array literals.  If however
later we find more uses of such rewriting, the class name will not be
appropriate.  It would be nice if there was a way to compose rewriters, instead
of adding more stuff to a monolithic rewriter, in this place.

[nickie, 2016-01-11 10:57:33]

Patchset #2 (id:20001) has been deleted

[rossberg, 2016-01-11 12:50:14]

lgtm

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5442: DCHECK_NOT_NULL(node);
Nit: having 2 null checks here may be a bit over-defensive. Doesn't the latter
one suffice?

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5498: if (spread == nullptr) {
Nit: add comments to both branches of this conditional that give the overall
rewriting.

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5574: lit->RewindSpreads();
Why is this needed?

[caitp (gmail), 2016-01-11 13:59:28]

Does this mean it's safe to delete some code from src/js/spread.js? I think
there's at least one which can be removed here

[nickie, 2016-01-12 12:11:16]

On 2016/01/11 13:59:28, caitp wrote:
> Does this mean it's safe to delete some code from src/js/spread.js? I think
> there's at least one which can be removed here

Both spread_arguments and spread_iterable are used in relation to spread
arguments in calls.  Maybe the name of this patch was a bit confusing; it is
only about rewriting spreads in array literals, not in arguments.  So, right
now, this code in src/js/spread.js cannot be removed.

As I understand, however, the same idea could be applied to spread arguments in
calls.  This would require a rewrite of Parser::PrepareSpreadArguments and
Parser::SpreadCall.  The internal array of arguments would be formed with (more
or less) the same method that is used to desugar array literals containing
spreads.

I intend to try this after cleaning up other (hopefully simpler) dead code
related to spreads.

[nickie, 2016-01-13 10:15:21]

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5442: DCHECK_NOT_NULL(node);
On 2016/01/11 12:50:14, rossberg wrote:
> Nit: having 2 null checks here may be a bit over-defensive. Doesn't the latter
> one suffice?

Acknowledged.  I will remove the first.  It was probably left from when I was
debugging.

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5498: if (spread == nullptr) {
On 2016/01/11 12:50:14, rossberg wrote:
> Nit: add comments to both branches of this conditional that give the overall
> rewriting.

Acknowledged.

https://codereview.chromium.org/1564083002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5574: lit->RewindSpreads();
On 2016/01/11 12:50:14, rossberg wrote:
> Why is this needed?

This is very important.  Line 5487 initializes the result of the do expression
(the temporary variable containing the array that we build).  The initial value
assigned there is the array literal (lit), spreads and all.  But it must not be
the whole array literal; it must be just the values up to (and excluding)
first_spread_index_, as the rest will be added with the calls to %AppendElement.
 So, I'm "rewinding" the array literal, truncating everything from
first_spread_index_ until the end.  This is done by ArrayLiteral::RewindSpreads,
defined in ast.h:1693.

I'll add some comments to the code, to explain all this.

[nickie, 2016-01-14 16:35:03]

https://codereview.chromium.org/1564083002/diff/80001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1564083002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5454: return result;
This is almost trivial; rewriting happens after validation (see 1567603005).

https://codereview.chromium.org/1564083002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5469: }
This is the only non-trivial addition to this rebase.
It was caused by 1567603005.
Validation occurs just once and then each pattern is rewritten.

[nickie, 2016-01-14 17:10:26]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-01-14 17:10:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1564083002/#ps80001
(title: "Rebase (just see comments)")

[commit-bot: I haz the power, 2016-01-14 17:10:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1564083002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1564083002/80001

[commit-bot: I haz the power, 2016-01-14 17:49:58]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-14 17:50:54]

Description was changed from

==========
Add spread rewriting

In short, array literals containing spreads, when used as expressions,
are rewritten using do expressions.  E.g.

    [1, 2, 3, ...x, 4, ...y, 5]

is roughly rewritten as:

    do {
      $R = [1, 2, 3];
      for ($i of x) %AppendElement($R, $i);
      %AppendElement($R, 4);
      for ($j of y) %AppendElement($R, $j);
      %AppendElement($R, 5);
      $R
    }

where $R, $i and $j are fresh temporary variables.

R=rossberg@chromium.org
BUG=
==========

to

==========
Add spread rewriting

In short, array literals containing spreads, when used as expressions,
are rewritten using do expressions.  E.g.

    [1, 2, 3, ...x, 4, ...y, 5]

is roughly rewritten as:

    do {
      $R = [1, 2, 3];
      for ($i of x) %AppendElement($R, $i);
      %AppendElement($R, 4);
      for ($j of y) %AppendElement($R, $j);
      %AppendElement($R, 5);
      $R
    }

where $R, $i and $j are fresh temporary variables.

R=rossberg@chromium.org
BUG=

Committed: https://crrev.com/07f1c36273e979d515bee81e9c847ca7dcf27355
Cr-Commit-Position: refs/heads/master@{#33307}
==========

[commit-bot: I haz the power, 2016-01-14 17:50:55]

Patchset 4 (id:??) landed as
https://crrev.com/07f1c36273e979d515bee81e9c847ca7dcf27355
Cr-Commit-Position: refs/heads/master@{#33307}