Skip to content

Commit 3e70d31

Browse files
connecGeoffreyBooth
authored andcommitted
Remove support for bound instance methods (#4530)
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.
1 parent 277975e commit 3e70d31

File tree

5 files changed

+13
-156
lines changed

5 files changed

+13
-156
lines changed

lib/coffeescript/nodes.js

+4-27
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/nodes.coffee

+3-16
Original file line numberDiff line numberDiff line change
@@ -1279,11 +1279,9 @@ exports.Class = class Class extends Base
12791279
result
12801280

12811281
compileClassDeclaration: (o) ->
1282-
@ctor ?= @makeDefaultConstructor() if @externalCtor or @boundMethods.length
1282+
@ctor ?= @makeDefaultConstructor() if @externalCtor
12831283
@ctor?.noReturn = true
12841284

1285-
@proxyBoundMethods o if @boundMethods.length
1286-
12871285
o.indent += TAB
12881286

12891287
result = []
@@ -1319,7 +1317,6 @@ exports.Class = class Class extends Base
13191317

13201318
walkBody: ->
13211319
@ctor = null
1322-
@boundMethods = []
13231320
executableBody = null
13241321

13251322
initializer = []
@@ -1363,11 +1360,8 @@ exports.Class = class Class extends Base
13631360
if method.ctor
13641361
method.error 'Cannot define more than one constructor in a class' if @ctor
13651362
@ctor = method
1366-
else if method.bound and method.isStatic
1363+
else if method.isStatic and method.bound
13671364
method.context = @name
1368-
else if method.bound
1369-
@boundMethods.push method.name
1370-
method.bound = false
13711365

13721366
if initializer.length isnt expressions.length
13731367
@body.expressions = (expression.hoist() for expression in initializer)
@@ -1405,7 +1399,7 @@ exports.Class = class Class extends Base
14051399
method.name = new (if methodName.shouldCache() then Index else Access) methodName
14061400
method.name.updateLocationDataIfMissing methodName.locationData
14071401
method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor'
1408-
method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor
1402+
method.error 'Methods cannot be bound functions' if method.bound
14091403

14101404
method
14111405

@@ -1424,13 +1418,6 @@ exports.Class = class Class extends Base
14241418

14251419
ctor
14261420

1427-
proxyBoundMethods: (o) ->
1428-
@ctor.thisAssignments = for name in @boundMethods by -1
1429-
name = new Value(new ThisLiteral, [ name ]).compile o
1430-
new Literal "#{name} = #{name}.bind(this)"
1431-
1432-
null
1433-
14341421
exports.ExecutableClassBody = class ExecutableClassBody extends Base
14351422
children: [ 'class', 'body' ]
14361423

test/assignment.coffee

+2-3
Original file line numberDiff line numberDiff line change
@@ -562,9 +562,8 @@ test "Assignment to variables similar to helper functions", ->
562562
extend = 3
563563
hasProp = 4
564564
value: 5
565-
method: (bind, bind1) => [bind, bind1, extend, hasProp, @value]
566-
{method} = new B
567-
arrayEq [1, 2, 3, 4, 5], method 1, 2
565+
method: (bind, bind1) -> [bind, bind1, extend, hasProp, @value]
566+
arrayEq [1, 2, 3, 4, 5], new B().method 1, 2
568567

569568
modulo = -1 %% 3
570569
eq 2, modulo

test/classes.coffee

+4-100
Original file line numberDiff line numberDiff line change
@@ -136,42 +136,20 @@ test "classes with JS-keyword properties", ->
136136
ok instance.name() is 'class'
137137

138138

139-
test "Classes with methods that are pre-bound to the instance, or statically, to the class", ->
139+
test "Classes with methods that are pre-bound statically, to the class", ->
140140

141141
class Dog
142142
constructor: (name) ->
143143
@name = name
144144

145-
bark: =>
146-
"#{@name} woofs!"
147-
148145
@static = =>
149146
new this('Dog')
150147

151-
spark = new Dog('Spark')
152-
fido = new Dog('Fido')
153-
fido.bark = spark.bark
154-
155-
ok fido.bark() is 'Spark woofs!'
156-
157148
obj = func: Dog.static
158149

159150
ok obj.func().name is 'Dog'
160151

161152

162-
test "a bound function in a bound function", ->
163-
164-
class Mini
165-
num: 10
166-
generate: =>
167-
for i in [1..3]
168-
=>
169-
@num
170-
171-
m = new Mini
172-
eq (func() for func in m.generate()).join(' '), '10 10 10'
173-
174-
175153
test "contructor called with varargs", ->
176154

177155
class Connection
@@ -476,21 +454,6 @@ test "#1182: execution order needs to be considered as well", ->
476454
@B: makeFn 2
477455
constructor: makeFn 3
478456

479-
test "#1182: external constructors with bound functions", ->
480-
fn = ->
481-
{one: 1}
482-
this
483-
class B
484-
class A
485-
constructor: fn
486-
method: => this instanceof A
487-
ok (new A).method.call(new B)
488-
489-
test "#1372: bound class methods with reserved names", ->
490-
class C
491-
delete: =>
492-
ok C::delete
493-
494457
test "#1380: `super` with reserved names", ->
495458
class C
496459
do: -> super()
@@ -559,7 +522,7 @@ test "#1842: Regression with bound functions within bound class methods", ->
559522
@unbound: ->
560523
eq this, Store
561524

562-
instance: =>
525+
instance: ->
563526
ok this instanceof Store
564527

565528
Store.bound()
@@ -722,57 +685,6 @@ test "extending native objects works with and without defining a constructor", -
722685
ok overrideArray instanceof OverrideArray
723686
eq 'yes!', overrideArray.method()
724687

725-
726-
test "#2782: non-alphanumeric-named bound functions", ->
727-
class A
728-
'b:c': =>
729-
'd'
730-
731-
eq (new A)['b:c'](), 'd'
732-
733-
734-
test "#2781: overriding bound functions", ->
735-
class A
736-
a: ->
737-
@b()
738-
b: =>
739-
1
740-
741-
class B extends A
742-
b: =>
743-
2
744-
745-
b = (new A).b
746-
eq b(), 1
747-
748-
b = (new B).b
749-
eq b(), 2
750-
751-
752-
test "#2791: bound function with destructured argument", ->
753-
class Foo
754-
method: ({a}) => 'Bar'
755-
756-
eq (new Foo).method({a: 'Bar'}), 'Bar'
757-
758-
759-
test "#2796: ditto, ditto, ditto", ->
760-
answer = null
761-
762-
outsideMethod = (func) ->
763-
func.call message: 'wrong!'
764-
765-
class Base
766-
constructor: ->
767-
@message = 'right!'
768-
outsideMethod @echo
769-
770-
echo: =>
771-
answer = @message
772-
773-
new Base
774-
eq answer, 'right!'
775-
776688
test "#3063: Class bodies cannot contain pure statements", ->
777689
throws -> CoffeeScript.compile """
778690
class extends S
@@ -984,9 +896,6 @@ test "`this` access after `super` in extended classes", ->
984896
eq result.super, this
985897
eq result.param, @param
986898
eq result.method, @method
987-
ok result.method isnt Test::method
988-
989-
method: =>
990899

991900
nonce = {}
992901
new Test nonce, {}
@@ -1006,8 +915,6 @@ test "`@`-params and bound methods with multiple `super` paths (blocks)", ->
1006915
super 'not param'
1007916
eq @name, 'not param'
1008917
eq @param, nonce
1009-
ok @method isnt Test::method
1010-
method: =>
1011918
new Test true, nonce
1012919
new Test false, nonce
1013920

@@ -1026,16 +933,13 @@ test "`@`-params and bound methods with multiple `super` paths (expressions)", -
1026933
eq (super 'param'), @;
1027934
eq @name, 'param';
1028935
eq @param, nonce;
1029-
ok @method isnt Test::method
1030936
)
1031937
else
1032938
result = (
1033939
eq (super 'not param'), @;
1034940
eq @name, 'not param';
1035941
eq @param, nonce;
1036-
ok @method isnt Test::method
1037942
)
1038-
method: =>
1039943
new Test true, nonce
1040944
new Test false, nonce
1041945

@@ -1237,15 +1141,15 @@ test "super in a bound function", ->
12371141
make: -> "Making a #{@drink}"
12381142

12391143
class B extends A
1240-
make: (@flavor) =>
1144+
make: (@flavor) ->
12411145
super() + " with #{@flavor}"
12421146

12431147
b = new B('Machiato')
12441148
eq b.make('vanilla'), "Making a Machiato with vanilla"
12451149

12461150
# super in a bound function in a bound function
12471151
class C extends A
1248-
make: (@flavor) =>
1152+
make: (@flavor) ->
12491153
func = () =>
12501154
super() + " with #{@flavor}"
12511155
func()

test/scope.coffee

-10
Original file line numberDiff line numberDiff line change
@@ -107,16 +107,6 @@ test "#1183: super + closures", ->
107107
ret
108108
eq (new B).foo(), 10
109109

110-
test "#2331: bound super regression", ->
111-
class A
112-
@value = 'A'
113-
method: -> @constructor.value
114-
115-
class B extends A
116-
method: => super()
117-
118-
eq (new B).method(), 'A'
119-
120110
test "#3259: leak with @-params within destructured parameters", ->
121111
fn = ({@foo}, [@bar], [{@baz}]) ->
122112
foo = bar = baz = false

0 commit comments

Comments
 (0)