Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Private members break proxies #106

Closed
jods4 opened this issue Jun 14, 2018 · 426 comments
Closed

Private members break proxies #106

jods4 opened this issue Jun 14, 2018 · 426 comments

Comments

@jods4
Copy link

jods4 commented Jun 14, 2018

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
Copy link
Member

ljharb commented Jun 14, 2018

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
Copy link
Author

jods4 commented Jun 14, 2018

@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
Copy link
Contributor

bakkot commented Jun 14, 2018

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
Copy link
Member

ljharb commented Jun 14, 2018

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
Copy link
Author

jods4 commented Jun 14, 2018

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
Copy link
Contributor

bakkot commented Jun 15, 2018

@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
Copy link
Member

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
Copy link
Member

ljharb commented Jun 15, 2018

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

@jods4
Copy link
Author

jods4 commented Jun 15, 2018

@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...

  • I don't see this information anywhere, so it's rather hard to get.
  • MDN describes proxies as a way to intercept operations, quote: "analogous to the concept of traps in operating systems". That seems in line with what I'm doing.
  • I clearly remember proxies being touted as a way to replace Object.observe when it was dropped from the spec. That's more or less the example I'm giving you here.
  • 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.
  • Maybe it wasn't an explicit goal but it works fine. Breaking it is a step backwards.

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
Copy link
Author

jods4 commented Jun 15, 2018

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
Copy link
Contributor

bakkot commented Jun 15, 2018

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
Copy link
Contributor

bakkot commented Jun 15, 2018

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
Copy link

rdking commented Jun 15, 2018

@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
Copy link

rdking commented Jun 15, 2018

@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
Copy link
Member

ljharb commented Jun 15, 2018

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

@bakkot
Copy link
Contributor

bakkot commented Jun 15, 2018

@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
Copy link
Author

jods4 commented Jun 16, 2018

@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
Copy link
Member

ljharb commented Jun 16, 2018

@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
Copy link
Author

jods4 commented Jun 16, 2018

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

Cons of current spec:

  • Creates a brittle base class problem.
  • Introduction of private inside well encapsulated classes can break existing code.
  • Divides JS features: either a piece of code is implemented using privates, or it can be called from a proxy, not both.
  • Makes writing proxies even more complicated and less performant than they are today.
  • Functionnalities possible today and desired by major frameworks (VueJS, Aurelia) become impossible if target uses private fields internally.
  • Although they are an encapsulation primitive, private fields are very much noticeable from outside code.

Pros of current spec:

  • More similar to WeakMap, although not the same. Replacing WeakMap by private can lead to TypeError.
  • (edited) Simpler to implement for browsers.
  • (edited) Can easily enforce brand check: nothing else than instances of your class can be used as receiver of your implementation, including proxies. (I put that under Pros but it is precisely the point we're debating -- I think proxies are special objects and being able to substitue an object for a proxy adds a lot of value to them).

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
Copy link
Member

ljharb commented Jun 16, 2018

  • I'm not sure how private state makes base classes more brittle - I'd argue it's quite the opposite.
  • 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.
  • Making Proxies easy isn't something that I find compelling; they're an advanced feature, not intended to be used casually.
  • can you elaborate? React, for example, would benefit highly by being able to maximize the privacy of its internal structure.
  • in what way are private fields "noticeable" from outside code?

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
Copy link
Contributor

bakkot commented Jun 16, 2018

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
Copy link
Author

jods4 commented Jun 16, 2018

@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
Copy link
Author

jods4 commented Jun 16, 2018

@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
Copy link
Member

ljharb commented Jun 16, 2018

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
Copy link

@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
Copy link
Member

ljharb commented Jun 17, 2018

@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
Copy link

erights commented Jun 17, 2018

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
Copy link

dead-claudia commented Jun 18, 2018

@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
Copy link

@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
Copy link
Member

littledan commented Jun 24, 2018

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.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2021

@cztomsik yes, but the feature is stage 4 now which means it won't be changed in any way that could break websites that rely on it.

@jods4 i'm not rewriting anything; this issue was indeed brought up long before class fields advanced to stage 3, even. Note that stage 3 is when browsers ship it (and shortly after that, when it can never be removed without breaking the web), and stage 4 is when the PR gets merged into the spec (which is when it's actually in the living standard), and June 2022 is just when the yearly snapshot is officially published by Ecma (which has no bearing on this topic).

Proxies are not meant for observation alone. They are meant, solely, to be used in concert with a membranes implementation, which completely solves the problems described here. In the absence of a membrane implementation, they're not meant to be used at all (altho there's tons of ways you can use them, you'll run into many warts like the internal slot/private field ones described here). Long before private fields were a thing, WeakMaps were a thing, and any class that put this in a WeakMap in the constructor, and threw in methods when the receiver wasn't a "proper" instance, would have caused these problems for misused Proxies as well. Also, don't forget this category:

const d = new Date();
d.toString() === new Proxy(d, {}).toString() // you might expect `true`, but this will instead throw, because Proxies do not tunnel

In general, if any particular library is designed in such a way that you are unable to use a language feature, either one of two things will likely happen: the language feature will go unused in that ecosystem, or the library will fall out of favor and be replaced by one designed to work well with the language. Only time will tell which will be the case here, of course.

@mbrowne
Copy link

mbrowne commented Aug 10, 2021

@ljharb If it's true that proxies were meant to be used "solely" in concert with a membranes implementation, then it seems like this was poorly communicated. I recall major documentation sites such as MDN showing examples of simple observation using proxies without mentioning membranes.

While it would be inaccurate to say that the introduction of private fields was a breaking change to proxies, it had most of the same negative effects to proxies as a true breaking change would have. The existing incompatibility with WeakMap as a way to achieve private members is a good point, but in practice not many people (percentage-wise) were doing that. Now that private fields are being used more, a lot more people are being affected by this, even those who aren't even using private fields at all and just want to continue using proxies for public fields only as they did before.

I'm not trying to dismiss your points, all of which are valid. I just want to underscore the impact of this, and the fact that if you consider the design of the language as a whole, it's currently a flaw.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2021

I completely agree with your comment. The flaw, however, is in Proxy, and that’s where it would need to be corrected somehow (simply tunneling by default would actually break a ton of security constraints)

@cztomsik
Copy link

@cztomsik yes, but the feature is stage 4 now which means it won't be changed in any way that could break websites that rely on it.

@ljharb True but we have lots of applications relying on proxies already.

The problem is not your observable model (which you have under control and where you can do membrane), the problem is that you cannot put anything 3rd party there (which many people did and do). So it will, eventually, break much more websites too.

For example, people use date libraries and these are often present in observable models. If date library author decides to use private fields you will not be able to update to a new version.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2021

Temporal is stage 3 and will soon obsolete both Date and every date library, and it uses internal slots, like every builtin. The ship sailed long before Proxy existed.

@jods4
Copy link
Author

jods4 commented Aug 10, 2021

Clearly dates were just an example.
What @cztomsik said is true of any 3rd party library you may use -- and browsers won't ship a built-in for everything.

And BTW even though Temporal will be a great replacement for date libraries "soon", those libraries will continue to be used for a long time. There are existing very large, critical applications using e.g. Moment or Dayjs and there's no way they'll be fully modified and retested just to use the newest web tech. Real life doesn't work like that.

@rdking
Copy link

rdking commented Aug 10, 2021

I completely agree with your comment. The flaw, however, is in Proxy, and that’s where it would need to be corrected somehow (simply tunneling by default would actually break a ton of security constraints)

@ljharb Proxy is indeed flawed, but not being able to tunnel internal slots really isn't one of the flaws. So the issue being discussed here isn't really about Proxy. It's the simple fact that a new proposal was pushed into place knowing it would cause these issues. What's worse is that it is fully possible to change the implementation of private fields in a way that keeps with the existing visible semantics without using internal slots and simultaneously avoiding the Proxy issue. Even worse still is that a solution to the Proxy vs internal slots issue was raised during the past discussions, yet no interest was given to the solution.

I could dig a well listing the process problems that led us to where we are, but that gets us nowhere. What we need is a solution, and not someone willing to "die on a hill" to hold down an ideal that cannot and will not produce viable results. Got any suggestions toward that end?

@ljharb
Copy link
Member

ljharb commented Aug 10, 2021

If private fields end up being reified as first-class private Symbols (ie, #foo becomes an actual value that can be used as a property key), then I have a pretty straightforward proposal I'd make (essentially, when creating the Proxy, you pass which private symbols you want tunneled - since having the private symbol would mean you have access to the field, allowing the tunneling isn't a security problem). It wouldn't likely solve it for Vue or other libraries automatically, but they'd be able to provide some kind of registration API where the class author grants the library access by passing it needed private Symbols).

However, short of that, I don't have any suggestions. The place to explore new proposals though is on https://es.discourse.group, not on the repo of a merged stage 4 proposal (which, typically, would have been archived already). I completely agree that rehashing old arguments here doesn't help anybody.

@cztomsik
Copy link

@ljharb not sure if this was discussed before but it's not just proxies it also breaks Object.create():

cztomsik$ deno
Deno 1.14.0
exit using ctrl+d or close()
> class X {
  #foo = 'foo'

  get foo() {
    return this.#foo
  }
}

const o = Object.create(new X())
console.log(o.foo)
Uncaught TypeError: Cannot read private member #foo from an object whose class did not declare it
    at X.get foo (<anonymous>:5:21)
    at <anonymous>:9:15
> 

I just hit this as I was refactoring some code to use private vars.

@ljharb
Copy link
Member

ljharb commented Sep 21, 2021

@cztomsik Object.create is meant to take a prototype object - in this case, X.prototype (not an instance), but yes, if you inherit from something incorrectly (ie, not with class extends) then you won't be guaranteed it will work.

This is the case for literally anything you use Object.create for where the constructor actually does something (which includes private fields installation)

@abhay187
Copy link

For now I have removed private variable from my classes. From the above comments it looks like a bit hard job to make it work in Vue3 as of now.
Still I wish Vue3 will support it in near future.

@rdking
Copy link

rdking commented Nov 27, 2021

@abhay187 If you really want to use private fields while getting around it's obvious issues, and you don't mind adding a little extra work, look here. I just got done deciding on good workarounds for the top 3 of these issues today.

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

No branches or pull requests