From b5f0ddb3cba6b587ab722913b83420ed6e8742d7 Mon Sep 17 00:00:00 2001 From: Chris Connelly Date: Thu, 27 Apr 2017 01:13:54 +0200 Subject: [PATCH] Remove support for bound instance methods Bound methods are implemented as assignments to `this` in the constructor. In derived classes, where `this` is unavailable until after `super` has been called, the binding is applied and assigned after the `super` call. This means that any references to 'bound' methods reachable from the parent constructor will actually point to the unbound prototype methods. This can lead to very subtle bugs where a method that is thought to be bound is handed off and later called with an incorrect context, and the only defence is for users to be vigilant about referencing bound methods in constructors. --- lib/coffeescript/nodes.js | 31 ++---------- src/nodes.coffee | 19 ++----- test/assignment.coffee | 5 +- test/classes.coffee | 104 ++------------------------------------ test/scope.coffee | 10 ---- 5 files changed, 13 insertions(+), 156 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index c814b8e402..15737dc452 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -1741,7 +1741,7 @@ compileClassDeclaration(o) { var ref1, result; - if (this.externalCtor || this.boundMethods.length) { + if (this.externalCtor) { if (this.ctor == null) { this.ctor = this.makeDefaultConstructor(); } @@ -1749,9 +1749,6 @@ if ((ref1 = this.ctor) != null) { ref1.noReturn = true; } - if (this.boundMethods.length) { - this.proxyBoundMethods(o); - } o.indent += TAB; result = []; result.push(this.makeCode("class ")); @@ -1799,7 +1796,6 @@ 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); @@ -1851,11 +1847,8 @@ method.error('Cannot define more than one constructor in a class'); } this.ctor = method; - } else if (method.bound && method.isStatic) { + } else if (method.isStatic && method.bound) { method.context = this.name; - } else if (method.bound) { - this.boundMethods.push(method.name); - method.bound = false; } } } @@ -1911,8 +1904,8 @@ if (methodName.value === 'constructor') { method.ctor = (this.parent ? 'derived' : 'base'); } - if (method.bound && method.ctor) { - method.error('Cannot define a constructor as a bound function'); + if (method.bound) { + method.error('Methods cannot be bound functions'); } } return method; @@ -1934,22 +1927,6 @@ 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']; diff --git a/src/nodes.coffee b/src/nodes.coffee index 220bb77306..1112afaf99 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -1279,11 +1279,9 @@ exports.Class = class Class extends Base result compileClassDeclaration: (o) -> - @ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length + @ctor ?= @makeDefaultConstructor() if @externalCtor @ctor?.noReturn = true - @proxyBoundMethods o if @boundMethods.length - o.indent += TAB result = [] @@ -1319,7 +1317,6 @@ exports.Class = class Class extends Base walkBody: -> @ctor = null - @boundMethods = [] executableBody = null initializer = [] @@ -1363,11 +1360,8 @@ exports.Class = class Class extends Base if method.ctor method.error 'Cannot define more than one constructor in a class' if @ctor @ctor = method - else if method.bound and method.isStatic + else if method.isStatic and method.bound method.context = @name - else if method.bound - @boundMethods.push method.name - method.bound = false if initializer.length isnt expressions.length @body.expressions = (expression.hoist() for expression in initializer) @@ -1405,7 +1399,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 'Methods cannot be bound functions' if method.bound method @@ -1424,13 +1418,6 @@ 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' ] diff --git a/test/assignment.coffee b/test/assignment.coffee index c02e9ab804..ec736fcfd0 100644 --- a/test/assignment.coffee +++ b/test/assignment.coffee @@ -562,9 +562,8 @@ test "Assignment to variables similar to helper functions", -> extend = 3 hasProp = 4 value: 5 - method: (bind, bind1) => [bind, bind1, extend, hasProp, @value] - {method} = new B - arrayEq [1, 2, 3, 4, 5], method 1, 2 + method: (bind, bind1) -> [bind, bind1, extend, hasProp, @value] + arrayEq [1, 2, 3, 4, 5], new B().method 1, 2 modulo = -1 %% 3 eq 2, modulo diff --git a/test/classes.coffee b/test/classes.coffee index c0d955389c..fbd399e1ca 100644 --- a/test/classes.coffee +++ b/test/classes.coffee @@ -136,42 +136,20 @@ test "classes with JS-keyword properties", -> ok instance.name() is 'class' -test "Classes with methods that are pre-bound to the instance, or statically, to the class", -> +test "Classes with methods that are pre-bound 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 @@ -476,21 +454,6 @@ 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() @@ -559,7 +522,7 @@ test "#1842: Regression with bound functions within bound class methods", -> @unbound: -> eq this, Store - instance: => + instance: -> ok this instanceof Store Store.bound() @@ -722,57 +685,6 @@ 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 @@ -984,9 +896,6 @@ 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, {} @@ -1006,8 +915,6 @@ 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 @@ -1026,16 +933,13 @@ 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 @@ -1237,7 +1141,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') @@ -1245,7 +1149,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 be7299bb9a..82025e49c4 100644 --- a/test/scope.coffee +++ b/test/scope.coffee @@ -107,16 +107,6 @@ 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