[CS2] Bound methods are unsafe to use with inheritance

[lluchs]

Bound methods are now only bound after calling the super constructor. If the super constructor calls any methods in the class, you end up with unbound references to functions.

Example:

class SomeView extends Backbone.View
  initialize: ->
    # Can't use @callback here - initialize is called from the super constructor
  someFunc: ->
    # It's *probably* safe to use @callback here - but it would suddenly get unsafe 
    # if we (or the super class or a derived class) decided to call @someFunc() in 
    # initialize later on.
  callback: => # ...

While it's not possible to bind the functions before calling the super constructor, there are some ways CS2 could make bound methods "safer" to use. Some ideas:

• In classes with inheritance, always bind references to bound methods where they're used. In the example above, @callback would always compile to this.callback.bind(this). While this solves the issue for local uses, it doesn't help for external access.
• Don't allow bound methods in classes with inheritance. Safest option, but not great.
• Rename the bound method and only set the proper name when binding this. Doesn't solve the issue, but would make errors like in the example above more visible. Would need additional thought to avoid naming conflicts and for interaction with non-CoffeeScript classes in the inheritance chain.

Related issue #4239 still stands, but now it's also an issue for the base class.

[connec]

I'd like to throw in a 4th option:

• Remove bound methods entirely.

The use-case for them is pretty small, and the behaviour is very implicit and has a lot of caveats. Explicitly binding methods at call time or in a constructor makes things much easier to follow, and lets users choose their own tradeoffs.

[GeoffreyBooth]

If they're not supported in ES classes, I vote for removing them.

[connec]

If they're not supported in ES classes

They're sort of supported through public class properties, for example. The proposal itself is here.

In general such properties cannot currently be represented in CoffeeScript, and the syntax conflicts with executable class bodies:

class Foo
  a = 1  # this is a plain assignment in the class body scope, not a public property initialiser

  a: 1   # this is a prototype assignment, not a public initialiser

  @a = 1 # this is an assignment to the class, equivalent to a public static property

  a: =>  # this is the closest thing we have to a public class property, as method is assigned in
         # the constructor

[GeoffreyBooth]

That proposal is only stage 2. I say we don’t support them until they release stage 4, but don’t paint ourselves into a corner by precluding what the likely supported syntax would be.

[GeoffreyBooth]

@lluchs is your original post still an issue since #4526? On 2 now your example compiles to:

var SomeView;

SomeView = class SomeView extends Backbone.View {
  constructor() {
    super(...arguments);
    this.callback = this.callback.bind(this);
  }

  initialize() {}

  someFunc() {}

  callback() {}

};

[lluchs]

Yeah, it's still binding callback only after calling super. I don't think it's possible to get CS1 bound method semantics with ES6 classes.

[GeoffreyBooth]

@Iluchs do you mind explaining what the current behavior's caveats are? The current behavior since #4526 I mean, my example above.

I'm thinking, rather than ban bound methods entirely and force a somewhat large breaking change, if we can detect the specific scenarios that are problematic we can throw errors in those cases. For example, calling @callback In another method before … something :) That would effectively be banning those cases, but leaving bound methods allowed at least some of the time.

[lluchs]

Just as I explained in the original issue text: if the super constructor tries to use a bound function, it will get the unbound one from the prototype.

I don't think it's possible to detect this statically as it depends on the super constructor's code.

[GeoffreyBooth]

So is this unwanted behavior serious enough that we should ban bound methods in classes? It seems like we don't have any other options?

[lluchs]

Yeah, it's a silent but major incompatibility with CS1. I think having a clear "bound methods aren't supported anymore" message makes upgrading easier than having to find all now-invalid uses of bound methods through testing.

Additionally, I feel like bound methods as in CS2 go against the CoffeeScript idea of "JS without the hard parts" by introducing a feature with tricky semantics which are easy to get wrong.

[connec]

This will likely irk plenty of people using classes with Backbone/React/other frontend frameworks where they want to pass around bound event handlers.

This has come up and been dropped before (#2136 and others), but perhaps we should switch to a lightweight 'binding access' operator, something like this~handleClick or something. Probably quite a long shot, but the boilerplate and duplication of this.blah.bind(this), console.log.bind(console) etc. is a fairly frustrating one.

Edit: turns out I've brought this up before 😅

[GeoffreyBooth]

So what do you propose? Only throwing an error when bound methods are used on a class that also extends (which I feel like is most classes, because you’re generally extending from a framework prototype)? Or just allowing the current behavior, and people are responsible for realizing what’s happening?

Or just take out bound methods entirely, and when the proposal reaches Stage 4, we add them back again and compile to ES public class properties?

[connec]

I've opened #4530 to show the changes for removing them and highlight the alternatives discussed here. Personally I think removing them would be the best option, but I've always had a bit of a prejudice against bound methods so I'm not the most objective 😅

[GeoffreyBooth]

What error would be thrown if a parent class called onRender in the below?

var SomeView;

SomeView = class SomeView extends Backbone.View {
  constructor() {
    super(...arguments);
    this.onRender = (options) => {}
  }

};

[connec]

That would throw a TypeError because this.onRender will be undefined until after the call to super returns (example).

The problem with CS' implementation of bound methods is that this.onRender will be the unbound prototype until super returns, leading to subtle bugs.

[GeoffreyBooth]

Wouldn't it be odd for a parent class to call a method it didn't itself have? Like if it calls onRender in its constructor, I would expect the parent to have such a method; the fact that it doesn't is what gives the TypeError. And any calls to that method after initialization would be to the child's version.

So if it's safe to assume the parent class has all the method it calls in its constructor, and it should if it wants to be instantiated by itself ever and not just extended, then the issue is really that it's calling it's own versions on initialization and the child's versions thereafter. I guess the question is how bad that would be.

[GeoffreyBooth]

Resolved in #4530.