From a6165e5d58c1788f3d8ae56339d149bc0f1045d5 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 14:19:44 -0500 Subject: [PATCH 01/12] bound method runtime check --- lib/coffeescript/nodes.js | 38 ++++++++++++++++++++++++++++++++++---- src/nodes.coffee | 28 ++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 6cddaa6a82..a6aa259de6 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1741,7 +1741,7 @@ compileClassDeclaration(o) { var ref1, result; - if (this.externalCtor) { + if (this.externalCtor || this.boundMethods.length) { if (this.ctor == null) { this.ctor = this.makeDefaultConstructor(); } @@ -1749,6 +1749,9 @@ if ((ref1 = this.ctor) != null) { ref1.noReturn = true; } + if (this.boundMethods.length) { + this.proxyBoundMethods(o); + } o.indent += TAB; result = []; result.push(this.makeCode("class ")); @@ -1796,6 +1799,7 @@ walkBody() { 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 = []; ({expressions} = this.body); @@ -1849,6 +1853,9 @@ this.ctor = method; } else if (method.isStatic && method.bound) { method.context = this.name; + } else if (method.bound) { + this.boundMethods.push(method.name); + method.constructorName = this.name; } } } @@ -1904,8 +1911,8 @@ if (methodName.value === 'constructor') { method.ctor = (this.parent ? 'derived' : 'base'); } - if (method.bound) { - method.error('Methods cannot be bound functions'); + if (method.bound && method.ctor) { + method.error('Cannot define a constructor as a bound function'); } } return method; @@ -1927,6 +1934,22 @@ return ctor; } + proxyBoundMethods(o) { + var name; + this.ctor.thisAssignments = (function() { + var j, ref1, results; + ref1 = this.boundMethods; + results = []; + for (j = ref1.length - 1; j >= 0; j += -1) { + name = ref1[j]; + name = new Value(new ThisLiteral, [name]).compile(o); + results.push(new Literal(`${name} = ${name}.bind(this)`)); + } + return results; + }).call(this); + return null; + } + }; Class.prototype.children = ['variable', 'parent', 'body']; @@ -2661,7 +2684,7 @@ } compileNode(o) { - var answer, body, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, signature, splatParamName, thisAssignments, wasEmpty; + var _boundMethodCheck, answer, body, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, signature, splatParamName, thisAssignments, wasEmpty; if (this.ctor) { if (this.isAsync) { this.name.error('Class constructor may not be async'); @@ -2805,6 +2828,10 @@ this.body.expressions.unshift(...thisAssignments); } this.body.expressions.unshift(...exprs); + if (this.isMethod && this.bound && !this.isStatic && this.constructorName) { + _boundMethodCheck = new Value(new Literal(utility('_boundMethodCheck', o))); + this.body.expressions.unshift(new Call(_boundMethodCheck, [new Value(new ThisLiteral), new Value(new Literal(this.constructorName))])); + } if (!(wasEmpty || this.noReturn)) { this.body.makeReturn(); } @@ -4161,6 +4188,9 @@ modulo: function() { return 'function(a, b) { return (+a % (b = +b) + b) % b; }'; }, + _boundMethodCheck: function() { + return "function(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new Error('Bound instance method accessed before binding') } }"; + }, hasProp: function() { return '{}.hasOwnProperty'; }, diff --git a/src/nodes.coffee b/src/nodes.coffee index cfa84b45e2..0765a2f5eb 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1279,9 +1279,11 @@ exports.Class = class Class extends Base result compileClassDeclaration: (o) -> - @ctor ?= @makeDefaultConstructor() if @externalCtor + @ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length @ctor?.noReturn = true + @proxyBoundMethods o if @boundMethods.length + o.indent += TAB result = [] @@ -1317,6 +1319,7 @@ exports.Class = class Class extends Base walkBody: -> @ctor = null + @boundMethods = [] executableBody = null initializer = [] @@ -1362,6 +1365,9 @@ exports.Class = class Class extends Base @ctor = method else if method.isStatic and method.bound method.context = @name + else if method.bound + @boundMethods.push method.name + method.constructorName = @name if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) @@ -1399,7 +1405,8 @@ exports.Class = class Class extends Base method.name = new (if methodName.shouldCache() then Index else Access) methodName method.name.updateLocationDataIfMissing methodName.locationData method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor' - method.error 'Methods cannot be bound functions' if method.bound + method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor + # method.error 'Cannot define a bound method in an anonymous class' if method.bound and not @name method @@ -1418,6 +1425,13 @@ exports.Class = class Class extends Base ctor + proxyBoundMethods: (o) -> + @ctor.thisAssignments = for name in @boundMethods by -1 + name = new Value(new ThisLiteral, [ name ]).compile o + new Literal "#{name} = #{name}.bind(this)" + + null + exports.ExecutableClassBody = class ExecutableClassBody extends Base children: [ 'class', 'body' ] @@ -2140,6 +2154,9 @@ exports.Code = class Code extends Base wasEmpty = @body.isEmpty() @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments @body.expressions.unshift exprs... + if @isMethod and @bound and not @isStatic and @constructorName + _boundMethodCheck = new Value new Literal utility '_boundMethodCheck', o + @body.expressions.unshift new Call(_boundMethodCheck, [new Value(new ThisLiteral), new Value new Literal @constructorName]) @body.makeReturn() unless wasEmpty or @noReturn # Assemble the output @@ -3097,6 +3114,13 @@ exports.If = class If extends Base UTILITIES = modulo: -> 'function(a, b) { return (+a % (b = +b) + b) % b; }' + _boundMethodCheck: -> " + function(instance, Constructor) { + if (!(instance instanceof Constructor)) { + throw new Error('Bound instance method accessed before binding') + } + } + " # Shortcuts to speed up the lookup time for native functions. hasProp: -> '{}.hasOwnProperty' From e2ee01606c7c6b2410751220541d971dec3e3435 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 15:10:41 -0500 Subject: [PATCH 02/12] restore bound method tests --- lib/coffeescript/nodes.js | 2 +- src/nodes.coffee | 2 +- test/assignment.coffee | 5 +- test/classes.coffee | 104 ++++++++++++++++++++++++++++++++++++-- test/scope.coffee | 10 ++++ 5 files changed, 115 insertions(+), 8 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index a6aa259de6..602c9e6be4 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2049,7 +2049,7 @@ return this.body.traverseChildren(false, (node) => { if (node instanceof ThisLiteral) { return node.value = this.name; - } else if (node instanceof Code && node.bound) { + } else if (node instanceof Code && node.bound && node.isStatic) { return node.context = this.name; } }); diff --git a/src/nodes.coffee b/src/nodes.coffee index 0765a2f5eb..ddc363edd9 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1515,7 +1515,7 @@ exports.ExecutableClassBody = class ExecutableClassBody extends Base @body.traverseChildren false, (node) => if node instanceof ThisLiteral node.value = @name - else if node instanceof Code and node.bound + else if node instanceof Code and node.bound and node.isStatic node.context = @name # Make class/prototype assignments for invalid ES properties diff --git a/test/assignment.coffee b/test/assignment.coffee index ec736fcfd0..c02e9ab804 100644 --- a/test/assignment.coffee +++ b/test/assignment.coffee @@ -562,8 +562,9 @@ test "Assignment to variables similar to helper functions", -> extend = 3 hasProp = 4 value: 5 - method: (bind, bind1) -> [bind, bind1, extend, hasProp, @value] - arrayEq [1, 2, 3, 4, 5], new B().method 1, 2 + method: (bind, bind1) => [bind, bind1, extend, hasProp, @value] + {method} = new B + arrayEq [1, 2, 3, 4, 5], method 1, 2 modulo = -1 %% 3 eq 2, modulo diff --git a/test/classes.coffee b/test/classes.coffee index fbd399e1ca..c0d955389c 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -136,20 +136,42 @@ test "classes with JS-keyword properties", -> ok instance.name() is 'class' -test "Classes with methods that are pre-bound statically, to the class", -> +test "Classes with methods that are pre-bound to the instance, or statically, to the class", -> class Dog constructor: (name) -> @name = name + bark: => + "#{@name} woofs!" + @static = => new this('Dog') + spark = new Dog('Spark') + fido = new Dog('Fido') + fido.bark = spark.bark + + ok fido.bark() is 'Spark woofs!' + obj = func: Dog.static ok obj.func().name is 'Dog' +test "a bound function in a bound function", -> + + class Mini + num: 10 + generate: => + for i in [1..3] + => + @num + + m = new Mini + eq (func() for func in m.generate()).join(' '), '10 10 10' + + test "contructor called with varargs", -> class Connection @@ -454,6 +476,21 @@ test "#1182: execution order needs to be considered as well", -> @B: makeFn 2 constructor: makeFn 3 +test "#1182: external constructors with bound functions", -> + fn = -> + {one: 1} + this + class B + class A + constructor: fn + method: => this instanceof A + ok (new A).method.call(new B) + +test "#1372: bound class methods with reserved names", -> + class C + delete: => + ok C::delete + test "#1380: `super` with reserved names", -> class C do: -> super() @@ -522,7 +559,7 @@ test "#1842: Regression with bound functions within bound class methods", -> @unbound: -> eq this, Store - instance: -> + instance: => ok this instanceof Store Store.bound() @@ -685,6 +722,57 @@ test "extending native objects works with and without defining a constructor", - ok overrideArray instanceof OverrideArray eq 'yes!', overrideArray.method() + +test "#2782: non-alphanumeric-named bound functions", -> + class A + 'b:c': => + 'd' + + eq (new A)['b:c'](), 'd' + + +test "#2781: overriding bound functions", -> + class A + a: -> + @b() + b: => + 1 + + class B extends A + b: => + 2 + + b = (new A).b + eq b(), 1 + + b = (new B).b + eq b(), 2 + + +test "#2791: bound function with destructured argument", -> + class Foo + method: ({a}) => 'Bar' + + eq (new Foo).method({a: 'Bar'}), 'Bar' + + +test "#2796: ditto, ditto, ditto", -> + answer = null + + outsideMethod = (func) -> + func.call message: 'wrong!' + + class Base + constructor: -> + @message = 'right!' + outsideMethod @echo + + echo: => + answer = @message + + new Base + eq answer, 'right!' + test "#3063: Class bodies cannot contain pure statements", -> throws -> CoffeeScript.compile """ class extends S @@ -896,6 +984,9 @@ test "`this` access after `super` in extended classes", -> eq result.super, this eq result.param, @param eq result.method, @method + ok result.method isnt Test::method + + method: => nonce = {} new Test nonce, {} @@ -915,6 +1006,8 @@ test "`@`-params and bound methods with multiple `super` paths (blocks)", -> super 'not param' eq @name, 'not param' eq @param, nonce + ok @method isnt Test::method + method: => new Test true, nonce new Test false, nonce @@ -933,13 +1026,16 @@ test "`@`-params and bound methods with multiple `super` paths (expressions)", - eq (super 'param'), @; eq @name, 'param'; eq @param, nonce; + ok @method isnt Test::method ) else result = ( eq (super 'not param'), @; eq @name, 'not param'; eq @param, nonce; + ok @method isnt Test::method ) + method: => new Test true, nonce new Test false, nonce @@ -1141,7 +1237,7 @@ test "super in a bound function", -> make: -> "Making a #{@drink}" class B extends A - make: (@flavor) -> + make: (@flavor) => super() + " with #{@flavor}" b = new B('Machiato') @@ -1149,7 +1245,7 @@ test "super in a bound function", -> # super in a bound function in a bound function class C extends A - make: (@flavor) -> + make: (@flavor) => func = () => super() + " with #{@flavor}" func() diff --git a/test/scope.coffee b/test/scope.coffee index 82025e49c4..be7299bb9a 100644 --- a/test/scope.coffee +++ b/test/scope.coffee @@ -107,6 +107,16 @@ test "#1183: super + closures", -> ret eq (new B).foo(), 10 +test "#2331: bound super regression", -> + class A + @value = 'A' + method: -> @constructor.value + + class B extends A + method: => super() + + eq (new B).method(), 'A' + test "#3259: leak with @-params within destructured parameters", -> fn = ({@foo}, [@bar], [{@baz}]) -> foo = bar = baz = false From 66b701682c8ab064f7982607f235b02155efae3d Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 15:47:20 -0500 Subject: [PATCH 03/12] bound method tests --- test/classes.coffee | 59 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/classes.coffee b/test/classes.coffee index c0d955389c..fb4f7298ae 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1670,3 +1670,62 @@ test "CS6 Class extends a CS1 compiled class with super()", -> eq B.className(), 'ExtendedCS1' b = new B('three') eq b.make(), "making a cafe ole with caramel and three shots of espresso" + +test 'Bound method called as callback before binding throws runtime error', -> + class Base + constructor: -> + f = @derivedBound + try + f() + ok no + catch e + eq e.message, 'Bound instance method accessed before binding' + + class Derived extends Base + derivedBound: => + ok no + d = new Derived + +test 'Bound method called normally before binding is ok', -> + class Base + constructor: -> + @setProp() + eq @derivedBound(), 3 + + class Derived extends Base + setProp: -> + @prop = 3 + + derivedBound: => + @prop + + d = new Derived + +test 'Bound method called as callback after super() is ok', -> + class Base + + class Derived extends Base + constructor: (@prop = 3) -> + super() + f = @derivedBound + eq f(), 3 + + derivedBound: => + @prop + + d = new Derived + {derivedBound} = d + eq derivedBound(), 3 + +test 'Bound method of base class called as callback is ok', -> + class Base + constructor: (@prop = 3) -> + f = @baseBound + eq f(), 3 + + baseBound: => + @prop + + b = new Base + {baseBound} = b + eq baseBound(), 3 From cb762ce92695f3c32ac1e67d7933eab6cf7d57f2 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 16:01:51 -0500 Subject: [PATCH 04/12] test bound method in prop-named class --- test/classes.coffee | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/classes.coffee b/test/classes.coffee index fb4f7298ae..6f5db1496c 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1729,3 +1729,17 @@ test 'Bound method of base class called as callback is ok', -> b = new Base {baseBound} = b eq baseBound(), 3 + +test 'Bound method of prop-named class called as callback is ok', -> + Hive = {} + class Hive.Bee + constructor: (@prop = 3) -> + f = @baseBound + eq f(), 3 + + baseBound: => + @prop + + b = new Hive.Bee + {baseBound} = b + eq baseBound(), 3 From f82c19da44c6fa35d496f4bc11d596b8a069e186 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 18:16:38 -0500 Subject: [PATCH 05/12] run check against parent class --- lib/coffeescript/nodes.js | 32 +++++++++++++++++++++++++++----- src/nodes.coffee | 26 +++++++++++++++++++++----- test/classes.coffee | 19 +++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 602c9e6be4..b8e6e3ff3b 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1716,7 +1716,7 @@ compileNode(o) { var assign, executableBody, parentName, result; this.name = this.determineName(); - executableBody = this.walkBody(); + executableBody = this.walkBody(o); if (this.parent instanceof Value && !this.parent.hasProperties()) { parentName = this.parent.base.value; } @@ -1754,6 +1754,11 @@ } o.indent += TAB; result = []; + if (this.assignParentRef) { + result.push(this.makeCode("(")); + result.push(...this.assignParentRef.compileToFragments(o)); + result.push(this.makeCode(", ")); + } result.push(this.makeCode("class ")); if (this.name) { result.push(this.makeCode(`${this.name} `)); @@ -1769,6 +1774,9 @@ result.push(this.makeCode(`\n${this.tab}`)); } result.push(this.makeCode('}')); + if (this.assignParentRef) { + result.push(this.makeCode(")")); + } return result; } @@ -1796,7 +1804,20 @@ } } - walkBody() { + setParentRef(o) { + var ref, ref1; + if (this._setParentRef) { + return; + } + this._setParentRef = true; + if ((ref1 = this.parent) != null ? ref1.shouldCache() : void 0) { + ref = new IdentifierLiteral(o.scope.freeVariable('ref')); + this.assignParentRef = new Assign(ref, this.parent); + return this.parent = ref; + } + } + + walkBody(o) { var assign, end, executableBody, expression, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, properties, pushSlice, ref1, start; this.ctor = null; this.boundMethods = []; @@ -1855,7 +1876,8 @@ method.context = this.name; } else if (method.bound) { this.boundMethods.push(method.name); - method.constructorName = this.name; + this.setParentRef(o); + method.parentClass = this.parent; } } } @@ -2828,9 +2850,9 @@ this.body.expressions.unshift(...thisAssignments); } this.body.expressions.unshift(...exprs); - if (this.isMethod && this.bound && !this.isStatic && this.constructorName) { + if (this.isMethod && this.bound && !this.isStatic && this.parentClass) { _boundMethodCheck = new Value(new Literal(utility('_boundMethodCheck', o))); - this.body.expressions.unshift(new Call(_boundMethodCheck, [new Value(new ThisLiteral), new Value(new Literal(this.constructorName))])); + this.body.expressions.unshift(new Call(_boundMethodCheck, [new Value(new ThisLiteral), this.parentClass])); } if (!(wasEmpty || this.noReturn)) { this.body.makeReturn(); diff --git a/src/nodes.coffee b/src/nodes.coffee index ddc363edd9..54ae3cdb85 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1256,7 +1256,7 @@ exports.Class = class Class extends Base compileNode: (o) -> @name = @determineName() - executableBody = @walkBody() + executableBody = @walkBody(o) # Special handling to allow `class expr.A extends A` declarations parentName = @parent.base.value if @parent instanceof Value and not @parent.hasProperties() @@ -1287,6 +1287,10 @@ exports.Class = class Class extends Base o.indent += TAB result = [] + if @assignParentRef + result.push @makeCode "(" + result.push @assignParentRef.compileToFragments(o)... + result.push @makeCode ", " result.push @makeCode "class " result.push @makeCode "#{@name} " if @name result.push @makeCode('extends '), @parent.compileToFragments(o)..., @makeCode ' ' if @parent @@ -1298,6 +1302,8 @@ exports.Class = class Class extends Base result.push @body.compileToFragments(o, LEVEL_TOP)... result.push @makeCode "\n#{@tab}" result.push @makeCode '}' + if @assignParentRef + result.push @makeCode ")" result @@ -1317,7 +1323,16 @@ exports.Class = class Class extends Base @variable.error message if message if name in JS_FORBIDDEN then "_#{name}" else name - walkBody: -> + setParentRef: (o) -> + return if @_setParentRef + @_setParentRef = yes + + if @parent?.shouldCache() + ref = new IdentifierLiteral o.scope.freeVariable 'ref' + @assignParentRef = new Assign ref, @parent + @parent = ref + + walkBody: (o) -> @ctor = null @boundMethods = [] executableBody = null @@ -1367,7 +1382,8 @@ exports.Class = class Class extends Base method.context = @name else if method.bound @boundMethods.push method.name - method.constructorName = @name + @setParentRef(o) + method.parentClass = @parent if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) @@ -2154,9 +2170,9 @@ exports.Code = class Code extends Base wasEmpty = @body.isEmpty() @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments @body.expressions.unshift exprs... - if @isMethod and @bound and not @isStatic and @constructorName + if @isMethod and @bound and not @isStatic and @parentClass _boundMethodCheck = new Value new Literal utility '_boundMethodCheck', o - @body.expressions.unshift new Call(_boundMethodCheck, [new Value(new ThisLiteral), new Value new Literal @constructorName]) + @body.expressions.unshift new Call(_boundMethodCheck, [new Value(new ThisLiteral), @parentClass]) @body.makeReturn() unless wasEmpty or @noReturn # Assemble the output diff --git a/test/classes.coffee b/test/classes.coffee index 6f5db1496c..4f301fbc4f 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1743,3 +1743,22 @@ test 'Bound method of prop-named class called as callback is ok', -> b = new Hive.Bee {baseBound} = b eq baseBound(), 3 + +test 'Bound method of class with expression base class called as callback is ok', -> + calledB = no + B = -> + throw new Error if calledB + calledB = yes + class + class A extends B() + constructor: (@prop = 3) -> + super() + f = @derivedBound + eq f(), 3 + + derivedBound: => + @prop + + b = new A + {derivedBound} = b + eq derivedBound(), 3 From d226da70a62bda56079b8db206e7cc0bcbe5d070 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 18:49:01 -0500 Subject: [PATCH 06/12] dummy commit --- test/classes.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/classes.coffee b/test/classes.coffee index 4f301fbc4f..2f13114664 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1699,7 +1699,7 @@ test 'Bound method called normally before binding is ok', -> derivedBound: => @prop - d = new Derived + d = new Derived test 'Bound method called as callback after super() is ok', -> class Base From 65b009c69ba874b9b9ecb32511549e924f4f1bc8 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 18:55:08 -0500 Subject: [PATCH 07/12] remove comment --- src/nodes.coffee | 1 - test/classes.coffee | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nodes.coffee b/src/nodes.coffee index 54ae3cdb85..5cf37b60e0 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1422,7 +1422,6 @@ exports.Class = class Class extends Base method.name.updateLocationDataIfMissing methodName.locationData method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor' method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor - # method.error 'Cannot define a bound method in an anonymous class' if method.bound and not @name method diff --git a/test/classes.coffee b/test/classes.coffee index 2f13114664..4f301fbc4f 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1699,7 +1699,7 @@ test 'Bound method called normally before binding is ok', -> derivedBound: => @prop - d = new Derived + d = new Derived test 'Bound method called as callback after super() is ok', -> class Base From 019d9fa03a1fb6a4a1134dc0078060725a77b639 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 6 Jun 2017 19:06:27 -0500 Subject: [PATCH 08/12] rename to boundMethodCheck --- lib/coffeescript/nodes.js | 8 ++++---- src/nodes.coffee | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index b8e6e3ff3b..51dfa3284e 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -2706,7 +2706,7 @@ } compileNode(o) { - var _boundMethodCheck, answer, body, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, signature, splatParamName, thisAssignments, wasEmpty; + var answer, body, boundMethodCheck, condition, exprs, haveBodyParam, haveSplatParam, i, ifTrue, j, k, len1, len2, m, methodScope, modifiers, name, param, paramNames, params, paramsAfterSplat, ref, ref1, ref2, ref3, ref4, ref5, signature, splatParamName, thisAssignments, wasEmpty; if (this.ctor) { if (this.isAsync) { this.name.error('Class constructor may not be async'); @@ -2851,8 +2851,8 @@ } this.body.expressions.unshift(...exprs); if (this.isMethod && this.bound && !this.isStatic && this.parentClass) { - _boundMethodCheck = new Value(new Literal(utility('_boundMethodCheck', o))); - this.body.expressions.unshift(new Call(_boundMethodCheck, [new Value(new ThisLiteral), this.parentClass])); + boundMethodCheck = new Value(new Literal(utility('boundMethodCheck', o))); + this.body.expressions.unshift(new Call(boundMethodCheck, [new Value(new ThisLiteral), this.parentClass])); } if (!(wasEmpty || this.noReturn)) { this.body.makeReturn(); @@ -4210,7 +4210,7 @@ modulo: function() { return 'function(a, b) { return (+a % (b = +b) + b) % b; }'; }, - _boundMethodCheck: function() { + boundMethodCheck: function() { return "function(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new Error('Bound instance method accessed before binding') } }"; }, hasProp: function() { diff --git a/src/nodes.coffee b/src/nodes.coffee index 5cf37b60e0..f82bf103e6 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -2170,8 +2170,8 @@ exports.Code = class Code extends Base @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments @body.expressions.unshift exprs... if @isMethod and @bound and not @isStatic and @parentClass - _boundMethodCheck = new Value new Literal utility '_boundMethodCheck', o - @body.expressions.unshift new Call(_boundMethodCheck, [new Value(new ThisLiteral), @parentClass]) + boundMethodCheck = new Value new Literal utility 'boundMethodCheck', o + @body.expressions.unshift new Call(boundMethodCheck, [new Value(new ThisLiteral), @parentClass]) @body.makeReturn() unless wasEmpty or @noReturn # Assemble the output @@ -3129,7 +3129,7 @@ exports.If = class If extends Base UTILITIES = modulo: -> 'function(a, b) { return (+a % (b = +b) + b) % b; }' - _boundMethodCheck: -> " + boundMethodCheck: -> " function(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new Error('Bound instance method accessed before binding') From dba119c8ca9bea618efd3972849fc0a1bbc2ebb1 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Wed, 7 Jun 2017 13:11:17 -0500 Subject: [PATCH 09/12] fixes from code review --- lib/coffeescript/nodes.js | 8 ++++---- src/nodes.coffee | 8 ++++---- test/classes.coffee | 15 --------------- test/error_messages.coffee | 15 +++++++++++++++ 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 51dfa3284e..13d7441373 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1934,7 +1934,7 @@ method.ctor = (this.parent ? 'derived' : 'base'); } if (method.bound && method.ctor) { - method.error('Cannot define a constructor as a bound function'); + method.error('Cannot define a constructor as a bound (fat arrow) function'); } } return method; @@ -1964,8 +1964,8 @@ results = []; for (j = ref1.length - 1; j >= 0; j += -1) { name = ref1[j]; - name = new Value(new ThisLiteral, [name]).compile(o); - results.push(new Literal(`${name} = ${name}.bind(this)`)); + name = new Value(new ThisLiteral, [name]); + results.push(new Assign(name, new Call(new Value(name, [new Access(new PropertyName('bind'))]), [new ThisLiteral]))); } return results; }).call(this); @@ -4211,7 +4211,7 @@ return 'function(a, b) { return (+a % (b = +b) + b) % b; }'; }, boundMethodCheck: function() { - return "function(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new Error('Bound instance method accessed before binding') } }"; + return "function(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new Error('Bound instance method accessed before binding'); } }"; }, hasProp: function() { return '{}.hasOwnProperty'; diff --git a/src/nodes.coffee b/src/nodes.coffee index f82bf103e6..bfb14fdcd0 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1421,7 +1421,7 @@ exports.Class = class Class extends Base method.name = new (if methodName.shouldCache() then Index else Access) methodName method.name.updateLocationDataIfMissing methodName.locationData method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor' - method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor + method.error 'Cannot define a constructor as a bound (fat arrow) function' if method.bound and method.ctor method @@ -1442,8 +1442,8 @@ exports.Class = class Class extends Base proxyBoundMethods: (o) -> @ctor.thisAssignments = for name in @boundMethods by -1 - name = new Value(new ThisLiteral, [ name ]).compile o - new Literal "#{name} = #{name}.bind(this)" + name = new Value(new ThisLiteral, [ name ]) + new Assign name, new Call(new Value(name, [new Access new PropertyName 'bind']), [new ThisLiteral]) null @@ -3132,7 +3132,7 @@ UTILITIES = boundMethodCheck: -> " function(instance, Constructor) { if (!(instance instanceof Constructor)) { - throw new Error('Bound instance method accessed before binding') + throw new Error('Bound instance method accessed before binding'); } } " diff --git a/test/classes.coffee b/test/classes.coffee index 4f301fbc4f..a8fc217ef2 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1671,21 +1671,6 @@ test "CS6 Class extends a CS1 compiled class with super()", -> b = new B('three') eq b.make(), "making a cafe ole with caramel and three shots of espresso" -test 'Bound method called as callback before binding throws runtime error', -> - class Base - constructor: -> - f = @derivedBound - try - f() - ok no - catch e - eq e.message, 'Bound instance method accessed before binding' - - class Derived extends Base - derivedBound: => - ok no - d = new Derived - test 'Bound method called normally before binding is ok', -> class Base constructor: -> diff --git a/test/error_messages.coffee b/test/error_messages.coffee index 63a1d4edaa..f10ed058c1 100644 --- a/test/error_messages.coffee +++ b/test/error_messages.coffee @@ -1545,3 +1545,18 @@ test "#4248: Unicode code point escapes", -> '\\u{a}\\u{1111110000}' \ ^\^^^^^^^^^^^^^ ''' + +test 'Bound method called as callback before binding throws runtime error', -> + class Base + constructor: -> + f = @derivedBound + try + f() + ok no + catch e + eq e.message, 'Bound instance method accessed before binding' + + class Derived extends Base + derivedBound: => + ok no + d = new Derived From 508e8e868dc2dc48e8508c845adfca3a56416431 Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Sun, 11 Jun 2017 15:47:55 -0500 Subject: [PATCH 10/12] use ref to own class for check --- lib/coffeescript/nodes.js | 46 ++++++++++++------------------- src/nodes.coffee | 32 +++++++-------------- test/classes.coffee | 58 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 51 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 03597a7449..fa3e24cf21 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1809,11 +1809,6 @@ } o.indent += TAB; result = []; - if (this.assignParentRef) { - result.push(this.makeCode("(")); - result.push(...this.assignParentRef.compileToFragments(o)); - result.push(this.makeCode(", ")); - } result.push(this.makeCode("class ")); if (this.name) { result.push(this.makeCode(`${this.name} `)); @@ -1829,9 +1824,6 @@ result.push(this.makeCode(`\n${this.tab}`)); } result.push(this.makeCode('}')); - if (this.assignParentRef) { - result.push(this.makeCode(")")); - } return result; } @@ -1859,19 +1851,6 @@ } } - setParentRef(o) { - var ref, ref1; - if (this._setParentRef) { - return; - } - this._setParentRef = true; - if ((ref1 = this.parent) != null ? ref1.shouldCache() : void 0) { - ref = new IdentifierLiteral(o.scope.freeVariable('ref')); - this.assignParentRef = new Assign(ref, this.parent); - return this.parent = ref; - } - } - walkBody(o) { var assign, end, executableBody, expression, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, properties, pushSlice, ref1, start; this.ctor = null; @@ -1930,9 +1909,7 @@ } else if (method.isStatic && method.bound) { method.context = this.name; } else if (method.bound) { - this.boundMethods.push(method.name); - this.setParentRef(o); - method.parentClass = this.parent; + this.boundMethods.push(method); } } } @@ -2012,14 +1989,25 @@ } proxyBoundMethods(o) { - var name; + var method, name; + if (this.parent) { + if (this.variable == null) { + this.variable = new IdentifierLiteral(o.scope.freeVariable('_class')); + } + } + if (this.variableRef == null) { + [this.variable, this.variableRef] = this.variable.cache(o); + } this.ctor.thisAssignments = (function() { var j, ref1, results; ref1 = this.boundMethods; results = []; for (j = ref1.length - 1; j >= 0; j += -1) { - name = ref1[j]; - name = new Value(new ThisLiteral, [name]); + method = ref1[j]; + if (this.parent) { + method.classVariable = this.variableRef; + } + name = new Value(new ThisLiteral, [method.name]); results.push(new Assign(name, new Call(new Value(name, [new Access(new PropertyName('bind'))]), [new ThisLiteral]))); } return results; @@ -2908,9 +2896,9 @@ this.body.expressions.unshift(...thisAssignments); } this.body.expressions.unshift(...exprs); - if (this.isMethod && this.bound && !this.isStatic && this.parentClass) { + if (this.isMethod && this.bound && !this.isStatic && this.classVariable) { boundMethodCheck = new Value(new Literal(utility('boundMethodCheck', o))); - this.body.expressions.unshift(new Call(boundMethodCheck, [new Value(new ThisLiteral), this.parentClass])); + this.body.expressions.unshift(new Call(boundMethodCheck, [new Value(new ThisLiteral), this.classVariable])); } if (!(wasEmpty || this.noReturn)) { this.body.makeReturn(); diff --git a/src/nodes.coffee b/src/nodes.coffee index 528f90d4a4..3a16eebe7b 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1325,10 +1325,6 @@ exports.Class = class Class extends Base o.indent += TAB result = [] - if @assignParentRef - result.push @makeCode "(" - result.push @assignParentRef.compileToFragments(o)... - result.push @makeCode ", " result.push @makeCode "class " result.push @makeCode "#{@name} " if @name result.push @makeCode('extends '), @parent.compileToFragments(o)..., @makeCode ' ' if @parent @@ -1340,8 +1336,6 @@ exports.Class = class Class extends Base result.push @body.compileToFragments(o, LEVEL_TOP)... result.push @makeCode "\n#{@tab}" result.push @makeCode '}' - if @assignParentRef - result.push @makeCode ")" result @@ -1361,15 +1355,6 @@ exports.Class = class Class extends Base @variable.error message if message if name in JS_FORBIDDEN then "_#{name}" else name - setParentRef: (o) -> - return if @_setParentRef - @_setParentRef = yes - - if @parent?.shouldCache() - ref = new IdentifierLiteral o.scope.freeVariable 'ref' - @assignParentRef = new Assign ref, @parent - @parent = ref - walkBody: (o) -> @ctor = null @boundMethods = [] @@ -1419,9 +1404,7 @@ exports.Class = class Class extends Base else if method.isStatic and method.bound method.context = @name else if method.bound - @boundMethods.push method.name - @setParentRef(o) - method.parentClass = @parent + @boundMethods.push method if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) @@ -1479,8 +1462,13 @@ exports.Class = class Class extends Base ctor proxyBoundMethods: (o) -> - @ctor.thisAssignments = for name in @boundMethods by -1 - name = new Value(new ThisLiteral, [ name ]) + @variable ?= new IdentifierLiteral o.scope.freeVariable '_class' if @parent + [@variable, @variableRef] = @variable.cache o unless @variableRef? + + @ctor.thisAssignments = for method in @boundMethods by -1 + method.classVariable = @variableRef if @parent + + name = new Value(new ThisLiteral, [ method.name ]) new Assign name, new Call(new Value(name, [new Access new PropertyName 'bind']), [new ThisLiteral]) null @@ -2208,9 +2196,9 @@ exports.Code = class Code extends Base wasEmpty = @body.isEmpty() @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments @body.expressions.unshift exprs... - if @isMethod and @bound and not @isStatic and @parentClass + if @isMethod and @bound and not @isStatic and @classVariable boundMethodCheck = new Value new Literal utility 'boundMethodCheck', o - @body.expressions.unshift new Call(boundMethodCheck, [new Value(new ThisLiteral), @parentClass]) + @body.expressions.unshift new Call(boundMethodCheck, [new Value(new ThisLiteral), @classVariable]) @body.makeReturn() unless wasEmpty or @noReturn # Assemble the output diff --git a/test/classes.coffee b/test/classes.coffee index a8fc217ef2..f7c572c52a 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1747,3 +1747,61 @@ test 'Bound method of class with expression base class called as callback is ok' b = new A {derivedBound} = b eq derivedBound(), 3 + +test 'Bound method of class with expression base class called as callback is ok', -> + calledF = no + obj = {} + B = class + f = -> + throw new Error if calledF + calledF = yes + obj + class f().A extends B + constructor: (@prop = 3) -> + super() + g = @derivedBound + eq g(), 3 + + derivedBound: => + @prop + + a = new obj.A + {derivedBound} = a + eq derivedBound(), 3 + +test 'Bound method of anonymous child class called as callback is ok', -> + f = -> + B = class + class extends B + constructor: (@prop = 3) -> + super() + g = @derivedBound + eq g(), 3 + + derivedBound: => + @prop + + a = new (f()) + {derivedBound} = a + eq derivedBound(), 3 + +# uncomment once #4436 is fixed +# test 'Bound method of immediately instantiated class with expression base class called as callback is ok', -> +# calledF = no +# obj = {} +# B = class +# f = -> +# throw new Error if calledF +# calledF = yes +# obj +# a = new class f().A extends B +# constructor: (@prop = 3) -> +# super() +# g = @derivedBound +# eq g(), 3 + +# derivedBound: => +# @prop + +# {derivedBound} = a +# eq derivedBound(), 3 From 72dfaa9887c9d5e6536a7e64b6b9a398c10c4d9e Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 13 Jun 2017 10:40:50 -0500 Subject: [PATCH 11/12] fixes from code review --- lib/coffeescript/nodes.js | 24 +++++++++++------------ src/nodes.coffee | 13 +++++++------ test/classes.coffee | 41 +++++++++++++++++++-------------------- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 19244802aa..68522422b1 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1771,7 +1771,7 @@ compileNode(o) { var executableBody, node, parentName; this.name = this.determineName(); - executableBody = this.walkBody(o); + executableBody = this.walkBody(); if (this.parent instanceof Value && !this.parent.hasProperties()) { parentName = this.parent.base.value; } @@ -1782,6 +1782,14 @@ } else if ((this.name == null) && o.level === LEVEL_TOP) { node = new Parens(node); } + if (this.boundMethods.length && this.parent) { + if (this.variable == null) { + this.variable = new IdentifierLiteral(o.scope.freeVariable('_class')); + } + if (this.variableRef == null) { + [this.variable, this.variableRef] = this.variable.cache(o); + } + } if (this.variable) { node = new Assign(this.variable, node, null, {moduleDeclaration: this.moduleDeclaration}); } @@ -1850,7 +1858,7 @@ } } - walkBody(o) { + walkBody() { var assign, end, executableBody, expression, expressions, exprs, i, initializer, initializerExpression, j, k, len1, len2, method, properties, pushSlice, ref1, start; this.ctor = null; this.boundMethods = []; @@ -1989,19 +1997,11 @@ proxyBoundMethods(o) { var method, name; - if (this.parent) { - if (this.variable == null) { - this.variable = new IdentifierLiteral(o.scope.freeVariable('_class')); - } - } - if (this.variableRef == null) { - [this.variable, this.variableRef] = this.variable.cache(o); - } this.ctor.thisAssignments = (function() { - var j, ref1, results; + var j, len1, ref1, results; ref1 = this.boundMethods; results = []; - for (j = ref1.length - 1; j >= 0; j += -1) { + for (j = 0, len1 = ref1.length; j < len1; j++) { method = ref1[j]; if (this.parent) { method.classVariable = this.variableRef; diff --git a/src/nodes.coffee b/src/nodes.coffee index 591d34aef7..3153561d67 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1294,7 +1294,7 @@ exports.Class = class Class extends Base compileNode: (o) -> @name = @determineName() - executableBody = @walkBody(o) + executableBody = @walkBody() # Special handling to allow `class expr.A extends A` declarations parentName = @parent.base.value if @parent instanceof Value and not @parent.hasProperties() @@ -1308,6 +1308,10 @@ exports.Class = class Class extends Base # Anonymous classes are only valid in expressions node = new Parens node + if @boundMethods.length and @parent + @variable ?= new IdentifierLiteral o.scope.freeVariable '_class' + [@variable, @variableRef] = @variable.cache o unless @variableRef? + if @variable node = new Assign @variable, node, null, { @moduleDeclaration } @@ -1356,7 +1360,7 @@ exports.Class = class Class extends Base @variable.error message if message if name in JS_FORBIDDEN then "_#{name}" else name - walkBody: (o) -> + walkBody: -> @ctor = null @boundMethods = [] executableBody = null @@ -1463,10 +1467,7 @@ exports.Class = class Class extends Base ctor proxyBoundMethods: (o) -> - @variable ?= new IdentifierLiteral o.scope.freeVariable '_class' if @parent - [@variable, @variableRef] = @variable.cache o unless @variableRef? - - @ctor.thisAssignments = for method in @boundMethods by -1 + @ctor.thisAssignments = for method in @boundMethods method.classVariable = @variableRef if @parent name = new Value(new ThisLiteral, [ method.name ]) diff --git a/test/classes.coffee b/test/classes.coffee index c281e2f579..192b9d0886 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -1752,7 +1752,7 @@ test 'Bound method of class with expression base class called as callback is ok' {derivedBound} = b eq derivedBound(), 3 -test 'Bound method of class with expression base class called as callback is ok', -> +test 'Bound method of class with expression class name called as callback is ok', -> calledF = no obj = {} B = class @@ -1789,23 +1789,22 @@ test 'Bound method of anonymous child class called as callback is ok', -> {derivedBound} = a eq derivedBound(), 3 -# uncomment once #4436 is fixed -# test 'Bound method of immediately instantiated class with expression base class called as callback is ok', -> -# calledF = no -# obj = {} -# B = class -# f = -> -# throw new Error if calledF -# calledF = yes -# obj -# a = new class f().A extends B -# constructor: (@prop = 3) -> -# super() -# g = @derivedBound -# eq g(), 3 - -# derivedBound: => -# @prop - -# {derivedBound} = a -# eq derivedBound(), 3 +test 'Bound method of immediately instantiated class with expression base class called as callback is ok', -> + calledF = no + obj = {} + B = class + f = -> + throw new Error if calledF + calledF = yes + obj + a = new class f().A extends B + constructor: (@prop = 3) -> + super() + g = @derivedBound + eq g(), 3 + + derivedBound: => + @prop + + {derivedBound} = a + eq derivedBound(), 3 From b0fb8052d447ced35003a54476a8c2403c97ddbd Mon Sep 17 00:00:00 2001 From: Julian Rosse Date: Tue, 13 Jun 2017 10:42:47 -0500 Subject: [PATCH 12/12] remove unneeded param --- lib/coffeescript/nodes.js | 4 ++-- src/nodes.coffee | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 68522422b1..4db33ee927 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1812,7 +1812,7 @@ ref1.noReturn = true; } if (this.boundMethods.length) { - this.proxyBoundMethods(o); + this.proxyBoundMethods(); } o.indent += TAB; result = []; @@ -1995,7 +1995,7 @@ return ctor; } - proxyBoundMethods(o) { + proxyBoundMethods() { var method, name; this.ctor.thisAssignments = (function() { var j, len1, ref1, results; diff --git a/src/nodes.coffee b/src/nodes.coffee index 3153561d67..00bef238c1 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1325,7 +1325,7 @@ exports.Class = class Class extends Base @ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length @ctor?.noReturn = true - @proxyBoundMethods o if @boundMethods.length + @proxyBoundMethods() if @boundMethods.length o.indent += TAB @@ -1466,7 +1466,7 @@ exports.Class = class Class extends Base ctor - proxyBoundMethods: (o) -> + proxyBoundMethods: -> @ctor.thisAssignments = for method in @boundMethods method.classVariable = @variableRef if @parent