tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Private members break proxies #106

Closed jods4 closed 5 years ago

jods4 commented 6 years ago

I commented on an old issue in tc39/proposal-private-fields#102 but after reading the answer I feel like I should re-open an issue.

It's been discussed before: current PrivateName spec doesn't tunnel through proxies. In my opinion the consequences have been under-evaluated.

Proxies are already quirky: they don't work with internal slots (bye Number, Date, Promise and friends), they change the identity of this (no equality, no WeakMap). But at least, they work on classes.

So they are useful. Libraries and frameworks can provide many features that motivated building proxies: automatic logging, lazy objects, dependency detection and change tracking.

My point here is that PrivateName and Proxy don't work together. You have to choose one of the two features and give up on the other. Partitionning JS features in this way is terrible.

Here is a basic example. Let's say a library provides logReads, a function that writes on the console every member that is read.

function logReads(target) { 
  return new Proxy(target, {
    get (obj, prop, receiver) {
      console.log(prop);
      return target[prop];
    },
  });
}

Now let's say I'm writing an application and I use private fields for encapsulation, because they're nice.

class TodoList {
  #items = [];
  threshold = 50;

  countCheap() {
    return this.#items.reduce((n, item) => item.price < this.threshold ? n : n + 1, 0);
  }
}

I would like to use that nice logging library to better understand what happens when I run my code. Seems legit from a naive user's perspective:

let list = logReads(new TodoList);
list.countCheap(); // BOOOM

Ahaha gotcha! And if you don't know the source code, why it crashes when inside a proxy might be a unpleasant mystery.

Please reconsider. All it takes to solve this is make PrivateName tunnel through proxy (no interception, encapsulation is fine).

Don't think that returning bound functions from the proxy will solve this. It might seem better but creates many new issues of its own.

ljharb commented 6 years ago

The purpose of Proxy is not to be a transparent wrapper alone - it also requires a membrane (iow, you need to control access to every builtin). Specifically yes, your proxy has to return a proxy around every object it provides, including functions.

It would be nice if Proxy was indeed a transparent replacement for any object, but that’s not it’s purpose and as such, it does not - and can not - work that way.

jods4 commented 6 years ago

@ljharb I am not sure I fully understand your answer, could you explain a little more?

Is logReads above foolish? It's supposed to log (non-private) members access on TodoList (not deep, just that instance) and it actually works today (as long as you don't rely on WeakMap).

How should I write logReads so that it works then?

Looking at MDN page on proxies: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy I have the impression several examples won't work anymore if you apply them to a class with private members.

Let's take the first one, "Basic example". It provides default values: anything undefined on proxified object is 37.

var handler = {
    get: function(obj, prop) {
        return prop in obj ?
            obj[prop] :
            37;
    }
};

var p = new Proxy({}, handler);
p.a = 1;
p.b = undefined;

console.log(p.a, p.b); // 1, undefined
console.log('c' in p, p.c); // false, 37

Wrap TodoList instead:

var p = new Proxy(new TodoList, handler);
p.c; // 37, yay.
p.activeCount(); // BOOOM
bakkot commented 6 years ago

You need to proxy methods if you want to make your proxy transparent. Of the top of my head, something like

const objectProxies = new WeakMap;
const proxiedFunctions = new WeakMap;
function logReads(target) { 
  const proxy = new Proxy(target, {
    get (obj, prop, receiver) {
      console.log(prop);
      const value = target[prop];
      if (typeof value === 'function') {
        if (proxiedFunctions.has(value)) {
          return proxiedFunctions.get(value);
        }
        const newFun = new Proxy(value, {
          apply (original, thisArg, args) {
            return original.apply(objectProxies.has(thisArg) ? objectProxies.get(thisArg) : thisArg, args);
          },
        });
        proxiedFunctions.set(value, newFun);
        return newFun;
      }
      return value;
    },
  });
  objectProxies.set(proxy, target);
  return proxy;
}

// Some random business class, using private fields because they're nice
class TodoList {
  #items = [];

  get getter() {
    return this.#items.reduce((n, item) => item.done ? n : n + 1, 0);
  }

  method() {
    return this.#items.reduce((n, item) => item.done ? n : n + 1, 0);
  }
}

// Try to use the class while logging reads
let list = logReads(new TodoList);
list.getter; // works!
list.method(); // works!
list.method.call(logReads(new TodoList)); // works!
list.method === logReads(new TodoList).method; // true!

Yes, the basic examples in the docs will break if you try to use them on an object with private fields, just as they'll break if you try to use them on an object which has been put into a WeakMap, or on an object where someone is testing equality, or an object with internal slots other than those for an array or function. But it's important to keep in mind here that private fields aren't actually giving the designer of the class any power they didn't have previously, just making it more ergonomic (and optimizable and explicit and so on). Anyone intending to provide transparent proxies has always needed to do the above work.

I don't think this is a bug in the design of WeakMaps or proxies (or private fields). This is how they're intended to work together. Proxies are not intended to allow you to violate invariants in other people's code.

ljharb commented 6 years ago

I’d do get(obj, prop, receiver) { return Reflect.has(obj, prop) ? Reflect.get(obj, prop, receiver) : 37; } - but if the property is a function, the only option you have is to return a proxy for that function that does the same thing on the apply trap.

jods4 commented 6 years ago

Please consider:

  1. Proxies from the MDN docs or like my example exist and work (within existing limitations) today. When coders decide to replace their typescript private fieldwith real #field, they will be quite surprised that their application breaks. This can be very tricky in large projects where responsibility of conflicting code is split across teams. Adopting private fields might break legitimate code.

  2. @bakkot your example "works" as in "doesn't TypeError" but doesn't achieve what my function does. Specifically it never logs items, which is the very purpose of the proxy: detect what fields are accessed when running opaque code. The problem is this bit of code: original.apply(objectProxies.has(thisArg) ? objectProxies.get(thisArg) It calls the function transparently with the original this instead of proxy. That makes PrivateName work, but that bypasses the proxy for anything accessed inside the function. That previously working code is impossible to replicate once private fields are introduced.

@ljharb

but if the property is a function, the only option you have is to return a proxy for that function that does the same thing on the apply trap.

See 2. above. It doesn't achieve what I intend to do.

  1. You keep coming back at WeakMap... Yes, it's a trap. I would be glad if new features could be more compatible with older ones. But please observe that the comparison is not as good as you make it to be. Sometimes WeakMap do work, private fields never do. For instance, if you wrap your ViewModel in a proxy for dependency detection from the very beginning, right when it's constructed, then only the proxified this is seen in code and it works using WeakMap and this comparison. You only need to ensure you don't share both the target and its proxy. Private fields never work when invoked on a proxy.

  2. Damn that's large chunk of code for seemingly basic needs. Funny how no introduction to proxies looks like that, makes me wonder how many people use those "fragile" proxies in real code?

I have made several points showing that not tunneling private fields is bad. I have proposed a reasonable and simple solution: tunnel private fields through proxies. Everything above just works.

You seem opposed to that idea. What is your motivation? What makes not tunneling private fields through proxies better for the language or the community? What benefits outweight the drawbacks I have mentionned?

bakkot commented 6 years ago

@jods4,

Adopting private fields might break legitimate code.

That's true anyway.

Specifically it never logs items, which is the very purpose of the proxy: detect what fields are accessed when running opaque code.

Assuming you meant some other, public field (it certainly shouldn't log #items!) - yes, that's true. But introspecting on what code you don't own is doing has never been a goal of proxies. You still get all the traps for your code, to which you can provide the proxy rather than its target.

For instance, if you wrap your ViewModel in a proxy for dependency detection from the very beginning, right when it's constructed

If the constructor is your code, and you want it instances of it to have both proxy behavior and a private field, you should just put the private fields on the proxy itself: instead of

class ViewModel extends Object /* or whatever */ {
  #priv = 0;
  method(){ return this.#priv; }
}
foo = new Proxy(new ViewModel, { /* handler */ });
foo.method(); // throws

do

function adapt(base, handler) {
  return class extends base {
    constructor(...args){
      return new Proxy(super(...args), handler);
    }
  };
}

class ViewModel extends adapt(Object /* or whatever */, { /* handler */ }) {
  #priv = 0;
  method(){ return this.#priv; }
}
foo = new ViewModel;
foo.method(); // works

Then you get private fields and also the proxy traps.

Funny how no introduction to proxies looks like that, makes me wonder how many people use those "fragile" proxies in real code?

Yes, I think it's kind of a shame that the documentation often does not make it clear that making proxies actually transparent is not trivial. But this is already the case. I don't think that justifies changing the semantics of private fields in the language.

You seem opposed to that idea. What is your motivation? What makes not tunneling private fields through proxies better for the language or the community? What benefits outweight the drawbacks I have mentionned?

The major benefits in my mind are:

  1. Other people don't get to violate invariants of my code. If I have
class Factory {
  static #nextId = 0;
  #id = Factory.#nextid++;
  constructor() {
    Object.defineProperty(this, 'prop', { value: 'always present' });
  }
}

then I can trust that there is one and only one object with a given #id. I can further trust that objects with #id have any other invariants I care to enforce: in this example, that they have a prop property whose value is "always present". Since the whole point of private fields is to allow class authors to maintain and reason about invariants of their code without exposing their internals to the world, this is not something to lightly forgo.

  1. The mental model for private fields is consistent: fields are attached to objects by the constructor; objects which were not created by the constructor do not have the private field, and as such attempting to access them will throw.
zenparsing commented 6 years ago

There are good reasons mentioned in this thread for leaving the current behavior as-is. Remember that one of the use cases for "private" fields is being able to more easily self-host fully encapsulated built-ins and platform APIs.

I also think that @jods4 makes some important observations: that if private field usage were to proliferate among user code it will break certain usage patterns based on proxies. Furthermore, this consequence is not particularly obvious to users given the current ("just replace _ with #") syntax.

(To me, this is another argument for having different syntax for different semantics.)

ljharb commented 6 years ago

It's a brand check - that's the point. It's supposed to change behavior.

jods4 commented 6 years ago

@bakkot

[introducing private can break...] That's true anyway.

Do you have examples? If the field you're encapsulating was truly private I don't see how. If it was used by outside code, sure. You can't encapsulate a non-encapsulated field. But then it's good that it breaks as it shows problems in your design.

The example I give worked and I don't see why it shouldn't work with private fields.

Assuming you meant some other, public field

Yes, sorry. Of course I meant public threshold field not private items.

But introspecting on what code you don't own is doing has never been a goal of proxies.

I'm not in the know of ES editors motivations, but...

If the constructor is your code [...]

That kind of would work. It's quite constrained, though, notably when it comes to using base classes. Correct me if I'm wrong but for example I don't think it's gonna work well if the base class uses private fields itself?

And that's one problem right here: encapsulation means you shouldn't know nor care whether base class has private fields or not. Yet with proxies it's a crucially observable difference.

Consider that I might not control my base classes. It might come from another team or a library. A new version of the library that encapsulate its private fields can break consumer code. -> Brittle base class problem, not good.

I don't think that justifies changing the semantics of private fields in the language.

Private fields are not in the language now, so we don't change their semantic per se. We change the proposal. I have demonstrated that changing the proposal can reduce breakage during private fields adoption, that seems worth considering to me.

Moving on to the [two] major benefits [of sticking with how it is]:

  1. I can trust that there is one and only one object with a given #id. The whole point of private fields is to allow class authors to maintain and reason about invariants of their code without exposing their internals to the world.

I don't get how tunneling private access through proxy to the actual target would break that. Note that I don't want to intercept private access. I want it to tunnel to the real target and work. Can you provide an actual example where tunneling would permit creating 2 Factory instances with the same #id?

  1. The mental model for private fields is consistent: fields are attached to objects by the constructor; objects which were not created by the constructor do not have the private field, and as such attempting to access them will throw.

I see what you mean and I personnally wouldn't agree.

Public fields are also attached to objects by the constructor, objects which were not created by the constructor do not have the public field, and yet they work on a proxy.

I could give more reasons why I disagree here but anyway I think this point is much weaker than problems above. In itself it can't justify not tunneling private fields.

jods4 commented 6 years ago

I don't know if it matters to you but it seems using Proxies pretty much as I'm describing here is on VueJS roadmap:

Reactivity system will be rewritten with Proxies with various improvements https://github.com/vuejs/roadmap

I can't speak for VueJS authors but I'm pretty sure classes using private fields internally won't work in VueJS vnext. I know Aurelia is considering the same move.

bakkot commented 6 years ago

Do you have examples?

Sure:

class Foo {
  metadata = makeMeta(this); // may return null
  getName() {
    return (this.metadata || {}).name;
  }
}
Foo.prototype.method.call({});

metadata really is effectively private in that code outside the class never attempts to access it, but making it private will still break this code.

I agree that's not ideal, but I don't think it's worth giving up the privacy model. Other usages of private fields will want to rely on the this.#id throwing for non-instances.

If I should only intercept my own code, then really you shouldn't have bothered with something as complex as proxies. I can instrument my own code alright without them.

Sorry, let me be more precise: you can use proxies to observe (some of) the behavior of code which consumes objects you provide it. So if you're sitting between Alice and Bob, when Alice gives you an object you can wrap it in a proxy before giving it to Bob. But you can't wrap it in a proxy before giving it to Alice.

I should have said "introspecting on what code you don't own is doing with objects it creates".

Correct me if I'm wrong but for example I don't think it's gonna work well if the base class uses private fields itself?

Yes, in that case you'd need to proxy the base class as well.

Can you provide an actual example where tunneling would permit creating 2 Factory instances with the same #id?

A proxy for an object is not the object itself. let x = new Factory and new Proxy(x, {}) are two different objects with the same #id. If code in my class treats them as the same, that will break. So will any other code relying on object identity of things which have the private field, such as this linked list class which avoids cycles (unless private fields tunnel through proxies):

class Node {
  #child = null;
  link(child) {
    let cursor = child;
    while (cursor !== null) {
      if (cursor === this) {
        throw new TypeError('Cycle!');
      }
      cursor = cursor.#child;
    }
    this.#child = child;
  }
  getTail() {
    let cursor = this;
    while (cursor.#child !== null) {
      cursor = cursor.#child;
    }
    return cursor;
  }
}
let x = new Node;
x.link(new Proxy(x, {}); // currently throws, but with tunneling it works
x.getTail(); // currently always terminates, but with tunneling it now spins in this example

Also, though, I want to focus on the other point I made in that paragraph, since it is the core conflict. The point of private fields is to let class authors maintain invariants without exposing their internals to the world. Invariants are not just the values in private fields. Allowing code outside the class to create a proxy for the class's instances which still have their private fields allows code outside the class to break those invariants.

bakkot commented 6 years ago

encapsulation means you shouldn't know nor care whether base class has private fields or not. Yet with proxies it's a crucially observable difference.

I do want to say that this is a fairly strong concern. I just don't think it outweighs the cost of allowing code outside the class to violate invariants it is enforcing.

@erights, you've thought more about proxies than I have, I would love to get your commentary here.

rdking commented 6 years ago

@jods4 I think you missed something important....

If I'm right, you were thinking that your handler.get would receive "#items" as the value for prop. That's the problem you think would cause an issue since this["#items"] would return undefined. According to the spec, however, prop would not be "#items", but rather the PrivateFieldName associated with "#items" in the associated class's [[PrivateFieldNames]]. Just think of that as a Symbol(). It's not significantly different. As such, prop would be set to that PrivateFieldName value.

Now given your code, it might still fail since this wouldn't have a member by that name, and this proposal's spec says that [] access to [[PrivateFieldValues]] is a no go. However, if you replaced return target[prop]; with return Reflect.get(target, prop, receiver); it should work since it would continue through the ES engine's internal get handler. Proxies aren't broken by this proposal, but they do have to be handled with even more care than normal.

rdking commented 6 years ago

@bakkot Unless I got something wrong in my other post, isn't this an issue for hard private?

Since the PrivateFieldName for every used private field would necessarily pass through a get or set handler of a Proxy wrapped around an instance with private fields, it's possible (albeit really convoluted) to not only get the PrivateFieldNames, but also access and alter them on an instance using a combination of Proxy and Reflect. The only way to stop this given the current proposal would be to do as @jods4 is expecting and break Proxy with respect to private fields.

A modification of the proposal (as given here: #104) can solve this without breaking anything else. Using that suggestion, the handler.get method would never be called for private fields.

ljharb commented 6 years ago

@rdking yes - which is also why the get handler does not fire for private field access currently.

bakkot commented 6 years ago

@rdking, the get trap on proxies is not called for private field access as currently spec'd, and as far as I can tell no one believes it does or is proposing that it should.

jods4 commented 6 years ago

@rdking

you were thinking that your handler.get would receive "#items" as the value for prop

No. There was one slightly confusing typo in my first comment where I wrote items instead of threshold. I don't want to see private fields in get. I think it would be better if they pierced through the proxy.

@bakkot About code breaking when introducing private in encapsulated code: nice one, JS is a language with weird features! I would say that passing arbitrary this to a function, with .call() or .apply(), was not encapsulated code in the first place. If you use an arbitrary receiver for a function, you take responsibility that your receiver shares a compatible implementation as the intended receiver. Assuming you know about the implementation is not encapsulation.

That was a good find but I still don't see an example where encapsulated code will break when introducing real private fields.

About breaking invariants: It's a tricky discussion because Proxy is very special and intended to behave like the object it wraps. When you say "there are two different objects with the same #id" it's not completely true. There's still only one Factory with that #id, and some proxies of it.

Let's look at your linked-list example. I think it supports my point more than yours.

  1. Consider how you would have done that pre-private fields. A WeakMap maybe? Cool thing is that with a WeakMap it works, even with proxies. You don't have cycles but you can have both the proxy and its target in the chain -> that should be expected since proxies have their own identity... this is outside of this discussion.

  2. Quick aside: @ljharb said private fields were design to work like WeakMap with nicer syntax. Note how they do not: we went from "actually works" to "TypeError".

  3. Your implementation doesn't support proxies because of this === cursor, which is a well-known limitation of proxies. So basically you're using private fields to detect the proxy and throw. TC39 has previously discussed whether the platform should expose isProxy() and maybe getProxyTarget(). They explicitely decided not to. Private fields are a great way to work around that decision as demonstrated here -> bad!

  4. Let's say I know I am not gonna insert cycles and I wish to use your linked list... and proxies. No can't do. This is what I meant when I said you're partitioning the web platform in two: proxies or private fields, pick one, don't mix.

To sum up: I still think everything would be better if private fields tunneled. The real problem with your example is this vs proxies, which is something unfortunate but out of scope here. You could document this limitation, or you could work-around it with something a little more clever than this === cursor. In fact, private fields can help you do that.

ljharb commented 6 years ago

@jods4 you’d have to wrap accesses in a try/catch to avoid the type error from the private field brand check, if that’s what you wanted.

You can already write an isProxy for every builtin object type except arrays and plain objects. This proposal means that a proxy for an object with private fields can also be distinguished; but that would be the same if your current implementation did such a brand check.

jods4 commented 6 years ago

@ljharb After looking at all code examples in this thread, my personal summary would be:

Cons of current spec:

Pros of current spec:

IMHO you failed to provide strong examples of why tunneling would be bad or why not tunneling would be better. Feel free to expand my list but right now my opinion is that there is no doubt private fields should tunnel through proxies.

ljharb commented 6 years ago

Tunneling would add a very high amount of complexity to Proxies, for the negligible value imo of making Proxies, an advanced feature, easier to use.

bakkot commented 6 years ago

If you use an arbitrary receiver for a function, you take responsibility that your receiver shares a compatible implementation as the intended receiver. Assuming you know about the implementation is not encapsulation.

I'd say the same for passing a proxy instead of a real object, personally.

It's a tricky discussion because Proxy is very special and intended to behave like the object it wraps.

Not in general: only with respect to code which doesn't have access to the original object.

Consider how you would have done that pre-private fields. A WeakMap maybe? Cool thing is that with a WeakMap it works, even with proxies.

It actually doesn't, not with the obvious implementation:

const childMap = new WeakMap;
class Node {
  constructor() {
    childMap.set(this, null);
  }
  link(child) {
    let cursor = child;
    while (cursor !== null) {
      if (cursor === this) {
        throw new TypeError('Cycle!');
      }
      cursor = childMap.get(cursor);
    }
    childMap.set(this, child);
  }
}
let x = new Node;
x.link(new Proxy(x, {}); // spins - that's even worse than throwing!

This is because I haven't checked that child is either an instance of Node or null, which I got for free with the obvious private field implementation. (This is what I mean about "private fields are good for enforcing invariants".)

And if I do introduce such a check - for example, if I put if (child !== null && !weakMap.has(child)) throw new TypeError('not a Node'); as the first line of link - then I can't link to a proxy (unless I first link that proxy to something else, in which case Node's consumers are now depending on implementation details of the particulars of how it's enforcing that the chain only contains Nodes, which is bad).

In any case, while you might be able to come up with an implementation which does allow a Node and a proxy for the node as distinct elements in the chain with distinct children, it's not obvious to me that that's desirable. In fact I'd expect that to be a huge source of bugs.

TC39 has previously discussed whether the platform should expose isProxy() and maybe getProxyTarget(). They explicitely decided not to. Private fields are a great way to work around that decision as demonstrated here -> bad!

As I understand it that decision was very specifically because it would allow people to detect a proxy without having access to its target, which wasn't a power we wanted to grant. The case of a class is detecting if you give it a proxy for an instance it created is very much not the concern. (And classes can already can already do that, of course, just by sticking their instances in a WeakSet when they construct them.)

Again, the point is that proxies are useful when you're sitting between Alice and Bob, wrapping objects that Alice gives you before handing them off to Bob. They are not so useful for wrapping objects Alice gives you before giving them back to Alice.

Let's say I know I am not gonna insert cycles and I wish to use your linked list... and proxies. No can't do.

Yeah. We've worked hard in the design of private fields to ensure that classes can enforce their own invariants. That does mean that people outside of the class who want to do something unusual and be trusted to enforce the invariants themselves don't get to.

Might have more comments this evening; gotta run now.

jods4 commented 6 years ago

@bakkot WeakMap implementation does work in a rather obvious way, you just have to use cursor == null instead of cursor === null. Bonus point: you now have lazy initialization. No need to init the child field for leaves.

Now take that working implementation, that could be in the wild today, replace WeakMap by a private field and boom.

I'd say the same for passing a proxy instead of a real object, personally.

Yeah, we already agreed that proxies are not fully transparent. Turns out that in practice it works in a lot of useful situations. Few people use WeakMap for private fields today, but with the new syntax it's gonna change.

They are not so useful for wrapping objects Alice gives you before giving them back to Alice.

Can you point out some official source for that claim? When they were in development, one motivation was that it would help replace Object.observe and that's very much the opposite of what you're saying. Most examples one can found (including on MDN) are not the kind you describe.

Even if it is true and intended usage, what does it change? Where does it fit in my list of Pros and Cons? There are things that people are doing today, that are working and that you are going to break and make impossible. Expected usage doesn't matter as much as practice.

it would allow people to detect a proxy without having access to its target

Strictly speaking your linked list code doesn't have access to the target, but to its class. I guess that's what you mean.

enforce their own invariants.

OK I'm gonna rephrase that with the only way you demonstrated it and edit my Pros list with it: private fields forbid calling your implementation with a class other than your own, including proxies.

@ljharb

I'm not sure how private state makes base classes more brittle - I'd argue it's quite the opposite.

Yes, private state helps better encapsulation -- it's great. But when it comes to tunneling or not, if you don't tunnel you create a brittle base class situation for anyone whoe is using proxies in his code.

// library v1
export class Lib {
  _age = 18;
  canDrinkBeer() { return this._age > 16 }
}

// consumer of lib
class Buddy extends Lib { 
  // Do my own stuff with it, no private in sight here
}

var proxy = new Proxy(new Buddy, { }); // for whatever reason
proxy.canDrinkBeen();  // public api, all fine

// library v2
export class Lib {
  #age = 18;
  canDrinkBeer() { return #age > 16 }
}

// After consumer upgrades, same perfectly fine code breaks.

Certainly introducing a brand check can break existing code, as will making public properties private, but that's fine - changing existing code can always break it.

Yes, changing code always has potential of breaking stuff. It's bad and we should reduce breaking changes as much as we can.

In preceding example, I was not willingly introducing a brand check. I was just making truly private, state that has always been (conceptually) private.

Consumer was only using public API. In the end, I believe that it is unexpected that it broke.

Making Proxies easy isn't something that I find compelling; they're an advanced feature, not intended to be used casually.

I find that shocking. Good design should be as easy to use as possible, no matter the intended target.

Code has to be written, tested, delivered to browser and run. Making stuff hard to use on purpose is offending the whole industry, who has to pay the ensuing cost.

If there is a problem with using a feature "casually" then maybe it was poorly designed.

can you elaborate?

Assuming you speak of frameworks: VueJS and Aurelia are considering using proxies for automatic ViewModel observation: i.e. detect when properties change so that they can update the UI accordingly, i.e. replace Object.observe. Look at VueJS roadmap, I linked it above.

The point here is that it won't work if there are private fields in ViewModel classes, or their hierarchy. Private fields are a very tempting feature so there will be conflict there for sure.

React, for example, would benefit highly by being able to maximize the privacy of its internal structure.

Yes, private fields are great. VueJS and Aurelia will benefit greatly as well. It's just the interaction with proxies that is going to create friction.

in what way are private fields "noticeable" from outside code?

See my example above. Consumer code crashes when library was updated to use private fields.

Tunneling would add a very high amount of complexity to Proxies, for the negligible value imo of making Proxies, an advanced feature, easier to use.

Maybe, but proxies have made their way into JS. They are advanced but some people, sometimes in frameworks or libraries, use them. You need to support them.

(edit): and very importantly, this is not about making proxies easier to use. It's enabling scenarios that are possible today but will be impossible once the target class uses private fields.

You don't need to do anything complicated to get a TypeError in your face.

class ViewModel {
  #count = 0;
  increment() { #count++; }
}
var p = new Proxy(new ViewModel, {}); // totally empty proxy
p.increment(); // Boom!
jods4 commented 6 years ago

@bakkot I was thinking about the "intended" usage of proxies and it just makes no sense to me. In today JS, there is absolutely no difference between passing a proxy to a function on Alice or passing the proxy as a receiver this for a function on Bob. In the end it's just functions and parameters.

In fact, the same things are broken on both sides: keeping references and comparing identity; attaching data with a WeakMap. In fact, Alice is more likely to extend the object with a WeakMap than Bob. Usually coders put their private directly on instances and don't bother with weak maps.

And your complex membrane code for that use case is not even 100%. If Alice wants to pass parameters dynamically, she might do stuff like Bob.prototype.someFunc.apply(proxy, [a,b,c]). Proxy ended up in Bob even in your intended use case.

I feel like your making a circular argument. The only difference between a function on Alice and one on Bob are upcoming private fields. So you're saying that we shouldn't use a proxy that way because it's how you want private fields to work.

Did I miss something?

ljharb commented 6 years ago

You don't need to do anything complicated

The design of Proxy requires that you do have to do complicated things to be able to successfully use them, including wrapping effectively every property value in a Proxy before returning it in a [[Get]]. This might be poor design, it might not be, but it's none the less the design.

dead-claudia commented 6 years ago

@ljharb

Here's an interesting tidbit that might inform the discussion: the spec, as far as I can tell, doesn't even state whether internal slots should or shouldn't be read through (it's undefined behavior), but that's what I'm observing. If you want a test, this should run without error if an implementation reads internal slots through proxies, and throw a TypeError on the second line if it doesn't:

var object = new Proxy([], {})
object.push(1)

My personal opinion on this is whatever engines do with internal slots, private slots should work identically. Anything else is surprising, as it breaks the encapsulation and transparency of proxies, as well as the existence of private fields within an object (whose existence should generally be private itself). Also, doing otherwise would make builtins impossible to implement at the language level, and IIRC userland implementations of builtins is one of the driving reasons for this proposal. (Correct me if I'm wrong here.)

ljharb commented 6 years ago

@isiahmeadows Array.prototype.push does not check internal slots; the only array method that does is Array.isArray. Thus, it shouldn't throw a TypeError in any case. Arrays are the special case that do tunnel through proxies, by explicitly specifying it.

erights commented 6 years ago

Proxies and WeakMaps were designed, and initially motivated, to support the creation of membranes. Proxies used standalone cannot be transparent, and cannot reasonably approximate transparency. Membranes come reasonably close to transparently emulating a realm boundary. For classes with private members, the emulation is essentially perfect. Let's take the class code

class Foo {
  #priv;
  constructor() {
    this.#priv = {};
  }
  both(other) {
    return [this.#priv, other.#priv];
  }
}

new Foo().both(new Foo()); // works

// foreignFoo is a Foo instance from another realm
// or foreignFoo is a membrane proxy for a Foo instance
// across a membrane boundary

new Foo().both(foreignFoo); // throws
foreignFoo.both(new Foo()); // throws

foreignFoo.both(foreignFoo); // works

The Foo code evaluated in another realm creates a behaviorally identical but distinct Foo class. Instances of each foo class can see into other instances of the same foo class but not instances of the other foo class. The expression foreignFoo.both, in the inter-realm case, evaluates to the both method from the foreign Foo class, i.e., the one that foreignFoo is an instance of. In the membrane case, foreignFoo.both evaluates to a proxy to the both method of the Foo class on the other side of the membrane.

The non-transparent case is that, for the builtins, like Date, with internal slots, the internal slots are visible across realms, which is broken but entrenched, and so is much too late to fix. Across a realm boundary, the internal slots act as-if Date is defined by a class and the internal slots are defined as Date's private members, and so don't work across the boundary as shown in the Foo example above.

There is much confusion about Array.isArray. Those thinking about proxies used standalone, which can never be reasonably transparent anyway, think that the so-called exception made for this in the proxy rules is a mistake. Instead, since Array.isArray does work across realms, it is consistent for it to work across a membrane boundary. Likewise typeof f === "function" works across realms and likewise works across membrane boundaries.

This is because the four-way distinction between normal objects, arrays, functions, and primitive values is fundamental to JavaScript. For example, the JSON.stringify algorithm distinguishes between objects, arrays, functions, and primitive values. The JSON.stringify algorithm thus works transparently across a membrane boundary because this four-way distinction is preserved.

By contrast, JSON.stringify has no specific knowledge of Date and RegExp, and no one would expect it to. Concepts like Date and RegExp are more like concepts that could have been provided by user code as classes. Across a membrane boundary, Date and RegExp in fact act as if they were provided by classes, which is less surprising than how they actually act across realm boundaries.

attn @ajvincent @tvcutsem

dead-claudia commented 6 years ago

@ljharb Good point - I didn't catch that. new Proxy(new Date(), {}).valueOf() fails correctly, and I forgot that Array.prototype.push is generic like 99% of the other Array methods. 😄

And now that I think about it closer, proxies don't have all the methods of dates, etc., so that behavior is in fact defined.

Edit: Example from Node's REPL:

> new Proxy(new Date(), {}).valueOf()
TypeError: this is not a Date object.
    at Proxy.valueOf (<anonymous>)
dead-claudia commented 6 years ago

@erights If proxies get turned into membranes, it might be worth it to start also translating the various builtins to use private fields as well, to keep it consistent. They would need to be a different type of private field, since they often are used across multiple classes. (Still think my proposal could help alleviate that issue by simply letting all builtins be defined within its own scope, scoped above the realm with a few builtins.)

littledan commented 6 years ago

Thanks for your explanations, @bakkot, @erights and others. The high-level point here seems to be, the semantics of private fields with Proxy follows the original design of Proxy; ~they do not transparently form part of a membrane because Proxy does not transparently form a membrane~ tunneling requires some particular logic in the Proxy and is not built-in. Any attempt at revisiting this property might be better done with something like a standard library for the membrane pattern.

@zenparsing

To me, this is another argument for having different syntax for different semantics.

I'm not sure if using -> rather than .# would imply for programmers that the semantics with respect to Proxies are so different. Maybe broader feedback from JS programmers could give us more information here.

erights commented 6 years ago

The high-level point here seems to be, the semantics of private fields with Proxy follows the original design of Proxy; they do not transparently form part of a membrane because Proxy does not transparently form a membrane.

Hi @littledan , I don't understand this statement of the "high-level point". Proxies do transparently form membranes. Indeed, @tvcutsem and I introduced Proxies and WeakMaps into EcmaScript in order to enable the building of transparent membranes.

Any attempt at revisiting this property might be better done with something like a standard library for the membrane pattern.

@ajvincent has built an interesting membrane library that does indeed provide an opportunity to revisit and explain how proxies are used to build membranes. In a fortunate bit of timing, @ajvincent and I will present, at the upcoming (July 2018) tc39 meeting, about this library and its lessons for tc39.

Borrowing from the current draft of the talk, the purpose of membranes are

The first two goals are served well by the membrane pattern that @tvcutsem and I originally wrote. However, this was a pattern rather than an abstraction mechanism. If the membrane is only transparent --- if it introduces no distortions --- it may as well not be there. All our distortions back then were ad-hoc variations on the basic pattern.

However, the basic pattern is complex enough, and hard enough to get right without distortions, that this is not an effective way to express distorting membranes. Hence the need for a membrane library, as an abstraction mechanism parameterized by user-provided distortion policies, that automatically propagates these distortions through the membrane surface as it grows.

See

@tvcutsem's very clear article Membranes in JavaScript

The following papers by @tvcutsem and I:

erights commented 6 years ago

Repeating the necessary qualifier explained upthread at https://github.com/tc39/proposal-class-fields/issues/106#issuecomment-397891307 :

Membranes would be transparent but for the legacy difference between builtin internal fields vs class private fields. Builtin internal fields are visible to builtin methods across realm boundaries. Class private fields are not. Both class private fields and builtin internal fields are not visible across membrane boundaries. Hence, a membrane boundary acts like a realm boundary except that builtin internal fields act as-if they were defined as class private fields.

littledan commented 6 years ago

@erights Thanks for your better phrasing above. I've tried to correct the sloppy wording in my comment above. I think there is no point of disagreement.

erights commented 6 years ago

Hi @littledan thanks. This is better. However, I don't understand:

tunneling requires some particular logic in the Proxy and is not built-in.

What do you mean by "tunneling"? If it means penetrating the encapsulation boundary of class instances to directly access their private fields, if that were possible, class private fields would be broken and need to be fixed. If proxies enabled such penetration, proxies would be broken and need to be fixed so that they could not. Fortunately, neither seems to be broken in this way, so such tunneling hopefully remains impossible.

bakkot commented 6 years ago

@erights "tunneling" would mean that

class A {
  #priv;
  constructor(priv){ this.#priv = priv; }
  m(){ return this.#priv; }
}
(new Proxy(new A('foo'), {})).m();

would return 'foo' rather than throwing.

Whether this penetrates the encapsulation boundary of class instances depends somewhat on your perspective.

erights commented 6 years ago

Hi @bakkot , thanks.

In that case, membranes tunnel fine.

(Of course, lone proxies don't. If you want transparent tunneling, use Proxies and WeakMaps to build membranes. That's what they are for.)

littledan commented 6 years ago

If I understand correctly, tunneling in cases like @bakkot mentions is largely a matter of getting it so that methods that are called on the proxy have the target, rather than the proxy, as the receiver. This might not be the best behavior for all situations, so maybe it was right that Proxy didn't take this as the default, but I don't think it breaks encapsulation.

In that case, membranes tunnel fine.

Right, I think if it were obvious how to make a Proxy into an effective membrane, it would solve the original issue that this bug were filed about. Private fields "break" Proxies in the sense that they expose the need for a little infrastructure to make membranes work as you might like them to.

Since all the ingredients are there already, I'd say we don't need to take any action within this proposal.

jods4 commented 6 years ago

@littledan

Right, I think if it were obvious how to make a Proxy into an effective membrane, it would solve the original issue that this bug were filed about.

I filed this issue and I don't understand how membranes are related to what I'm doing.

Here's the use-case again:

We use proxies to wrap user classes (not native objects; and not trying to be transitive here) and detect what fields on instances are read or written. Either from outside code or class methods. We avoid this identity issues by wrapping the class very early in the instance lifecycle. If you try hard enough you can break it but in practice it works super well.

We do the same with arrays to produce observable arrays. Something that is impossible to achieve without proxies. Again, it works perfectly, never had any issue with it.

This is existing JS code that works great. As I said previously, that concept is on the roadmap of VueJS (I don't know what their implementation strategy will be) and Aurelia (pretty much the way I described above).

With current specs, the moment you add a private field to the user class, which is very tempting as private fields are great, this code breaks -- because private fields are not accessible through the proxy.

To me it seems introducing private fields that way is breaking seemingly unrelated code. Plus it limits the usefulness of proxies. Conceptually I don't see why what I described above shouldn't work when the class uses private fields internally.

I don't want to intercept access to private fields, they're a private implementation detail. I just would like them to work.

littledan commented 6 years ago

We avoid this identity issues by wrapping the class very early in the instance lifecycle. If you try hard enough you can break it but in practice it works super well.

Thanks for explaining. Yes, it's true that this pattern of Proxy use will require some changes. These changes are also required if you want to build many kinds of observed built-in objects.

The resources linked from https://github.com/tc39/proposal-class-fields/issues/106#issuecomment-399766209 might be able to help you upgrade to a "membrane" pattern. It's a little more complicated to implement, but it means you no longer have to be as careful about this, and that both private fields and built-in classes will work. Would this be feasible for your system? Let's get in contact with Vue and Aurelia to learn more about the details of their plans and see if a membrane would work for them.

jods4 commented 6 years ago

@littledan so far nobody in this thread has been able to provide an example that would actually work the same. Either field access from members are not detected, or it fails when the target class uses inheritance. If you have alternative ideas I would gladly look at them.

I don't get why people keep mentionning "membranes".

What is a membrane? A membrane is a wrapper around an entire object graph, as opposed to a wrapper for just a single object.

I just care about a single instance.

littledan commented 6 years ago

@jods4 A core relevant technique of membrane systems is that, when a method is called on the Proxy, the receiver that's passed should be the target, not the Proxy. You can implement this with the same technique that the membrane code uses: Maintain a WeakMap from the Proxy to the target, and whenever a method is accessed through the Proxy, wrap it in another method which will look up the target of the receiver and call the underlying method. You could just wrap methods and leave out all the other wrapping/unwrapping if it's not useful for you.

Hmm, I guess this technique wouldn't work well if the mutations you're trying to track are performed from inside of the class, though... Maybe I'm missing something that @erights can fill in.

Personally, I was imagining that decorators would generally be the mechanism to track mutation from frameworks, along the lines of Glimmer's @tracked decorator. This mechanism extends cleanly to private fields.

jods4 commented 6 years ago

@littledan Yes, decorators are a good alternative for that scenario. You can even use them to instrument private fields. Proxies are an alternative with pros and cons. For example a proxy can detect the creation of additional fields. Another example I already mentionned is observing changes to arrays.

If your key point with membranes was to substitute the real this when making method calls, then you defeat the purpose of this use case. Detecting changes from member methods (like the click handler of a ViewModel) is a key scenario.

I don't get the dichotomy between member methods and outside functions. In JS before private fields, they are exactly the same. this is just another parameter to a function, albeit with a strange syntax/conventions. Anything broken inside the method is broken in outside functions. Moreover, outside functions can bypass your membrane and call methods on the proxy. Think of dynamic parameters scenarios, such as MyClass.prototype.func.call(proxy, 1, 2, 3).

Of course, this changes with the introduction of private fields. As of the current spec it creates a strong difference between a member method and an outside function.

littledan commented 6 years ago

Maybe, to get the benefits of each, a pattern could be to use a Proxy for Arrays and map-like objects, and a decorator for fields on class declarations. Would that work for your system? For one, I think it might be more efficient in some implementations to use decorators (as they don't have as much of a continued cost when using the instances as Proxy may have).

jods4 commented 6 years ago

Beware of looking too much into one specific use-case and not into the underlying issue.

The framework I'm working on works fine with decorators. Due to how old design decisions, it won't work for Aurelia. Map-like objects could easily be classes, maybe with private fields. Everything is fragile, for instance if browsers decide to implement arrays with private fields internally, it would break.

littledan commented 6 years ago

Beware of looking too much into one specific use-case and not into the underlying issue.

If there were an underlying issue, I really think Proxy would be the place to look, not private fields, given the correspondence with internal slot semantics mentioned upthread. @erights is more of a Proxy expert than me, so maybe he could speak more to that, but it seemed like he was confident that this could be worked through with the current Proxy design.

Due to how old design decisions, it won't work for Aurelia.

Could you give some more background about the Aurelia case?

Everything is fragile, for instance if browsers decide to implement arrays with private fields internally, it would break.

I'm not sure what you mean; the JS spec is really specific about the semantics of a Proxy over an Array. If a browser implemented it some other way, it wouldn't be spec-compliant.

jods4 commented 6 years ago

Could you give some more background about the Aurelia case?

Aurelia was designed to work on plain classes, before decorators were even introduced. It doesn't use read detection but rather analyzes expressions inside template bindings and instruments them on the fly.

A big issue with this design is that it can't look into methods or getters to grab dependencies. Currently users need to manually manage those cases. For example a @computedFrom([...]) decorator was introduced to easily declare which fields a getter depends on.

Today, proxies are available in all modern browsers. Idea is to wrap the view model into a proxy before evaluation, so that the framework could intercept whatever field a getter uses.

Aurelia is years old. There are back compat issues with mandating fields decoration. It would be a drastic design change as well: working on vanilla classes has been a key philosophy from the start.

I'm not sure what you mean; the JS spec is really specific about the semantics of a Proxy over an Array. If a browser implemented it some other way, it wouldn't be spec-compliant.

Sure but private fields are a new addition to the spec. Is there something specifically forbidding implementors to use private fields inside array methods?

rdking commented 6 years ago

I've figured it out. It's not at all straight forward, but the proxy problem is solvable without any modifications.

var container = new WeakMap();

class Temp {
    constructor() {
        container.set(this, { _isWorking: true });
    }

    testMe() {
        console.log(`Is it working? ${container.get(this)._isWorking}`)
    }
}

//This is the magic here. Proxy the proxy handler!
var p = new Proxy({}, { get: (target, key, receiver) => {
    return function(...args) {
        var retval = Reflect[key].apply(target, args);
        if (typeof(retval) == "function") {               
            retval = retval.bind(args[0]);
        }
        return retval;
    }
} });

temp = new Proxy(new Temp(), p);
temp.testMe() //displays: "Is it working? true"

Even though there's a work-around, it is still an issue for this proposal that proxy can reveal the existence of private members in a class.

jods4 commented 6 years ago

@rdking I don't see how this help? To be clear the problem here is that we can't pass a proxified this to a member method that uses private fields. And I want to pass the proxy, not its target.

@littledan My remark about this being just another parameter made me realize... There's more! It's not just about this, but you can't pass proxified instances to any parameter. So if you have a class like:

class VM {
  #id

  same(other) {
    return #id == other.#id;
  }
}

Then you can't pass a proxified VM to same(proxy).

Considering this might be called by any code manipulating proxies, it's really hard to make robust membrane. Even more so given that the proxified member might be tucked inside a deep object graph rather than passed directly inside a parameter.

In fact, unless you modify every prototype method from very early on (before any code grabs one reference) it seems impossible to write a robust membrane.

In particular, for non-membrane scenarios like those I've given as examples, it is impossible to use proxies in conjunction with private fields.

@ljharb tried to make a point that proxies were meant to intercept the behavior of consumer code, not private implementations (a point that I don't agree with). Even in this case you're not safe: there are several ways the consumer code might end up injecting proxies into class methods and then they will throw.

littledan commented 6 years ago

@jods4

It's not just about this, but you can't pass proxified instances to any parameter.

Well, a membrane can do exactly this: unwrap all the arguments. Maybe you do want a membrane.

Aurelia is years old. There are back compat issues with mandating fields decoration. It would be a drastic design change as well: working on vanilla classes has been a key philosophy from the start.

I don't understand--if they're using vanilla classes, how can they also be facing problems from using private fields?

jods4 commented 6 years ago

Well, a membrane can do exactly this: unwrap all the arguments.

How are you gonna do that? What if arguments are a large graph? Are you gonna explore the whole graph, taking cycles into account and swap parts? Are you cloning or swapping parts in place? In place: what if the object is sealed? Cloning has huge downsides as well such as identity management. Not even talking about perf here.

Maybe you do want a membrane.

Nope, personnally I just want private fields to tunnel through a proxy. Seems easier than graph traversal.

I don't understand--if they're using vanilla classes, how can they also be facing problems from using private fields?

Those classes are written by consumers of the framework. When private fields become real, they might be very tempted to use some in their view models. Or are you saying a class using private fields wouldn't be vanilla? By vanilla I meant: free of framework-specific annotations.