Throw error when missing commas in Array and Object expressions

[gabejohnson]

Fixes #605

Formerly:

[1 2 3]; // [1, 2, 3];

let [a b c] = []; // let [a, b, c] = [];

({ a: 1 b: 2 c: 3 }); // ({a: 1, b:2, c: 3});

let { a b c } = {}; // let { a, b, c } = {};

let [...rest, a] = []; // let [...rest_0] = [];

[...[],]; // [...[],,];

All of these cases save the last should throw (it was simply parsing incorrectly). Now they do:

[1 2 3]
// Error: unexpected token
// __2__ 3

let [a b c] = [];
// Error: unexpected token
// __b__ c

({ a: 1 b: 2 c: 3 })
// Error: unexpected token
// __b__ : 2 c : 3

let { a b c } = {};
// Error: unexpected token
// __b__ c

let [...rest, a] = [];
// Error: Rest element must be last element in array
// , a

[...[],]; // [...[]];

[gabejohnson]

@disnet I realized after the fact that I pushed this branch to the main repo. Do you want me working in here or should I delete the branch and resubmit from my repo?

[gabejohnson]

This isn't quite right yet. I messed up with the spread operator.

[gabejohnson]

It's ready to review.

[disnet]

This looks great! Are there some test262 tests we can enable now to catch this?

[disnet]

btw creating branches on this repo is totally fine

[gabejohnson]

Are there some test262 tests we can enable now to catch this?

If not, we should add some.

We need to start running the fail and early tests. I'm sure that would give us tons of work to do 😄

[gabejohnson]

Do you want me to submit a PR to enable the early and fail tests before merging this one? It would be nice to make sure we have a regression test in place.

[disnet]

Yeah that would be great On Wed, Mar 22, 2017 at 7:32 AM Gabe Johnson notifications@github.com wrote:

Do you want me to submit a PR to enable the early and fail tests before merging this one? It would be nice to make sure we have a regression test in place.

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/sweet-js/sweet.js/pull/650#issuecomment-288417250, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAJOmN-MpqX-SqJ4yf2w7W5c_HLKwxUks5roTEFgaJpZM4MhhPs .

[gabejohnson]

These aren't bugs w/ the new code, I think the problem is here. We should be a checking to see if = follows.

[gabejohnson]

While ☝️ is true, the problem is greater. The transformDestructuring trick won't work as the rules for what can follow a rest element are different than those for a spread element.

This is going to bite us again once object rest/spread lands.

I propose a refactor that uses double lookahead in instances where there might be an AssignmentExpression and deals with the bindings right away.

As far as this PR goes, do you want to hold on it? Or merge it, comment out the offending tests and proceed with a refactor?

[disnet]

Let's merge it now and then refactor.