Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CS2] Destructuring object spreads #4493

Merged
merged 90 commits into from
Jun 30, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

Replacing #4473 and GeoffreyBooth#4. From original descriptions by @zdenko:

Connected to #3894.

In CS2 emit

obj = { x: 1, y: 2}
obj1 = {obj..., z: 3}

to

obj1 = {...obj, z: 3}

Improve destructuring assignment for object literal:

{a, b, x...} = {a:1, b:2, c:3, d:4, e:5}
a = 1
b = 2
x = {c:3, d:4, e:5}
{c, x..., e} = {a:1, b:2, c:3, d:4, e:5}
c = 3
e = 5
x = {a:1, b:2, d:4}
{c, x..., a:p} = {a:1, b:2, c:3, d:4, e:5}
c = 3
p = 1
x = {b:2, d:4, e:5}
futurists =
  sculptor: "Umberto Boccioni"
  painter:  "Vladimir Burliuk"
  poet:
    name:   "F.T. Marinetti"
    address: [
      "Via Roma 42R"
      "Bellagio, Italy 22021"
    ]

{poet: {name, addr1...}} = futurists
addr1 = {address: [ "Via Roma 42R",  "Bellagio, Italy 22021"]}

{poet: {addr2..., name:title}} = futurists
addr2 = {address: [ "Via Roma 42R",  "Bellagio, Italy 22021"]}

Basically, implement proposal for rest properties for ECMAScript in CS: {a, b, c...} = x

Since this proposal isn't yet at stage-4, CS doesn't compile to ES.
In my PR I catch rest element and assign remaining values to it:
{a, b, c...} = x compiles to:
a = x.a, b = x.b, c = extractObjectWithoutKeys(x, ['a', 'b'])

Multiple rest elements are disallowed. ES also requires the rest element to be the last, so compiler currently throws an error.

IMHO, CS should allow rest element anywhere, just like for arrays.

Later, when proposal reaches stage-4 we could implement similar conversion as you did for the function parameters, and ensure rest element is the last in the compiled JS.

GeoffreyBooth and others added 25 commits February 4, 2017 22:26
…ture `wrapInBraces` that uses `{` and `wrapInBrackets` that uses `[`
This dramatically improves the appearance of destructured imports.
…ture `wrapInBraces` that uses `{` and `wrapInBrackets` that uses `[`
@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 9, 2017
@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Apr 9, 2017

@zdenko Can you please merge 2 into the destructuring_object branch and resolve conflicts? I took a crack at it, but there are a lot of conflicts to resolve. Basically the destructuring branch in #4478 overlapped with much of what you were doing. You branch started from my original destructuring approach, which wasn’t nearly as clean or robust as @connec’s, so for #4478 we abandoned mine in favor of his; and his got merged into 2. So now you need to work backwards a bit and start from this new starting point, where object destructuring is already implemented in 2, and you only need to add the object spread destructuring. The 2 branch also already has destructured objects in function parameters.

If it’s easier to start a new branch that branches off of 2, and then cherry-picks some work from the desctructuring_object branch, that works too. I can change the branch that this PR pulls from. Please put the new branch on my repo, though, so the rest of us can contribute. Thanks!

@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 9, 2017
@danielbayley
Copy link
Contributor

danielbayley commented Apr 9, 2017

Multiple rest elements are disallowed.

What about when merging objects like this?

let obj = { a:1, b:2, c:3, d:4 }
let override = { b: 'b' , d: 'd'}
const merged = {...obj, ...override}

Which works with the stage-3 preset in babel…

@GeoffreyBooth
Copy link
Collaborator Author

@connec was that last commit it? Does this now have your blessing?

@lydell and @zdenko, do either of you want to final pass at this, or can I merge it in?

@connec
Copy link
Collaborator

connec commented Jun 30, 2017

OK, that's definitely me done now @GeoffreyBooth. My last concern was the grammar, which didn't account for things like {super()...} or {this...}. There might still be some missing cases since using anything broader like Value or Expression leads to ambiguity (as I'm sure @zdenko can testify).

I didn't add tests for those things, and it's really late here now. Other than that, I think this is good to go (of course, as a contributor to the PR I now can't be trusted to be impartial 😄).

@GeoffreyBooth
Copy link
Collaborator Author

At this point I’d rather merge it in and handle additional edge cases as further PRs. This has become an epic.

Last call!

@GeoffreyBooth
Copy link
Collaborator Author

So I was writing the documentation for this, and the example I came up with doesn’t compile:

user =
  name: 'Werner Heisenberg'
  occupation: 'theoretical physicist'

currentUser =
  user...
  status: 'Uncertain'

[stdin]:6:6: error: unexpected ...
	user...
	    ^^^

However changing currentUser to currentUser = { user..., status: 'Uncertain' } makes it work as expected. A similar example using arrays works as expected. Is this a bug?

@zdenko
Copy link
Collaborator

zdenko commented Jun 30, 2017

@GeoffreyBooth this works

currentUser = {
  user...
  status: 'Uncertain'
}

I've tried to make it work without the {} in the past but always ended with ambiguity issues.

@GeoffreyBooth
Copy link
Collaborator Author

Okay, well at least it just throws a compiler error for now. So if we figure out how to support that syntax later, we can add it in a later PR.

@GeoffreyBooth GeoffreyBooth merged commit a7a6006 into jashkenas:2 Jun 30, 2017
@GeoffreyBooth GeoffreyBooth changed the title [WIP][CS2] Destructuring object spreads [CS2] Destructuring object spreads Jun 30, 2017
@connec
Copy link
Collaborator

connec commented Jun 30, 2017

That's probably related to lookObjectish in the rewriter, which seems to only accept implicit object literals when there's a : involved (which is consistent with being unable to use shorthand properties in implicit objects.

Tbh we probably want to leave it like this, or we'll need to face down ambiguities in expressions like

f k: v, rest...

Which, currently, will compile to two arguments (an object literal and a splat param) but would be ambiguous if we allowed splats to exist in implicit objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants