Skip to content

[CS2] Fix #4651: object spread destructuring bug #4652

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Sep 1, 2017
Merged

[CS2] Fix #4651: object spread destructuring bug #4652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9367ba
fix object spread destructuring bug: #4651
Aug 18, 2017
2664c2c
small fix
Aug 18, 2017
232041d
fixed issue with nested properties
Aug 21, 2017
2149c35
ensure Value; breaking test for {a={b...}} = c
helixbass Aug 21, 2017
1a6477a
resolve merge conflicts
helixbass Aug 21, 2017
2491d32
fix assign in nested properties
Aug 21, 2017
5a709ed
improve variable declaration
Aug 22, 2017
c212e6e
refactor
Aug 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions lib/coffeescript/nodes.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 15 additions & 4 deletions src/nodes.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2122,8 +2122,9 @@ exports.Assign = class Assign extends Base
@variable.base.lhs = yes
return @compileDestructuring o unless @variable.isAssignable()
# Object destructuring. Can be removed once ES proposal hits Stage 4.
return @compileObjectDestruct(o) if @variable.isObject() and @variable.contains (node) ->
objDestructAnswer = @compileObjectDestruct(o) if @variable.isObject() and @variable.contains (node) ->
node instanceof Obj and node.hasSplat()
return objDestructAnswer if objDestructAnswer

return @compileSplice o if @variable.isSplice()
return @compileConditional o if @context in ['||=', '&&=', '?=']
Expand Down Expand Up @@ -2216,12 +2217,16 @@ exports.Assign = class Assign extends Base
traverseRest = (properties, source) =>
restElements = []
restIndex = undefined
source = new Value source unless source.properties?

for prop, index in properties
nestedSourceDefault = nestedSource = nestedProperties = null
setScopeVar prop.unwrap()
if prop instanceof Assign
# prop is `k: expr`, we need to check `expr` for nested splats
if prop.value.isObject?()
# prop is `k = {...} `
continue unless prop.context is 'object'
# prop is `k: {...}`
nestedProperties = prop.value.base.properties
else if prop.value instanceof Assign and prop.value.variable.isObject()
Expand All @@ -2247,13 +2252,19 @@ exports.Assign = class Assign extends Base

restElements

# Cache the value for reuse with rest elements
[@value, valueRef] = @value.cache o
# Cache the value for reuse with rest elements.
if @value.shouldCache()
Copy link
Collaborator

@connec connec Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@connec connec Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator Author

@zdenko zdenko Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@zdenko zdenko Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']);
###

Copy link
Collaborator

@connec connec Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']);

Copy link
Collaborator

@connec connec Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@zdenko zdenko Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']);
###

Copy link
Collaborator

@connec connec Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, good catch 😢

In that case, this LGTM!

valueRefTemp = new IdentifierLiteral o.scope.freeVariable 'ref', reserve: false
else
valueRefTemp = @value.base

# Find all rest elements.
restElements = traverseRest @variable.base.properties, valueRef
restElements = traverseRest @variable.base.properties, valueRefTemp
return no unless restElements and restElements.length > 0

[@value, valueRef] = @value.cache o
result = new Block [@]

for restElement in restElements
value = new Call new Value(new Literal utility 'objectWithoutKeys', o), [restElement.source, restElement.excludeProps]
result.push new Assign restElement.name, value
Expand Down
23 changes: 23 additions & 0 deletions test/assignment.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,29 @@ test "destructuring assignment with multiple splats in different objects", ->
deepEqual a, val: 1
deepEqual b, val: 2

o = {
props: {
p: {
n: 1
m: 5
}
s: 6
}
}
{p: {m, q..., t = {obj...}}, r...} = o.props
eq m, o.props.p.m
deepEqual r, s: 6
deepEqual q, n: 1
deepEqual t, obj

@props = o.props
{p: {m}, r...} = @props
eq m, @props.p.m
deepEqual r, s: 6

{p: {m}, r...} = {o.props..., p:{m:9}}
eq m, 9

# Should not trigger implicit call, e.g. rest ... => rest(...)
{
a: {
Expand Down