Skip to content
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

[CS2] Remove support for bound instance methods #4530

Merged
merged 1 commit into from
Apr 28, 2017
Merged

[CS2] Remove support for bound instance methods #4530

merged 1 commit into from
Apr 28, 2017

Conversation

connec
Copy link
Collaborator

@connec connec commented Apr 26, 2017

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.


This is probably the most extreme solution to the problem, and would leave heavy users of bound methods with an unpleasant refactoring task, however the alternatives aren’t terribly appetising either:

  • Restrict the error to derived classes only. A common use of bound methods is for event handlers in frontend frameworks such as Backbone and React, both of which rely on extending base classes. Choosing this approach would not help these users, and would make bound instance methods even more of a fringe feature.

  • Hoist the entire method definition into the constructor, ensuring it isn’t assigned at all until super has been called. This would at least make any bugs easier to spot, and also looks very similar to the class public fields ECMA Stage 2 proposal. Matching some ES equivalent is probably the best strategy long term, but as this proposal is only Stage 2 we can’t rely on it not being changed.

  • Leave everything as it is. We would could simply document the caveats of bound method interactions with super and let everyone figure it out.

Edit: #4512, #3093, and others propose a binding property access (as - @vendethiel would be quick to add - is available Coco/LS). This would make the necessary refactoring for this or option 1 slightly easier, and is also an ES proposal.

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.
@GeoffreyBooth
Copy link
Collaborator

This looks good to me. @lydell, did you have any feedback? I assume you agree with this solution to #4497?

@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 28, 2017
@lydell
Copy link
Collaborator

lydell commented Apr 28, 2017

I think this looks good to me, too.

@xixixao
Copy link
Contributor

xixixao commented May 2, 2017

Wait, shouldn't this compile to a property initializer?

class A
  b: => this.x
class A {
  b = () => this.x;
}

There is no way to get the above output or any equivalent now?

@xixixao
Copy link
Contributor

xixixao commented May 7, 2017

@connec @lydell

@connec
Copy link
Collaborator Author

connec commented May 7, 2017

@xixixao property initializers are just a Stage 2 proposal, so there is no support for them currently.

The equivalent would be:

class A
  constructor: ->
    @b = => @x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants