Fix #2489 again: preserve bound methods length property

[epidemian]

So, here's another attempt at fixing #2489. This one has a more straightforward output, it's basically the old __bind but inlined.

Sample input:

class Foo 
  method: (a, b) => one

  'bar:baz': (bar) => two

  '--more crazy methods--': (lol) => tres

Old bare output:

var Foo,
  __bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };

Foo = (function() {
  function Foo() {
    this['--more crazy methods--'] = __bind(this['--more crazy methods--'], this);
    this['bar:baz'] = __bind(this['bar:baz'], this);
    this.method = __bind(this.method, this);
  }

  Foo.prototype.method = function(a, b) {
    return one;
  };

  Foo.prototype['bar:baz'] = function(bar) {
    return two;
  };

  Foo.prototype['--more crazy methods--'] = function(lol) {
    return tres;
  };

  return Foo;

})();

This PR's output:

var Foo;

Foo = (function() {
  function Foo() {
    var _bound, _bound1, _method,
      _this = this;
    _method = this.method;
    this.method = function(a, b) {
      return _method.apply(_this, arguments);
    };
    _bound = this['bar:baz'];
    this['bar:baz'] = function(bar) {
      return _bound.apply(_this, arguments);
    };
    _bound1 = this['--more crazy methods--'];
    this['--more crazy methods--'] = function(lol) {
      return _bound1.apply(_this, arguments);
    };
  }

  Foo.prototype.method = function(a, b) {
    return one;
  };

  Foo.prototype['bar:baz'] = function(bar) {
    return two;
  };

  Foo.prototype['--more crazy methods--'] = function(lol) {
    return tres;
  };

  return Foo;

})();

As you can see, the parameter names and number is preserved in the bound version of the method, thus making length work as expected.

Now, this PR works, but the code is very bad 💩. Or at least it contains a couple of really ugly hacks that i'm not proud of having produced. When i started with this i though that it would be just a matter of reverting this commit 67de35f and then changing the Class#addBoundFunctions method a bit to make the compiled output match what i wanted. But things got pretty hairy pretty quick. Scoping issues, mutable state in destructuring parameters that prevented them from being used in more than one function (BTW, as of now, this produces some unnecessary code for bound methods with destructuring parameters), etc.

So, it's not production ready, but i wanted to open the PR to see if it would be a reasonable change or not. I personally wouldn't mind this being included or not, as i don't have much sympathy for bound methods.

[michaelficarra]

@epidemian: This implementation won't respect changes to Foo.prototype. Can we just save a reference to the prototype and do the member access within the bound function?

[epidemian]

@michaelficarra,

This implementation won't respect changes to Foo.prototype

That's right. It's aimed at fixing #2489 only. The current binding method doesn't respect changes to the prototype either.

Can we just save a reference to the prototype and do the member access within the bound function?

I think it's not that easy. At least, if those bindings are made at the same position they are made now, we would screw bound method overrides in subclasses again (#2773), because the bindings are made before the super call.

So, if the bindings are made before the super call, we screw method overriding, but if they are made after the super call we would also break something: the current inheritance behaviour is that if you call a bound method in the superclass constructor, it will actually call the subclass' version of the method. If we make the bindings after the super call then we would be screwing that behaviour i think.

In conclusion, i don't see an easy fix for #1819. This PR is not an attempt at that.

[mu-is-too-short]

I just came across this problem figuring out why someone's comparator method in a Backbone collection wasn't working:

Backbone collection comparator

Turns out that comparator: (item) => rather than comparator: (item) -> was confusing Backbone's arity check. Granted, I can't think of a reason for using a bound method for a comparator but the behavior was very confusing.

[epidemian]

Hey @mu-is-too-short, thanks for the mention :)

Granted, I can't think of a reason for using a bound method for a comparator [...]

Me neither. Mostly because i don't like bound methods in general hehe. But also because, as the Backbone snippet you pasted on the SO answer shows, Backbone.Collection::sort takes care of calling the comparator method with the correct instance.

However, it seems that this is yet another gotcha of bound methods: they don't respect function arity.

I know that relying of function arity in the JS world is generally kinda fragile (e.g. es5-shim's Function::bind does not respect it), but i think being able to ask a function its arity is a nice feature for a language, and we should embrace it, instead of contributing to that general fragility 😺

[GeoffreyBooth]

Closing as this would need heavy refactoring for CS2.