-
Notifications
You must be signed in to change notification settings - Fork 113
Branding guarantees do not match web platform classes, due to return override #179
Comments
Can you elaborate on why the return override technique is needed to self-host classes involved in inheritance hierarchies? |
Is this because these classes don't respect overriding the super class? Text.__proto__ = function () {
throw new Error('UNREACHABLE');
};
new Text();
// => Text "" |
@ljharb it's not; it's only used to illustrate a "failure" of the brand checks in the example. @jridgewell good catch, I think you are right. I've lost track of the current proposal in that regard: can we still use private fields with constructors that never call |
@domenic then why does it mean this?
I believe that in a subclass, calling |
Under normal circumstances, no. Fields are installed in a weird step after the parent constructor returns, but before the If these classes don't actually have a comparable |
@ljharb I don't understand; the two things are disconnected. @jridgewell thanks. That's not great; it drives home the point that we could not use private fields for self-hosting any classes involved in inheritance hierarchies. I guess back to weak maps for those, unless this proposal's champions have any possible fixes? |
Don't weak maps/sets have the same problem? var set = new WeakSet()
class Base {}
class Ex extends Base {
constructor() {
super();
set.add(this);
}
isExample() {
return set.has(this)
}
}
var o = {}
Ex.__proto__ = function () { console.log("called"); return o };
new Ex;
Ex.prototype.isExample.call(o); // true |
@domenic i guess i'm not understanding why you'd override |
The behavior you describe looks to me like the branding is on the base class, rather than the subclass. For example, from the behavior you describe, I'd expect class Node {
#isNode;
static assertNode(o) {
o.#isNode;
}
}
const assertNode = Node.assertNode;
delete Node.assertNode; // yes, I know this is awkward; decorators can make it a bit simpler, but it's just for illustration
class Text extends Node {
splitText(offset) {
assertNode(this);
// remaining implementation
}
} This matches the behavior you observe: (new Text).splitText(); // works
const o = {};
Text.__proto__ = function () { return o; };
new Text();
Text.prototype.splitText.call(o); // throws!! Of course, this doesn't match with the observed behavior of not actually invoking the superclass.
Two possible alternatives:
|
Edit: sorry, I misunderstood. The following works for web platform classes: const o = {};
Text.__proto__ = function () { return o; };
Text.prototype.splitText.call(new Text) // does not throw and not for the implementation below. So, yes, I can't see a way of making this work without a Original incorrect idea follows: On further thought, a third alternative would be to do something like: class Text extends Node {
#privateStuff = 'abc';
constructor() {
if (Text.__proto__ !== Node) {
return { __proto__: Text.__proto__ };
}
super(); // installs #privateStuff on `this`, which is guaranteed to be a Node
}
splitText(arg) {
return this.#privateStuff.split(arg);
}
} I think this matches web platform classes in all particulars, at least as described in this thread, down to not invoking artificial (and therefore observable) superclasses: (new Text).splitText('b'); // works
const o = {};
Text.__proto__ = function () { console.log('reached'); return o; };
new Text(); // does not print 'reached', is not === o
Text.prototype.splitText.call(o); // throws |
Yes, it's a core issue with subclassing.
He's not. He want's to implement Further, if we were to fix the override difference, there's still the issue of allowing a subclasses privates to be installed on a foreign instance by overriding the superclass.
Doesn't this defeat the point of branding by default if I still have to manually brand? @domenic: Do these classes actually use inheritance, other than to define the prototype's inheritance? If they don't, they could be implemented as base classes, then override the prototypes. |
To clarify what I meant about "back to weak maps", I think the following would be the only way to implement web platform / ES262 class semantics: const _data = new WeakMap();
class Text extends Node { // let's ignore CharacterData for this example
constructor(data = "") {
const o = Object.create(new.target.prototype);
_data.set(o, data);
return o;
}
splitText(offset) {
if (!_data.has(this)) {
throw new TypeError("Not a Text!");
}
return _data.get(this).substring(offset); // (not really what splitText does)
}
} It's a shame we couldn't do something similar to class Text extends Node {
#data;
constructor(data = "") {
const o = Object.create(new.target.prototype);
o.#data = data; // !?!
return o;
}
splitText(offset) {
return this.#data.substring(offset);
}
} i.e. it's a shame there's no way to use private fields with the return-override feature that apparently is always in use by web platform and ES262 classes. (Does ES402 have any subclasses?) @bakkot regarding your (1) and (2), do you think it would be reasonable to do either of those two alternatives to all the class hierarchies in the ES262 spec which also have this problem? I think that's mostly the typed arrays, although maybe also the various |
I personally wouldn't be opposed to changing either 1 or 2 for classes in the spec, but would prefer other options. Spec classes can be weird in other ways anyway. Per @jridgewell's comment, could you do something like class Text {
#data;
constructor(data = "") {
this.#data = data;
}
splitText(offset) {
return this.#data.substring(offset);
}
}
Text.prototype.__proto__ = Node.prototype;
Text.__proto__ = Node; ? i.e. manually wire up the class hierarchy, but keep
This only works as long as
You can seriously abuse the return-override trick to install private fields on arbitrary objects if you really want to: class DoNotDoThis {
constructor(o) {
return o;
}
}
class Text extends Node {
#data;
constructor(data = "") {
const thiz = new Node();
thiz.__proto__ = Text.prototype;
Text.__proto__ = DoNotDoThis;
super(thiz);
Text.__proto__ = Node;
this.#data = data;
}
} buuuut don't. |
Do you think they should be weird in these ways, though? I guess I'm unclear why spec classes (in particular ES262 classes) don't follow the same patterns as author-defined ones.
Hmm yeah, that seems superior to my Object.create() approach at least....
Right, they kind of do... i.e. these things should be branded as But I guess this gets into a whole thing where the real requirements are much more complicated than my simple examples so far, e.g. you want "protected" type behavior so the base class can declare private fields that So not being able to meet the real requirements is OK-ish, I guess. (Which makes me wonder why I opened this thread in the first place...) I'm not sure there's much actionable left for this thread; thanks for all the attention to detail on walking me through it. I guess it might still be worth pursuing the question as to whether ES262 classes (and probably web classes, following them) should call into their super-constructor. |
Then my approach won't work. You have to construct a Or, use decorators to extract the necessary private field's key from This would be simpler if you could install private fields onto foreign objects. It's easy to achieve the same guarantees as branding on top of general encapsulation, but rather cumbersome to do normal property semantics the other way. |
This is the real problem. Branding semantic of current proposal is designed for emulate host object behavior. (at least, one of the reasons) The question is, why host object have such behavior? Not very clear to me. I guess just the historical reason because it's not easy to make host object fully follow JS object semantics. On the other hand, do most JS programmers need such branding semantic which just break prototype-inheritance and proxy transparency? Very doubtful. The most ironic, current fields proposal still can't satisfy simple cases from @domenic , so you still need to use weakmap or other "DoNotDoThis" trick manually! So I just don't understand what's the rationale of the current design. Sigh... |
@domenic Really insightful post. This issue is a bit broader than just how private interacts with inheritance: If any logic at all is in the superclass constructor, prototype chain mutation can make it not happen, and make other stuff happen instead. Platform objects don't tend to traverse the dynamic prototype chain to run instantiation behavior in their constructor; they just initialize themselves fully from the constructor. So, they're logically following the "original" constructor prototype chain already. At the limit: If we were to deeply freeze things in built-in modules, then they would already have immutable prototypes, and the issue would not occur. But, this causes other problems as well (e.g., you couldn't monkey-patch behavior onto existing platform-provided objects, only ones that are vended by the wrapper). Maybe this is the point where we should really reconsider adding a way to freeze the There's already thought going into these new classes having non-enumerable methods, and putting them in built-in modules, and AFAIK no one complained about freezing For an API to freeze the prototype: What if Object.preventExtensions took an options bag, which you could use to control the level of freezing, like Another direction would be to make it so that we could use |
use static-functions instead of class-methods. problem solved. |
@kaizhu256 I believe the fundamental thing here is about how private fields work, not methods. |
This comment has been minimized.
This comment has been minimized.
As an original poster really interested in discussing the issues at hand, I'd strongly request that the champions and repo admins mark off-topic comments as "off-topic" to keep the thread focused, and those wishing to discuss "the design of private fields" and "example UX-workflows" and so on move that to another thread. |
@kaizhu256 I've hidden your comment #179 (comment) . There are many other open threads to choose from, or you can start another one, arguing that private is not a good idea. Let's use this thread to work through the issue that @domenic raised. |
Funny. This is exactly why I describe branding as merely a means to ensure the desired "fields" exist.
Why can't you? class Text extends Node {
#data;
constructor(data = "") {
Text.__proto__ = function() { return Object.create(new.target.prototype); };
super();
this.#data = data; // !?!
}
splitText(offset) {
return this.#data.substring(offset);
}
} |
Or better still... class Text extends Node {
#data;
//Using set/getPrototypeOf to avoid __proto__ deprecation
//Replacing the original value to ensure existence of static members
constructor(data = "") {
let original = Object.getPrototypeOf(Text);
Object.setPrototypeOf(Text, function() { return Object.create(new.target.prototype); });
super();
Object.setPrototypeOf(Text, original);
this.#data = data; // !?!
}
splitText(offset) {
return this.#data.substring(offset);
}
} |
One of the original motivating goals of private fields was to "explain JS with JS": to provide syntax for the internal slot capability of built-ins. Issues like this illustrate that private fields are too restrictive to accomplish that goal. If the answer to this particular issue is to "use WeakMaps", and WeakMaps are the only first-class mechanism we can use to represent non-reflective private state, then I think we should consider whether WeakMaps were the correct feature for self-hosting all along. I now believe that for the vast majority of application and library code, normal symbols are the most appropriate mechanism, as they interoperate well with all other language features. Given the above, I'm not sure where private fields (or private symbols for that matter) fits in anymore. |
Private fields are only too restrictive to accomplish that goal because web platform classes and TypedArrays have this behavior where they have both inheritance and mutable prototypes, like ES6 classes, but will always invoke their original prototype even if the prototype has changed, unlike ES6 classes (presumably because this matches the common pre-ES6 "class" pattern using a function). But they can't be called, only That is, fundamentally this is a mismatch between ES6 classes and web platform classes. If that difference could be patched over - for example, by making classes with this behavior have immutable prototypes - then private fields + ES6 classes would explain web platform classes perfectly modulo cross realm issues. I think that would actually make it easier to understand web platform classes, although it has some ramifications for the ability to polyfill certain kinds of potential future changes. That said, there's a question of what we mean by "explain". One answer to this issue is to say that it's a perfectly good explanation as long as no one tries to modify the prototype of a built-in or host class, which ought to be vanishingly rare. Frankly I am tempted by that answer. |
@bakkot After all effort, and pay lots of cost, we can never fully emulate host behavior, so maybe we'd better let it go.
As my 20 years web development experience, it wouldn't. Most programmers just use APIs and never care about branding (they even never heard the word "brand checking"). Only the authors of polyfills and libraries like jsdom care about it, and they have comprehensive knowledge about the quirks of platform classes and don't need use class field to learn the behavior. On the other side, make prototypes immutable could limit the ability of them. |
I know many people are not especially inclined to ask how things work. But some people are so inclined, even as beginners. I think it would be a shame not to have a better answer to give to those people than "magic", at least for the broad strokes.
Even in the absence of private state, I would still think immutable prototypes (or making these things invoke their superclasses) could be a good change. Having a class-like thing which does not invoke its nominal superclass is weird, from the perspective of ES6 classes. Private state isn't really relevant to that. |
Actually it's "magic", or let's say it's a host object which can not follow the rules of normal JS objects. I don't think it's hard for beginners to understand it. And we already know even you can use class field to explain some behavior, you still leave the holes like cross-realm. And you also need to explain why an object have private will fail on proxy, prototype-inheritance while an object without private works fine.
Actually I'm ok with it. But I don't know whether some author of polyfills would blame you. And you are suggesting a web compatibility breaking change. Good luck.
Agree. And I hope we never mixed the very different motivation together, or we will just get the weird thing like current class fields. |
Would immutable prototypes mean that I can't reassign |
If the constructor chain were immutable, it would not be observable whether these things actually called their superclass or not (because the superclass they start with does not have observable side effects when called). At least, I'd hope that would be the case. |
So the follow up to that is: Would immutable supers be available to normal ES6 classes (say, either by default or by some opt-in keyword)? But even if we do that, it doesn't erase the attack vector, it pushes it earlier. Instead of being able to override the super after the subclass-with-private is defined, I just have to override the constructor before the subclass is defined. This may not be a huge problem. Local definitions would be fine, and probably module imports? But anything defined on the global object has the potential to be overridden. " |
See tc39/ecma262#538, but I'd hope so.
Not entirely sure what you mean by "attack vector" in the context of this thread, although generally speaking it's pretty much impossible to be totally defensive against code which runs before you, except in extremely limited circumstances. Code which is trying to be defensive against other hostile code has to run before that code. |
Using branding as an assertion of "instance of this class". If I can overwrite the super class before you define the subclass, then I can use the same That makes branding really just as "having one implies having all the others" check. |
I imagine this would cause significant implementation complexity for browsers, letting all sorts of useless edge cases be visible when they aren't currently. I don't see any reason to go in this direction. |
FWIW I documented the "brand" guarantees of this proposal in this FAQ entry. The next step for me is to follow up with the |
I wrote up a very early proposal draft for |
x-posting from irc: With the original example that is roughly i think the fact that |
@devsnek Sorry, I'm not sure what you're getting at. I see this as something that makes private fields analogous to internal slots, which would be meeting the goal expressed in the original post. |
@littledan my understanding was that this was the request: class X {
#y;
constructor() {
const O = getObjectSomehow();
O.#y = 5; // this should be possible somehow
return O;
}
} My thoughts, looking at this, is that the
|
@devsnek, yeap |
@devsnek I don't quite follow. Internal slots are not tied to things that have a certain prototype chain, but rather to what the constructor outputs. Public fields work like this too--they're just a way that the constructor can add things to the object, and don't have anything to do with prototype chains. The issue here was, even if you mess up the prototype chain of the constructor, you should be able to have the private fields added to the instance. I agree that it's OK to be able to write a class that doesn't have the private fields and instead returns something else. The question was, how should you be able to use the original prototype chain and still add the private fields/methods to the instance, when you want to? @Igmat I can see how making private symbols that you can add to arbitrary existing instances is a solution to the problem, but does @jridgewell 's proposal (which doesn't do this, at least in the first step) address it? |
@littledan @devsnek Or put more concisely, what does it matter where the object came from? Is the constructor of a class that has private fields free to put those fields on any object? |
In this issue, we're talking about matching the branding semantics of web platform and JS builtin classes, which do something special with things that were the output of |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'm going to unsubscribe from my own thread as this appears to have degenerated into being the same as every other issue thread in this repository, i.e. a place for people to complain about the current proposal and push their counterproposals. Let me know through out-of-band communication if any more interesting on-topic discussion requires my attention. |
I've hidden comments that were about other proposals. Folks can follow up about private symbols in https://github.com/zenparsing/proposal-private-symbols . |
That's a prime example of how existing "weird" (exotic) behaviors are cutting into what a good language feature can actually be, at the expense of end users. New features don't have to match what existing weird/exotic objects do. Give users the best features for their code, and leave the old mistakes as they are without limiting innovation just to explain existing mistakes. rant
Also a place for people who aren't paid while writing here to feel as if there's little hope. :) Consider yourself very lucky if you can write here while "at work" and getting paid! More importantly than creating other proposals, is stopping or slowing this one. Requiring that some other proposal be accepted in order to stop this one (or else this one will get released and implemented) is not a nice thing for the community to be subjected to. "Make a better proposal now or else this one will be implemented with its flaws and all." Its in some ways similar to "Jane gets the electric chair in one week, for killing John, unless you can prove she didn't do it" while having seen someone else do it with your own eyes but not fortunate enough to have evidence. |
I think we have examined this topic sufficiently, so I'm closing the thread. |
Consider the following:
Now consider the analogous setup with the
Node
andText
classes:or, using ES262 classes:
This mismatch is worrying. It means that we cannot use private fields for self-hosting any classes involved in inheritance hierarchies.
I'm not sure what a good fix would be, but it seems like we should investigate what can be done here.
The text was updated successfully, but these errors were encountered: