Skip to content

[CS2] Restore bound class methods via runtime check to avoid premature calling of bound method before binding #4561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
66 changes: 59 additions & 7 deletions lib/coffeescript/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

50 changes: 45 additions & 5 deletions src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -1279,12 +1279,18 @@ 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 = []
if @assignParentRef
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have bound methods and the parent class expression should be cached (as determined by setParentRef()), turn the class expression into a (ref = parent.class().expression, class Child extends ref {...})-style expression

Copy link
Collaborator

@connec connec Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst this is a nice way of doing (so I'm not suggesting changing it), but the .cache mechanism exists for this purpose, and could be used to generate code like:

class Child extends (ref = parent.class().expression) {}

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
Expand All @@ -1296,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

Expand All @@ -1315,8 +1323,18 @@ 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) ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but this doesn't seem to be needed any more

@ctor = null
@boundMethods = []
executableBody = null

initializer = []
Expand Down Expand Up @@ -1362,6 +1380,10 @@ 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
@setParentRef(o)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have any bound methods, see if the parent class expression should be cached to a variable ref and save the (possibly updated) parent on method so that it can be used in the runtime check

method.parentClass = @parent

if initializer.length isnt expressions.length
@body.expressions = (expression.hoist() for expression in initializer)
Expand Down Expand Up @@ -1399,7 +1421,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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In error messages, let’s use the phrasing “bound (fat arrow) function”. MDN calls => functions “arrow functions,” so that’s what most people will probably know them as. I think we should be explicit about “fat” arrow to distinguish from ->.

# method.error 'Cannot define a bound method in an anonymous class' if method.bound and not @name
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For illustration, this would be option (1) (disallow bound methods in anonymous classes) of the three options I outlined for dealing with anonymous classes with bound methods


method

Expand All @@ -1418,6 +1441,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)"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than new Literal, shouldn’t this be new Assign etc.?


null

exports.ExecutableClassBody = class ExecutableClassBody extends Base
children: [ 'class', 'body' ]

Expand Down Expand Up @@ -1501,7 +1531,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
Expand Down Expand Up @@ -2140,6 +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 @parentClass
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the runtime check, basically _boundMethodCheck(this, parentClassRef). Will only be added for child class bound methods

_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
Expand Down Expand Up @@ -3097,6 +3130,13 @@ exports.If = class If extends Base

UTILITIES =
modulo: -> 'function(a, b) { return (+a % (b = +b) + b) % b; }'
_boundMethodCheck: -> "
Copy link
Collaborator Author

@helixbass helixbass Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helper that throws runtime error if bound method called with a this that isn't an instance of its parent class

as discussed in coffeescript6/discuss#84, this results in a stack trace where the offending method is in the second line

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this helper’s name starts with an underscore? Our other helpers’ names don’t.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeoffreyBooth I think @connec was mimicking babel's _classCallCheck. Updated to boundMethodCheck

function(instance, Constructor) {
if (!(instance instanceof Constructor)) {
throw new Error('Bound instance method accessed before binding')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add semicolon.

}
}
"

# Shortcuts to speed up the lookup time for native functions.
hasProp: -> '{}.hasOwnProperty'
Expand Down
5 changes: 3 additions & 2 deletions test/assignment.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading