From c8c8c167d1edbe3fddd420bff37daab121c9d40c Mon Sep 17 00:00:00 2001 From: zdenko Date: Tue, 16 Jan 2018 04:29:48 +0100 Subject: [PATCH] Fix #4843: bad output when assigning to @prop in destructuring assignment with defaults (#4848) * fix #4843 * improvements * typo * small fix --- lib/coffeescript/nodes.js | 38 +++++++++++++++++++++++---------- src/nodes.coffee | 31 +++++++++++++++++++++------ test/function_invocation.coffee | 9 ++++++++ 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index e9b879b1..64af6312 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1499,7 +1499,7 @@ constructor({ content: content1, newLine: newLine1, - unshift: unshift + unshift }) { super(); this.content = content1; @@ -1546,7 +1546,7 @@ constructor({ content: content1, newLine: newLine1, - unshift: unshift + unshift }) { super(); this.content = content1; @@ -3888,8 +3888,8 @@ haveBodyParam = false; // Check for duplicate parameters and separate `this` assignments. paramNames = []; - this.eachParamName(function(name, node, param) { - var target; + this.eachParamName(function(name, node, param, obj) { + var replacement, target; if (indexOf.call(paramNames, name) >= 0) { node.error(`multiple parameters named '${name}'`); } @@ -3899,8 +3899,14 @@ if (indexOf.call(JS_FORBIDDEN, name) >= 0) { name = `_${name}`; } - target = new IdentifierLiteral(o.scope.freeVariable(name)); - param.renameParam(node, target); + target = new IdentifierLiteral(o.scope.freeVariable(name, { + reserve: false + })); + // `Param` is object destructuring with a default value: ({@prop = 1}) -> + // In a case when the variable name is already reserved, we have to assign + // a new variable name to the destructured variable: ({prop:prop1 = 1}) -> + replacement = param.name instanceof Obj && obj instanceof Assign && obj.operatorToken.value === '=' ? new Assign(new IdentifierLiteral(name), target, 'object') : target; //, operatorToken: new Literal ':' + param.renameParam(node, replacement); return thisAssignments.push(new Assign(node, target)); } }); @@ -4321,9 +4327,9 @@ // `name` is the name of the parameter and `node` is the AST node corresponding // to that name. eachName(iterator, name = this.name) { - var atParam, j, len1, node, obj, ref1, ref2; - atParam = (obj) => { - return iterator(`@${obj.properties[0].name.value}`, obj, this); + var atParam, j, len1, nObj, node, obj, ref1, ref2; + atParam = (obj, originalObj = null) => { + return iterator(`@${obj.properties[0].name.value}`, obj, this, originalObj); }; if (name instanceof Literal) { // * simple literals `foo` @@ -4336,6 +4342,8 @@ ref2 = (ref1 = name.objects) != null ? ref1 : []; for (j = 0, len1 = ref2.length; j < len1; j++) { obj = ref2[j]; + // Save original obj. + nObj = obj; // * destructured parameter with default value if (obj instanceof Assign && (obj.context == null)) { obj = obj.variable; @@ -4359,7 +4367,7 @@ this.eachName(iterator, obj.base); // * at-params within destructured parameters `{@foo}` } else if (obj.this) { - atParam(obj); + atParam(obj, nObj); } else { // * simple destructured parameters {foo} iterator(obj.base.value, obj.base, this); @@ -4386,7 +4394,15 @@ if (node.this) { key = node.properties[0].name; } - return new Assign(new Value(key), newNode, 'object'); + // No need to assign a new variable for the destructured variable if the variable isn't reserved. + // Examples: + // `({@foo}) ->` should compile to `({foo}) { this.foo = foo}` + // `foo = 1; ({@foo}) ->` should compile to `foo = 1; ({foo:foo1}) { this.foo = foo1 }` + if (node.this && key.value === newNode.value) { + return new Value(newNode); + } else { + return new Assign(new Value(key), newNode, 'object'); + } } else { return newNode; } diff --git a/src/nodes.coffee b/src/nodes.coffee index d7b55026..90359a14 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2605,14 +2605,24 @@ exports.Code = class Code extends Base # Check for duplicate parameters and separate `this` assignments. paramNames = [] - @eachParamName (name, node, param) -> + @eachParamName (name, node, param, obj) -> node.error "multiple parameters named '#{name}'" if name in paramNames paramNames.push name + if node.this name = node.properties[0].name.value name = "_#{name}" if name in JS_FORBIDDEN - target = new IdentifierLiteral o.scope.freeVariable name - param.renameParam node, target + target = new IdentifierLiteral o.scope.freeVariable name, reserve: no + # `Param` is object destructuring with a default value: ({@prop = 1}) -> + # In a case when the variable name is already reserved, we have to assign + # a new variable name to the destructured variable: ({prop:prop1 = 1}) -> + replacement = + if param.name instanceof Obj and obj instanceof Assign and + obj.operatorToken.value is '=' + new Assign (new IdentifierLiteral name), target, 'object' #, operatorToken: new Literal ':' + else + target + param.renameParam node, replacement thisAssignments.push new Assign node, target # Parse the parameters, adding them to the list of parameters to put in the @@ -2901,12 +2911,14 @@ exports.Param = class Param extends Base # `name` is the name of the parameter and `node` is the AST node corresponding # to that name. eachName: (iterator, name = @name) -> - atParam = (obj) => iterator "@#{obj.properties[0].name.value}", obj, @ + atParam = (obj, originalObj = null) => iterator "@#{obj.properties[0].name.value}", obj, @, originalObj # * simple literals `foo` return iterator name.value, name, @ if name instanceof Literal # * at-params `@foo` return atParam name if name instanceof Value for obj in name.objects ? [] + # Save original obj. + nObj = obj # * destructured parameter with default value if obj instanceof Assign and not obj.context? obj = obj.variable @@ -2928,7 +2940,7 @@ exports.Param = class Param extends Base @eachName iterator, obj.base # * at-params within destructured parameters `{@foo}` else if obj.this - atParam obj + atParam obj, nObj # * simple destructured parameters {foo} else iterator obj.base.value, obj.base, @ else if obj instanceof Elision @@ -2945,7 +2957,14 @@ exports.Param = class Param extends Base if parent instanceof Obj key = node key = node.properties[0].name if node.this - new Assign new Value(key), newNode, 'object' + # No need to assign a new variable for the destructured variable if the variable isn't reserved. + # Examples: + # `({@foo}) ->` should compile to `({foo}) { this.foo = foo}` + # `foo = 1; ({@foo}) ->` should compile to `foo = 1; ({foo:foo1}) { this.foo = foo1 }` + if node.this and key.value is newNode.value + new Value newNode + else + new Assign new Value(key), newNode, 'object' else newNode diff --git a/test/function_invocation.coffee b/test/function_invocation.coffee index 78255fa4..65a9821d 100644 --- a/test/function_invocation.coffee +++ b/test/function_invocation.coffee @@ -329,6 +329,15 @@ test "Simple Destructuring function arguments with same-named variables in scope eq f([2]), 2 eq x, 1 +test "#4843: Bad output when assigning to @prop in destructuring assignment with defaults", -> + works = "maybe" + drinks = "beer" + class A + constructor: ({@works = 'no', @drinks = 'wine'}) -> + a = new A {works: 'yes', drinks: 'coffee'} + eq a.works, 'yes' + eq a.drinks, 'coffee' + test "caching base value", -> obj =