From f9367bacf117b0ecab4bcb700982273852d86325 Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Fri, 18 Aug 2017 04:06:37 +0200 Subject: [PATCH 1/7] fix object spread destructuring bug: #4651 --- lib/coffeescript/nodes.js | 26 ++++++++++++++++++++------ src/nodes.coffee | 19 +++++++++++++++---- test/assignment.coffee | 21 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 1d5609bd96..2191fc1a23 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3234,7 +3234,7 @@ // Check object destructuring variable for rest elements; // can be removed once ES proposal hits Stage 4. compileObjectDestruct(o) { - var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, traverseRest, value, valueRef; + var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, shouldCache, traverseRest, value, valueRef; // Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration, // if we’re destructuring without declaring, the destructuring assignment // must be wrapped in parentheses: `({a, b} = obj)`. Helper function @@ -3301,9 +3301,13 @@ [prop.value.value, nestedSourceDefault] = prop.value.value.cache(o); } if (nestedProperties) { - nestedSource = new Value(source.base, source.properties.concat([new Access(getPropKey(prop))])); - if (nestedSourceDefault) { - nestedSource = new Value(new Op('?', nestedSource, nestedSourceDefault)); + if (source.properties) { + nestedSource = new Value(source.base, source.properties.concat([new Access(getPropKey(prop))])); + if (nestedSourceDefault) { + nestedSource = new Value(new Op('?', nestedSource, nestedSourceDefault)); + } + } else { + nestedSource = source; } restElements.push(...traverseRest(nestedProperties, nestedSource)); } @@ -3335,8 +3339,18 @@ } return restElements; }; - // Cache the value for reuse with rest elements - [this.value, valueRef] = this.value.cache(o); + // Cache the value for reuse with rest elements. + // `Obj` should be always cached. + // Examples: + // {a, r...} = {a:1, b:2, c:3} + // {a, r...} = {a:1, obj...} + shouldCache = (value) => { + if (value.base instanceof Obj) { + return true; + } + return value.shouldCache(); + }; + [this.value, valueRef] = this.value.cache(o, false, shouldCache); // Find all rest elements. restElements = traverseRest(this.variable.base.properties, valueRef); result = new Block([this]); diff --git a/src/nodes.coffee b/src/nodes.coffee index 677d4df028..23518255c9 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2229,8 +2229,11 @@ exports.Assign = class Assign extends Base nestedProperties = prop.value.variable.base.properties [prop.value.value, nestedSourceDefault] = prop.value.value.cache o if nestedProperties - nestedSource = new Value source.base, source.properties.concat [new Access getPropKey prop] - nestedSource = new Value new Op '?', nestedSource, nestedSourceDefault if nestedSourceDefault + if source.properties + nestedSource = new Value source.base, source.properties.concat [new Access getPropKey prop] + nestedSource = new Value new Op '?', nestedSource, nestedSourceDefault if nestedSourceDefault + else + nestedSource = source restElements.push traverseRest(nestedProperties, nestedSource)... else if prop instanceof Splat prop.error "multiple rest elements are disallowed in object destructuring" if restIndex? @@ -2247,8 +2250,16 @@ 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. + # `Obj` should be always cached. + # Examples: + # {a, r...} = {a:1, b:2, c:3} + # {a, r...} = {a:1, obj...} + shouldCache = (value) => + return yes if value.base instanceof Obj + value.shouldCache() + + [@value, valueRef] = @value.cache o, false, shouldCache # Find all rest elements. restElements = traverseRest @variable.base.properties, valueRef diff --git a/test/assignment.coffee b/test/assignment.coffee index 7b5ea8409f..1000386e61 100644 --- a/test/assignment.coffee +++ b/test/assignment.coffee @@ -296,6 +296,27 @@ 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}, r...} = o.props + eq m, o.props.p.m + deepEqual r, s: 6 + + @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: { From 2664c2c108d4682580cb6fa9f59acfe4f29a9fd5 Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Fri, 18 Aug 2017 08:51:11 +0200 Subject: [PATCH 2/7] small fix --- lib/coffeescript/nodes.js | 2 +- src/nodes.coffee | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 2191fc1a23..5bac1b5ac6 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3344,7 +3344,7 @@ // Examples: // {a, r...} = {a:1, b:2, c:3} // {a, r...} = {a:1, obj...} - shouldCache = (value) => { + shouldCache = function(value) { if (value.base instanceof Obj) { return true; } diff --git a/src/nodes.coffee b/src/nodes.coffee index 23518255c9..f17fff88d7 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2255,7 +2255,7 @@ exports.Assign = class Assign extends Base # Examples: # {a, r...} = {a:1, b:2, c:3} # {a, r...} = {a:1, obj...} - shouldCache = (value) => + shouldCache = (value) -> return yes if value.base instanceof Obj value.shouldCache() From 232041db2ab5162099bf03f1df53e53d618af230 Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Mon, 21 Aug 2017 16:22:17 +0200 Subject: [PATCH 3/7] fixed issue with nested properties --- lib/coffeescript/nodes.js | 16 +++------------- src/nodes.coffee | 12 ++---------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 5bac1b5ac6..3545a43af6 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3234,7 +3234,7 @@ // Check object destructuring variable for rest elements; // can be removed once ES proposal hits Stage 4. compileObjectDestruct(o) { - var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, shouldCache, traverseRest, value, valueRef; + var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, traverseRest, value, valueRef; // Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration, // if we’re destructuring without declaring, the destructuring assignment // must be wrapped in parentheses: `({a, b} = obj)`. Helper function @@ -3307,7 +3307,7 @@ nestedSource = new Value(new Op('?', nestedSource, nestedSourceDefault)); } } else { - nestedSource = source; + nestedSource = new Value(source, [new Access(getPropKey(prop))]); } restElements.push(...traverseRest(nestedProperties, nestedSource)); } @@ -3340,17 +3340,7 @@ return restElements; }; // Cache the value for reuse with rest elements. - // `Obj` should be always cached. - // Examples: - // {a, r...} = {a:1, b:2, c:3} - // {a, r...} = {a:1, obj...} - shouldCache = function(value) { - if (value.base instanceof Obj) { - return true; - } - return value.shouldCache(); - }; - [this.value, valueRef] = this.value.cache(o, false, shouldCache); + [this.value, valueRef] = this.value.cache(o); // Find all rest elements. restElements = traverseRest(this.variable.base.properties, valueRef); result = new Block([this]); diff --git a/src/nodes.coffee b/src/nodes.coffee index f17fff88d7..c07fb7b699 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2233,7 +2233,7 @@ exports.Assign = class Assign extends Base nestedSource = new Value source.base, source.properties.concat [new Access getPropKey prop] nestedSource = new Value new Op '?', nestedSource, nestedSourceDefault if nestedSourceDefault else - nestedSource = source + nestedSource = new Value source, [new Access getPropKey prop] restElements.push traverseRest(nestedProperties, nestedSource)... else if prop instanceof Splat prop.error "multiple rest elements are disallowed in object destructuring" if restIndex? @@ -2251,15 +2251,7 @@ exports.Assign = class Assign extends Base restElements # Cache the value for reuse with rest elements. - # `Obj` should be always cached. - # Examples: - # {a, r...} = {a:1, b:2, c:3} - # {a, r...} = {a:1, obj...} - shouldCache = (value) -> - return yes if value.base instanceof Obj - value.shouldCache() - - [@value, valueRef] = @value.cache o, false, shouldCache + [@value, valueRef] = @value.cache o # Find all rest elements. restElements = traverseRest @variable.base.properties, valueRef From 2149c3561b30db0ee9a8fdd6a0c4766ebfc4c745 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Mon, 21 Aug 2017 10:34:33 -0400 Subject: [PATCH 4/7] ensure Value; breaking test for {a={b...}} = c --- lib/coffeescript/nodes.js | 14 +++++++------- src/nodes.coffee | 9 ++++----- test/assignment.coffee | 4 +++- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 5bac1b5ac6..b08dac84e3 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3288,8 +3288,12 @@ var base1, index, j, len1, nestedProperties, nestedSource, nestedSourceDefault, p, prop, restElements, restIndex; restElements = []; restIndex = void 0; + if (source.properties == null) { + source = new Value(source); + } for (index = j = 0, len1 = properties.length; j < len1; index = ++j) { prop = properties[index]; + nestedSourceDefault = nestedSource = nestedProperties = null; setScopeVar(prop.unwrap()); if (prop instanceof Assign) { if (typeof (base1 = prop.value).isObject === "function" ? base1.isObject() : void 0) { @@ -3301,13 +3305,9 @@ [prop.value.value, nestedSourceDefault] = prop.value.value.cache(o); } if (nestedProperties) { - if (source.properties) { - nestedSource = new Value(source.base, source.properties.concat([new Access(getPropKey(prop))])); - if (nestedSourceDefault) { - nestedSource = new Value(new Op('?', nestedSource, nestedSourceDefault)); - } - } else { - nestedSource = source; + nestedSource = new Value(source.base, source.properties.concat([new Access(getPropKey(prop))])); + if (nestedSourceDefault) { + nestedSource = new Value(new Op('?', nestedSource, nestedSourceDefault)); } restElements.push(...traverseRest(nestedProperties, nestedSource)); } diff --git a/src/nodes.coffee b/src/nodes.coffee index f17fff88d7..307b423c9c 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2216,8 +2216,10 @@ 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 @@ -2229,11 +2231,8 @@ exports.Assign = class Assign extends Base nestedProperties = prop.value.variable.base.properties [prop.value.value, nestedSourceDefault] = prop.value.value.cache o if nestedProperties - if source.properties - nestedSource = new Value source.base, source.properties.concat [new Access getPropKey prop] - nestedSource = new Value new Op '?', nestedSource, nestedSourceDefault if nestedSourceDefault - else - nestedSource = source + nestedSource = new Value source.base, source.properties.concat [new Access getPropKey prop] + nestedSource = new Value new Op '?', nestedSource, nestedSourceDefault if nestedSourceDefault restElements.push traverseRest(nestedProperties, nestedSource)... else if prop instanceof Splat prop.error "multiple rest elements are disallowed in object destructuring" if restIndex? diff --git a/test/assignment.coffee b/test/assignment.coffee index 1000386e61..1588f23e30 100644 --- a/test/assignment.coffee +++ b/test/assignment.coffee @@ -305,9 +305,11 @@ test "destructuring assignment with multiple splats in different objects", -> s: 6 } } - {p: {m}, r...} = o.props + {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 From 2491d3286d8a90fdd6d18fe1a2943ea8eb55a96b Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Mon, 21 Aug 2017 21:12:31 +0200 Subject: [PATCH 5/7] fix assign in nested properties fix assign in nested properties --- lib/coffeescript/nodes.js | 20 ++++++++++++++++---- src/nodes.coffee | 13 ++++++++++--- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 39f696333a..c778152fd2 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3142,7 +3142,7 @@ // we've been assigned to, for correct internal references. If the variable // has not been seen yet within the current scope, declare it. compileNode(o) { - var answer, compiledName, isValue, j, name, properties, prototype, ref1, ref2, ref3, ref4, ref5, val, varBase; + var answer, compiledName, isValue, j, name, objDestructAnswer, properties, prototype, ref1, ref2, ref3, ref4, ref5, val, varBase; isValue = this.variable instanceof Value; if (isValue) { // When compiling `@variable`, remember if it is part of a function parameter. @@ -3163,7 +3163,10 @@ return node instanceof Obj && node.hasSplat(); })) { // Object destructuring. Can be removed once ES proposal hits Stage 4. - return this.compileObjectDestruct(o); + objDestructAnswer = this.compileObjectDestruct(o); + } + if (objDestructAnswer) { + return objDestructAnswer; } } if (this.variable.isSplice()) { @@ -3297,8 +3300,14 @@ setScopeVar(prop.unwrap()); if (prop instanceof Assign) { if (typeof (base1 = prop.value).isObject === "function" ? base1.isObject() : void 0) { - // prop is `k: {...}` - nestedProperties = prop.value.base.properties; + if (prop.operatorToken.unwrap().value === ':') { + // prop is `k: {...}` + nestedProperties = prop.value.base.properties; + } + if (prop.operatorToken.unwrap().value === '=') { + // prop is `k = {...} ` + continue; + } } else if (prop.value instanceof Assign && prop.value.variable.isObject()) { // prop is `k: {...} = default` nestedProperties = prop.value.variable.base.properties; @@ -3343,6 +3352,9 @@ [this.value, valueRef] = this.value.cache(o); // Find all rest elements. restElements = traverseRest(this.variable.base.properties, valueRef); + if (!(restElements && restElements.length > 0)) { + return false; + } result = new Block([this]); for (j = 0, len1 = restElements.length; j < len1; j++) { restElement = restElements[j]; diff --git a/src/nodes.coffee b/src/nodes.coffee index 6fb3e36eea..44574cf3b8 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -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 ['||=', '&&=', '?='] @@ -2224,8 +2225,12 @@ exports.Assign = class Assign extends Base if prop instanceof Assign # prop is `k: expr`, we need to check `expr` for nested splats if prop.value.isObject?() - # prop is `k: {...}` - nestedProperties = prop.value.base.properties + if prop.operatorToken.unwrap().value is ':' + # prop is `k: {...}` + nestedProperties = prop.value.base.properties + if prop.operatorToken.unwrap().value is '=' + # prop is `k = {...} ` + continue else if prop.value instanceof Assign and prop.value.variable.isObject() # prop is `k: {...} = default` nestedProperties = prop.value.variable.base.properties @@ -2254,8 +2259,10 @@ exports.Assign = class Assign extends Base # Find all rest elements. restElements = traverseRest @variable.base.properties, valueRef + return false unless restElements and restElements.length > 0 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 From 5a709ed4a8a1768458c7d3a278e3e49ff669113b Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Tue, 22 Aug 2017 10:48:12 +0200 Subject: [PATCH 6/7] improve variable declaration --- lib/coffeescript/nodes.js | 13 ++++++++++--- src/nodes.coffee | 10 +++++++--- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index c778152fd2..2a179f221a 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3237,7 +3237,7 @@ // Check object destructuring variable for rest elements; // can be removed once ES proposal hits Stage 4. compileObjectDestruct(o) { - var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, traverseRest, value, valueRef; + var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, setScopeVar, traverseRest, value, valueRef, valueRefTemp; // Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration, // if we’re destructuring without declaring, the destructuring assignment // must be wrapped in parentheses: `({a, b} = obj)`. Helper function @@ -3349,12 +3349,19 @@ return restElements; }; // Cache the value for reuse with rest elements. - [this.value, valueRef] = this.value.cache(o); + if (this.value.shouldCache()) { + valueRefTemp = new IdentifierLiteral(o.scope.freeVariable('ref', { + reserve: false + })); + } else { + valueRefTemp = this.value.base; + } // Find all rest elements. - restElements = traverseRest(this.variable.base.properties, valueRef); + restElements = traverseRest(this.variable.base.properties, valueRefTemp); if (!(restElements && restElements.length > 0)) { return false; } + [this.value, valueRef] = this.value.cache(o); result = new Block([this]); for (j = 0, len1 = restElements.length; j < len1; j++) { restElement = restElements[j]; diff --git a/src/nodes.coffee b/src/nodes.coffee index 44574cf3b8..0256624ea4 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2255,12 +2255,16 @@ exports.Assign = class Assign extends Base restElements # Cache the value for reuse with rest elements. - [@value, valueRef] = @value.cache o + if @value.shouldCache() + valueRefTemp = new IdentifierLiteral o.scope.freeVariable 'ref', reserve: false + else + valueRefTemp = @value.base # Find all rest elements. - restElements = traverseRest @variable.base.properties, valueRef - return false unless restElements and restElements.length > 0 + 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 From c212e6e9abdf4a13dbfdbb76a008e02cd8fac6d2 Mon Sep 17 00:00:00 2001 From: Zdenko Vujasinovic Date: Tue, 22 Aug 2017 21:19:56 +0200 Subject: [PATCH 7/7] refactor --- lib/coffeescript/nodes.js | 8 +++----- src/nodes.coffee | 10 ++++------ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 2a179f221a..1156e970c3 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3300,14 +3300,12 @@ setScopeVar(prop.unwrap()); if (prop instanceof Assign) { if (typeof (base1 = prop.value).isObject === "function" ? base1.isObject() : void 0) { - if (prop.operatorToken.unwrap().value === ':') { - // prop is `k: {...}` - nestedProperties = prop.value.base.properties; - } - if (prop.operatorToken.unwrap().value === '=') { + if (prop.context !== 'object') { // prop is `k = {...} ` continue; } + // prop is `k: {...}` + nestedProperties = prop.value.base.properties; } else if (prop.value instanceof Assign && prop.value.variable.isObject()) { // prop is `k: {...} = default` nestedProperties = prop.value.variable.base.properties; diff --git a/src/nodes.coffee b/src/nodes.coffee index 0256624ea4..2e30bfe4f1 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2225,12 +2225,10 @@ exports.Assign = class Assign extends Base if prop instanceof Assign # prop is `k: expr`, we need to check `expr` for nested splats if prop.value.isObject?() - if prop.operatorToken.unwrap().value is ':' - # prop is `k: {...}` - nestedProperties = prop.value.base.properties - if prop.operatorToken.unwrap().value is '=' - # prop is `k = {...} ` - continue + # 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() # prop is `k: {...} = default` nestedProperties = prop.value.variable.base.properties