From becdf504ce8a5ca0032d13fb46f6b2a20f9a38cd Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Tue, 29 Aug 2017 20:56:17 -0700 Subject: [PATCH 1/3] Fix #4464: backticked expressions in class body should be left in the body, not hoisted --- lib/coffeescript/nodes.js | 34 +++++++++++++++++++---------- src/nodes.coffee | 45 +++++++++++++++++++++++---------------- test/classes.coffee | 15 +++++++++++++ 3 files changed, 65 insertions(+), 29 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 4a9e1f4a13..a7b5c95107 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2466,17 +2466,17 @@ } compileNode(o) { - var executableBody, node, parentName; + var node, parentName; this.name = this.determineName(); - executableBody = this.walkBody(); + this.walkBody(); if (this.parent instanceof Value && !this.parent.hasProperties()) { // Special handling to allow `class expr.A extends A` declarations parentName = this.parent.base.value; } this.hasNameClash = (this.name != null) && this.name === parentName; node = this; - if (executableBody || this.hasNameClash) { - node = new ExecutableClassBody(node, executableBody); + if (this.executableBody || this.hasNameClash) { + node = new ExecutableClassBody(node, this.executableBody); } else if ((this.name == null) && o.level === LEVEL_TOP) { // Anonymous classes are only valid in expressions node = new Parens(node); @@ -2523,10 +2523,18 @@ result.push(this.makeCode('extends '), ...this.parent.compileToFragments(o), this.makeCode(' ')); } result.push(this.makeCode('{')); - if (!this.body.isEmpty()) { + if (!(this.passthroughBody.isEmpty() && this.body.isEmpty())) { this.body.spaced = true; result.push(this.makeCode('\n')); - result.push(...this.body.compileToFragments(o, LEVEL_TOP)); + if (!this.passthroughBody.isEmpty()) { + result.push(...this.passthroughBody.compileToFragments(o, LEVEL_TOP)); + if (!this.body.isEmpty()) { + result.push(this.makeCode('\n\n')); + } + } + if (!this.body.isEmpty()) { + result.push(...this.body.compileToFragments(o, LEVEL_TOP)); + } result.push(this.makeCode(`\n${this.tab}`)); } result.push(this.makeCode('}')); @@ -2559,16 +2567,16 @@ } walkBody() { - var assign, end, executableBody, expression, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, properties, pushSlice, ref1, start; + var assign, end, expression, expressionIndex, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, passthroughBodyExpressions, properties, pushSlice, ref1, start; this.ctor = null; this.boundMethods = []; - executableBody = null; initializer = []; + passthroughBodyExpressions = []; ({expressions} = this.body); i = 0; ref1 = expressions.slice(); - for (j = 0, len1 = ref1.length; j < len1; j++) { - expression = ref1[j]; + for (expressionIndex = j = 0, len1 = ref1.length; j < len1; expressionIndex = ++j) { + expression = ref1[expressionIndex]; if (expression instanceof Value && expression.isObject(true)) { ({properties} = expression.base); exprs = []; @@ -2591,6 +2599,9 @@ pushSlice(); splice.apply(expressions, [i, i - i + 1].concat(exprs)), exprs; i += exprs.length; + } else if (expression instanceof Value && expression.base instanceof PassthroughLiteral) { + passthroughBodyExpressions.push(expression); + expressions.splice(expressionIndex, 1); } else { if (initializerExpression = this.addInitializerExpression(expression)) { initializer.push(initializerExpression); @@ -2614,6 +2625,7 @@ } } } + this.passthroughBody = new Block(passthroughBodyExpressions); if (initializer.length !== expressions.length) { this.body.expressions = (function() { var l, len3, results; @@ -2624,7 +2636,7 @@ } return results; })(); - return new Block(expressions); + return this.executableBody = new Block(expressions); } } diff --git a/src/nodes.coffee b/src/nodes.coffee index ba03134295..e6f1940421 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1636,17 +1636,16 @@ exports.Class = class Class extends Base super() compileNode: (o) -> - @name = @determineName() - executableBody = @walkBody() + @name = @determineName() + @walkBody() # Special handling to allow `class expr.A extends A` declarations parentName = @parent.base.value if @parent instanceof Value and not @parent.hasProperties() @hasNameClash = @name? and @name is parentName node = @ - - if executableBody or @hasNameClash - node = new ExecutableClassBody node, executableBody + if @executableBody or @hasNameClash + node = new ExecutableClassBody node, @executableBody else if not @name? and o.level is LEVEL_TOP # Anonymous classes are only valid in expressions node = new Parens node @@ -1666,7 +1665,7 @@ exports.Class = class Class extends Base compileClassDeclaration: (o) -> @ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length - @ctor?.noReturn = true + @ctor?.noReturn = yes @proxyBoundMethods() if @boundMethods.length @@ -1678,10 +1677,14 @@ exports.Class = class Class extends Base result.push @makeCode('extends '), @parent.compileToFragments(o)..., @makeCode ' ' if @parent result.push @makeCode '{' - unless @body.isEmpty() - @body.spaced = true + unless @passthroughBody.isEmpty() and @body.isEmpty() + @body.spaced = yes result.push @makeCode '\n' - result.push @body.compileToFragments(o, LEVEL_TOP)... + unless @passthroughBody.isEmpty() + result.push @passthroughBody.compileToFragments(o, LEVEL_TOP)... + result.push @makeCode '\n\n' unless @body.isEmpty() + unless @body.isEmpty() + result.push @body.compileToFragments(o, LEVEL_TOP)... result.push @makeCode "\n#{@tab}" result.push @makeCode '}' @@ -1704,21 +1707,21 @@ exports.Class = class Class extends Base if name in JS_FORBIDDEN then "_#{name}" else name walkBody: -> - @ctor = null - @boundMethods = [] - executableBody = null + @ctor = null + @boundMethods = [] - initializer = [] - { expressions } = @body + initializer = [] + passthroughBodyExpressions = [] + { expressions } = @body i = 0 - for expression in expressions.slice() - if expression instanceof Value and expression.isObject true + for expression, expressionIndex in expressions.slice() + if expression instanceof Value and expression.isObject yes { properties } = expression.base exprs = [] end = 0 start = 0 - pushSlice = -> exprs.push new Value new Obj properties[start...end], true if end > start + pushSlice = -> exprs.push new Value new Obj properties[start...end], yes if end > start while assign = properties[end] if initializerExpression = @addInitializerExpression assign @@ -1731,6 +1734,9 @@ exports.Class = class Class extends Base expressions[i..i] = exprs i += exprs.length + else if expression instanceof Value and expression.base instanceof PassthroughLiteral + passthroughBodyExpressions.push expression + expressions.splice expressionIndex, 1 else if initializerExpression = @addInitializerExpression expression initializer.push initializerExpression @@ -1746,9 +1752,12 @@ exports.Class = class Class extends Base else if method.bound @boundMethods.push method + @passthroughBody = new Block passthroughBodyExpressions + if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) - new Block expressions + @executableBody = new Block expressions + # Add an expression to the class initializer # diff --git a/test/classes.coffee b/test/classes.coffee index c4366f215a..75474b9069 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1833,3 +1833,18 @@ test "#4591: super.x.y, super['x'].y", -> eq 2, b.t eq 2, b.s eq 2, b.r + +test "#4464: backticked expressions in class body", -> + class A + `get x() { return 42; }` + + class B + `get x() { return 42; }` + constructor: -> + @y = 84 + + a = new A + eq 42, a.x + b = new B + eq 42, b.x + eq 84, b.y From 1d3af8c4327791caa30f86d287c34036720b7254 Mon Sep 17 00:00:00 2001 From: Chris Connelly Date: Wed, 20 Sep 2017 15:58:03 +0100 Subject: [PATCH 2/3] Simplify fix for #4464 This uses more of the existing machinery for moving class body expressions into the initializer. --- lib/coffeescript/nodes.js | 41 +++++++++++------------------- src/nodes.coffee | 52 ++++++++++++++++----------------------- 2 files changed, 35 insertions(+), 58 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index a7b5c95107..60b4565c52 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2466,17 +2466,17 @@ } compileNode(o) { - var node, parentName; + var executableBody, node, parentName; this.name = this.determineName(); - this.walkBody(); + executableBody = this.walkBody(); if (this.parent instanceof Value && !this.parent.hasProperties()) { // Special handling to allow `class expr.A extends A` declarations parentName = this.parent.base.value; } this.hasNameClash = (this.name != null) && this.name === parentName; node = this; - if (this.executableBody || this.hasNameClash) { - node = new ExecutableClassBody(node, this.executableBody); + if (executableBody || this.hasNameClash) { + node = new ExecutableClassBody(node, executableBody); } else if ((this.name == null) && o.level === LEVEL_TOP) { // Anonymous classes are only valid in expressions node = new Parens(node); @@ -2523,18 +2523,10 @@ result.push(this.makeCode('extends '), ...this.parent.compileToFragments(o), this.makeCode(' ')); } result.push(this.makeCode('{')); - if (!(this.passthroughBody.isEmpty() && this.body.isEmpty())) { + if (!this.body.isEmpty()) { this.body.spaced = true; result.push(this.makeCode('\n')); - if (!this.passthroughBody.isEmpty()) { - result.push(...this.passthroughBody.compileToFragments(o, LEVEL_TOP)); - if (!this.body.isEmpty()) { - result.push(this.makeCode('\n\n')); - } - } - if (!this.body.isEmpty()) { - result.push(...this.body.compileToFragments(o, LEVEL_TOP)); - } + result.push(...this.body.compileToFragments(o, LEVEL_TOP)); result.push(this.makeCode(`\n${this.tab}`)); } result.push(this.makeCode('}')); @@ -2567,16 +2559,16 @@ } walkBody() { - var assign, end, expression, expressionIndex, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, passthroughBodyExpressions, properties, pushSlice, ref1, start; + var assign, end, executableBody, expression, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, properties, pushSlice, ref1, start; this.ctor = null; this.boundMethods = []; + executableBody = null; initializer = []; - passthroughBodyExpressions = []; ({expressions} = this.body); i = 0; ref1 = expressions.slice(); - for (expressionIndex = j = 0, len1 = ref1.length; j < len1; expressionIndex = ++j) { - expression = ref1[expressionIndex]; + for (j = 0, len1 = ref1.length; j < len1; j++) { + expression = ref1[j]; if (expression instanceof Value && expression.isObject(true)) { ({properties} = expression.base); exprs = []; @@ -2599,9 +2591,6 @@ pushSlice(); splice.apply(expressions, [i, i - i + 1].concat(exprs)), exprs; i += exprs.length; - } else if (expression instanceof Value && expression.base instanceof PassthroughLiteral) { - passthroughBodyExpressions.push(expression); - expressions.splice(expressionIndex, 1); } else { if (initializerExpression = this.addInitializerExpression(expression)) { initializer.push(initializerExpression); @@ -2625,7 +2614,6 @@ } } } - this.passthroughBody = new Block(passthroughBodyExpressions); if (initializer.length !== expressions.length) { this.body.expressions = (function() { var l, len3, results; @@ -2636,16 +2624,15 @@ } return results; })(); - return this.executableBody = new Block(expressions); + return new Block(expressions); } } // Add an expression to the class initializer - - // NOTE Currently, only methods and static methods are valid in ES class initializers. - // When additional expressions become valid, this method should be updated to handle them. addInitializerExpression(node) { - if (this.validInitializerMethod(node)) { + if (node.unwrapAll() instanceof PassthroughLiteral) { + return node; + } else if (this.validInitializerMethod(node)) { return this.addInitializerMethod(node); } else { return null; diff --git a/src/nodes.coffee b/src/nodes.coffee index e6f1940421..eedd29bb4b 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1636,16 +1636,17 @@ exports.Class = class Class extends Base super() compileNode: (o) -> - @name = @determineName() - @walkBody() + @name = @determineName() + executableBody = @walkBody() # Special handling to allow `class expr.A extends A` declarations parentName = @parent.base.value if @parent instanceof Value and not @parent.hasProperties() @hasNameClash = @name? and @name is parentName node = @ - if @executableBody or @hasNameClash - node = new ExecutableClassBody node, @executableBody + + if executableBody or @hasNameClash + node = new ExecutableClassBody node, executableBody else if not @name? and o.level is LEVEL_TOP # Anonymous classes are only valid in expressions node = new Parens node @@ -1665,7 +1666,7 @@ exports.Class = class Class extends Base compileClassDeclaration: (o) -> @ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length - @ctor?.noReturn = yes + @ctor?.noReturn = true @proxyBoundMethods() if @boundMethods.length @@ -1677,14 +1678,10 @@ exports.Class = class Class extends Base result.push @makeCode('extends '), @parent.compileToFragments(o)..., @makeCode ' ' if @parent result.push @makeCode '{' - unless @passthroughBody.isEmpty() and @body.isEmpty() - @body.spaced = yes + unless @body.isEmpty() + @body.spaced = true result.push @makeCode '\n' - unless @passthroughBody.isEmpty() - result.push @passthroughBody.compileToFragments(o, LEVEL_TOP)... - result.push @makeCode '\n\n' unless @body.isEmpty() - unless @body.isEmpty() - result.push @body.compileToFragments(o, LEVEL_TOP)... + result.push @body.compileToFragments(o, LEVEL_TOP)... result.push @makeCode "\n#{@tab}" result.push @makeCode '}' @@ -1707,21 +1704,21 @@ exports.Class = class Class extends Base if name in JS_FORBIDDEN then "_#{name}" else name walkBody: -> - @ctor = null - @boundMethods = [] + @ctor = null + @boundMethods = [] + executableBody = null - initializer = [] - passthroughBodyExpressions = [] - { expressions } = @body + initializer = [] + { expressions } = @body i = 0 - for expression, expressionIndex in expressions.slice() - if expression instanceof Value and expression.isObject yes + for expression in expressions.slice() + if expression instanceof Value and expression.isObject true { properties } = expression.base exprs = [] end = 0 start = 0 - pushSlice = -> exprs.push new Value new Obj properties[start...end], yes if end > start + pushSlice = -> exprs.push new Value new Obj properties[start...end], true if end > start while assign = properties[end] if initializerExpression = @addInitializerExpression assign @@ -1734,9 +1731,6 @@ exports.Class = class Class extends Base expressions[i..i] = exprs i += exprs.length - else if expression instanceof Value and expression.base instanceof PassthroughLiteral - passthroughBodyExpressions.push expression - expressions.splice expressionIndex, 1 else if initializerExpression = @addInitializerExpression expression initializer.push initializerExpression @@ -1752,19 +1746,15 @@ exports.Class = class Class extends Base else if method.bound @boundMethods.push method - @passthroughBody = new Block passthroughBodyExpressions - if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) - @executableBody = new Block expressions - + new Block expressions # Add an expression to the class initializer - # - # NOTE Currently, only methods and static methods are valid in ES class initializers. - # When additional expressions become valid, this method should be updated to handle them. addInitializerExpression: (node) -> - if @validInitializerMethod node + if node.unwrapAll() instanceof PassthroughLiteral + node + else if @validInitializerMethod node @addInitializerMethod node else null From 921eb3f09450268374786e3e0b1226e528573c7d Mon Sep 17 00:00:00 2001 From: Chris Connelly Date: Wed, 20 Sep 2017 16:18:51 +0100 Subject: [PATCH 3/3] Clarify the purpose of Class::addInitializerExpression --- lib/coffeescript/nodes.js | 4 ++++ src/nodes.coffee | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 60b4565c52..b179c96261 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2629,6 +2629,10 @@ } // Add an expression to the class initializer + // This is the key method for determining whether an expression in a class body should appear in + // the initializer or the executable body. If the given `node` is valid in a class body the method + // will return a (new, modified, or identical) node for inclusion in the class initializer, + // otherwise nothing will be returned and the node will appear in the executable body. addInitializerExpression(node) { if (node.unwrapAll() instanceof PassthroughLiteral) { return node; diff --git a/src/nodes.coffee b/src/nodes.coffee index eedd29bb4b..bab1ee8276 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1751,6 +1751,10 @@ exports.Class = class Class extends Base new Block expressions # Add an expression to the class initializer + # This is the key method for determining whether an expression in a class body should appear in + # the initializer or the executable body. If the given `node` is valid in a class body the method + # will return a (new, modified, or identical) node for inclusion in the class initializer, + # otherwise nothing will be returned and the node will appear in the executable body. addInitializerExpression: (node) -> if node.unwrapAll() instanceof PassthroughLiteral node