Closed Bug 933276 (spread-assignment) Opened 11 years ago Closed 10 years ago

Implement [...x] in assignment target (spread operator)

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: jorendorff, Assigned: Swatinem)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(3 files, 6 obsolete files)

Implement [...x] in assignment target (spread operator)
69.17 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
part2: revert recursive case
27.44 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
part 3: make [call()] an invalid destructuring target
16.18 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review 
js> [h, ...t] = [1, 2, 3]
    typein:2:4 ReferenceError: invalid assignment left-hand side

This should assign 1 to h and a new array [2, 3] to t.

It should also work in all the other places where the spec says to use BindingPattern or AssignmentPattern, indicated by _ below:

    [_] = expr;
    {parts: _} expr;

    var/let/const _ = expr;
    var/let/const [_] = expr;
    var/let/const {x: _} = expr;
    for (_ in expr) stmt;
    for (var/let/const _ in expr) stmt;
    function tail(_) { return t; }
Blocks: es6
So this is actually my first *real* js engine bug.

Would you mind mentoring me through this? (or suggest someone else who might have the patience to do so?)

With this patch `Reflect.parse('var [a, b, ...c] = [1,2,3]');` now gives me the following for declarator.id (see below)

Actually running the code crashes on me however. So what would be the next steps to make this actually run?

{
  "type": "ArrayPattern",
  "elements": [
    {
      "type": "Identifier",
      "name": "a"
    },
    {
      "type": "Identifier",
      "name": "b"
    },
    {
      "type": "SpreadExpression",
      "expression": {
        "type": "Identifier",
        "name": "c"
      }
    }
  ]
}
Assignee: general → arpad.borsos
Status: NEW → ASSIGNED
Attachment #8424307 - Flags: review?(till)
Comment on attachment 8424307 [details] [diff] [review]
part1: add parser and reflect support

Review of attachment 8424307 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +3114,5 @@
>                      ok = checkDestructuring(data, pn, false);
>                  } else {
>                      if (data) {
> +                        if (pn->isKind(PNK_SPREAD)) {
> +                            ok = bindDestructuringVar(data, pn->pn_kid);

Ok, I need to check that kid is actually an identifier, otherwise things like `var [a, b, ...[c,d]] = arr` fail.
Attached patch arraypatternrest.patch (obsolete) — — Splinter Review 
Yay, now I actually got some code that runs, even though it treats the rest param as a normal arg (binding the next array elem instead of the rest)

next step: make it actually do the right thing. and get some tests going :-)
Attachment #8424307 - Attachment is obsolete: true
Attachment #8424307 - Flags: review?(till)
Other places this needs to work:
catch(BindingPattern)
comprehensions
Arpad, thanks a lot for working on this!

I'm not a good reviewer for this, though, as I'm not too familiar with the code it touches.
Jorendorff is probably the best person here, so once you have a patch in a state that you want to get feedback on, you should request that from him.
Depends on: 1015742
(In reply to Arpad Borsos (Swatinem) from comment #4)
> Other places this needs to work:
> catch(BindingPattern)
> comprehensions

comprehensions is bug 980828. Also blocking bug 905359 since I think those cases will both be solved at the same time.
Blocks: 980828, 905359
No longer depends on: 1015742
Depends on: 1015742
Attached patch arraypatternrest.patch (obsolete) — — Splinter Review 
Cool, works now like it should.

I have only tested it locally, I still need to run it through the test suite.

(this is on top of bug 1015742 btw)
Attachment #8428316 - Attachment is obsolete: true
Attachment #8428878 - Flags: review?(jorendorff)
FAILURES:
js/src/jit-test/tests/arguments/destructuring-exprbody.js
js/src/jit-test/tests/auto-regress/bug532363.js
js/src/jit-test/tests/auto-regress/bug596817.js
js/src/jit-test/tests/auto-regress/bug785776.js
js/src/jit-test/tests/baseline/bug843429.js
js/src/jit-test/tests/basic/bug685321-1.js
js/src/jit-test/tests/basic/bug685321-2.js
js/src/jit-test/tests/basic/bug778268.js
js/src/jit-test/tests/basic/expression-autopsy.js
js/src/jit-test/tests/basic/spread-array.js
js/src/jit-test/tests/basic/spread-call-eval.js
js/src/jit-test/tests/basic/spread-call-funapply.js
js/src/jit-test/tests/basic/spread-call-length.js
js/src/jit-test/tests/basic/spread-call-setcall.js
js/src/jit-test/tests/basic/spread-call.js
js/src/jit-test/tests/basic/testBug714650.js
js/src/jit-test/tests/basic/testDynamicLookup.js
js/src/jit-test/tests/basic/testDynamicUsage.js
js/src/jit-test/tests/basic/testLet.js
js/src/jit-test/tests/basic/testNonStubGetter.js
js/src/jit-test/tests/basic/testScriptCloning.js
js/src/jit-test/tests/closures/bug540348.js
js/src/jit-test/tests/ion/bug743099.js
js/src/jit-test/tests/ion/bug821794.js
js/src/jit-test/tests/parser/bug-889628.js

(including those from bug 1015742)
Will look at it tomorrow.
Attached patch arraypatternrest.patch (obsolete) — — Splinter Review 
So I fixed most of the tests manually, most of them were assigning non-iterables.
There is still some issue with `let` though. I will only have time on thursday to look at those though.

Some early feedback would be welcome however :-)

FAILURES:
    js/src/jit-test/tests/basic/testDynamicLookup.js
    js/src/jit-test/tests/basic/testDynamicUsage.js
    js/src/jit-test/tests/basic/testLet.js
    js/src/jit-test/tests/basic/testScriptCloning.js
Attachment #8428878 - Attachment is obsolete: true
Attachment #8428878 - Flags: review?(jorendorff)
Attachment #8429494 - Flags: review?(jorendorff)
Attached patch arraypatternrest.patch (obsolete) — — Splinter Review 
I fixed the `let` case.

I also tried to fix all the tests that wrongly assume that the destructuring array pattern works on non-iterables.
(I probably should have split up the patch; next time…)

This is slowly greening up: https://tbpl.mozilla.org/?tree=Try&rev=42d85d958ad2
(now that I think about it, I maybe should have selected mochitests as well, to catch the cases where code inside firefox makes the same wrong assumption about iterables)
Attachment #8429494 - Attachment is obsolete: true
Attachment #8429494 - Flags: review?(jorendorff)
Attachment #8430736 - Flags: review?(jorendorff)
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Yes it would be nice if this worked:

function bundleOfSticks(a, {b,c}, [d,e, ...f], ...g){;}
Comment on attachment 8430736 [details] [diff] [review]
arraypatternrest.patch

Review of attachment 8430736 [details] [diff] [review]:
-----------------------------------------------------------------

Here are my comments so far. I need to look at the emitter more. Sorry for the slow review.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3005,5 @@
> +        return false;
> +    if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)                    // ITER NEXT ITER UNDEFINED
> +        return false;
> +    if (EmitCall(cx, bce, JSOP_CALL, 1) < 0)                   // ITER RESULT
> +        return false;

I think ES6 requires us to call the .next() method with no arguments.

See <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-iteratordestructuringassignmentevaluation>.
Under the last grammatical production, step 5.a reads:
>     a.  Let next be IteratorStep(iterator).

which calls <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratorstep>
which says
> 1.  Let result be IteratorNext(iterator).

which calls <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratornext>
which says
> 1.  If value was not passed,
>     a.  Let result be Invoke(iterator, "next", ( )).
> 2.  Else,
>     a.  Let result be Invoke(iterator, "next", (value)).

Please add a test that checks for this. :-|

::: js/src/frontend/Parser.cpp
@@ +3109,1 @@
>  

While you're here, would you mind making these changes to the comment on checkDestructuring?

1. Change CheckDestructuring to checkDestructuring with a lowercase 'c'.
2. Replace "a valid destructuring expression"
   with "a valid AssignmentPattern", to match the term the ES6 draft uses.

Please also delete the unused "toplevel" argument and the last paragraph of the comment (which mentions it).

This can go in a separate changeset if you like.

@@ +3114,5 @@
>                      ok = checkDestructuring(data, pn, false);
>                  } else {
>                      if (data) {
> +                        if (pn->isKind(PNK_SPREAD) && pn->pn_kid->isKind(PNK_NAME)) {
> +                            ok = bindDestructuringVar(data, pn->pn_kid);

I'm OK with landing this as-is, but it seems like it might be easier to fix properly than to fix only the nonrecursive case!

It seems like adding

    if (pn->isKind(PNK_SPREAD))
        pn = pn->pn_kid;

in between these two lines:

    if (!pn->isKind(PNK_ELISION)) {
        if (pn->isKind(PNK_ARRAY) || pn->isKind(PNK_OBJECT)) {

would do the trick.

@@ +5509,5 @@
> +        if (flavor == KeyedDestructuringAssignment) {
> +            if (!pn->pn_kid->isKind(PNK_NAME)) {
> +                report(ParseError, false, null(), JSMSG_BAD_DESTRUCT_ASS);
> +                return false;
> +            }

Supporting the recursive case also requires deleting these four lines of code.

::: js/src/jit-test/tests/baseline/bug843429.js
@@ +4,1 @@
>      const x = [] = {};

Try this instead:
  const x = [] = [];
That way the test can run.

::: js/src/jit-test/tests/basic/destructuring-rest.js
@@ +1,1 @@
> +

Very nice tests!

Probably none of them loop long enough to turn on the JIT; can you make a test that loops at least 10x or so?

Please test that the following things happen in the correct order:
- the iterator is finished and returns {done: true}
- the new array is assigned to 'rest'
- values are assigned to any other variables mentioned in the same destructuring assignment

Please test that this code:
    [...x] = y;
does not invoke Array.prototype.@@iterator, but this code:
    [...[...x]] = y;
does! (At least, I think that is what ES6 requires.)

We already have tests where destructuring assignment fails because the value being assigned isn't iterable, right? Can we easily add a [...rest] test case to those existing tests?

Please add a test where we throw during destructuring assignment to a catch-variable.

@@ +15,5 @@
> +var patternDeep = '[, [, ...rest]]';
> +var patternObject = '{a: [, ...rest]}';
> +
> +// XXX: AssignmentRestElement takes a LeftHandSideExpression, so these could be
> +// arbitrarily nested. Once that is implemented, remove the assertions below.

Please file the follow-up bug and cite it here ("See bug 1234567.")

Please say "update the assertions below" rather than remove. :)

@@ +79,5 @@
> +function testArgument(pattern, input) {
> +  return new Function('input',
> +    'return (function (' + pattern + ') {' +
> +    'return rest; })(input);'
> +  )(input);

I think ES6 also requires
    Function(pattern, "return rest;")
to work, in case you'd like to test both...

::: js/src/jit-test/tests/basic/testLet.js
@@ +62,5 @@
> +//test('return let ([[], []] = x) x;');
> +//test('return let ([[[[]]], [], , [], [[]]] = x) x;');
> +//test('return let ({x: []} = x) x;');
> +//test('return let ({x: [], y: {x: []}} = x) "ponies";', {y:{}});
> +//test('return let ({x: []} = x, [{x: []}] = x) "ponies";');

Fix them or delete them. Your call.

The same goes for all the other changes in this file.

::: js/src/jit-test/tests/ion/bug743099.js
@@ +7,5 @@
>    return( weekday < 0 ? 7 + weekday : weekday );
>  }
>  var expect = 'No Error';
>  for (var i = 0; i < 50; i++) {
>      var [] = expect ? WeekDay( i.a ) : true, uneval;

Instead of wrapping in assertThrows, please change this line to say

var [] = [expect ? WeekDay(i.a) : true], uneval;

This allows the test to run.

::: js/src/jit-test/tests/ion/bug821794.js
@@ +1,3 @@
>  
>  gczeal(2);
> +// XXX: according to the name, what is this supposed to test?

This test was created by a fuzzer, a program that throws tons of random code at the JS engine in the hope of finding crashing bugs. Any identifiers in the code are therefore most likely meaningless.

Some fuzzers generate random code partly by mashing together existing tests; that's where the names usually come from.

There's no telling what these things are supposed to be testing, so it's best to keep the test as close to its original semantics as possible.

@@ +4,2 @@
>  function bitsinbyte() {
> +    var [ summary  ]  = [];

var summary = true[0];

::: js/src/jsreflect.cpp
@@ +2952,1 @@
>  

Changes in jsreflect.cpp should come with corresponding tests in js/src/tests/js1_8_5/extensions/reflect-parse.js.

@@ +2959,5 @@
>              elts.infallibleAppend(NullValue());
> +        } else if (next->isKind(PNK_SPREAD)) {
> +            RootedValue ident(cx);
> +            RootedValue spread(cx);
> +            if (!pattern(next->pn_kid, pkind, &ident) ||

It may not be an identifier, with the changes I suggested, so perhaps name the variable "target" rather than "ident".

@@ +2961,5 @@
> +            RootedValue ident(cx);
> +            RootedValue spread(cx);
> +            if (!pattern(next->pn_kid, pkind, &ident) ||
> +                !builder.spreadExpression(ident, &next->pn_pos, &spread))
> +                return false;

The way to punctuate an if statement like this is:

    if (!pattern(next->pn_kid, pkind, &ident) ||
        !builder.spreadExpression(ident, &next->pn_pos, &spread))
    {
        return false;
    }

Alternatively, make two separate if-statements:

    if (!pattern(...))
        return false;
    if (!builder.spreadExpression(...))
        return false;

::: js/src/tests/js1_5/extensions/regress-469625.js
@@ +21,5 @@
>    printStatus (summary);
>   
>    jit(true);
>  
> +  for (var j = 0; j < 3; ++j) [[, ]] = ['a'];

Retain the original weirdness:

    [].__proto__[0] = 'a';
    for (var j = 0; j < 3; ++j) [[, ]] = [,];

::: js/src/tests/js1_7/extensions/regress-351102-03.js
@@ +26,5 @@
>        try
>        {
>          d.d.d;
>        }
> +      catch(x if gc())

catch ({} if gc())

This case where a declaration doesn't introduce any bindings is actually a weird special case in the frontend IIRC.
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Here are my comments so far. I need to look at the emitter more. Sorry for
> the slow review.

Thanks for the review so far. Tell me if there is anything I can do from my end to speed things up.

> I think ES6 requires us to call the .next() method with no arguments.
> 
> Please add a test that checks for this. :-|

arai is taking care of that in bug 923028. I will have to completely rebase my patch on top of his anyway. (and yes, there was a lot of copy-pasting involved in both cases :-)

> I'm OK with landing this as-is, but it seems like it might be easier to fix
> properly than to fix only the nonrecursive case!

I will take another look at it.

> Please test that the following things happen in the correct order:
> - the iterator is finished and returns {done: true}
> - the new array is assigned to 'rest'
> - values are assigned to any other variables mentioned in the same
> destructuring assignment
> 
> Please test that this code:
>     [...x] = y;
> does not invoke Array.prototype.@@iterator, but this code:
>     [...[...x]] = y;
> does! (At least, I think that is what ES6 requires.)

You mean on the LHS? It sure does invoke @@iterator for `y`.

> We already have tests where destructuring assignment fails because the value
> being assigned isn't iterable, right? Can we easily add a [...rest] test
> case to those existing tests?

The other way around (all those tests I needed to fix). I will add some that really assert that it throws.

> Please add a test where we throw during destructuring assignment to a
> catch-variable.

You mean like `catch (err) { {err} = {err: 'foo'}; }` ?
Otherwise I don’t understand what you mean.

> > +// XXX: AssignmentRestElement takes a LeftHandSideExpression, so these could be
> > +// arbitrarily nested. Once that is implemented, remove the assertions below.
> 
> Please file the follow-up bug and cite it here ("See bug 1234567.")
> 
> Please say "update the assertions below" rather than remove. :)

Since you recommended I fix the recursive case right away, those will be fixed as well.

Also, I need to update the tests for my misinterpretation of the syntax (bug 1018453, haha)

> 
> @@ +79,5 @@
> > +function testArgument(pattern, input) {
> > +  return new Function('input',
> > +    'return (function (' + pattern + ') {' +
> > +    'return rest; })(input);'
> > +  )(input);
> 
> I think ES6 also requires
>     Function(pattern, "return rest;")
> to work, in case you'd like to test both...

Thanks, I will add those.

> ::: js/src/jit-test/tests/basic/testLet.js
> @@ +62,5 @@
> > +//test('return let ([[], []] = x) x;');
> > +//test('return let ([[[[]]], [], , [], [[]]] = x) x;');
> > +//test('return let ({x: []} = x) x;');
> > +//test('return let ({x: [], y: {x: []}} = x) "ponies";', {y:{}});
> > +//test('return let ({x: []} = x, [{x: []}] = x) "ponies";');
> 
> Fix them or delete them. Your call.
> 
> The same goes for all the other changes in this file.

I might just remove those, since they all assume that destructuring works on non-iterables.


And thanks for the other suggestions. I will wait for bug 923028 to settle a bit before I rebase my patch here.
Comment on attachment 8430736 [details] [diff] [review]
arraypatternrest.patch

Review of attachment 8430736 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag for now.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3272,1 @@
>              JS_ASSERT(pn->isKind(PNK_OBJECT));

You can remove this assertion now, since the code just checked that this is true.

@@ +3339,5 @@
> +            } else {
> +                if (!EmitIteratorNext(cx, bce))                            // ITER RESULT
> +                    return false;
> +                if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce)) // ITER VALUE
> +                    return false;

The standard says we Get result.done first, and only Get result.value if result.done is truthy.

This means if result.done is a getter, it is called; if result is a proxy, its "get" trap is called.

I guess this means we have to implement a little conditionial expression here, like
   (result.done ? undefined : result.value)

Please fix it and add a test!

@@ +3383,5 @@
> +    }
> +
> +    /*
> +     * Pop the iterator.
> +     * Depending on PushInitialValues, it may not be on the top of the stack however.

Hmm. I don't understand. The code in the PNK_ARRAY branch inside the loop seems to assume that the iterator is on top of the stack. But the code in the `if (emitOption == PushInitialValues)` block inside the loop only moves *one* value. In the PNK_ARRAY case, aren't there two values to move (the value being destructured, and the iterator)?

@@ +3491,5 @@
> +        // Walk the lhs looking for a rest pattern. Do not emit a GroupAssignment in that case.
> +        for (ParseNode *l = lhs->pn_head; l; l = l->pn_next) {
> +            if (l->isKind(PNK_SPREAD))
> +                return true;
> +        }

AddSpreadElement already sets the PNX_SPECIALLARRAYINIT bit to the array node, so this should not be necessary.
Attachment #8430736 - Flags: review?(jorendorff)
> @@ +3339,5 @@
> > +            } else {
> > +                if (!EmitIteratorNext(cx, bce))                            // ITER RESULT
> > +                    return false;
> > +                if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce)) // ITER VALUE
> > +                    return false;
> 
> The standard says we Get result.done first, and only Get result.value if
> result.done is truthy.
> 
> This means if result.done is a getter, it is called; if result is a proxy,
> its "get" trap is called.
> 
> I guess this means we have to implement a little conditionial expression
> here, like
>    (result.done ? undefined : result.value)
> 
> Please fix it and add a test!

Thanks

> @@ +3383,5 @@
> > +    }
> > +
> > +    /*
> > +     * Pop the iterator.
> > +     * Depending on PushInitialValues, it may not be on the top of the stack however.
> 
> Hmm. I don't understand. The code in the PNK_ARRAY branch inside the loop
> seems to assume that the iterator is on top of the stack. But the code in
> the `if (emitOption == PushInitialValues)` block inside the loop only moves
> *one* value. In the PNK_ARRAY case, aren't there two values to move (the
> value being destructured, and the iterator)?

I tried it with a second PICK here: http://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#3285
But for some reason it didn’t work. Or maybe I just did it wrong.

> @@ +3491,5 @@
> > +        // Walk the lhs looking for a rest pattern. Do not emit a GroupAssignment in that case.
> > +        for (ParseNode *l = lhs->pn_head; l; l = l->pn_next) {
> > +            if (l->isKind(PNK_SPREAD))
> > +                return true;
> > +        }
> 
> AddSpreadElement already sets the PNX_SPECIALLARRAYINIT bit to the array
> node, so this should not be necessary.

I think I added that because of `let`, but I’m not sure. Will look at it again.
(In reply to Arpad Borsos (Swatinem) from comment #15)
> I tried it with a second PICK here:
> http://dxr.mozilla.org/mozilla-central/source/js/src/frontend/
> BytecodeEmitter.cpp#3285
> But for some reason it didn’t work. Or maybe I just did it wrong.

Can you use dis() to dump the bytecode for a simple example, and attach it here? I can believe it works this way, but I'd like to see why. :)

> > AddSpreadElement already sets the PNX_SPECIALLARRAYINIT bit to the array
> > node, so this should not be necessary.
> 
> I think I added that because of `let`, but I’m not sure. Will look at it
> again.

I don't understand yet --- thanks for taking a look.
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> @@ +79,5 @@
> > +function testArgument(pattern, input) {
> > +  return new Function('input',
> > +    'return (function (' + pattern + ') {' +
> > +    'return rest; })(input);'
> > +  )(input);
> 
> I think ES6 also requires
>     Function(pattern, "return rest;")
> to work, in case you'd like to test both...

It should, I filed bug 1037939 about that.

I updated the patch to support recursive matching (yay), though I still have a few things to fix. Will have a patch ready at the end of the week, hopefully.
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Please test that the following things happen in the correct order:
> - the iterator is finished and returns {done: true}
> - the new array is assigned to 'rest'
> - values are assigned to any other variables mentioned in the same
> destructuring assignment
> 
> Please add a test where we throw during destructuring assignment to a
> catch-variable.
Attachment #8455293 - Flags: review?(jorendorff)
> Please test that the following things happen in the correct order:
> - the iterator is finished and returns {done: true}
> - the new array is assigned to 'rest'
> - values are assigned to any other variables mentioned in the same
> destructuring assignment
> 
> Please add a test where we throw during destructuring assignment to a
> catch-variable.

Meh, forgot to actually ask to further explain those things :-)

Running on try now: https://tbpl.mozilla.org/?tree=Try&rev=3e2ba650800b
(In reply to Arpad Borsos (Swatinem) from comment #19)
> Meh, forgot to actually ask to further explain those things :-)

Do you mean you want me to explain them better? Here's my attempt:

> > Please test that the following things happen in the correct order:
> > - the iterator is finished and returns {done: true}
> > - the new array is assigned to 'rest'
> > - values are assigned to any other variables mentioned in the same
> > destructuring assignment

These are all things that can happen during destructuring assignment. The spec tells us exactly how all these things are supposed to happen, in what sequence.

Devise test cases that observe this sequence (you may have to be creative, using global setters, proxies, custom @@iterator implementations, etc.) and check that they agree with the spec.

For example:

    var [...vals] = {"@@iterator": function*() { yield 1; assertEq(vals, undefined); } };

Here we're checking that vals is *not* populated until after the iterator is exhausted.

> > Please add a test where we throw during destructuring assignment to a
> > catch-variable.

Like this:

    assertThrowsValue(function () {
        try {
            throw {"@@iterator": function () { throw "SURPRISE"; }};
        } catch ([x, y]) {
            throw "this should not happen";
        }
    }, "SURPRISE");
Arpad, I just realized it's necessary to delete the "group assignment" optimization.

Try this and you'll see what I mean:

js> delete Array.prototype["@@iterator"];
js> (function () { var [x, y] = [1, 2]; })() // should throw TypeError because Array#@@iterator is gone
js> dis(function () { var [x, y] = [1, 2]; })


Fortunately that should be very easy to delete. Please do it in a separate patch though, preferably *under* (before) this patch.
Comment on attachment 8455293 [details] [diff] [review]
Implement [...x] in assignment target (spread operator)

Review of attachment 8455293 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Still just a few things to change.

I managed to crash this with: $JS -e '[...x, y] = []'
Hit MOZ_CRASH(EmitDestructuringLHS: bad name op) at /home/jorendorff/dev/gecko/js/src/frontend/BytecodeEmitter.cpp:3145

Fix it and add a test! :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3245,5 @@
>      JS_ASSERT(pn->isArity(PN_LIST));
>      JS_ASSERT(pn->isKind(PNK_ARRAY) || pn->isKind(PNK_OBJECT));
>  #endif
>  
> +    /* When destructuring an array, use an iterator to walk it, instead of index lookup. */

Use dis(function(obj) { var [...rest] = obj; }) to see the bytecode this generates.

There are a few things that I think could be improved, noted below.

@@ +3248,5 @@
>  
> +    /* When destructuring an array, use an iterator to walk it, instead of index lookup. */
> +    if (pn->isKind(PNK_ARRAY)) {
> +        if (Emit1(cx, bce, JSOP_DUP) < 0)                              // OBJ OBJ
> +            return false;

Remove this DUP and the corresponding POP at the end; there's no need to leave OBJ on the stack this whole time.

@@ +3322,5 @@
> +                    return false;
> +                if (Emit1(cx, bce, JSOP_SWAP) < 0)                         // ITER ARRAY INDEX ITER
> +                    return false;
> +                if (!EmitSpread(cx, bce))                                  // ITER ARRAY INDEX
> +                    return false;

This results in the following bytecode:

 00107:  newarray 0                     ;; obj iter array
 00111:  dupat 1                        ;; obj iter array iter
 00115:  zero                           ;; obj iter array iter index
 00116:  swap                           ;; obj iter array index iter
 00117:  pick 2                         ;; obj iter index iter array
 00119:  pick 2                         ;; obj iter iter array index

This is pretty silly-looking. The stack traffic is weird by itself, but a separate weirdness is that the stack location being dup'd in instruction 111 will never be used again.

Here's the set of changes I recommend:
*   Make a variable 'bool needToPopIterator = false;' in this function,
    set it to true after calling EmitIterator() above,
    but set it false if we call EmitSpread(), because that's going to pop ITER for us.
*   Emit the extra JSOP_POP to pop the iterator only if 'needToPopIterator' is true.
*   Delete the EmitDupAt since we definitely don't need it anymore.
*   Change EmitForOf so that it does not emit those JSOP_PICK opcodes when type is STMT_SPREAD. Instead, it should expect the stack to already look like ITER ARR INDEX.
*   Change both EmitSpread call sites (this is one; the other is in EmitArray) to set up the stack correctly before calling EmitSpread. I think this means deleting the JSOP_SWAP above.

Something like that ought to work! :)

@@ +3405,5 @@
> +                                                                           // ITER X Y OBJ
> +                        if (Emit2(cx, bce, JSOP_PICK, (jsbytecode)(pickDistance + 1)) < 0)
> +                            return false;
> +                                                                           // X Y OBJ ITER
> +                        if (Emit2(cx, bce, JSOP_PICK, (jsbytecode)(pickDistance + 1)) < 0)

For clarity, please put each of these comments on a line *after* the Emit2 that puts the stack into that state. (Each comment illustrates the stack *after* the PICK.) You could also add a comment before the first one, showing the stack before the first PICK, if you think it helps.

::: js/src/frontend/Parser.cpp
@@ +5601,5 @@
>  
> +      case PNK_SPREAD:
> +        if (flavor == KeyedDestructuringAssignment)
> +            return checkAndMarkAsAssignmentLhs(pn->pn_kid, flavor);
> +        return false;

I was confused reading this for a second (even though I've read it before!), so please add this just before the |return false;|:
    MOZ_ASSERT_UNREACHABLE("spread syntax outside an array should not get this far");
Attachment #8455293 - Flags: review?(jorendorff)
Depends on: 1040699
(In reply to Jason Orendorff [:jorendorff] from comment #22)
> Remove this DUP and the corresponding POP at the end; there's no need to
> leave OBJ on the stack this whole time.

Indeed, that should simplify quite a few things.

Thanks for all the suggestions.

I filed the "remove group assignment" as a separate bug since I think it might be quite big itself.
(In reply to Jason Orendorff [:jorendorff] from comment #22)
> Remove this DUP and the corresponding POP at the end; there's no need to
> leave OBJ on the stack this whole time.

You still need it in one case, I added a comment.

> ::: js/src/frontend/Parser.cpp
> @@ +5601,5 @@
> >  
> > +      case PNK_SPREAD:
> > +        if (flavor == KeyedDestructuringAssignment)
> > +            return checkAndMarkAsAssignmentLhs(pn->pn_kid, flavor);
> > +        return false;
> 
> I was confused reading this for a second (even though I've read it before!),
> so please add this just before the |return false;|:
>     MOZ_ASSERT_UNREACHABLE("spread syntax outside an array should not get
> this far");

I solved it differently.
As much as the deprecated let is a pain in the rear and I will definitely cheer when it will be removed, it still brings forward some mistakes in my code; also for my destructuring-defaults patch, haha.
Attachment #8459145 - Flags: review?(jorendorff)
Attachment #8430736 - Attachment is obsolete: true
Attachment #8455293 - Attachment is obsolete: true
Depends on: 1041341
(In reply to Arpad Borsos (Swatinem) from comment #24)
> (In reply to Jason Orendorff [:jorendorff] from comment #22)
> > ::: js/src/frontend/Parser.cpp
> > > +      case PNK_SPREAD:
> > > +        if (flavor == KeyedDestructuringAssignment)
> > > +            return checkAndMarkAsAssignmentLhs(pn->pn_kid, flavor);
> > > +        return false;
> > 
> > I was confused reading this for a second (even though I've read it before!),
> > so please add this just before the |return false;|:
> >     MOZ_ASSERT_UNREACHABLE("spread syntax outside an array should not get
> > this far");
> 
> I solved it differently.

Heh! The way you solved it is much better than what I recommended. Thanks.
(In reply to Arpad Borsos (Swatinem) from comment #24)
> (In reply to Jason Orendorff [:jorendorff] from comment #22)
> > Remove this DUP and the corresponding POP at the end; there's no need to
> > leave OBJ on the stack this whole time.
> 
> You still need it in one case, I added a comment.

I see. I think the comment is too specific, though. This case can also happen in ordinary assignment, right?

    a = [...rest] = [1, 2];

Or is that something different?
Comment on attachment 8459145 [details] [diff] [review]
Implement [...x] in assignment target (spread operator)

Review of attachment 8459145 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r=me, but: I think I missed something in the spec, and this cannot land until that is addressed.

12.14.5.1 <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-assignment-static-semantics-early-errors> says:
> AssignmentRestElement : ... DestructuringAssignmentTarget
>
> *   It is a Syntax Error if IsValidSimpleAssignmentTarget of DestructuringAssignmentTarget is false.

This means that an assignment like |[...[]] = x| should be a SyntaxError, right?

The same goes for declarations. 13.2.3 <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-binding-patterns>:

> BindingRestElement[Yield, GeneratorParameter] :
>     [+GeneratorParameter] ... BindingIdentifier[Yield]
>     [~GeneratorParameter] ... BindingIdentifier[?Yield]

Assuming I'm correct that your current patch permits some assignments/bindings that should be errors, please write a new "part 2" patch, on top of this patch, to fix it. That'll be much easier to review. You can fold the two patches into a single changeset when you land, or not, your choice.

Thanks again for these patches -- this is good stuff, I'm looking forward to seeing it land.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3036,2 @@
>              if (!emitter(cx, bce, prologOp, element))
>                  return false;

While you're in here: if the function pointer stuff strikes you as silly, feel free to replace it with some plain old if statements or a switch.

::: js/src/frontend/Parser.cpp
@@ +3131,5 @@
>   *   simple names; the destructuring defines them as new variables.
>   *
>   * In both cases, other code parses the pattern as an arbitrary
> + * primaryExpr, and then, here in checkDestructuring, verify that the
> + * tree is a valid AssignmentPattern.

AssignmentPattern or BindingPattern.

It would be good to change the comment to switch from "declaration-like" to the standard terminology, which is "binding".

@@ +3174,5 @@
> +                    if (pn->pn_next) {
> +                        report(ParseError, false, pn->pn_next, JSMSG_PARAMETER_AFTER_REST);
> +                        return false;
> +                    }
> +                    pn = pn->pn_kid;

You fixed this in bug 932080, but could you fix it in this changeset instead? (Separate "target" variable to avoid messing up the iteration.)
Attachment #8459145 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #26)
> (In reply to Arpad Borsos (Swatinem) from comment #24)
> > (In reply to Jason Orendorff [:jorendorff] from comment #22)
> > > Remove this DUP and the corresponding POP at the end; there's no need to
> > > leave OBJ on the stack this whole time.
> > 
> > You still need it in one case, I added a comment.
> 
> I see. I think the comment is too specific, though. This case can also
> happen in ordinary assignment, right?
> 
>     a = [...rest] = [1, 2];
> 
> Or is that something different?

let is the only case where we use PushInitialValues. Thats why I added those tests :-)
Anything else uses InitializeVars.
(In reply to Jason Orendorff [:jorendorff] from comment #27)
> OK, r=me, but: I think I missed something in the spec, and this cannot land
> until that is addressed.
> 
> 12.14.5.1
> <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-
> assignment-static-semantics-early-errors> says:
> > AssignmentRestElement : ... DestructuringAssignmentTarget
> >
> > *   It is a Syntax Error if IsValidSimpleAssignmentTarget of DestructuringAssignmentTarget is false.
> 
> This means that an assignment like |[...[]] = x| should be a SyntaxError,
> right?
> 
> The same goes for declarations. 13.2.3
> <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-
> binding-patterns>:
> 
> > BindingRestElement[Yield, GeneratorParameter] :
> >     [+GeneratorParameter] ... BindingIdentifier[Yield]
> >     [~GeneratorParameter] ... BindingIdentifier[?Yield]
> 
> Assuming I'm correct that your current patch permits some
> assignments/bindings that should be errors, please write a new "part 2"
> patch, on top of this patch, to fix it. That'll be much easier to review.
> You can fold the two patches into a single changeset when you land, or not,
> your choice.

IsValidSimpleAssignmentTarget for PrimaryExpression :
* ArrayInitializer
* ObjectLiteral
is false

(And also for "eval" and "arguments" in strict code)

So it seems there is no recursive case after all. Seems like I just got confused by the 

DestructuringAssignmentTarget[Yield] :
LeftHandSideExpression[?Yield]

bit. Thanks a lot for your patience! I’m getting more confident with the code and the spec with time.

(Note to self: also need to add tests for other ValidSimpleAssignmentTargets, like CallExpressions and the like)
Ok, reverted the recursive case for now, added and updated a lot more tests, and addressed the comments

Hopefully I got the spec right this time :-)
Attachment #8462962 - Flags: review?(jorendorff)
Depends on: 1032150
Comment on attachment 8462962 [details] [diff] [review]
part2: revert recursive case

Review of attachment 8462962 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review because there's something I don't understand.

::: js/src/frontend/Parser.cpp
@@ +3183,5 @@
>                      }
> +                    // the RestElement should not support nested patterns
> +                    if (!target->pn_kid->isKind(PNK_ARRAY) && !target->pn_kid->isKind(PNK_OBJECT)) {
> +                        target = target->pn_kid;
> +                    }

Isn't this where we should report an error and throw if the !target->pn_kid->isKind(PNK_NAME)?

Since we later assert that it is a PNK_NAME, and you added a SyntaxError test for [...[]]=, it seems like the test should assert. Obviously I'm missing anything, but I can't tell what.

::: js/src/jit-test/tests/basic/destructuring-rest.js
@@ +4,5 @@
>  
> +assertThrowsInstanceOf(() => new Function('[...a, ,] = []'), SyntaxError, 'trailing elision');
> +assertThrowsInstanceOf(() => new Function('[a, ...b, c] = []'), SyntaxError, 'trailing param');
> +assertThrowsInstanceOf(() => new Function('var [...[a]] = []'), SyntaxError, 'nested pattern');
> +assertThrowsInstanceOf(() => new Function('var [...a.x] = []'), SyntaxError, 'lvalue expression in declaration');

Good, but please also test for these, just for thoroughness:

     [...{a}]
     [...a()]
     [...a=b]

and test that they all (except [...a.x]) throw even when used for a destructuring assignment pattern (and not a binding pattern---that is, without var).

@@ +76,5 @@
> +
> +strictIdentifiers.forEach(ident =>
> +  assertThrowsInstanceOf(() =>
> +    new Function('"use strict"; [...' + ident + '] = []'), SyntaxError));
> +

If it's convenient, please break this into two tests.
Attachment #8462962 - Flags: review?(jorendorff)
Comment on attachment 8462962 [details] [diff] [review]
part2: revert recursive case

Review of attachment 8462962 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +3183,5 @@
>                      }
> +                    // the RestElement should not support nested patterns
> +                    if (!target->pn_kid->isKind(PNK_ARRAY) && !target->pn_kid->isKind(PNK_OBJECT)) {
> +                        target = target->pn_kid;
> +                    }

checkAndMarkAsAssignmentLhs would have raised the error in that case. But yes. Its clearer to throw the error right away.
> Good, but please also test for these, just for thoroughness:
> 
>      [...{a}]
>      [...a()]
>      [...a=b]
> 
> and test that they all (except [...a.x]) throw even when used for a
> destructuring assignment pattern (and not a binding pattern---that is,
> without var).

This was a little more involved, had to change around quite a bit.
So I added the earlier error, which is also a real SyntaxError like its supposed to be, instead of a ReferenceError.
Attachment #8470416 - Flags: review?(jorendorff)
Meh, I should have folded this into part2. So if its fine now, you might also want to r+ part2. I will fold those into a single patch for landing anyway, since there is quite some churn here…
Comment on attachment 8470416 [details] [diff] [review]
part 3: make [call()] an invalid destructuring target

Review of attachment 8470416 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +3221,1 @@
>                      // the RestElement should not support nested patterns

Pre-existing style nit: put a blank line before this comment.

Also, since the comment is a complete sentence, give it a capital letter at the beginning and a period at the end.

::: js/src/jit-test/tests/basic/spread-call-setcall.js
@@ +11,5 @@
>  function check(expr) {
>    assertThrowsInstanceOf(Function(expr), ReferenceError);
>  }
> +function checkDestructuring(expr) {
> +  assertThrowsInstanceOf(() => Function(expr)(), SyntaxError);

Please remove the second () here. It should read:
    assertThrowsInstanceOf(() => Function(expr), SyntaxError);
Creating the function should throw the SyntaxError; we should never get to the point of calling it.

::: js/src/tests/js1_8_5/regress/regress-609617.js
@@ +40,5 @@
> +             * NB: JSOP_SETCALL throws only JSMSG_BAD_LEFTSIDE_OF_ASS, it does not
> +             * specialize for ++ and -- as the compiler's error reporting does. See
> +             * the next section's forked assertEq code.
> +             */
> +            assertEq(e.message, "invalid assignment left-hand side");

Style nit: Please add curly braces around this:

    if (/\[/.test(...)) {
        assertEq(e.message, ...);
    } else {
        /*
         *...
         */
        assertEq(...);
    }
Attachment #8470416 - Flags: review?(jorendorff) → review+
Attachment #8462962 - Flags: review+
Hello, this has caused bc1 bustage.
https://hg.mozilla.org/mozilla-central/rev/5c2363e6e9ad
https://hg.mozilla.org/mozilla-central/rev/ff1dd4dd6984
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1060258
Thanks a lot :arai!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: