Fix parsing object rest spread #149

danez · 2016-09-29T21:07:41Z

This makes object-rest-spread behave according to spec and only
allow one spread/rest operator and enforces it to be the last
param in the object.

Also move all object-rest-spread tests to a own folder.

The fix is pretty simple, as soon as we parsed the spread operator we expect the object to end.

The only thing I'm not 100% sure is if trailing commas are aloud, but I would assume it is the same as with array spread to be not allowed. (see test 8) //cc @sebmarkbage

Fixes #148

hzoo · 2016-09-29T21:14:31Z

test/fixtures/flow/type-parameter-declaration/class-with-jsx/expected.json

@@ -0,0 +1,33 @@
+{


This file looks unrelated?

Upps removed

hzoo · 2016-09-29T21:16:44Z

src/parser/expression.js

@@ -713,7 +713,8 @@ pp.parseObj = function (isPattern, refShorthandDefaultPos) {
      prop = this.parseSpread();
      prop.type = isPattern ? "RestProperty" : "SpreadProperty";
      node.properties.push(prop);
-      continue;
+      this.expect(tt.braceR);


Is it easy to have have more specific error messages for these?

let { ...x, y, z } = obj; - spread has to be the last element (would result in the same value as the right hand side)
let { x, ...y, ...z } = obj; - cannot have multiple spreads (would result in the same value for both y and z)?

Done, I also added a separate message when having an invalid trailing comma

sebmarkbage · 2016-09-29T21:27:03Z

The single rest at the end only applies to binding let { x, ...y } = obj; and assignment ({ x, ...y } = obj). It doesn't apply to spread let obj = { ...x, y, ...z }; // works.

I think you need to make this conditional on the isPattern flag, no?

hzoo · 2016-09-29T21:34:09Z

Yeah only for rest 👍

The failing test from babel is example of this (valid)

babel-plugin-transform-object-rest-spread/object rest spread expression:
     SyntaxError: /home/travis/build/babel/babylon/build/babel/packages/babel-plugin-transform-object-rest-spread/test/fixtures/object-rest-spread/expression/actual.js: Unexpected token (1:10)
  > 1 | ({ x, ...y, a, ...b, c });

      if (isPattern) {
        this.expect(tt.braceR);
        break;
      } else {
        continue;
      }

This makes object-rest-spread behave according to spec and only allow one spread/rest operator and enforces it to be the last param in the object. Also move all object-rest-spread tests to a own folder.

danez · 2016-09-29T21:47:38Z

Is trailing comma allowed in this case?

const { x, ...s, } = obj;

because this seems not to be allowed:

const [ x, ...s, ] = arr;

sebmarkbage · 2016-09-29T21:51:30Z

@danez No, trailing comma is not allowed. EDIT: The rationale for this is that the use case for trailing comma is that you can add something at the end without affecting the line above. E.g. for version control software. However, that is never the case here.

codecov-io · 2016-09-29T22:01:23Z

Current coverage is 94.49% (diff: 100%)

Merging #149 into master will increase coverage by 0.02%

@@             master       #149   diff @@
==========================================
  Files            19         19          
  Lines          3111       3124    +13   
  Methods         327        327          
  Messages          0          0          
  Branches        818        823     +5   
==========================================
+ Hits           2939       2952    +13   
  Misses           94         94          
  Partials         78         78

Powered by Codecov. Last update 9cc0981...47bc157

hzoo · 2016-09-29T22:47:21Z

👍 Other than I think we could give a better message

hzoo · 2016-09-30T12:08:13Z

src/parser/expression.js

-        break;
+        const position = this.state.start;
+        if (firstRestLocation !== null) {
+          this.unexpected(firstRestLocation, "Cannot have multiple rest elements in destruction expression");


Maybe "Cannot have multiple rest elements in a destructuring assignment" or "Cannot have multiple rest elements when destructuring"?

I did not use assignment as I think this is not an assignment if I'm not mistaken:

function ({a,b,...c,...c}) {}

But this would issue the same error. Should add a test for it if there isn't one yet.

Can use "Cannot have multiple rest elements when destructuring".

Can use "Cannot have multiple rest elements when destructuring". 👍

hzoo · 2016-09-30T12:27:32Z

src/parser/expression.js

@@ -753,6 +769,10 @@ pp.parseObj = function (isPattern, refShorthandDefaultPos) {
    node.properties.push(prop);
  }

+  if (firstRestLocation !== null) {


These can be moved right below the while (!this.eat(tt.braceR)) { block right? - I guess it was left at the end as convention? Works either way

no it has to be at the end after the loop, because otherwise the multiple destructuring check does not work.

Basically the logic is:

loop through all params: if spread if havent seen spread before AND this is not the end of the object pattern: record that we have seen a spread else if havent seen spread before: warning that this is already the second spread if we finished the loop and recorded a spread: error that it was not at the end

Oh I think I misread the last }!

hzoo · 2016-09-30T12:54:38Z

Just gotta fix the error messages in the tests 👍

istarkov · 2016-10-03T10:43:41Z

This PR does not work for arrow functions,

const x = ({ ...props, y }) => 1; // THIS WORK
function xx({ ...props, y }) { return 1;} // THIS FAIL

danez · 2016-10-03T10:50:29Z

Good catch, thanks for reporting.

istarkov · 2016-10-03T10:55:42Z

This catch will cost me a day to fix hundreds of my code lines ;-) (Will read the specs in the future)

* Fix parsing object rest This makes object-rest-spread behave according to spec and only allow one rest operator and enforces it to be the last param in the object. Also move all object-rest-spread tests to a own folder. * Show nicer error messages

danez added Tag: Bug Fix PR: needs review spec-violation labels Sep 29, 2016

hzoo reviewed Sep 29, 2016

View reviewed changes

Fix parsing object rest spread

8c960e2

This makes object-rest-spread behave according to spec and only allow one spread/rest operator and enforces it to be the last param in the object. Also move all object-rest-spread tests to a own folder.

Show nicer error messages

c09acbe

hzoo reviewed Sep 30, 2016

View reviewed changes

Adapt messages

5d63e8e

hzoo added 2 commits September 30, 2016 10:41

update test [skip ci]

648a249

update test message

47bc157

hzoo merged commit 7877829 into babel:master Sep 30, 2016

hzoo mentioned this pull request Oct 1, 2016

A trailing comma is not permitted after the rest element - ListView.js 457:14 facebook/react-native#10199

Closed

danez deleted the object-rest-spread branch October 3, 2016 10:50

not-an-aardvark mentioned this pull request Jan 5, 2017

Trailing commas after rest properties should be a syntax error eslint/espree#310

Closed

hzoo mentioned this pull request Jan 11, 2017

shouldn't allow trailing comma after rest parameter in objects babel/babel#5092

Closed

danharper mentioned this pull request Mar 23, 2017

trailingComma option is not applied after spread in object prettier/prettier#1079

Closed

hzoo mentioned this pull request Apr 12, 2018

Babel 7 Take 2 Automattic/wp-calypso#23424

Merged

4 tasks

merunga mentioned this pull request Jul 27, 2018

A trailing comma is not permitted after the rest element Jacse/react-native-app-intro-slider#58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing object rest spread #149

Fix parsing object rest spread #149

danez commented Sep 29, 2016

hzoo Sep 29, 2016

danez Sep 29, 2016

hzoo Sep 29, 2016

danez Sep 30, 2016

sebmarkbage commented Sep 29, 2016 •

edited

hzoo commented Sep 29, 2016 •

edited

danez commented Sep 29, 2016 •

edited

sebmarkbage commented Sep 29, 2016 •

edited

codecov-io commented Sep 29, 2016 •

edited

hzoo commented Sep 29, 2016

hzoo Sep 30, 2016

danez Sep 30, 2016

hzoo Sep 30, 2016

hzoo Sep 30, 2016

danez Sep 30, 2016

hzoo Sep 30, 2016

hzoo commented Sep 30, 2016

istarkov commented Oct 3, 2016

danez commented Oct 3, 2016

istarkov commented Oct 3, 2016

Fix parsing object rest spread #149

Fix parsing object rest spread #149

Conversation

danez commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Sep 29, 2016 • edited

hzoo commented Sep 29, 2016 • edited

danez commented Sep 29, 2016 • edited

sebmarkbage commented Sep 29, 2016 • edited

codecov-io commented Sep 29, 2016 • edited

Current coverage is 94.49% (diff: 100%)

hzoo commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Sep 30, 2016

istarkov commented Oct 3, 2016

danez commented Oct 3, 2016

istarkov commented Oct 3, 2016

sebmarkbage commented Sep 29, 2016 •

edited

hzoo commented Sep 29, 2016 •

edited

danez commented Sep 29, 2016 •

edited

sebmarkbage commented Sep 29, 2016 •

edited

codecov-io commented Sep 29, 2016 •

edited