Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Function.length in fat arrow class method #2489

Closed
leeola opened this Issue Aug 11, 2012 · 24 comments

Comments

Projects
None yet
7 participants

leeola commented Aug 11, 2012

In the following code, how is it possible to get the length of wtf.bar ?

foo = (a, b, c) ->
bar = (a, b, c) =>

class Wtf
  foo: (a, b, c) ->
  bar: (a, b, c) =>

wtf = new Wtf()
results =
[
  foo.length
  bar.length
  wtf.foo.length
  wtf.bar.length
]
console.log results
# Ouputs: [ 3, 3, 3, 0 ]

As you can see, the combination of the fat arrow and a class instance causes wtf.bar.length to return zero. Is there a workaround for this? Is this "working as intended"? All sorts of various code uses the handy Function.length checks, and the inability to trust Function.length if the user declared the function as a fat-arrow function could cause problems (and already has for me).. Thoughts?

leeola commented Aug 11, 2012

For convenience, here is what the above code compiles to:

var Wtf, bar, foo, results, wtf,
  _this = this,
  __bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };

foo = function(a, b, c) {};

bar = function(a, b, c) {};

Wtf = (function() {

  function Wtf() {
    this.bar = __bind(this.bar, this);

  }

  Wtf.prototype.foo = function(a, b, c) {};

  Wtf.prototype.bar = function(a, b, c) {};

  return Wtf;

})();

wtf = new Wtf();

results = [foo.length, bar.length, wtf.foo.length, wtf.bar.length];

console.log(results);
Collaborator

michaelficarra commented Aug 11, 2012

0 is the length of wtf.bar. If you want the length of the function it's calling internally, access Wtf::bar.length.

leeola commented Aug 11, 2012

I wish this was simply possible...

  __bind = function(fn, me){
      f = function(){ return fn.apply(me, arguments); };
      f.length = fn.length;
      return f
    };

But alas, it seems not to be.

Collaborator

michaelficarra commented Aug 11, 2012

Yep, length is a non-writable property.

leeola commented Aug 11, 2012

0 is the length of wtf.bar. If you want the length of the function it's calling internally, access Wtf::bar.length

Oh i am aware that it is the length of wtf.bar, i feel that it is incorrect to convert the intention of => into wrap your function.

For example, when you use => from outside of a class it basically means "make this refer to the current context, no matter where the function is run from". However, in a class, that is not what it means. => inside a class, and => outside of a class have two distinct differences, and their usage differs.

The point of binding a function is to allow that function to be passed anywhere while preserving the context of this, correct? If so then the usage of => inside of a class works, but it also has side effects. The side effects being that the function you attempt to use, is not your function. Is is a wrapped function, with different contents, and in this example, a different Function.length.

leeola commented Aug 11, 2012

And my point being, is simply.. is this wrong? Should the side affects be addressed? Wtf::bar.length does not work if the function is passed somewhere else, such as a callback in another library. That library is simply going to ignorantly assume that callback.length is the length you want to measure..

leeola commented Aug 12, 2012

@michaelficarra

Why was this issue closed? My point is that CoffeeScript's implementation is not equal to that of Function.prototype.bind, and that it should be resolved.. is this disagreed with? Methods don't get the same equality as Functions?

edit: Is there no way to make a custom implementation of => and __bind that matches Function.prototype.bind?

Collaborator

michaelficarra commented Aug 12, 2012

I'd love to see your suggested compilation. Without abusing eval, there's no way to do what you're asking.

Contributor

epidemian commented Aug 12, 2012

Without abusing eval, there's no way to do what you're asking.

Unless using Function::bind. But that wouldn't work on IE 6-8, so it wouldn't be acceptable.

@michaelficarra, BTW (and semi-off-topic), are you planing on adding different JS runtime "targets" in the new CS compiler? For example, if someone wanted to target only ECMAScript 5 compliant runtimes, the compiler could rely on things things like Array::indexOf or Function::bind being present at runtime and therefore generate smaller/simpler code.

leeola commented Aug 12, 2012

Are compile options a common thing? Perhaps we could allow for optional targets that would allow CoffeeScript to compile using modernized functions, rather than pigeonholing CoffeeScript into a browser compatibility box? (ie, to actually use Function.prototype.bind) ?

leeola commented Aug 12, 2012

@epidemian hah, i wrote my post before i saw yours (just couldn't submit it at the time). Same train of thought.

Collaborator

satyr commented Aug 13, 2012

Without abusing eval, there's no way to do what you're asking.

Since the parameters are known at compile time,

this.bar = __bind(this.bar, this);

could be:

var _this = this;
this.bar = function(a, b, c) {
  return Wtf.prototype.bar.apply(_this, arguments);
};

No need to use eval there.

leeola commented Aug 13, 2012

Give @satyr a medal!

Is there anything this feature is lacking? For example, the only thing i know of that __bind is lacking is Function.length, so is there any shortcomings for

var _this = this;
this.bar = function(a, b, c) {
  return Wtf.prototype.bar.apply(_this, arguments);
};

?

Collaborator

michaelficarra commented Aug 13, 2012

so is there any shortcomings [...]

  • Lots of repeated boilerplate to account for one uncommon scenario.
  • It's not like it proxies any other properties of the function -- it just has the same length now.
  • Slight change of the behaviour of bound methods. They would now respect prototype changes after the instance they're bound to has been created. See #1819 and #1821.

But yeah, @satyr's solution works.

Collaborator

michaelficarra commented Aug 13, 2012

@epidemian: In a way, yes. The typical usage of the compiler will be simply jsAST = CoffeeScript.Compiler.compile csAST, options, but one may customise the compilation process by instantiating a new compiler and overriding rules for their target environment.

es6Compiler = new CoffeeScript.Compiler
es6Compiler.addRule CoffeeScript.Nodes.Class, ->
  # create native JS "class" node
jsAST = es6Compiler.compile csAST, options

If some compilation targets are commonly used, I may pull in sets of predefined rules.

@jashkenas jashkenas closed this in cf11a57 Mar 4, 2013

Collaborator

michaelficarra commented Mar 7, 2013

@michaelficarra michaelficarra reopened this Mar 7, 2013

Owner

jashkenas commented Mar 17, 2013

So the attempted fix earlier was totally a minehole of breakage and disaster. Is there a workable fix, or should we retire this issue?

leeola commented Mar 18, 2013

I think the conclusion was that there is no "fix", and that compile targets (eg, use __bind() if you want old browser support, Function.bind() for ~modern) is the only solution. Unless i am mistaken, Redux will help quite a bit on this front.

edit: What am i thinking, @satyr totally had a solution. My apologies. It's not a true fix, and compile targets will still be better, but at the very least his solution completely resolves the Function.length issue.

Contributor

epidemian commented Mar 18, 2013

While i certainly would prefer the use of Function#bind to keep the value of the length property of bound methods, there could be other solutions.

Considering that we know how many parameter a bound method expects (otherwise the behaviour on 1.6.0 wouldn't be possible), then:

class Foo
  method: (bar, baz) =>
    @qux

Could generate a constructor like:

function Foo() {
  this.method = __bind2(this.method, this);
}

Where __bind2 could be defined as:

var __bind2 = function(fn, me){ 
  return function(p1, p2){ return fn.apply(me, arguments); }; 
};

Quite ugly, i know, as it would require multiple variations of __bind<n> if one uses bound methods with different number of parameters.


Another possibility is to inline the definition of __bind<n>, with the extra benefit of being able to preserve the parameter names:

function Foo() {
  var _this = this, _method = this.method;
  this.method = function(bar, baz) {
    return _method.apply(_this, arguments);
  };
}

I find this solution to be no more verbose than the one used in 1.6.0, while fixing the length property issue and keeping the same behaviour as __bind). What do you think?


Another possibility is doing some eval magic on __bind to generate a function with the same number of parameters as the length property of the passed function. It is very very ugly though; not worthy if it's only to preserve the length of bound functions.

Owner

jashkenas commented Mar 18, 2013

My apologies. It's not a true fix, and compile targets will still be better, but at the very least his solution completely resolves the Function.length issue.

... we tried it. It breaks a whole slew of other stuff. It's not a viable change.

Where __bind2 could be defined as..

That's not going to cut it either, I'm afraid.

while fixing the length property issue and keeping the same behaviour as __bind). What do you think?

I have some serious doubts that it will actually preserve the current behavior -- but feel free to give it a shot, and see if the test cases pass. If they do, then we're gold.

Another possibility is doing some eval magic

Not gonna happen.

leeola commented Mar 18, 2013

So i guess the only option is eventual compile targets?

@epidemian epidemian added a commit to epidemian/coffee-script that referenced this issue Mar 21, 2013

@epidemian epidemian Fixes #2489: gets rid of __bind and instead use its inlined form, dec…
…laring the same parameters in the bound function as in the prototype's one

Ugliest code ever 💩
51581e8

@epidemian epidemian added a commit to epidemian/coffee-script that referenced this issue Mar 21, 2013

@epidemian epidemian Fixes #2489: gets rid of __bind and instead use its inlined form, dec…
…laring the same parameters in the bound function as in the prototype's one

Ugliest code ever 💩
137b3da

@epidemian epidemian added a commit to epidemian/coffee-script that referenced this issue Mar 21, 2013

@epidemian epidemian Fixes #2489: gets rid of __bind and instead use its inlined form, dec…
…laring the same parameters in the bound function as in the prototype's one

Ugliest code ever 💩
9e3ca4f

@marchaefner marchaefner added a commit to marchaefner/coffee-script that referenced this issue Nov 26, 2013

@marchaefner marchaefner Fixes #2489 -- Preserve bound methods arity
  * 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
6503876
Collaborator

GeoffreyBooth commented Apr 19, 2017 edited

Believe it or not, this is still an issue in CS2. The example above now (2.0.0-beta1) compiles to:

var Wtf, bar, foo, results, wtf,
  bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };

foo = function(a, b, c) {};

bar = (a, b, c) => {};

Wtf = class Wtf {
  constructor() {
    this.bar = bind(this.bar, this);
  }

  foo(a, b, c) {}

  bar(a, b, c) {}

};

wtf = new Wtf();

results = [foo.length, bar.length, wtf.foo.length, wtf.bar.length];

console.log(results);

And still outputs [ 3, 3, 3, 0 ].

But if we change the class part to:

Wtf = class Wtf {
  constructor() {
    this.bar = (a, b, c) => {}
  }

  foo(a, b, c) {}

};

It suddenly outputs [ 3, 3, 3, 3 ] as desired. And we would be able to get rid of the bind helper function, as we would no longer need it. @connec, does this seem like a doable change?

@GeoffreyBooth GeoffreyBooth changed the title from Fat Arrow and Function.length? to Function.length in fat arrow class method Apr 19, 2017

@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 19, 2017

@GeoffreyBooth GeoffreyBooth added bug and removed enhancement labels Apr 19, 2017

Collaborator

GeoffreyBooth commented Apr 26, 2017

Thanks to #4526, this now compiles to:

var Wtf, bar, foo, results, wtf;

foo = function(a, b, c) {};

bar = (a, b, c) => {};

Wtf = class Wtf {
  constructor() {
    this.bar = this.bar.bind(this);
  }

  foo(a, b, c) {}

  bar(a, b, c) {}

};

wtf = new Wtf();

results = [foo.length, bar.length, wtf.foo.length, wtf.bar.length];

console.log(results);

And it outputs [ 3, 3, 3, 3 ] 😄 So this bug is finally fixed.

I’m not sure it’s worth the effort to compile to the alternative in my last comment, or if that’s even an improvement, but that can be left for another issue or PR.

@GeoffreyBooth GeoffreyBooth added fixed and removed bug labels Apr 26, 2017

Collaborator

connec commented Apr 26, 2017

I think the current compilation is best - the method still exists on the prototype, but a 'copy' is bound to the instance on construction. It means a bound method is still a method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment