tc39 / proposal-bind-operator

This-Binding Syntax for ECMAScript
1.75k stars 30 forks source link

::object.func !== ::object.func #17

Closed lukescott closed 8 years ago

lukescott commented 9 years ago

I'm concerned that this syntax can be very misleading. For example:

var object = {
  clickHandler() {}
}

element.addEventListener("click", ::object.clickHandler);
// later in the code...
element.removeEventListener("click", ::object.clickHandler); // doesn't work!

console.log(::object.clickHandler === ::object.clickHandler); // false

It's not immediately clear the two are not the same thing - something new is being created each time, which is being masked by the hidden bind.

zenparsing commented 9 years ago

So there's some footgun potential here. This is a good point - thanks for bringing it up.

shovon commented 9 years ago

Which is precisely why azu/idempotent-bind was created.

Not sure how it can be handled with this new syntax. idempotent-bind applies a form of memoization, where as the :: operator uses Function.prototype.bind.

zenparsing commented 9 years ago

@shovon Unfortunately, there's no good place to store the memoization without "infecting" the entire object model. I see you're using WeakMap (which is cool) but that's not something we'd want to put into the core of the language.

@domenic what do you think? I feel like this might be fatal for the unary operator.

domenic commented 9 years ago

IMO this is totally fine... A few arguments for it:

Maybe @erights has more thoughts, since I know he was a fan of the unary operator version.

erights commented 9 years ago

@shovon , the memoization would be fatal unless you froze the bound function. Otherwise, merely sharing a frozen query-only-function would enable a communications channel through the memoization table. For example

Assume a SES or SES-like environment in which all the primordials are frozen.

Assume at this point that we have access to alice and bob that do not have any means to communicate with each other unless we provide such access. In other words, neglecting transitively immutable objects, the only connectivity between alice and bob at this moment is that we have access to both. Neither alice nor bob have access, direct or indirect, to each other.

const f = Object.freeze(function{return 3;});

alice(f);

bob(f);

Here, we have shared with alice and bob only a transitively immutable object, which should therefore not enable them to communicate. However

alice says:

const bound = ::f.toString; bound.x = somethingIWantToTellBob;

bob says:

const bound = ::f.toString; const somethingAliceWantedToTellMe = bound.x;

If there's no memoization, then alice and bob get separate bound functions that share only f, thereby not providing any mutability beyond that provided by f. On the other hand, if ::f.toString meant approximately

Object.freeze(f.toString.bind(f))

then the bound object would still not provide any mutability beyond that provided by f. In that case, memoizing it would not be fatal. But JS programmers might find this freezing surprising.

erights commented 9 years ago

@domenic I agree, but your first few examples weaken your argument. Unlike "delete" or "++", there's nothing side-effectly about ::f.x beyond the creation of a fresh identity. Your "new" example is perfect.

shovon commented 9 years ago

I see you're using WeakMap

@zenparsing Off-topic. I love the recognition, but I can't take credit for azu/idempotent-bind. I did however write shovon/idempotent.js, an unrelated library.

I know, it's easy to conflate between the two.

shovon commented 9 years ago

@erights that wasn't my point. I am fully aware of the plethora of things that can go wrong with sharing references, including sharing references to functions.

My point was that the "secret sauce" behind being able to generate a bound equivalent of a function in an idempotent manner was to use memoization. No where did I mention it was a silver bullet.

lukescott commented 9 years ago

@domenic When someone first looks at ::x.y they may not readily make the comparison to new x.y. I believe that the same problem also applies to x::x.y. The syntax isn't evident that something new is being created each time. The :: operator is also used in several other languages as a static operator. Using it in a dynamic context, even for existing JavaScript developers, will be quite surprising.

For example:

function foo() {
}
var foo1 = foo;
var foo2 = foo;
console.log(foo1 === foo2); // true

vs

function foo() {
}
var obj = {};
var foo1 = obj::foo;
var foo2 = obj::foo;
console.log(foo1 === foo2); // false?

vs

function foo() {
}
var obj = {};
var foo1 = foo.bind(obj);
var foo2 = foo.bind(obj);
console.log(foo1 === foo2); // definitely false!

Memorization in some form would fix this. Browser vendors would likely implement this in a different way than made possible in a pollyfill. For example, modifying ::x.y may end up modifying y. Since this isn't possible in a pollyfil, freezing the memorized function would be prudent to prevent such a pattern from being relied upon.

Given this is an ES7 proposed syntax, adding a static identifier with Object.defineProperty (w/ all the options set to false) to support the memorization wouldn't necessarily be a bad thing. It would only really be an issue with IE 8 and below - depending on the implementation (you would have more of an issue if adding an identifier to x of x::y than y since you are most likely to enumerate over x).

zenparsing commented 9 years ago

My feeling right now on this issue:

erights commented 9 years ago

You didn't state a conclusion though. Reading only slightly between the lines, you recommend that we pay the surprise-cost of inequality? That sounds fine to me.

On Sat, Jun 6, 2015 at 10:38 AM, zenparsing notifications@github.com wrote:

My feeling right now on this issue:

  • The "non-equality" of extracted methods will be surprising to some users. In other languages (e.g. Python, Ruby), extracted methods have equality.
  • Introducing a global memozation table of frozen bound methods to resolve the equality problem seems really heavy-handed, and I think it will bust our complexity budget for this feature.
  • The "frozen"-ness of bound methods coming out of this table will likely be surprising to users anyway (although perhaps less so than inequality).

— Reply to this email directly or view it on GitHub https://github.com/zenparsing/es-function-bind/issues/17#issuecomment-109622155 .

Text by me above is hereby placed in the public domain

Cheers, --MarkM

zenparsing commented 9 years ago

Very astute @erights : )

I was avoiding stating a conclusion because I'm not sure what I want yet, and I'm not sure it will be well-received.

First, I don't think we can get away with a memoization table.

But stepping back, what I really want is extension methods. I think this thing is awesome:

https://github.com/gsklee/bound-native-methods

And personally, I would be fine dropping method extraction to get just extension methods. Others might feel differently though, and that's OK.

erights commented 9 years ago

And personally, I would be fine dropping method extraction to get just extension methods

What are you proposing? What does "get just extension methods" mean? I don't get it.

nathggns commented 9 years ago

To be honest, I think the prefix form ::console.log is going to be far more welcomed than the arr::map(..) form.

zenparsing commented 9 years ago

Sorry, I shouldn't introduce terminology into the middle of a discussion without defining it.

I just mean the binary form of :: in this proposal.

Naddiseo commented 9 years ago

@zenparsing wrote:

Unfortunately, there's no good place to store the memoization without "infecting" the entire object model.

@lukescott wrote:

Given this is an ES7 proposed syntax, adding a static identifier with Object.defineProperty (w/ all the options set to false) to support the memorization wouldn't necessarily be a bad thing.

Am I correct in thinking you're both referring to something like adding a [@@boundTo] symbol/slot to a method? Which, without too much thinking, is the best way I can come up with to solve memoization without a global object.

lukescott commented 9 years ago

@Naddiseo I'm not sure memorization is really needed. The two use cases from the examples in the proposal seem to be about solving: function chaining for infix (utility libs) and method binding for prefix. Memorized binding is only needed as a result of foot stomping from a bi-product of the actual problems trying to be solved. As discussed in #18, if we split the two and work to solve those problems specifically, memorization should not be needed. -- Assuming of course that my assumptions are correct.

zenparsing commented 9 years ago

@erights In your opinion, is a memoization table of frozen bound functions arising out of this operator something doable from an implementation point of view? Like I said before, it seems really heavy-handed, but I don't want to assume anything.

erights commented 9 years ago

If I thought

::object.func === ::object.func

was an essential feature, I would implement the semantics a different way. Note that === must already case split before it compares, where it pointer compares only on one of the cases. Thus, having instances of value types compare by contents won't cause === to be slower on normal objects, so neither should the following proposal.

Internally, I would have the kinds of frozen bound function that you're talking about not actually be memoized, but rather, compare as === based on being the same binding of the same underlying function. If we get the freezing right, there should be no observable difference between such an implementation and a memoizing implementation. In fact, this very question is a good test of whether we got the freezing right.

Well, except for one thing :(.

We can explain why instances of value types cannot serve as keys in WeakMaps. However, for this internal equality change to not be observably different than memoization, these bound functions would need to be able to be WeakMap keys looked up according to these equality behaviors. In order for the WeakMap to serve its (technically unobservable) purpose, these entries in the WeakMap should still be collected when an equivalent key can no longer be created, i.e., when the underlying function and bound value are also deemed unreachable.

Having now gone through the implementation complexity on this path, I continue to advocate that

::object.func !== ::object.func

is the best solution.

Naddiseo commented 9 years ago

@erights, or solve it at the origin of the problem: where the method is defined (as @lukescott has suggested)

lukescott commented 9 years ago

For reference, I'm suggesting:

class Foo {
  handleEvent() => {
      // this is bound to instance
  }
}

Which is what #16 (edit, fix issue number) is suggesting, but re-use the =>. I think ::object.func is overkill if we can solve it at the source.

domenic commented 9 years ago

"Solving it at the source" is exactly wrong. We do not want a language feature to encourage that kind of bad programming. Instead we want one to make good programming easier, where you only create per-instance bound versions as necessary.

erights commented 9 years ago

Aha! After @Naddiseo 's message I tried to find which of @lukescott 's messages was meant. I completely missed this one and have not previously seen this suggestion. Interesting. I have to think about this.

erights commented 9 years ago

A lot is said at #15 and I am not finding this specifically. Can you point me at where this is explained, or better, state a self-contained explanation here? Can this syntax be explained by expansion to the existing language? Even approximately with prose stating remaining differences?

lukescott commented 9 years ago

Ah, sorry it was #16. It was mentioned as an additional syntax. There was some back and forth on several topics in #18. I'm suggesting that the idea in #16 replace ::object.func and re-use the fat-arrow syntax already in ES6.

Naddiseo commented 9 years ago

@domenic, forgive my ignorance here, but why is specifying it "bad" programming?

Are you saying this:

// This is bad?
function A() { this.bound = (function(){}).bind(this) };

// Because you're binding every instantiation
let objs = [];
for (i = 0; i <10; i++) {
    objs.push(new A());
}

// Even though you only maybe use the bound method for one instance?
let ret = objs[0].bound();

My counter example (which will probably show my ignorance):

function A() { this.method = function() {} };

function makeVirtualDom(ctrl) {
   // the method is rebound every time the dom is redrawn.
   return React.createElement('div', { onclick: ctrl.method.bind(ctrl) });
}

// but the constructor is only created once (which is why I was thinking "solving it at the source" would be better.
let ctrl = new A();
// "Redraw loop"
for (i = 0; i<10; i++) {
   redraw(makeVirtualDom(ctrl));
}

However, my counter example is only that: one specific usecase. But, the virtual dom callback situation is by far the biggest use of .bind in my code bases. @domenic, are you suggesting this pattern can be solved in a different way?

lukescott commented 9 years ago

@domenic Allowing methods to be copied from a class is bad programming. Other languages don't allow this. But it's already part of the language. Since it's already too late for that, I think it would be prudent to fix the binding at the source. Most of the time this is needed for event handlers / callbacks. Allowing an extension to => would be a good way of documenting that the method is a callback.

erights commented 9 years ago

From that, I still don't have a precise sense of what you are proposing. Can you explain your proposed semantics of

class Foo {
  handleEvent() => {
      // this is bound to instance
  }
}

precisely in terms of an expansion to the existing language? If not, please try to get as close as possible by expansion, and then explain remaining differences in prose.

lukescott commented 9 years ago

@erights I copied this from #16,

var Foo = (function() {
    var Foo = function() {
        this.handleEvent = this.handleEvent.bind(this);
    };

    Foo.prototype.handleEvent = function() {
        // this will always be instance of Foo
    };

    return Foo;
})();

The idea is the same, just different syntax. method() => {} instead of ::method() {}.

zenparsing commented 9 years ago

@Naddiseo @lukescott AFAICT, it would mean creating a set of new bound functions for each and every instance (duplicating per-instance state), which is not really desirable.

lukescott commented 9 years ago

@zenparsing ::object.func creates a newly bound function too. By doing method() => {} the original method gets replaced with the correct binding. It's also an opt-in feature. You would only do it on event handlers - not every method.

Naddiseo commented 9 years ago

@zenparsing, I see what you're saying. Do you know how other languages solve the problem?

zenparsing commented 9 years ago

@lukescott But how are users supposed to know when to opt-in, and when not to? It's likely that, given the choice and imperfect understanding, they will just opt for doing => methods every time. I think that's what @domenic means by encouraging.

sebmck commented 9 years ago

You can already get instance-bound methods with two current class extension proposals, does there really need to be a third?

Class properties

class Person {
  constructor(firstName, lastName) {
    this.firstName = firstName;
    this.lastName  = lastName;
  }

  getName = () => `${this.firstName} ${this.lastName}`;
}

new Person("John", "Doe").getName.call(null); // "John Doe"

Decorators

function autobind(target, key, descriptor) {
  return {
    get: function () {
      return descriptor.value.bind(this);
    }
  };
}

class Person {
  constructor(firstName, lastName) {
    this.firstName = firstName;
    this.lastName  = lastName;
  }

  @autobind
  getName() {
    return `${this.firstName} ${this.lastName}`;
  }
}

new Person("John", "Doe").getName.call(null); // "John Doe"
erights commented 9 years ago

Yes, this is essentially simulating objects-as-closures, which, if implemented by expansion, requires allocating a function per-method * per-instance. The original class proposals from long ago proposed that support for this be moved into the implementation where it can be optimized. (The E implementation, while low performance in other regards, does demonstrate the needed optimization.)

However, implementors, for understandable reasons, balked. The needed optimization is approximately orthogonal to their existing huge investment in optimizations, and the two would interact leading to even more complex optimization code.

Unlike these earlier class proposals, there's also a semantic problem with the expansion as shown: inheritance. If a subclass overrides handleEvent, the handleEvent in the superclass constructor will simply override the one in the subclass constructor.

lukescott commented 9 years ago

@zenparsing The semantics of () => {} should be known. So I'm not sure it's true that people will opt for it every time. Also the engine could possibly optimize this by not creating a new function / wrapping the original.

@ssube That's part of the class properties proposal. method() => {} seems like a natural expansion of the drafted ES6 spec.

ssube commented 9 years ago

@Naddiseo Many languages with this kind of feature make "binding" a function very cheap, as they just push the context onto the stack rather than allocating anything (essentially just always .applying the function).

@sebmck Encouraging class properties for bound methods works logically (i.e., methods are on the prototype and these bound methods are on the instance) but feels less readable. It's the same function each time, just bound to a different context.

erights commented 9 years ago

@sebmck I like the @autobind decorator a whole lot. Now that I've see it, I think I would actually use it. We could also have it provide a setter that works around the override mistake, much like SES's tamperProof uses a setter for this purpose.

It has the advantage that it does not prepay a multiplicative allocation cost. It works well with inheritance. If stdized and directly provided by the implementation, where the implementation can recognize and special case it, the implementation could realize that

   person.getName()

does not actually require the extra allocation. Skipping that extra allocation would actually be unobservable, which the implementation can know by recognizing its own getter. This does a surprising amount of the optimizations I originally had in mind! Congrats!

However, it still has the same cost that motivated this thread in the first place:

    person.getName !== person.getName

This cost will not stop me from using this decorator, nor from encouraging others to. I encourage someone to propose it for standardization, so implementations can eventually skip the extra allocation.

Thanks!

sebmck commented 9 years ago

@erights The example was a bit concise, but to get person.getName === person.getName (and better perf) you could change the autobind decorator to:

function autobind(target, key, descriptor) {
  return {
    get: function () {
      var fn = descriptor.value.bind(this);
      Object.defineProperty(this, key, { value: fn });
      return fn;
    }
  };
}
erights commented 9 years ago

Which mutates the instance, which might be frozen. This cure is worse than the disease.

sebmck commented 9 years ago

Ah right, good point, hadn't considered that.

ssube commented 9 years ago

The instance being frozen shouldn't be an issue if this is done in the constructor (as @lukescott showed) rather than using a decorator.

domenic commented 9 years ago

@Naddiseo yes, that is what I am saying. The fact that React requires such wasteful programming patterns is an unfortunate relic of its architecture, and should not inform language design. (Compare to other frameworks.)

erights commented 9 years ago

Doing it in the constructor requires prepaying the multiplicative allocation cost.

ssube commented 9 years ago

@erights Is that a huge problem? Allocating objects is (in my experience) already understood to be relatively expensive. It increases the cost, but not in a spot that would be surprising.

Naddiseo commented 9 years ago

@sebmck, good point, and I hadn't thought to use a decorator. (slightly off topic) I just did some perfing with the options to see if there were any reasons to choose one over the other. The results are interesting, but I'm not sure how valid they are given that creation could be optimized by the engine.

Class creation: using class properties is a lot faster, docorators are very slow Class instantiation: Decorators fastest, @lukescott's method is slowest Bound method calling: class properties are much faster, decoratored are much slower.

@domenic, I accept defeat on the subject. You're right, the framework architecture shouldn't force the language down dark alleys and beat it with a crowbar.

erights commented 9 years ago

The multiplicative cost is why the objects-as-closures pattern is not widely enough used, and why early class proposals based on them failed to achieve consensus. Although this decorator would also need optimization, unlike those class proposals, these optimizations should be much more self-contained.

Even without these optimizations, I will be using @autobind a lot. Thanks!

erights commented 9 years ago

Although it is obviously too late, I am trying to remember whether we had considered making the @autobind behavior the default for classes. I don't remember. If we had, I don't remember why we rejected it.

A shame. Perhaps to reconsider for const classes and for StrongScript classes.

erights commented 9 years ago

@ssube @lukescott Unlike @autobind, doing it in the constructor does not work with inheritance. This is a fatal flaw that has nothing to do with performance.

lukescott commented 9 years ago

@erights @sebmck perhaps I'm missing something, but inheritance doesn't work with autobind either. I get Person instead of Luke:

class Person {
  @autobind
  logthis() {
    console.log(this.constructor.name);
  }
}

class Luke extends Person {
  logthis() {
    super.logthis();
  }
}

function autobind(target, key, descriptor) {
  return {
    get: function () {
      var fn = descriptor.value.bind(target);
      //Object.defineProperty(target, key, { value: fn });
      return fn;
    }
  };
}

var luke = new Luke;
var logthis = luke.logthis;
logthis();

Without the autobind and calling the method I get Luke:

class Person {
  logthis() {
    console.log(this.constructor.name);
  }
}

class Luke extends Person {
  logthis() {
    super.logthis();
  }
}

var luke = new Luke;
luke.logthis();

I had to comment out Object.defineProperty(target, key, { value: fn }) because I was getting Cannot redefine property: logthis on babel.

The original code had this, but that was undefined. I assumed it was supposed to be target.