Fix #4876: remove polyfill for object rest/spread

[zdenko]

Fixes #4876. This PR removes polyfills for object rest/spread syntax.

{a, b, rest...} = obj

var a, b, rest;
({a, b, ...rest} = obj);

---

newObj = {a:1, obj1..., b:2, obj2...}

var newObj;
newObj = { a: 1, ...obj1, b: 2, ...obj2};

---

foo = ({a, b, rest...}) ->

foo = function({a, b, ...rest}) {};

The rest property can be positioned anywhere. In the compiled code it will be moved at the end:

{a, rest..., b} = obj

var a, b, rest;
({a, b, ...rest} = obj);

---

foo = ({a, rest..., b}) ->

foo = function({a, b, ...rest}) {};

I commented a few existing tests since it seems the following code isn’t valid:

{{a...}...} = obj

({...{...a}} = obj);
          ^
SyntaxError: `...` must be followed by an assignable reference in assignment contexts

[GeoffreyBooth]

We can’t merge this with commented-out tests. If the tests are no longer valid, they should be removed; but looking at them, I’m wondering if we should instead try to make them pass. Take the simplest case:

{{g}...} = g: 1

var g;

({...{g}} = {
  g: 1
});

This throws Uncaught SyntaxError: `...` must be followed by an assignable reference in assignment contexts. To avoid the error, we can work around it:

h = {g}
{h...} = g: 1

var h;

h = {g};

({...h} = {
  g: 1
});

This doesn’t throw any errors, and should be equivalent to the first example unless I’m missing something. So I’m thinking, if all we need to do to avoid this “must be followed by an assignable reference” error is to make a reference to the assignment target, couldn’t the compiler do that automatically? As in:

{{g}...} = g: 1

var g, ref;

ref = {g};
({...ref} = {
  g: 1
});

We already have lots of functions for creating temporary “reference” variables. Let’s try to use them to see if we can avoid a potential breaking change.

One last thought: what’s going on here?

{({g})...} = g: 1

var g, ref,
  slice = [].slice;

ref = {
  g: 1
}, {g} = slice.call(ref, 0);

[GeoffreyBooth]

Different question, for @jashkenas and @lydell and @connec and @vendethiel and other interested parties: How would you all feel if we dropped support for Node 6? As you can see in the build log, there are a fair number of tests scattered around that don’t parse in Node 6 after this PR; though the compiler itself still never uses object spread/destructuring, so it runs in Node 6 even if some of the tests fail.

To keep support for Node 6, we would need to refactor the tests to silo off the object destructuring ones into files that aren’t loaded in Node < 8, similar to how we have the async tests. But I’m wondering if that’s worth the effort, versus just dropping Node 6 and keeping the codebase simpler. See also #4881; as we add more Node 8+, ES2018+ features, this will be an increasingly common issue.

I think I lean toward the side of keeping support for Node 6, even in tests, even if it means more complication in moving test files around; but I feel like we should discuss it.

[vendethiel]

I havn't used nodejs as a platform myself for too long to have an informed opinion, sorry.

[zdenko]

Both ...[ and ...{ are disallowed from object rest/spread syntax.

I'm not sure if there is any value of supporting this syntax, because {{a}...} = objor {{a...}...} = obj is equivalent to {a...} = obj.
Supporting it would mean bringing back Assign::compileObjectDestruct, which would compile {{a...}...} = obj into

var ref, a;
({...ref} = obj);
({...a} = ref);

Or, perhaps this example {a, {b, c, {d, e...}...}...} = obj (equivalent to {a, b, c, d, e...} = obj)

var ref, ref1, a, b, c, d, e;
({a, ...ref} = obj);
({b, c, ...ref1} = ref);
({d, ...e} = ref1);

I would propose alternative solution, e.g. stripping redundant {...}...:
{{a...}...} = obj => {a...} = obj
{a, {b, c, {d, e...}...}...} = obj => {a, b, c, d, e...} = obj.

But then, what about this {[a]...} = obj? Is this the same as

{ref...} = obj
a = [ref]

# or

a = [{a...} = obj]

# or
 
a = [obj]

A more complex example:

obj = {a: 1, b: 2, c:3, d: 4}
{a, b, {[x]...}...} = obj

# should compile to?

{a, b, ...ref} = obj
x = [ref]

###
a = 1
b = 2
x = [{c: 3, d: 4}]
###

I might be missing something, but it seems to much work and trouble.

@GeoffreyBooth {({g})...} = g: 1 looks like a bug. I'll check it and open new PR.

[zdenko]

I think support for Node 6 shouldn't be dropped. The end of life is scheduled for April 2019, and there are projects still running on this version.

I followed the path of async tests approach and move all object rest/spread tests in a separate file which is excluded from the tests for Node 6.

[aminland]

This seems to be a stage 3 thing in babel (though it seems to work without transpilation in everything not IE)

The active support date for node 6 is end of march 2018, so if we're to drop node 6 support, I'd suggest just lumping #4880, #4881, #4884, #4994, and #5006 together and call it version 2.5. There are enough syntax additions that it warrants skipping a few versions.

If there's any weird bugs that remain on 2.2.x maybe we can just backport the fixes? That is assuming the branches don't diverge that much in the year that node 6 has left on its 'maintenance' lifespan.

Also doing a semi-major version bump can indicate that underlying philosophy has changed and that we're now merging in features that are marked stage 3 on babel.

[GeoffreyBooth]

This feature is Stage 4.

I see no reason to skip versions or maintain separate forks. The compatibility info is for convenience, not a promise; it's up to the user to ensure that their code runs in whatever runtime or browser they want to run it in, or they should transpile.

[helixbass]

@zdenko can you remove or @variable.isArray() and hasSplat? So that now eg

[{...a}] = b

would compile to [{...a}] = b instead of ({...a} = b[0])?

[zdenko]

Done.

[vendethiel]

why not do this check in @reorderProperties?

[zdenko]

Nice catch. Thanks.

[helixbass]

@zdenko I believe you can also stop treating object spreads as complexObjects() in compileDestructuring()? So eg [..., {r...}] = arr compiles to

[{...r}] = slice.call(arr, -1);

instead of

ref = slice.call(arr, -1), ({...r} = ref[0]);

If so, not sure exactly what code inside compileDestructuring() can be removed, at least the stuff related to hasObjSpreads()?

[zdenko]

Last commit fixes unassignable rest properties, e.g. {{a}...}, {{a...}...}

{{a...}...} = b
# ({...ref} = b), ({...a} = ref);

---

f = ({{a...}...}) ->
###
f = function(arg) {
  var a, arg, ref;
  ({...ref} = arg), ({...a} = ref);
};
###

[vendethiel]

I see no point allowing it

[GeoffreyBooth]

@helixbass What does ({a}...) -> mean, exactly? I can type

var fn = function(...{a}) { return a; };

into Chrome console but no matter what I pass into fn the function returns undefined.

[helixbass]

@GeoffreyBooth try var fn = function(...{length}) { return length; };

The splatted param is an array, so that's about the only way I see to meaningfully treat the array as an object

If getting rid of it seems reasonable(/acceptably breaking if anyone has actually used it), seems like throwing an error in Param.constructor() if @name instanceof Obj and @splat would be much simpler than introducing a grammar change to distinguish between top-level ParamVars vs splattable param vars

[helixbass]

Although I figure it's against our policy to not support a destructuring syntax that's supported in ES6. And that it'd be strange/more work than it's worth to allow it in the simple case ({a}...) -> but not in the case that triggers compileDestructuring() (({a}..., arg) ->). So then I think the fix I suggested above is a simple way to have the compileDestructuring() version output valid JS

[GeoffreyBooth]

Yes, in general we want to be able to output any valid JS, aside from known exclusions like == and const etc.

So you’re saying that really literally only (...{length}) works, and it’s always parsing the arguments object, so it’s only destructuring arguments.length. So

(function(...{length}) { return length; })(1, 'b', null)

returns 3, the number of arguments passed in. This can easily be something we ignore until someone opens an issue asking why they can’t destructure the length property of the arguments object, or maybe they’ll think of another, less ridiculous edge case.

[helixbass]

@GeoffreyBooth that stance would make sense if we didn't already support it. But my understanding of the current status is that the simple case (eg ({a}...) ->) already works fine. The thing that's broken is the corresponding case when we have to invoke compileDestructuring() because of a non-trailing splat eg ({a}..., b) ->. To fix that case I think all that's needed is to add or param.name instanceof Obj as described above

[GeoffreyBooth]

@helixbass I’m not following. Can you please explain with examples?

[GeoffreyBooth]

@zdenko Do you think we should make the change @helixbass is proposing?

Separate question: I’m considering whether to release 2.3.0 with the PRs that have been merged in so far plus this one, and leave the completely new features (supporting accessors without brackets, “sub-structuring”) as a potential 2.4.0. This would give the ES2018 stuff its own release, and a period of potential patches separate from the new functionality. Thoughts?

[zdenko]

Do you think we should make the change @helixbass is proposing

Yes, I think @helixbass is right. This PR already outputs object splats ({{a}..., b} = obj), as valid JS. Adding the same support for function parameters (({a}..., b) ->) and arrays ([{a}..., b] = arr) seems the right way to go IMO.

I’m considering whether to release 2.3.0...

That's a good call. I agree. ES2018 stuff in 2.3.0, and new features later in 2.4.0.

[helixbass]

@zdenko @GeoffreyBooth I pushed a commit to my object_rest_spread branch with support and tests for what we’ve been discussing

@zdenko brought up a good point which is that the corresponding assignment cases (as opposed to function params) also should be covered. Turned out that both array-destructured and object-destructured non-final splats (in array destructuring assignments) were both broken (and not covered by my suggested fix). Eg both of these were failing but are now passing:

[{length}..., three] = [1, 2, 3]
eq length, 2
eq three, 3

[[one, two]..., three] = [1, 2, 3]
eq one, 1
eq two, 2
eq three, 3

For the param cases, the fix I suggested above (adding or param.name instanceof Obj) did get the object-destructured non-final splat param case working (array-destructured non-final splat params were already working). Eg this was failing but now passes:

f = ({length}..., last) -> [length, last]
arrayEq f(4, 5, 6), [2, 6]

Let me know if you have any other questions about what I committed or what’s covered, thanks

[zdenko]

@helixbass it seems we're both working on the same stuff at the same time 😄
How should this case compile?

[x, {a}..., y] = [1, {a: 42}, 2]

# x = 1
# a = 42
# y = 2

IMO, this desugars into

[x, r..., y] = [1, {a: 42}, 2]
{a} = r

[helixbass]

@zdenko I think your example has the right idea for translating {a}... into r... and {a} = r, it just also needs additional translation of [x, r..., y] = since we can't output a non-final splat. But that's the gist of what my commit does for your example: translates {a}... into ref... before invoking the core processObjects() logic of compileDestructuring(), then assign {a} = ref at the end

Currently my branch compiles your example into:

var a, ref, ref1, x, y,
  splice = [].splice;

ref1 = [
  1,
  {
    a: 42
  },
  2
], [x, ...ref] = ref1, [y] = splice.call(ref, -1), ({a} = ref);

Not sure why there's the additional caching to ref1 of the right-hand-side (seems unnecessary), but other than that I believe this is the desired compilation. Compare to the compilation of [x, r..., y] = [1, {a: 42}, 2]:

var r, ref, x, y,
  splice = [].splice;

ref = [
  1,
  {
    a: 42
  },
  2
], [x, ...r] = ref, [y] = splice.call(r, -1);

[zdenko]

[x, ...ref] = ref1, [y] = splice.call(ref, -1), ({a} = ref

x = 1
y = [ { a: 42 } ]
a = undefined

Instead of ({a} = ref), it should be [{a}] = ref, or perhaps {a} = ref[0]

[helixbass]

@zdenko no, it should agree with ES meaning of {a}..., even though it's not very useful. That's why {length}... is pretty much the only meaningful usage of this syntax that I'm aware of and why I used length in the tests. We have other tests that use this syntax like {"0": a, "1": b, "2": c}... which I guess could perhaps be useful as well

But it is definitely an object-dereferencing of an array (the splatted array), not an object-dereferencing of the first element of an array

[vendethiel]

I suppose you could technically pluck arguments at index, i.e. ({0: a, 17: b}...) -> a + b. Not sure it’s very interesting tho...

[zdenko]

...it should agree with ES meaning of {a}..., even though it's not very useful

In that case should we allow {{a}...} = obj? The Initial proposal for object rest/spread properties had this pattern (e.g. destructuring own properties), which was later removed.
Babel supports this syntax in stage-3, as well as the current CS branch.

This PR desugars {{a}...} = obj into {ref...} = obj; {a} = ref.
My proposal is that we also enable syntactic sugars for [{a}...] = arr and ({a}...) ->.
On my branch (not commited yet), these desugars into [ref...] = arr; [{a}] = ref, and (arg...) -> [{a}] = arg.

Examples:

arr = [1, {a: 42, b: 43}, 2, 3, 4]
[x, {a}..., y, z] = arr
# x = 1, a= 42, y = 3, z = 4

---

foo = (x, {a}..., y, z) -> [x, a, y, z]
[x, a, y, z] = foo 1, {a:42}, 2, 3, 4
# x = 1, a= 42, y = 3, z = 4

[helixbass]

@zdenko I'm indifferent to allowing object-destructuring of an object rest spread (eg {{a}...} = obj) - I'm guessing it was removed from the spec since it seems like you could always achieve the same results by "expanding" your nested destructuring into the top level (eg {{a, b...}..., c} -> {a, b..., c}. But if we already support it I don't see a reason to remove it

But as far as making object-destructuring of an "array splat" implicitly dereference the first element of the array, that's a very bad idea, let me explain

Start from the difference between the two types of destructured/left-hand-side splats: when the splat is inside an object, it's an "object rest spread" and the splat variable will be an object (eg a will be an object in {a...} = b). When the splat is inside an array, it's an "array splat" and the splat variable will be an array (eg a will be an array in [a...] = b). Splats as (top-level) function params are "array splat"s

So we are discussing the "array splat" case only, and specifically when the splat variable is itself a destructured object. You are saying the object that gets destructured should be the first element of the array splat and I'm saying it should be the array splat itself. Eg whether [{a}...] = b should mean [ref...] = b, {a} = ref[0] or [ref...] = b, {a} = ref

Here are my reasons why it should be the latter (object-dereference the array splat itself):

• Compatibility with ES6 behavior
• Compatibility with existing Coffeescript behavior
• Expected symbolic meaning of dereferencing syntax

To illustrate ES6 behavior, run these in Chrome console or wherever:

const f = (...{length}) => length
f() // 0
f(1, 1, 1) // 3

and the corresponding array-destructured assignments:

let [...{length}] = []
length // 0
[...{length}] = [1, 1, 1]
length // 3

Existing Coffeescript behavior matches ES6 behavior, eg [{a}...] = b is compiled (correctly) into [...{a}] = b and ({a}...) -> is compiled into function(...{a}) {}

And as far as intuitive/expected behavior, it's clearly a confusing syntax regardless. I imagine that your arguments for making it dereference the first element of the array splat are that it may be more useful (since the use cases for object-dereferencing an array are very limited), may seem more consistent with the meaning of {a}... in an "object rest spread" context, or may seem like the expected meaning of {a}...

But that "consistency" is not a valid point, as clearly a... has two different meanings in an "array splat" vs an "object rest spread" context (a will be either an array or an object), so {a}... can correspondingly have two different meanings in those two contexts

As far as expected meaning, clearly our brain may think "{a} is dereferencing an object" when we see {a}.... But to introduce an implicit dereferencing of the first element of the array splat (in order to give it a chance to be an actual object rather than an array) is kind of arbitrary and doesn't follow this guiding rule: dereferencing syntax should visually correspond exactly to the structure of what it's dereferencing. You should be able to directly substitute a nested dereferencing for a variable and know that the nested dereferencing is performed on what the variable would have been. Eg [a] = d vs [{b: c}] = d, {b: c} represents the same object as a. Basically you should always be able to perform the nested destructuring as a subsequent destructuring assignment, eg [{b: c}] = d is equivalent to [a] = d, {b: c} = a. Your suggested compilation violates this rule. You are not making [{b: c}...] = d equivalent to [a...] = d, {b: c} = a, you are making it equivalent to [a...] = d, [{b: c}] = a

But so then it should be clear how to achieve the desired effect of dereferencing the first element of the array splat: just wrap the object-destructuring in [], eg [[{b: c}]...] = d. So your examples should be written as [x, [{a}]..., y, z] = arr and (x, [{a}]..., y, z) ->. This looks like it compiles correctly at least on my object_rest_spread branch

So to me it is clear that compatibility with ES6, compatibility with existing Coffeescript behavior, and maintaining this implicit rule of destructuring-syntax precision are more than enough reasons to prefer the meaning "object-destructure the array splat itself" over "object-destructure the first element of the array splat"

If you see my argument, I'd encourage you to use the commit on my object_rest_spread branch at least as a reference (if you don't want to just merge it into your branch), as I feel confident that it covers the basic cases of object-dereferencing an array splat and has useful tests for those cases

[zdenko]

@helixbass you made a very sound argument. I was indeed looking at {a}... from the "inside out".
I already used some of your code, and I might just merge the rest of it. I'll try to wrap everything up tomorrow and have this PR ready for a merge.

[GeoffreyBooth]

Is there anything holding up this PR?

[zdenko]

@GeoffreyBooth I think it's ready to be merged.
@helixbass any notes?

[helixbass]

@zdenko @GeoffreyBooth lgtm

[GeoffreyBooth]

Thanks much @zdenko and @helixbass. I’ll prepare a new release soon.