[CS2] Fix #4651: object spread destructuring bug

[zdenko]

FIxes #4651.
Compiler was throwing an error for object spread destructuring of values with properties (e.g. @props, a.b.c) or object literals.

# {a: {b}, r...} = @props
({
  a: {b}
} = ref = this.props);
r = objectWithoutKeys(ref, ['a']);

Besides that, caching of the object literal value is improved:

Before:

# {a, r...} = {a:1, b:2}
({a} = {a: 1, b: 2});
r = objectWithoutKeys({a: 1, b: 2}, ['a']),

After:

# {a, r...} = {a:1, b:2}
({a} = ref = {a: 1, b: 2});
r = objectWithoutKeys(ref, ['a']),

[GeoffreyBooth]

@helixbass, @connec?

[connec]

Should we change Obj::shouldCache to always be true? I guess there may be unintended consequences of that, but I can't think of a situation where we'd rather duplicate an object literal rather than reusing it.

[zdenko]

I've already tried that, but then tests fail.
I also think object destructuring with spreads is the only place where this is necessary.
Besides, spreads might soon reach stage-4.

[connec]

Is this another case that would work better if whatever source is was wrapped in a Value? @helixbass

Alternatively, should the case where there are no properties not wrap source in a Value? I can't really remember how this all works, but in the case that source has no properties, the next level of key won't be added (source.properties.concat [new Access getPropKey prop]). It must be working if the tests pass, but I don't really understand why...

[GeoffreyBooth]

@connec I’m not sure I follow your comment. Is there something that needs to change here?

[connec]

I guess it's just not clear to me why we need to check if source.properties is set. I imagine this is roughly equivalent to source instanceof Value, and my 2 questions are:

1. Would it be easier to ensure source is a Value (e.g. in the grammar/wherever source is created)?
2. If source is not a value (e.g. not source.properties) then we do not execute the line where we concat the property key. Why does this work?

I'll try and find some time to poke around to answer my own questions, since it's probably just my own misunderstandings. Since the tests work etc. this can probably go in, and if I find any issues I can submit a new ticket/fix.

[helixbass]

@connec you're right there's some bugginess here, I'm poking around and ran into eg this:

{c:{a...}} = d: 1

compiles to this:

({
  c: {}
} = ref = {
  d: 1
});
a = objectWithoutKeys(ref, []);

This would seem to be a case of nestedSource not getting set correctly (the first arg to objectWithoutKeys() shouldn't be ref, I guess it should be ref.c?)

If I can figure it out I'll push a fix to this branch, otherwise I'll at least push breaking tests

[zdenko]

@connec @helixbass you're both right. Now it should work as expected.

In case when source is without properties (e.g. IdentifierLiteral ref),
the value of nestedProperties should be new Value source, [new Access getPropKey prop].
Also, caching of object literal value is not needed anymore.

{c:{a...}} = d: 1
###
({
  c: {}
} = ref = {
  d: 1
});
a = objectWithoutKeys(ref.c, []);
###

{a: {b}, r...} = @props
###
({
  a: {b}
} = ref = this.props);
r = objectWithoutKeys(ref, ['a']);
###

[zdenko]

@helixbass would you mind reviewing my last commit? I think it should work now.

[helixbass]

@zdenko I was also working on a fix, I think it's a little cleaner so I merged it in (along with tests) - instead of preserving the if source.properties conditional, I just wrap source in a new Value outside of the loop and remove the conditional

Added to destructuring assignment with multiple splats in different objects test:

• test the above (by adding q... and deepEqual q, n: 1)
• another thing that's currently breaking: added t = {obj...} and deepEqual t, obj

From what I could tell, that currently-failing case t = {obj...} fails b/c it goes into compileObjectDestruct() mode when it shouldn't. Looking at a simplest case {a = {b...}} = c, currently it'll more-or-less mistake this for {a: {b...}} = c and compile to:

({a = {}} = c);
b = objectWithoutKeys(c[a], []);

I tried changing the condition inside traverseRest() from if prop.value.isObject?() to if prop.value.isObject?() and prop.context is 'object' but this created an infinite loop, seemingly b/c it keeps calling into compileObjectDestruct(). Really that's the problem, the condition for calling compileObjectDestruct() needs to be refined (ie it needs to stop confusing {a = {b...}} = c with {a: {b...}} = c). So I tried adding and @context is 'object' to the condition in return @compileObjectDestruct(o) if ..., but then "top-level" destructuring (eg {a...} = b) breaks

So I'm not sure how you can distinguish between an Assign using = inside an object vs one that's not inside an object (or how else you could add the necessary refinement to the condition around compileObjectDestruct() to not call it when it sees {a = {b...}}), @connec @zdenko?

[zdenko]

@helixbass It's fine with me. I'll also take a look later.
The issue might be connected to the property lhs which is set to true to all object's nested properties in case object has lhs set to true.

# If this object is the left-hand side of an assignment, all its children
# are too.
if @lhs
  for prop in props when prop instanceof Assign
    {value} = prop
    unwrappedVal = value.unwrapAll()
    if unwrappedVal instanceof Arr or unwrappedVal instanceof Obj
      unwrappedVal.lhs = yes
    else if unwrappedVal instanceof Assign
      unwrappedVal.nestedLhs = yes

[zdenko]

@helixbass I've made some changes and it seems to be working.
Basically, I'm checking the operatorToken in traverseRest() when Assign is detected.
If token is = the current prop will be skipped. Another change is checking of returned value from the compileObjectDestruct(), and if it's false @compileNode() will continue forward.

{a = {b...}} = c
###
({a = Object.assign({}, b)} = c)
###

{c:{a...}} = d: 1
###
({
  c: {}
} = ref = {
  d: 1
});
a = objectWithoutKeys(ref.c, [])
###

[GeoffreyBooth]

Are all these scenarios covered by tests? Also @zdenko #4644 needs some tests.

[zdenko]

@GeoffreyBooth these cases are covered by tests. @helixbass added them in the previous commit.

I noticed that unnecessary ref variables were generated in some cases. My last commit fixes that.
Before:

{a = {b...}} = c:1
###
({a = Object.assign({}, b)} = ref = {
  c: 1
})
###

{p: {m, q..., t = {obj...}}, r...} = c: 2
###
({
  p: {m, t = Object.assign({}, obj)}
} = ref1 = ref = {
  c: 2
});
q = objectWithoutKeys(ref.p, ['m', 't']);
r = objectWithoutKeys(ref, ['p'])
###

After:

{a = {b...}} = c:1
###
({a = Object.assign({}, b)} = {
  c: 1
})
###

{p: {m, q..., t = {obj...}}, r...} = c: 2
###
({
  p: {m, t = Object.assign({}, obj)}
} = ref = {
  c: 2
});
q = objectWithoutKeys(ref.p, ['m', 't']);
r = objectWithoutKeys(ref, ['p'])
###

[helixbass]

@zdenko nice, I'd just say the conditional based on operatorToken could be a little cleaner:

• from what I can tell, it's a little simpler and more typical to use @context rather than @operatorToken to distinguish (inside an Obj) an = Assign vs an : Assign (@connec?). So your condition could be unless prop.context is 'object'
• and to my taste, it's cleaner and more readable to use an early-exit continue rather than two mutually exclusive if clauses, so:

continue unless prop.context is 'object'
nestedProperties = prop.value.base.properties

[GeoffreyBooth]

So @helixbass and @connec, how does the current version look?

@zdenko and @helixbass, your efforts in tackling this have been heroic!

[GeoffreyBooth]

@helixbass, does the last commit satisfy all your notes? Can I merge this in?

[helixbass]

@GeoffreyBooth I'd defer to @connec on whether it's ready to be merged but the cases I'd encountered are looking good

[helixbass]

@zdenko @GeoffreyBooth not specifically related to this issue (and sorry if it's already been addressed) but just saw that apparently Object.assign() isn't transpiled by eg babel-loader, you need to separately use babel-core/polyfill. So then is it a good idea to use Object.assign() in the implementation of object spreads (if it's in fact the only thing output by CS2 that necessitates babel-core/polyfill)?

[GeoffreyBooth]

@helixbass Are you sure?

✦ mkdir object-destructuring
✦ cd object-destructuring
✦ npm i babel-cli babel-preset-stage-3
+ babel-preset-stage-3@6.24.1
+ babel-cli@6.26.0
added 248 packages in 24.522s
✦ echo 'var {a = {...b}} = {c: 1}' > test.js
✦ ./node_modules/babel-cli/bin/babel.js test.js --presets=stage-3
var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

var { a = _extends({}, b) } = { c: 1 };

[connec]

I've given this another look over and it seems to solve the problem. In the process I logged #4673, but that also existed before this PR.

[helixbass]

@GeoffreyBooth but try the same test where test.js contains Object.assign() (like the output of current CS2 object spread would), rather than the reference to Object.assign being generated in Babel's own transpiling of an object spread. Ie once we compile object spreads directly into object spreads, it won't be an issue, but since we're currently compiling into Object.assign() (until object spreads hit whichever stage), Babel will see Object.assign() (and seemingly not do anything to it unless you use the separate polyfill)

[helixbass]

@GeoffreyBooth so if my understanding is correct, we should be outputting/using an _extends helper like Babel's (which can default to using Object.assign like theirs) rather than outputting Object.assign() directly

[connec]

2...connec:fix-4651-alt

I ended up playing with an alternative approach that treats the lhs status of Obj nodes as 'unknown' (undefined) by default. When looking for spreads we check the lhs but default to trying them (as with now). In compileObjectDestruct if we find a default value that is an object we explicitly mark it as not lhs.

(Ab)using lhs in this way feels a bit hacky, but if we can add an lhs check to decide whether to compileObjectDestruct it does simplify the fix somewhat (no need to support returning false etc.). A proper fix for this would need to somehow set lhs more proactively than we currently do (we currently set lhs on properties when we compile the Obj, which is too late to test it).

There's not really much between the two options in that they both ultimately allow compileObjectDestruct to 'fall through' if there are no actual spreads to compile. The approach in this PR is a bit nicer I think, though I think ultimately we may want to revisit how we handle lhs and compileObjectDestruct.

[connec]

I don't think this is needed any more? I was having no problems without any of the valueRefTemp stuff.

[connec]

Also - in the PR description you mention that object literals were not being cached, however they do appear to be on 2 currently:

http://coffeescript.org/v2/#try:%7Ba%2C%20r...%7D%20%3D%20%7Ba%3A1%2C%20b%3A2%7D

[GeoffreyBooth]

@connec how did you get rid of the valueRefTemp references? How did you revise the

restElements = traverseRest @variable.base.properties, valueRefTemp

line?

@zdenko please ignore the polyfill discussion, that’s been taken care of in a separate PR. Do you have time to address these last few notes so that we can merge this?

[zdenko]

@GeoffreyBooth yes, I'll take a look a.s.a.p.

[zdenko]

in the PR description you mention that object literals were not being cached

@connec I was wrong regarding the caching. It was probably my mistake in the code.
::shouldCache() and valueRefTemp are used to assign object literal value to ref variable, e.g. {p: {m, q..., t = {obj...}}, r...} = c: 2 => ({p: {m, t = Object.assign({}, obj)}} = ref = { c: 2}).
The reason for use valueRefTemp here, are cases where there are no rest elements in the destructuring, e.g. {a = {b...}} = c:1 => ({a = Object.assign({}, b)} = {c: 1}).

So, tests will pass if you remove valueRefTemp and use @value as a parameter for the traverseRest(), only the compiled code will be different.

With traverseRestTemp and ::shouldCache()

{p: {m, q..., t = {obj...}}, r...} = c: 2
###
({
  p: {m, t = Object.assign({}, obj)}
} = ref = {
  c: 2
});
q = objectWithoutKeys(ref.p, ['m', 't']);
r = objectWithoutKeys(ref, ['p']);
###

and without

{p: {m, q..., t = {obj...}}, r...} = c: 2
###
({
  p: {m, t = Object.assign({}, obj)}
} = {
  c: 2
});
q = objectWithoutKeys({
  c: 2
}.p, ['m', 't']);
r = objectWithoutKeys({
  c: 2
}, ['p']);
###

[connec]

Sorry @zdenko, I think I must be misunderstanding something. If I remove the valueTempRef stuff:

diff --git a/src/nodes.coffee b/src/nodes.coffee
index 2e30bfe4..5e331f34 100644
--- a/src/nodes.coffee
+++ b/src/nodes.coffee
@@ -2253,16 +2253,12 @@ exports.Assign = class Assign extends Base
       restElements
 
     # Cache the value for reuse with rest elements.
-    if @value.shouldCache()
-      valueRefTemp = new IdentifierLiteral o.scope.freeVariable 'ref', reserve: false
-    else
-      valueRefTemp = @value.base
+    [@value, valueRef] = @value.cache o
 
     # Find all rest elements.
-    restElements = traverseRest @variable.base.properties, valueRefTemp
+    restElements = traverseRest @variable.base.properties, valueRef
     return no unless restElements and restElements.length > 0
 
-    [@value, valueRef] = @value.cache o
     result = new Block [@]
 
     for restElement in restElements

And run the example you gave:

echo '{p: {m, q..., t = {obj...}}, r...} = c: 2' | bin/coffee -bcs

I get the desired output:

// Generated by CoffeeScript 2.0.0-beta4
var m, q, r, ref, ref1, t,
  objectWithoutKeys = function(o, ks) { var res = {}; for (var k in o) ([].indexOf.call(ks, k) < 0 && {}.hasOwnProperty.call(o, k)) && (res[k] = o[k]); return res; };

({
  p: {m, t = Object.assign({}, obj)}
} = ref1 = ref = {
  c: 2
});
q = objectWithoutKeys(ref.p, ['m', 't']);
r = objectWithoutKeys(ref, ['p']);

[connec]

Edit: in fact, I see that that output 'double-cached' {c:2} (as ref and ref1). I guess this is because @value is assigned even if the function returns. The following patch avoids the double-cache:

diff --git a/src/nodes.coffee b/src/nodes.coffee
index 2e30bfe4..081c2a71 100644
--- a/src/nodes.coffee
+++ b/src/nodes.coffee
@@ -2253,16 +2253,13 @@ exports.Assign = class Assign extends Base
       restElements
 
     # Cache the value for reuse with rest elements.
-    if @value.shouldCache()
-      valueRefTemp = new IdentifierLiteral o.scope.freeVariable 'ref', reserve: false
-    else
-      valueRefTemp = @value.base
+    [cachedValue, valueRef] = @value.cache o
 
     # Find all rest elements.
-    restElements = traverseRest @variable.base.properties, valueRefTemp
+    restElements = traverseRest @variable.base.properties, valueRef
     return no unless restElements and restElements.length > 0
 
-    [@value, valueRef] = @value.cache o
+    @value = cachedValue
     result = new Block [@]
 
     for restElement in restElements

Sorry to be fussy about this, but I'd rather not proliferate manual caching code (e.g. new IndentifierLiteral o.scope.freeVariable) when Node::cache could be used instead.

[zdenko]

I've tried that before. With this approach ref variable is still declared in the scope by call from ::cache method:

if complex
  ref = new IdentifierLiteral o.scope.freeVariable 'ref'
  sub = new Assign ref, this

Check the full output:

{a = {b...}} = c:1
###
var a, ref;
       ^^^
({a = Object.assign({}, b)} = {
  c: 1
});
###

{p: {m, q..., t = {obj...}}, r...} = c: 2
###
var m, q, r, ref, ref1, t,
                  ^^^^
  objectWithoutKeys = function(o, ks) { var res = {}; for (var k in o) ([].indexOf.call(ks, k) < 0 && {}.hasOwnProperty.call(o, k)) && (res[k] = o[k]); return res; };

({
  p: {m, t = Object.assign({}, obj)}
} = ref = {
  c: 2
});
q = objectWithoutKeys(ref.p, ['m', 't']);
r = objectWithoutKeys(ref, ['p']);
###

[connec]

Aha, good catch 😢

In that case, this LGTM!

[connec]

@helixbass I think Object.assign is pretty established now. I don't know if we have explicit target runtimes, but most up-to-date browsers and Node 4+ support it.

[GeoffreyBooth]

I think Object.assign is pretty established now. I don’t know if we have explicit target runtimes, but most up-to-date browsers and Node 4+ support it.

Internet Explorer (versions 11 and prior) don’t support Object.assign, and they still make up between 4 and 17% of the market, depending on who’s counting.

CoffeeScript 2 takes the position that the tests should run in Node 7.6+, the first version of Node to support async/await. Modules and JSX aren’t supported in any version of Node, which is why those tests are string-based (as are the comments tests). The docs say that if you want browser compatibility, it’s on you to run your code through Babel. But @helixbass makes a good point, in that we should say in the docs that babel-polyfill is required, if it is.

So the question is, is Object.assign the only thing we require babel-polyfill for, for general browser compatibility? If it is, maybe it makes sense to just do what Babel does, and create an _extends helper and just automatically use it, and then people don’t need to think about polyfills. Loading babel-polyfill is a pain, so if it’s easy for us to prevent that need, we should. Though if Object.assign is just the tip of the iceberg and there are other polyfills required for other features, it’s probably better to make this clear in the docs.

EDIT: Split off the polyfill discussion into #4674. Let’s please handle that in a separate PR, however we decide to address it.

[GeoffreyBooth]

Thanks everyone!