Skip to content

Fix #2489 once more -- preserve bound methods arity #3260

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

Closed
wants to merge 1 commit into from

Conversation

marchaefner
Copy link
Collaborator

Since compiler hacking is way more fun than CLI fixing, here's my version of "Fix #2489 again"

  • Adds a "private" _bindMethods function wherein all binding takes place.
    This preserves the readability of the compiled constructor and avoids any potential memory leaks (related: Fat arrow leaks generated _this reference to other closures #3143).
  • Wrappers clone the parameters of the wrapped method and therefore have the same number of parameters and, for the most part (see example below), the same parameter names.
  • Superclass constructors do neither override previously bound methods (as in redux) nor mindlessly wrap already wrapped methods (as is currently happening)

This comes with a hefty dose of ugly. Though I doubt the compilation could get much nicer. Compare #3258 which fixes this issue in a different way (prettier classes but more helpers).

Based upon prior art by @epidemian and @michaelficarra. Feel free to bash...

Side effect

This PR unintentionally "fixes" #1819 (the same way redux does). This breaks other fun stuff:

class Nobody then expect: => console.log 'fluffy bunny'
callback = (new Nobody).expect
# ...
setTimeout callback, 1000
# ...
Nobody::expect = -> console.log 'spanish inquisition'

I can't really assess the severity of this change since I can't imagine why anybody would ever do anything like that (or #1819 for that matter).

Example compilation

class Foo extends Bar
  baz: (@this=1, that, [which, what]) =>
  qux: (args...) =>
/* ... */

Foo = (function(_super) {
  var _bindMethods;

  __extends(Foo, _super);

  _bindMethods = function(_this) {
    if (Foo.prototype.baz === _this.baz) {
      _this.baz = function(_this1, that, _arg) {
        return Foo.prototype.baz.apply(_this, arguments);
      };
    }
    if (Foo.prototype.qux === _this.qux) {
      _this.qux = function() {
        var args;
        return Foo.prototype.qux.apply(_this, arguments);
      };
    }
  };

  function Foo() {
    _bindMethods(this);
    return Foo.__super__.constructor.apply(this, arguments);
  }

  Foo.prototype.baz = function(_this, that, _arg) {
    /* ... */
  };

  Foo.prototype.qux = function() {
    /* ... */
  };

  return Foo;

})(Bar);

  * Add `_bindMethods` to class body and do all method binding there
  * Add `Code::fakeSignature` flag
    (Suppresses compilation of parameter defaults and destructuring)
  * Reuse (cloned) method parameters in wrapper
  * Remove `__bind` helper
@GeoffreyBooth
Copy link
Collaborator

Closing as this would need heavy refactoring for CS2.

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

Successfully merging this pull request may close these issues.

Function.length in fat arrow class method
2 participants