From 3a6ffa6a8595446c9da33869a872f9d6fb59ba9c Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Mon, 14 Aug 2017 02:42:01 +0000 Subject: [PATCH 1/3] Clean up function parameter compilation to get name for scope --- lib/coffeescript/nodes.js | 29 +++++++++++++++-------------- src/nodes.coffee | 12 +++++++----- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index ad9c6e5b94..5e93d0a3e3 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -145,6 +145,10 @@ return fragments; } + compileToFragmentsWithoutComments(o, lvl) { + return this.compileWithoutComments(o, lvl, 'compileToFragments'); + } + // Statements converted into expressions via closure-wrapping share a scope // object with their parent closure, to preserve the expected lexical scope. compileClosure(o) { @@ -3671,7 +3675,7 @@ // parameters after the splat, they are declared via expressions in the // function body. compileNode(o) { - var answer, body, boundMethodCheck, comment, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, l, len1, len2, len3, m, methodScope, modifiers, name, param, paramNames, paramToAddToScope, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, ref6, ref7, ref8, salvagedComments, signature, splatParamName, thisAssignments, wasEmpty; + var answer, body, boundMethodCheck, comment, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, l, len1, len2, len3, m, methodScope, modifiers, name, param, paramNames, paramToAddToScope, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, ref6, ref7, signature, splatParamName, thisAssignments, wasEmpty; if (this.ctor) { if (this.isAsync) { this.name.error('Class constructor may not be async'); @@ -3823,14 +3827,7 @@ // compilation, so that they get output the “real” time this param // is compiled. paramToAddToScope = param.value != null ? param : ref; - if ((ref5 = paramToAddToScope.name) != null ? ref5.comments : void 0) { - salvagedComments = paramToAddToScope.name.comments; - delete paramToAddToScope.name.comments; - } - o.scope.parameter(fragmentsToText(paramToAddToScope.compileToFragments(o))); - if (salvagedComments) { - paramToAddToScope.name.comments = salvagedComments; - } + o.scope.parameter(fragmentsToText(paramToAddToScope.compileToFragmentsWithoutComments(o))); } params.push(ref); } else { @@ -3843,7 +3840,7 @@ ifTrue = new Assign(new Value(param.name), param.value); exprs.push(new If(condition, ifTrue)); } - if (((ref6 = param.name) != null ? ref6.value : void 0) != null) { + if (((ref5 = param.name) != null ? ref5.value : void 0) != null) { // Add this parameter to the scope, since it wouldn’t have been added // yet since it was skipped earlier. o.scope.add(param.name.value, 'var', true); @@ -3907,10 +3904,10 @@ } signature.push(this.makeCode(')')); // Block comments between `)` and `->`/`=>` get output between `)` and `{`. - if (((ref7 = this.funcGlyph) != null ? ref7.comments : void 0) != null) { - ref8 = this.funcGlyph.comments; - for (l = 0, len3 = ref8.length; l < len3; l++) { - comment = ref8[l]; + if (((ref6 = this.funcGlyph) != null ? ref6.comments : void 0) != null) { + ref7 = this.funcGlyph.comments; + for (l = 0, len3 = ref7.length; l < len3; l++) { + comment = ref7[l]; comment.unshift = false; } this.compileCommentFragments(o, this.funcGlyph, signature); @@ -4068,6 +4065,10 @@ return this.name.compileToFragments(o, LEVEL_LIST); } + compileToFragmentsWithoutComments(o) { + return this.name.compileToFragmentsWithoutComments(o, LEVEL_LIST); + } + asReference(o) { var name, node; if (this.reference) { diff --git a/src/nodes.coffee b/src/nodes.coffee index 677cf27ab1..cb335b1a37 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -107,6 +107,9 @@ exports.Base = class Base @compileCommentFragments o, node, fragments fragments + compileToFragmentsWithoutComments: (o, lvl) -> + @compileWithoutComments o, lvl, 'compileToFragments' + # Statements converted into expressions via closure-wrapping share a scope # object with their parent closure, to preserve the expected lexical scope. compileClosure: (o) -> @@ -2626,11 +2629,7 @@ exports.Code = class Code extends Base # compilation, so that they get output the “real” time this param # is compiled. paramToAddToScope = if param.value? then param else ref - if paramToAddToScope.name?.comments - salvagedComments = paramToAddToScope.name.comments - delete paramToAddToScope.name.comments - o.scope.parameter fragmentsToText paramToAddToScope.compileToFragments o - paramToAddToScope.name.comments = salvagedComments if salvagedComments + o.scope.parameter fragmentsToText paramToAddToScope.compileToFragmentsWithoutComments o params.push ref else paramsAfterSplat.push param @@ -2774,6 +2773,9 @@ exports.Param = class Param extends Base compileToFragments: (o) -> @name.compileToFragments o, LEVEL_LIST + compileToFragmentsWithoutComments: (o) -> + @name.compileToFragmentsWithoutComments o, LEVEL_LIST + asReference: (o) -> return @reference if @reference node = @name From 52795587ec9237f91c6ad01ffd04bb734e61cb0d Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Mon, 14 Aug 2017 02:56:58 +0000 Subject: [PATCH 2/3] If compiling a function parameter creates any generated variables (e.g. `ref`), shift the declarations for those variables into the parent scope; fixes #4413 --- lib/coffeescript/nodes.js | 10 +++++++++- src/nodes.coffee | 7 +++++++ test/functions.coffee | 9 +++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 5e93d0a3e3..1d5609bd96 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -3675,7 +3675,7 @@ // parameters after the splat, they are declared via expressions in the // function body. compileNode(o) { - var answer, body, boundMethodCheck, comment, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, l, len1, len2, len3, m, methodScope, modifiers, name, param, paramNames, paramToAddToScope, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, ref6, ref7, signature, splatParamName, thisAssignments, wasEmpty; + var answer, body, boundMethodCheck, comment, condition, exprs, generatedVariables, haveBodyParam, haveSplatParam, i, ifTrue, j, k, l, len1, len2, len3, m, methodScope, modifiers, name, param, paramNames, paramToAddToScope, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, ref6, ref7, scopeVariablesCount, signature, splatParamName, thisAssignments, wasEmpty; if (this.ctor) { if (this.isAsync) { this.name.error('Class constructor may not be async'); @@ -3900,7 +3900,15 @@ if (haveSplatParam && i === params.length - 1) { signature.push(this.makeCode('...')); } + // Compile this parameter, but if any generated variables get created + // (e.g. `ref`), shift those into the parent scope since we can’t put a + // `var` line inside a function parameter list. + scopeVariablesCount = o.scope.variables.length; signature.push(...param.compileToFragments(o)); + if (scopeVariablesCount !== o.scope.variables.length) { + generatedVariables = o.scope.variables.splice(scopeVariablesCount); + o.scope.parent.variables.push(...generatedVariables); + } } signature.push(this.makeCode(')')); // Block comments between `)` and `->`/`=>` get output between `)` and `{`. diff --git a/src/nodes.coffee b/src/nodes.coffee index cb335b1a37..677d4df028 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2674,7 +2674,14 @@ exports.Code = class Code extends Base for param, i in params signature.push @makeCode ', ' if i isnt 0 signature.push @makeCode '...' if haveSplatParam and i is params.length - 1 + # Compile this parameter, but if any generated variables get created + # (e.g. `ref`), shift those into the parent scope since we can’t put a + # `var` line inside a function parameter list. + scopeVariablesCount = o.scope.variables.length signature.push param.compileToFragments(o)... + if scopeVariablesCount isnt o.scope.variables.length + generatedVariables = o.scope.variables.splice scopeVariablesCount + o.scope.parent.variables.push generatedVariables... signature.push @makeCode ')' # Block comments between `)` and `->`/`=>` get output between `)` and `{`. if @funcGlyph?.comments? diff --git a/test/functions.coffee b/test/functions.coffee index 47fa9ae305..148606558a 100644 --- a/test/functions.coffee +++ b/test/functions.coffee @@ -471,3 +471,12 @@ test "#3845/#3446: chain after function glyph", -> doThing() .then (@result) => .catch handleError + +test "#4413: expressions in function parameters that create generated variables have those variables declared correctly", -> + 'use strict' + # We’re in strict mode because we want an error to be thrown if the generated + # variable (`ref`) is assigned before being declared. + foo = -> null + bar = -> 33 + f = (a = foo() ? bar()) -> a + eq f(), 33 From 911c21f7bee271f14d7b6aacf2579c0c06fa1eee Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Mon, 14 Aug 2017 03:06:38 +0000 Subject: [PATCH 3/3] Update test to prove that there's no collision in generated variables --- test/functions.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/functions.coffee b/test/functions.coffee index 148606558a..a94ed0529c 100644 --- a/test/functions.coffee +++ b/test/functions.coffee @@ -479,4 +479,6 @@ test "#4413: expressions in function parameters that create generated variables foo = -> null bar = -> 33 f = (a = foo() ? bar()) -> a + g = (a = foo() ? bar()) -> a + 1 eq f(), 33 + eq g(), 34