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 fields breaks POI #317

Closed rdking closed 4 years ago

rdking commented 4 years ago

I'm not sure if this has been brought up before, but it appears that an instance object of a class using private fields cannot be used as a prototype object.

var testObj = Object.create(new class {
   #data = 0;
   test() {
      return ++this.#data;
   }
});

testObj.test(); // Throws!

The reasoning is simple enough. Since testObj does not own the private slots being accessed by test(), the private name lookup fails. Doesn't this break user expectations about instance object that are not from built-in classes? With prototype oriented inheritance being a major feature of the language, this seems to be a mistake.

Is it not possible to have private fields follow the same property convention for public properties? Is there an issue with having the private field be read from the prototype chain if not on the topmost instance, and defined on the topmost instance object on write?

ljharb commented 4 years ago
Object.create(function () {}).toString(); // throws!
Object.create(Promise.resolve()).then(); // throws!
Object.create(new RegExp()).test(); // throws!
Object.create(new Date()).valueOf(); // throws!
Object.create(new Map()).size; // throws!
Object.create(new Set()).size; // throws!
Object.create(new String()).toString(); // throws!

etc. In other words, the extension mechanism that already must be used to avoid this problem is class extends and super(); Object.create has had this behavior since its inception in ES5, so I'd say users should be expecting this very behavior.

rdking commented 4 years ago

You're missing the point while proving it. Before this proposal, there is no object that a user can create that cannot be used as a prototype object. That's by design. Now, the user will be able to create objects that cannot be used as a prototype. It's perfectly acceptable for built-ins (as in your example) and external objects (as in HTML objects, etc) to cause this kind of issue. Their code does not come from an ES language source file. There are obviously going to be some things that get lost in translation. However, for something completely defined within the language to suddenly begin behaving as though it were something imported with native code.... That can't be a good thing.


All opinions aside, what I'm looking for here is not an excuse to get rid of this proposal (not that I don't want it gone). What I'm looking for is the reason why private fields were modeled in a way that doesn't lend them the same freedoms they have without private fields. It that results in another reason to hate private fields, oh well.

ljharb commented 4 years ago

My example includes a bunch of user-created objects. The same is true for any class extends one of these builtins.

Here's an entirely user-created object that doesn't work:

const o = {
  f() {
    if (this !== o) { throw 'nope'; }
  }
};
Object.create(o).f() // throws

There is no difference here between "native" or "builtin" things, and user-created things.

rdking commented 4 years ago

Ok. That was an error in wording on my part. I meant user-created classes. As for your "entirely user-created object that doesn't work", for that not to work is expected as the ES source code for it pretty much specifically reads "don't work for that case". No ES programmer would look at that and have any misconceptions about why it doesn't work. On the other hand, the same cannot be said for an object containing private fields.

The key difference here is that, by design, private fields look like they're just another part of the object that just happens to be hidden in a special way so that they cannot be detected by anything external to the declaring scope. Even the champions of this proposal (and you as well) have stated on many occasions that the presence of a private field should cause interference with existing operations. Prototype manipulation is still a fairly common procedure. If someone with working code substitutes a normal object with one having private fields and passes it along to some function that will use that object as a prototype, where it worked before, it will now fail.

The last time this kind of issue was brought up, it was surrounding Proxy. TC39 artfully dodged that issue by essentially claiming prior art and shelving it as part of a larger issue. In this case, however, there is no prior art. A user-defined class is not the same as a built-in class. Nor is it the same as an externally injected class. To have a user-defined class behave in a way similar to a built-in class just due to the presence of a private field runs a non-0 risk of error.


I know this doesn't convince you of the folly of this proposal. I'm also fairly certain nothing will. That's why I'm just going to make a suggestion:

Change OrdinarySetPrototypeOf(O, V) so that it throws a TypeError if theV parameter contains private fields.

At least then, there will be a clear error at the place where the problem will occur.

ljharb commented 4 years ago

I'd say the same, because "private fields" explicitly mean "anything that isn't 'this class' or 'something that extends this class' will break". That may be something folks have to learn, but that's still the mental model.

ljharb commented 4 years ago

Your suggestion would make "private fields" observable, and thus utterly dismantle the entire concept of "hard privacy", so I don't think that's viable.

rdking commented 4 years ago

@ljharb

Your suggestion would make "private fields" observable, and thus utterly dismantle the entire concept of "hard privacy", so I don't think that's viable.

How so? If I remember correctly, one of your previous arguments was that as long as individual private fields are not themselves identifiable, it was acceptable for one to be able to notice that an object contained private fields. The suggestion above is of exactly that nature. The name of the private field remains a secret, but the object itself can be flagged as having private fields.

I'd say the same, because "private fields" explicitly mean "anything that isn't 'this class' or 'something that extends this class' will break". That may be something folks have to learn, but that's still the mental model.

That's the problem. If it's "something that extends this class" in a prototype oriented language, then the scenario in the OP shouldn't break. To be strict, the above scenario created and instance of "this class". The fact that the instance didn't pass through the constructor of "this class" is the problem. So does this mean that private fields seeks to change the definition of what an "instance of a class" is? Unless I'm mistaken, that's a pretty huge semantics change. It's not as simple as learning a new mental model. The problem is that if the new definition of "instance of class X" is "an object that has passed through class X's constructor", then that means work needs to be done to revamp instanceof again. Plus some kind of flagging needs to be set even on objects produced by classes that don't use private fields to make it all consistent.

Somehow, I don't think those are the consequences you intended, so try to re-word your meaning. Remember, I'm no longer looking for a way to kill this disaster of a proposal. I'm only looking for a way to make it at least reasonably usable on a broader scale than the very narrow use cases TC39 has considered. Right now, this proposal is only something to be avoided unless you only intended to write flat classes, at which point, there's no point in using them except to save a few keystrokes on boilerplate. At least with the boilerplate, I have enough control to avoid all the pitfalls in this proposal.

ljharb commented 4 years ago

It's ok if the default behavior exposes the existence of nonzero private fields - however, a class can guard field accesses to avoid that being a problem. With your suggestion, how would I prevent someone from determining that?

rdking commented 4 years ago

@ljharb

...however, a class can guard field accesses to avoid that being a problem. With your suggestion, how would I prevent someone from determining that?

You don't need to prevent anything. That's the point here. It's not about whether or not private fields can be detected. It's about the fact that you'll get an unanticipated error when trying to do something like what is in the OP. Even worse, that error will not occur anywhere near where the problem exists. Because the error can be so far removed from the cause, it becomes necessary for developers to implement tests for the existence of private fields when doing prototype manipulation. However, that becomes extremely difficult and unpredictable since private fields are designed to be hidden from most simple forms of detection.

The long and short is, if you're going to go this far to ensure that private fields can't be detected, you also need to go as far as to ensure that they don't cause arbitrary errors that are hard to track down.

ljharb commented 4 years ago

Yes, and that's entirely possible to do with the current proposal. If OrdinarySetPrototypeOf threw when the second parameter "contained private fields", then it would become impossible to do it.

In other words, if you want to use a non-super means to extend a class, or if you're extending a legacy constructor function, the only option you have (and one that has always been available) is to test at runtime that you can successfully instantiate and use it, at which point you would have been able to react to the exception, whether it was caused by the receiver lacking private fields, or caused by any of the pre-ES6 or ES6+ identity-based mechanisms that would fail when all intended constructor code is not used.

trusktr commented 4 years ago

The OP snippet wouldn't throw if private fields were based on WeakMap semantics. That'd be better because it would allow authors of private-field-containing classes more ways to handle inheritance; they could decide to store values on the foreign objects, or to throw. (Even if some people dislike inheritance, other people like it, and we shouldn't limit the language in favor of non-inheritance when people who dislike inheritance can simply avoid it or throw in those cases.)

Your suggestion would make "private fields" observable, and thus utterly dismantle the entire concept of "hard privacy", so I don't think that's viable.

The name of the private field remains a secret, but the object itself can be flagged as having private fields.

The name isn't a secret, SomeClass.toString() gives it to you. So does the error thrown in the OP.

The fact that we can detect private fields is soft privacy in the same way as alternative private foo = 123 proposals reveal that there's a private field on access (instead of indirectly via method call).

I feel that the "soft" vs "hard" privacy argument has been a moot point.

The form private foo = 123, even without ability for private properties to be shadowed by protected or public properties of the name, would be better as far as hard vs soft privacy is concerned, because the private foo form is "less hard" by a very insignificant margin.

In either type of proposals (#foo vs private foo), I can see the goods in the house, but I can not get beyond the security system in order to steal the goods. People prefer the private foo syntax a lot more (as has been clearly demonstrated in other threads), and it'd be great to have considering the hard/soft argument in favor of #foo form is not really that solid.

Anything that isn't 'this class' or 'something that extends this class

@ljharb The existing language, before and after private fields, disagrees:

class Foo {
   #data = 0;
   test() { return ++this.#data }
}

var testObj = Object.create(new Foo)

console.log(testObj instanceof Foo) // true

Yes, and that's entirely possible to do with the current proposal. If OrdinarySetPrototypeOf threw when the second parameter "contained private fields", then it would become impossible to do it.

Unfortunately that doesn't fix the fact that it is completely undesirable for someone's existing (and common) code idioms to break because someone else decided to use a private field as internal implementation detail.

rdking commented 4 years ago

@ljharb

Yes, and that's entirely possible to do with the current proposal. If OrdinarySetPrototypeOf threw when the second parameter "contained private fields", then it would become impossible to do it.

Incorrect again. You seem to have forgotten that a similar test can be performed using Proxy. Exactly how much are you discounting the severity of the Proxy problem? Even if the Proxy problem didn't exist, it would still be possible to check for the presence of private fields.

class Ex {
   #data = 0;
   test() {
      return ++this.#data;
   }
}

try {
   Ex.prototype.test.call({});
   console.log("No privacy mechanism found.");
}
catch (e) {
   console.log("Privacy mechanism detected!");
}

try {
   (new Ex).test.call({});
   console.log("No privacy mechanism found.");
}
catch (e) {
   console.log("Privacy mechanism detected!");
}

...And that's just how you'd do it if you weren't expecting to use the TC39 discounted .toString() approach. Adjusting [[OrdinarySetPrototypeOf]] only serves to ensure that if someone unwittingly makes this kind of mistake, that it will throw where the mistake exists, and not further down the line where it is much more difficult to correlate the error with the cause.

ljharb commented 4 years ago

@trusktr I can .bind my class, blocking toString introspection. The name is absolutely a secret. Private fields are based on WeakMap semantics, which is why the identity of this matters.

instanceof is unreliable and forgeable, and thus isn’t a metric for “is an instance of the class” that i care about.

ljharb commented 4 years ago

@rdking you’re assuming I’ve authored my class to actually let those exceptions be thrown - i wouldn’t. I’d never reference a private field without wrapping in a guard (try/catch or in, when that proposal advances), and if i waged an exception I’d throw it, and you wouldn’t be able to determine that it was due to private fields, or WeakMap semantics, or even just keeping all known instances in a closed-over memory-leaking array i pushed into at construction time.

ljharb commented 4 years ago

At this point, the thread seems to have devolved into a many-times-rehashed debate about private fields themselves; I’ve answered the OP’s point how private fields don’t break anything that isn’t already “broken”; I’ll try to bail from further debate, and I’d prefer you both try to do the same.

rdking commented 4 years ago

@ljharb

you’re assuming I’ve authored my class to actually let those exceptions be thrown

? Huh? Since when was any of this about you? You're just a single programmer in an ocean of programmers. It would be utterly foolish to think every programmer thought, behaved, and understood as you do. My existence alone is proof of the folly of that idea. Please, try to be more open and recognize that you are not the only user of this language, and that the usage patterns preferred by TC39 are not the only viable usage patterns for this language.

I’ve answered the OP’s point how private fields don’t break anything that isn’t already “broken”

Yes, you have, but the answer is incorrect. That's the only reason I'm bothering to carry on this conversation. If you're that adamant that your point of view is the only correct one, then you're right; there is no longer any point in persisting.

trusktr commented 4 years ago

I’d never reference a private field without wrapping in a guard

I'd like to see a code base that in practice actually does that at every site where a private field is used. I bet we'll be hard pressed to find one.

ljharb commented 4 years ago

@trusktr private fields are pretty new, so it'd be hard to find a codebase that uses private fields much yet :-) i can assure you every usage of private fields in my code forever will do that.

rdking commented 4 years ago

@ljharb Do you honestly and fully believe it wise to shape a broadly used language according to your personal usage patterns with a reasonable expectation that everyone will code as you do?

ljharb commented 4 years ago

@rdking i did not claim to be doing any such thing; i was responding to "i bet we'll be hard pressed to find one".

rdking commented 4 years ago

@ljharb

@rdking i did not claim to be doing any such thing; i was responding to "i bet we'll be hard pressed to find one".

I understand that. However, as has been the case with so many conversations you've been in, you've failed to take in the context of the conversation. When you make a comment like

i can assure you every usage of private fields in my code forever will do that.

in a conversation that more or less relates community usage patterns to how TC39 is making decisions, it ends up reading as though you think "because I do it this way, everyone else is too." Even if that's not your intent, that's how your comments, and indeed the general consensus of TC39 is reading to the community.

A thought experiment... Try this thought on for size. I live in a city where most roads contain a median to control cross-going traffic. While that reduces the risk of accidents, it also impedes access to places on the opposite side of the street. To alleviate this, the roads are designed to make use of U-turns at intersections. Now lets suppose some members of the city council (our TC39 analogs in this example) decide that even U-turns are too risky based on the fact that a relatively small (as in <1%), but non-0% of drivers have died (Node vs NPM?) in car accidents caused by poor U-turn usage (library developer complaints, foot guns, etc...) and create a bill to eliminate U-turns across the city in favor of circling the block (private fields, set vs define, etc...). When some of the citizens of the city who have an active interest in this (we actively dissenting, non-TC39 members of these discussions) finally hear about it, we raise complaints that this will increase the level of traffic noise (ugliness of syntax), accident risks for children playing in the streets (Proxy issue, breaking inheritance with accessors, etc...), and other issues. However, the council evaluates the risk of the individual new issues and rightly finds them individually uncompelling, ignoring entirely how compelling those issues become when taken together as a whole. Upon finding no reason to stop, they push forward with the new bill. Other suggestions come along, like adding a U-turn phase to the traffic lights (the numerous alternative proposals), that reduce the risk of accidents without incurring the new risks, but the council arbitrarily dismisses many of them without so much as reason why. The few that are not dismissed off-hand are still eventually dismissed due to misunderstanding, politics, or specific agendas that aren't necessarily conducive to the city's or citizen's well being. Then, one day a citizen happens to get the attention of one of the council members who is known for being open and direct with the community. The citizen asks about the new bill and raises the concern that most citizens won't know of or think about needing to find the nearest location where they can go around the block to turn around. The council member replies in a polite but overly worded way that he always does because he always plots his course before every drive anywhere. Would you like to live in such a city? Especially if your house was in the path of the new proposed "circle the block" routes and you had children or needed to leave the house for work in a soon-to-be very busy area? I hope you read this and understand my point. The problematic difference here is that as a citizen in a city, we get to vote on the bills that affect us and have a chance to veto them. We can also affect what gets passed by voting in or out a particular politician. Neither of these are available to us when it comes to TC39 and its members, yet we're stuck with the effects of their decisions, be it good or otherwise. Again, I accept that TC39 is doing what they think is the best they can, but that doesn't in any way imply that this is the best that can be done, or even the best that they can do.
ljharb commented 4 years ago

It certainly is not my intent to imply in any way that TC39 makes decisions based solely on the personal styles of those in the room; it does not.

rdking commented 4 years ago

@ljharb Going back to the original post... Let me ask a direct question. Given that

It's ok if the default behavior exposes the existence of nonzero private fields...

does this also imply that it is also ok that one of the most common and useful usage patterns (prototype extension) in the language will break should the prototype object contain private fields when a method of the prototype that uses those private fields is called from an instance object that was created via non-class prototype extension? Please consider the fact that even though class has been in play for quite some time now, there are many libraries that forego the use of this keyword and make use of prototype extension directly.

ljharb commented 4 years ago

As has been stated many times, if you inherit from any X (class or archaic), you are already hoping/relying on that X implements itself in a way that's compatible with (your exact flavor of) subclassing.

extends/super provides an explicit way to subclass modern classes, but you are still hoping/relying on that X implements itself in a way that's compatible with (your exact flavor of) subclassing.

There has simply never been a way, and still does not exist a way, to generically subclass something without intimately knowing its implementation details, and that remains true (altho super/extends provides a conventional protocol for doing so). Private fields do not change that in any way except perhaps making it more likely that less knowledgeable users will be more likely to use such a feature.

rdking commented 4 years ago

Actually, there is a way to generically subclass any class, and it has nothing to do with either super, extends, or even Reflect.construct. It involves a concept I call sub-instancing and involves one of the paths not taken for implementation of class. I've implemented it in the past and it does work, but that's a topic for another discussion. This is why when you said

I’ve answered the OP’s point how private fields don’t break anything that isn’t already “broken”

I replied

Yes, you have, but the answer is incorrect.

That being said, I'm beginning to recognize my arguments as diverging too far from the existing bad path that ES is already irrevocably on. Given the existing limitations of the design of class and the common patterns of inheritance when not using class, your response is reasonable. While I don't agree that it was necessary to add private fields to the short list of implementations that break prototype inheritance, arguing about it would be pointless.

trusktr commented 4 years ago

i can assure you every usage of private fields in my code forever will do that.

That's a pain. Easy to forget. Plus try-catching every usage of a private field is totally non-ergonomic.

trusktr commented 4 years ago

As has been stated many times, if you inherit from any X (class or archaic), you are already hoping/relying on that X implements itself in a way that's compatible with (your exact flavor of) subclassing.

Who said the community wanted two forked versions of subclassing? class was supposed to be syntax sugar.

Do you know that probably most people have no idea about the subclassing implementation fork, and thus no idea about the foot guns when they do things one way vs the other? How do you rectify 1.5 decades of articles on prototypal inheritance?

trusktr commented 4 years ago

Private fields do not change that in any way except perhaps making it more likely that less knowledgeable users will be more likely to use such a feature.

I believe you're underestimating the size of "less knowledgeable users". I guarantee you, most people do not want to learn about the intricacies of what now exists, because they want to write apps, but they'll be forced to learn (and waste their time) when their code one day mysteriously breaks.

ljharb commented 4 years ago

I agree about try/catch fwiw, see my stage 2 proposal: https://github.com/tc39/proposal-private-fields-in-in

trusktr commented 4 years ago

arguing about it would be pointless.

Perhaps it isn't pointless, because if it helps more people in the community become aware, then any future mistakes might be more avoidable.

trusktr commented 4 years ago

I agree about try/catch fwiw, see my stage 2 proposal: https://github.com/tc39/proposal-private-fields-in-in

That seems like a syntax bandaid for the existing wound. If private fields actually used WeakMap semantics, we wouldn't need that.

rdking commented 4 years ago

@trusktr Part of the problem with this proposal is the fact that it uses WeakMap semantics. If it instead focused on the only existing privacy in the language (closures) then there wouldn't be nearly as many issues. Isn't the argument for creating this proposal the sheer difficulty of having closures over instances? That being said, regardless of the semantics used, you'd still end up in a situation where in order to hide what kind of privacy you've implemented, you're stuck doing tests of some kind.

@ljharb I get what you're trying to do, but I don't see where the in syntax makes any significant improvement. The cumbersome part is that you have to test at all. By the time you put in all those tests, you could've just used WeakMap and had more control over less code. I also don't see how this syntax should work without turning #<name> into a first-class expression. Right now it's <"string">|<Symbol> in <object>. To make it <"string">|<Symbol>|<PrivateName> in <object>, doesn't <PrivateName> need to be valid as a RHS value (a first class object)?

trusktr commented 4 years ago

it uses WeakMap semantics

(Except we get undefined instead of an error (Proxy wrapper won't throw).)

With the private foo = 123; ... this.foo proposals (where public code can't set a public var with the same name as the private one), does the Proxy issue still exist?

ljharb commented 4 years ago

The "Proxy issue" is simply that the this is incorrect. With that (nonviable) approach, the issue would still exist just the same, because the approach does not alter Proxy itself.

rdking commented 4 years ago

To be more precise, Proxy blows because the value to be accessed doesn't exist on the Proxy object itself and isn't accessible via any of Proxy's handlers since computed property names don't work for private members. Even if computed names were allowed, it still wouldn't work unless the Proxy handler methods were themselves defined within the lexical scope of the class.

@ljharb We once discussed the idea of adding a "resolve" handler added to the Proxy Handler methods. Was this a viable idea? As a reminder, the "resolve" handler would be triggered as a way to change the access target of a request without altering the receiver. The signature would be similar to

receiver(target, isPrivate, isWrite, receiver): object

where

The return value would be a non-null object to be used to complete the access request. With this, a Proxy could correctly redirect the access attempt to the original owning object so that the get or set operation can correctly complete, all without leaking the PrivateName being accessed or the values being passed around.