From 537aa2d6fb23252ed7ca628298687d0c0185290e Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Sun, 3 Sep 2017 14:18:59 -0400 Subject: [PATCH 1/4] destructuring fixes [Fixes #4673] [Fixes #4657] --- lib/coffeescript/nodes.js | 74 ++++++++++++++------------------------- src/nodes.coffee | 52 ++++++++++----------------- test/assignment.coffee | 12 +++++++ test/functions.coffee | 14 ++++++++ 4 files changed, 72 insertions(+), 80 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 3a87f604f6..5fe5378269 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2109,15 +2109,15 @@ // Check if object contains splat. hasSplat() { - var j, len1, prop, ref1, splat; + var j, len1, prop, ref1; ref1 = this.properties; for (j = 0, len1 = ref1.length; j < len1; j++) { prop = ref1[j]; if (prop instanceof Splat) { - splat = true; + return true; } } - return splat != null ? splat : false; + return false; } compileNode(o) { @@ -2240,7 +2240,7 @@ } // Object spread properties. https://github.com/tc39/proposal-object-rest-spread/blob/master/Spread.md - // `obj2 = {a: 1, obj..., c: 3, d: 4}` → `obj2 = Object.assign({}, {a: 1}, obj, {c: 3, d: 4})` + // `obj2 = {a: 1, obj..., c: 3, d: 4}` → `obj2 = _extends({}, {a: 1}, obj, {c: 3, d: 4})` compileSpread(o) { var _extends, addSlice, j, len1, prop, propSlices, props, slices, splatSlice; props = this.properties; @@ -3200,6 +3200,8 @@ this.checkAssignability(o, name); if (this.moduleDeclaration) { return o.scope.add(name.value, this.moduleDeclaration); + } else if (this.param) { + return o.scope.add(name.value, 'var'); } else { return o.scope.find(name.value); } @@ -3230,7 +3232,7 @@ answer = compiledName.concat(this.makeCode(` ${this.context || '='} `), val); // 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. - if (o.level > LEVEL_LIST || (o.level === LEVEL_TOP && isValue && this.variable.base instanceof Obj && !this.nestedLhs && !this.param)) { + if (o.level > LEVEL_LIST || o.level === LEVEL_TOP && isValue && this.variable.base instanceof Obj && !this.nestedLhs && !(this.param === true)) { return this.wrapInParentheses(answer); } else { return answer; @@ -3240,31 +3242,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, 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 - // `setScopeVar()` declares variables `a` and `b` at the top of the - // current scope. - setScopeVar = function(prop) { - var newVar; - newVar = false; - if (prop instanceof Assign && prop.value.base instanceof Obj) { - return; - } - if (prop instanceof Assign) { - if (prop.value.base instanceof IdentifierLiteral) { - newVar = prop.value.base.compileWithoutComments(o); - } else { - newVar = prop.variable.base.compileWithoutComments(o); - } - } else { - newVar = prop.compileWithoutComments(o); - } - if (newVar) { - return o.scope.add(newVar, 'var', true); - } - }; + var fragments, getPropKey, getPropName, j, len1, restElement, restElements, result, traverseRest, value, valueRef, valueRefTemp; // Returns a safe (cached) reference to the key for a given property getPropKey = function(prop) { var key; @@ -3300,7 +3278,6 @@ 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) { if (prop.context !== 'object') { @@ -3350,13 +3327,9 @@ return restElements; }; // Cache the value for reuse with rest elements. - if (this.value.shouldCache()) { - valueRefTemp = new IdentifierLiteral(o.scope.freeVariable('ref', { - reserve: false - })); - } else { - valueRefTemp = this.value.base; - } + valueRefTemp = this.value.shouldCache() ? new IdentifierLiteral(o.scope.freeVariable('ref', { + reserve: false + })) : this.value.base; // Find all rest elements. restElements = traverseRest(this.variable.base.properties, valueRefTemp); if (!(restElements && restElements.length > 0)) { @@ -3367,7 +3340,9 @@ for (j = 0, len1 = restElements.length; j < len1; j++) { restElement = restElements[j]; value = new Call(new Value(new Literal(utility('objectWithoutKeys', o))), [restElement.source, restElement.excludeProps]); - result.push(new Assign(restElement.name, value)); + result.push(new Assign(new Value(restElement.name), value, null, { + param: this.param ? 'alwaysDeclare' : null + })); } fragments = result.compileToFragments(o); if (o.level === LEVEL_TOP) { @@ -3802,7 +3777,9 @@ ifTrue = new Assign(new Value(param.name), param.value); exprs.push(new If(condition, ifTrue)); } else { - exprs.push(new Assign(new Value(param.name), param.asReference(o))); + exprs.push(new Assign(new Value(param.name), param.asReference(o), null, { + param: 'alwaysDeclare' + })); } } // If this parameter comes before the splat or expansion, it will go @@ -3817,7 +3794,7 @@ } else { if ((param.value != null) && !param.assignedInBody) { ref = new Assign(new Value(param.name), param.value, null, { - param: true + param: 'alwaysDeclare' }); } else { ref = param; @@ -3827,22 +3804,25 @@ if (param.name instanceof Arr || param.name instanceof Obj) { // This parameter is destructured. param.name.lhs = true; - param.name.eachName(function(prop) { - return o.scope.parameter(prop.value); - }); // Compile `foo({a, b...}) ->` to `foo(arg) -> {a, b...} = arg`. // Can be removed once ES proposal hits Stage 4. if (param.name instanceof Obj && param.name.hasSplat()) { splatParamName = o.scope.freeVariable('arg'); o.scope.parameter(splatParamName); ref = new Value(new IdentifierLiteral(splatParamName)); - exprs.push(new Assign(new Value(param.name), ref)); + exprs.push(new Assign(new Value(param.name), ref, null, { + param: 'alwaysDeclare' + })); // Compile `foo({a, b...} = {}) ->` to `foo(arg = {}) -> {a, b...} = arg`. if ((param.value != null) && !param.assignedInBody) { ref = new Assign(ref, param.value, null, { param: true }); } + } else if (!param.shouldCache()) { + param.name.eachName(function(prop) { + return o.scope.parameter(prop.value); + }); } } else { // This compilation of the parameter is only to get its name to add @@ -4077,12 +4057,12 @@ // as well as be a splat, gathering up a group of parameters into an array. exports.Param = Param = (function() { class Param extends Base { - constructor(name1, value1, splat1) { + constructor(name1, value1, splat) { var message, token; super(); this.name = name1; this.value = value1; - this.splat = splat1; + this.splat = splat; message = isUnassignable(this.name.unwrapAll().value); if (message) { this.name.error(message); diff --git a/src/nodes.coffee b/src/nodes.coffee index 3f946a44a9..683c0f32bd 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1427,8 +1427,8 @@ exports.Obj = class Obj extends Base # Check if object contains splat. hasSplat: -> - splat = yes for prop in @properties when prop instanceof Splat - splat ? no + return yes for prop in @properties when prop instanceof Splat + no compileNode: (o) -> props = @properties @@ -1510,7 +1510,7 @@ exports.Obj = class Obj extends Base prop.eachName iterator if prop.eachName? # Object spread properties. https://github.com/tc39/proposal-object-rest-spread/blob/master/Spread.md - # `obj2 = {a: 1, obj..., c: 3, d: 4}` → `obj2 = Object.assign({}, {a: 1}, obj, {c: 3, d: 4})` + # `obj2 = {a: 1, obj..., c: 3, d: 4}` → `obj2 = _extends({}, {a: 1}, obj, {c: 3, d: 4})` compileSpread: (o) -> props = @properties # Store object spreads. @@ -2144,6 +2144,8 @@ exports.Assign = class Assign extends Base @checkAssignability o, name if @moduleDeclaration o.scope.add name.value, @moduleDeclaration + else if @param + o.scope.add name.value, 'var' else o.scope.find name.value @@ -2167,7 +2169,7 @@ exports.Assign = class Assign extends Base answer = compiledName.concat @makeCode(" #{ @context or '=' } "), val # 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. - if o.level > LEVEL_LIST or (o.level is LEVEL_TOP and isValue and @variable.base instanceof Obj and not @nestedLhs and not @param) + if o.level > LEVEL_LIST or o.level is LEVEL_TOP and isValue and @variable.base instanceof Obj and not @nestedLhs and not (@param is yes) @wrapInParentheses answer else answer @@ -2175,23 +2177,6 @@ exports.Assign = class Assign extends Base # Check object destructuring variable for rest elements; # can be removed once ES proposal hits Stage 4. compileObjectDestruct: (o) -> - # 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 - # `setScopeVar()` declares variables `a` and `b` at the top of the - # current scope. - setScopeVar = (prop) -> - newVar = false - return if prop instanceof Assign and prop.value.base instanceof Obj - if prop instanceof Assign - if prop.value.base instanceof IdentifierLiteral - newVar = prop.value.base.compileWithoutComments o - else - newVar = prop.variable.base.compileWithoutComments o - else - newVar = prop.compileWithoutComments o - o.scope.add(newVar, 'var', true) if newVar - # Returns a safe (cached) reference to the key for a given property getPropKey = (prop) -> if prop instanceof Assign @@ -2220,7 +2205,6 @@ exports.Assign = class Assign extends Base 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?() @@ -2252,10 +2236,11 @@ 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 + valueRefTemp = + if @value.shouldCache() + new IdentifierLiteral o.scope.freeVariable 'ref', reserve: false + else + @value.base # Find all rest elements. restElements = traverseRest @variable.base.properties, valueRefTemp @@ -2266,7 +2251,7 @@ exports.Assign = class Assign extends Base 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 + result.push new Assign new Value(restElement.name), value, null, param: if @param then 'alwaysDeclare' else null fragments = result.compileToFragments o if o.level is LEVEL_TOP @@ -2600,7 +2585,7 @@ exports.Code = class Code extends Base ifTrue = new Assign new Value(param.name), param.value exprs.push new If condition, ifTrue else - exprs.push new Assign new Value(param.name), param.asReference(o) + exprs.push new Assign new Value(param.name), param.asReference(o), null, param: 'alwaysDeclare' # If this parameter comes before the splat or expansion, it will go # in the function definition parameter list. @@ -2613,25 +2598,26 @@ exports.Code = class Code extends Base ref = param.asReference o else if param.value? and not param.assignedInBody - ref = new Assign new Value(param.name), param.value, null, param: yes + ref = new Assign new Value(param.name), param.value, null, param: 'alwaysDeclare' else ref = param # Add this parameter’s reference(s) to the function scope. if param.name instanceof Arr or param.name instanceof Obj # This parameter is destructured. param.name.lhs = yes - param.name.eachName (prop) -> - o.scope.parameter prop.value # Compile `foo({a, b...}) ->` to `foo(arg) -> {a, b...} = arg`. # Can be removed once ES proposal hits Stage 4. if param.name instanceof Obj and param.name.hasSplat() splatParamName = o.scope.freeVariable 'arg' o.scope.parameter splatParamName ref = new Value new IdentifierLiteral splatParamName - exprs.push new Assign new Value(param.name), ref + exprs.push new Assign new Value(param.name), ref, null, param: 'alwaysDeclare' # Compile `foo({a, b...} = {}) ->` to `foo(arg = {}) -> {a, b...} = arg`. - if param.value? and not param.assignedInBody + if param.value? and not param.assignedInBody ref = new Assign ref, param.value, null, param: yes + else unless param.shouldCache() + param.name.eachName (prop) -> + o.scope.parameter prop.value else # This compilation of the parameter is only to get its name to add # to the scope name tracking; since the compilation output here diff --git a/test/assignment.coffee b/test/assignment.coffee index 752626e6d7..b62f6cd1a8 100644 --- a/test/assignment.coffee +++ b/test/assignment.coffee @@ -915,3 +915,15 @@ test "#4674: _extends utility for object spreads 2", -> e = {a..., c...} eq e.b, 1 eq e.d, 2 + +test "#4673: complex destructured object spread variables", -> + b = c: 1 + {{a...}...} = b + eq a.c, 1 + + d = {} + {d.e...} = f: 1 + eq d.e.f, 1 + + {{g}...} = g: 1 + eq g, 1 diff --git a/test/functions.coffee b/test/functions.coffee index 75807bb47a..160723988e 100644 --- a/test/functions.coffee +++ b/test/functions.coffee @@ -549,3 +549,17 @@ test "#4413: expressions in function parameters that create generated variables g = (a = foo() ? bar()) -> a + 1 eq f(), 33 eq g(), 34 + +test "#4673: complex destructured object spread variables", -> + b = c: 1 + f = ({{a...}...}) -> + a + eq f(c: 1).c, 1 + +test "#4657: destructured array param declarations", -> + a = 1 + b = 2 + f = ([a..., b]) -> + f [3, 4, 5] + eq a, 1 + eq b, 2 From cdbea4bbda5676f33b13b4c6123e96c936fa2f1c Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Sun, 3 Sep 2017 14:43:05 -0400 Subject: [PATCH 2/4] test for destructured @prop --- test/functions.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functions.coffee b/test/functions.coffee index 160723988e..9d9ff377bf 100644 --- a/test/functions.coffee +++ b/test/functions.coffee @@ -551,11 +551,14 @@ test "#4413: expressions in function parameters that create generated variables eq g(), 34 test "#4673: complex destructured object spread variables", -> - b = c: 1 f = ({{a...}...}) -> a eq f(c: 1).c, 1 + g = ({@y...}) -> + eq @y.b, 1 + g b: 1 + test "#4657: destructured array param declarations", -> a = 1 b = 2 From d734ef81f13645b8a3160a1f9ac06358a1f238a5 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Sun, 3 Sep 2017 13:07:38 -0700 Subject: [PATCH 3/4] Add another test to cover #4657 cases --- test/functions.coffee | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functions.coffee b/test/functions.coffee index 9d9ff377bf..15b70834f4 100644 --- a/test/functions.coffee +++ b/test/functions.coffee @@ -566,3 +566,9 @@ test "#4657: destructured array param declarations", -> f [3, 4, 5] eq a, 1 eq b, 2 + +test "#4657: destructured array parameters", -> + f = ([a..., b]) -> {a, b} + result = f [1, 2, 3, 4] + arrayEq result.a, [1, 2, 3] + eq result.b, 4 From f64f505e92eb892cc7c06beaa91be47abf63557c Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Sun, 3 Sep 2017 17:03:55 -0400 Subject: [PATCH 4/4] don't declare actual params --- lib/coffeescript/nodes.js | 4 ++-- src/nodes.coffee | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 5fe5378269..581e2d9c4f 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3201,7 +3201,7 @@ if (this.moduleDeclaration) { return o.scope.add(name.value, this.moduleDeclaration); } else if (this.param) { - return o.scope.add(name.value, 'var'); + return o.scope.add(name.value, this.param === 'alwaysDeclare' ? 'var' : 'param'); } else { return o.scope.find(name.value); } @@ -3794,7 +3794,7 @@ } else { if ((param.value != null) && !param.assignedInBody) { ref = new Assign(new Value(param.name), param.value, null, { - param: 'alwaysDeclare' + param: true }); } else { ref = param; diff --git a/src/nodes.coffee b/src/nodes.coffee index 683c0f32bd..f98093772f 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2145,7 +2145,11 @@ exports.Assign = class Assign extends Base if @moduleDeclaration o.scope.add name.value, @moduleDeclaration else if @param - o.scope.add name.value, 'var' + o.scope.add name.value, + if @param is 'alwaysDeclare' + 'var' + else + 'param' else o.scope.find name.value @@ -2598,7 +2602,7 @@ exports.Code = class Code extends Base ref = param.asReference o else if param.value? and not param.assignedInBody - ref = new Assign new Value(param.name), param.value, null, param: 'alwaysDeclare' + ref = new Assign new Value(param.name), param.value, null, param: yes else ref = param # Add this parameter’s reference(s) to the function scope.