fixes #1819: bound instance methods should reflect prototype changes

[michaelficarra]

depends on the acceptance of the proposal in #1819

[jashkenas]

The semantics of this seem incorrect.

If you have a bound function in a class definition, and then later replace it with an unbound one ... the function should now be unbound -- not just a new bound version of the new function. Does that make sense?

[michaelficarra]

Both the current and proposed semantics equally clash with JS's prototypal paradigm. I just think the proposed semantics are more useful.

[jashkenas]

Yes -- I'm not saying that the current implementation isn't wrong for this case ... I'm just saying that this fix doesn't seem complete.

[TrevorBurnham]

Hmm. I don't think it's possible to achieve the consistent behavior jashkenas is describing without storing additional state. We want the constructor to bind a method only if it's the original, defined with => on the class; but the constructor has no way of knowing.

An implementation that would give us the desired consistency would compile

class Foo
  boundMethod: => 23
  unboundMethod: -> 42

into something like this:

Foo = (function() {
  function Foo() {
    for (method in Foo.prototype) {
      if (Foo.prototype[method].bind) {
        this[method] = __bind(Foo.prototype[method], this);
      };
    };
  }
  Foo.prototype.boundMethod = function() { 23 };
  Foo.prototype.boundMethod.bind = true;
  Foo.prototype.unboundMethod = function() { 42 };
  return Foo;
})();

(Edit: Nope, this won't work either; in the example below, before would still have the bound method even after the prototype has been modified. But it may be possible if you replace Michael's __bindMethod with something that takes both the function as it is now and the name of the method, and you check whether the two are equal in the returned function? I'm just thinking out loud at this point; gotta get some sleep...)

I believe this is the test case we want to solve:

class D
  a: => this
before = new D
eq before, before.a.call {}

D::a = -> this
after = new D
# we want before.a and after.a to both be unbound now

altContext = {}
eq altContext, before.a.call(altContext)
eq altContext, after.a.call(altContext)

[TrevorBurnham]

Ignore previous meandering comment (except the test case, which is legit); here's a bona fide solution: Compile

class Foo
  boundMethod: => 23
  unboundMethod: -> 42

to

Foo = (function() {
  function Foo() {
    var _i, _name;
    for (_name in Foo.prototype) {
      if (__indexOf.call(_boundMethods, Foo.prototype[_name]) >= 0) {
        this[_name] = __bindMethod(_name, Foo.prototype[_name], this);
      }
    };
  }
  Foo.prototype.boundMethod = function() { return 23 };
  Foo.prototype.unboundMethod = function() { return 42 };
  var _boundMethods = [Foo.prototype.boundMethod];
  return Foo;
})();

where the __bindMethod helper is defined as

__bindMethod = function(name, method, me) {
  var proto = me.constructor.prototype;
  return function() {
    return proto[name] === method ? method.apply(me, arguments) : proto[name].apply(this, arguments);
  }
};

(Edit: Some small errors above corrected. I've run the code and verified that it passes the test from my previous comment.)

The two checks—one in the constructor, the other in __bindMethod—together ensure that Foo::boundMethod is bound if and only if it's the same method that we defined on class Foo.

Does that sound good to you, Michael?

[michaelficarra]

Yeah, that LGTM. I wish there was a somewhat simpler method, though.

[TrevorBurnham]

I wish there were as well, but after tinkering with this for a while, I'm fairly certain that both checks are necessary:

You need a test in the function returned from __bindMethod, because otherwise, there's no way for before.boundFunction to do something different after you've modified Foo::boundFunction.

And you need a test in the constructor (on some data structure containing references to the bound functions used in the original class definition), because otherwise, after.boundFunction will be a bound form of the newly-defined Foo::boundFunction.

With both checks, before.boundFunction and after.boundFunction will always have the same behavior.

[michaelficarra]

I've come up with what I believe is a significantly cleaner implementation:

var __bindMethods = function(methods, me) {
  var proto = me.constructor.prototype;
  for(var m in methods)
    (function(method, protoMethod) {
      if(protoMethod === method)
        me[m] = function() {
          return protoMethod === method ? method.apply(me, arguments) : protoMethod.apply(this, arguments);
        };
    }(methods[m], proto[m]));
};

var Foo = (function() {
  function Foo() {
    __bindMethods(_bound, this);
  }
  var _bound = {};
  Foo.prototype.boundMethod = _bound.boundMethod = function() { return 23 };
  Foo.prototype.unboundMethod = function() { return 42 };
  return Foo;
})();

I moved almost all of the boilerplate code out of each constructor and into the helper. We don't want to duplicate all that code for each class. I've also eliminated the dependency on all other helper functions. Thoughts? Personally, I'm pretty in favour of this change.

[TrevorBurnham]

Ha, I woke up this morning thinking about a similar revision. Definitely an improvement. There's also one small edge case with both implementations, though:

class D
  a: => this
original = D::a
before = new D
D::a = -> this
after = new D
D::a = original
# now a is bound on before but unbound on after

The fix is simply to kill the if(proto[m] === methods[m]) check in the loop. With that change, everything seems to work just fine. Tests here.