domenic commented 5 years ago

Consider the following:

class Base {}
class Ex extends Base {
  #foo = 1;
  isExample() {
    return this.#foo === 1;
  }
}
const o = {};
Ex.__proto__ = function() { return o; };
new Ex;
Ex.prototype.isExample.call(o); // => true

Now consider the analogous setup with the Node and Text classes:

const o = {};
Text.__proto__ = function () { return o; };
new Text();

Text.prototype.splitText.call(o); // throws!!

or, using ES262 classes:

const o = {};
Uint8Array.__proto__ = function () { return o; };
new Uint8Array();

ArrayBuffer.isView(o); // false

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.

ljharb commented 5 years ago

Can you elaborate on why the return override technique is needed to self-host classes involved in inheritance hierarchies?

jridgewell commented 5 years ago

Is this because these classes don't respect overriding the super class?


Text.__proto__ = function () {
  throw new Error('UNREACHABLE');
};
new Text();
// => Text ""

domenic commented 5 years ago

@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 super()?

ljharb commented 5 years ago

It means that we cannot use private fields for self-hosting any classes involved in inheritance hierarchies.

@domenic then why does it mean this?

can we still use private fields with constructors that never call super()?

I believe that in a subclass, calling super is a requirement to install any private fields.

jridgewell commented 5 years ago

I've lost track of the current proposal in that regard: can we still use private fields with constructors that never call super()?

Under normal circumstances, no. Fields are installed in a weird step after the parent constructor returns, but before the super returns.

If these classes don't actually have a comparable super() call, then I don't know where they'd be installed.

domenic commented 5 years ago

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

nicolo-ribaudo commented 5 years ago

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

ljharb commented 5 years ago

@domenic i guess i'm not understanding why you'd override __proto__ to self-host in an inheritance chain rather than using class extends and super. (if this is off topic we can certainly discuss separately)

bakkot commented 5 years ago

@domenic:

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 Text and Node to look something like

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.

I guess back to weak maps for those, unless this proposal's champions have any possible fixes?

Two possible alternatives:

make the web platform classes actually invoke their superclass, like a regular class; I imagine implementations would very quickly add a fast path to do what they currently do as long as the superclass is the expected thing
make web platform classes have immutable prototype chains (at least up through the base class in the hierarchy), possibly after revisiting https://github.com/tc39/ecma262/issues/538.

bakkot commented 5 years ago

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 WeakMap or changes to how web platform classes work.

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

jridgewell commented 5 years ago

@nicolo-ribaudo: Don't weak maps/sets have the same problem?

Yes, it's a core issue with subclassing.

@ljharb: i guess i'm not understanding why you'd override proto to self-host in an inheritance chain rather than using class extends and super.

He's not. He want's to implement class Text extends Node and Uint8Array natively. But with native class syntax, there's the ability to override the superclass after declaration. These builtin classes do not allow this, meaning there's a difference.

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.

@bakkot: For example, from the behavior you describe, I'd expect Text and Node to look something like... assertNode(this)

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.

domenic commented 5 years ago

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 Function subclasses.

bakkot commented 5 years ago

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 Text as nominally a "base" class, i.e., such that it does not invoke Node in the process of constructing its instances. This gives you all of

the correct inheritance for the class objects themselves
the correct inheritance for instances
Text has private fields
constructing Text does not invoke Text.__proto__
if you modify the Text.__proto__, instances of Text inherit from it, but still have the private fields of Text.

This only works as long as Node does not have its own private fields it needs to install on instances of Text. (Same for your example with Object.create, of course.)

It's a shame we couldn't do something similar to

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.

domenic commented 5 years ago

Spec classes can be weird in other ways anyway.

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.

Per @jridgewell's comment, could you do something like

Hmm yeah, that seems superior to my Object.create() approach at least....

This only works as long as Node does not have its own private fields it needs to install on instances of Text.

Right, they kind of do... i.e. these things should be branded as Nodes, not just as Texts.

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 Text's methods can access. And the architecture for doing that properly would probably instead involve a whole different setup (the impl/wrapper distinction).

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.

jridgewell commented 5 years ago

Right, they kind of do... i.e. these things should be branded as Nodes, not just as Texts.

Then my approach won't work. You have to construct a Node instance to install its private fields.

Or, use decorators to extract the necessary private field's key from Node, and another decorator to install that key onto Text. That would let you keep the same "Text is a base class" hack.

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.

hax commented 5 years ago

I guess I'm unclear why spec classes (in particular ES262 classes) don't follow the same patterns as author-defined ones.

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

littledan commented 5 years ago

@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 __proto__ of objects. We added "Immutable Prototype Exotic Objects" to explain how Object.prototype, Window.prototype, etc. have an immutable __proto__. New-style classes provided by the system could have the __proto__ of both the constructor and the prototype be frozen.

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 Object.prototype.__proto__ or Window.prototype.__proto__, so I don't think the break would be very painful.

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 Object.preventExtensions({proto: true, properties: false}). If the options bag is omitted, it's interpreted as today--this means, for example, if a Proxy trap doesn't forward through the options bag, a more conservative option is used, preserving some notions of safety.

Another direction would be to make it so that we could use Reflect.construct on a fixed superclass, and then "manually" add the private fields (and methods) of the current class to the result. I'm not sure what this would look like; probably new syntax.

kaizhu256 commented 5 years ago

use static-functions instead of class-methods. problem solved.

littledan commented 5 years ago

@kaizhu256 I believe the fundamental thing here is about how private fields work, not methods.

kaizhu256 commented 5 years ago

@littledan the way private-fields work seem more trouble than they're worth.

as a js-dev, can you give me example UX-workflow problem where private-fields would help? e.g., what kind of business-problem would subclassing Uint8Array (or Node-element) with private-fields allow me to solve, that i couldn't with static-functions?

for example:

here's a business-problem where we want to emulate a persistent nodejs-server inside the browser (to speed up product-development/devops by not having to deploy a real backend). one UX-workflow issue was how to emulate image/video uploads.

you could try and reinvent the entire nodejs Buffer toolchain in browser by subclassing Uint8Array, Blob, FormData, and then integrate them with reinvented nodejs-http module. which turned out to be an unmanageable waste of time.

the more efficient approach was to identify the UX-workflow for file-uploads from translating a dom-file-input element -> Blob -> FormData -> Uint8Array -> fake xhr-request to send Uint8Array payload to embedded server -> base64 string -> persistent-storage in IndexedDb.

and then write a bunch of static-functions to facilitate that workflow.

like this real-world example here: https://kaizhu256.github.io/node-swgg/build..master..travis-ci.org/app/#!swgg_id_file_fileUploadManyByForm_1_1

domenic commented 5 years ago

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.

littledan commented 5 years ago

@kaizhu256 I've hidden your comment https://github.com/tc39/proposal-class-fields/issues/179#issuecomment-446185113 . 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.

rdking commented 5 years ago

Funny. This is exactly why I describe branding as merely a means to ensure the desired "fields" exist.

@domenic

It's a shame we couldn't do something similar to...

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);
  }
}

rdking commented 5 years ago

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);
  }
}

zenparsing commented 5 years ago

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.

bakkot commented 5 years ago

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 new'd, which leaves them in this awkward position between ES5 function-style classes and ES6 class-style classes.

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.

hax commented 5 years ago

...modulo cross realm issues

@bakkot After all effort, and pay lots of cost, we can never fully emulate host behavior, so maybe we'd better let it go.

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.

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.

bakkot commented 5 years ago

Most programmers just use APIs and never care about branding

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.

On the other side, make prototypes immutable could limit the ability of them.

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.

hax commented 5 years ago

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.

immutable prototypes could be a good change

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.

Private state isn't really relevant to that.

Agree. And I hope we never mixed the very different motivation together, or we will just get the weird thing like current class fields.

nicolo-ribaudo commented 5 years ago

Would immutable prototypes mean that I can't reassign .prototype or that I can't change its properties? In the first case I don't think that it would conflict with polyfills.

hax commented 5 years ago

@nicolo-ribaudo Not sure. Maybe there are cases you need to insert a class in the hierarchy tree?

ljharb commented 5 years ago

I believe "immutable prototype" means you can't change the [[Prototype]]; ie, via __proto__ or Object.setPrototypeOf.

bakkot commented 5 years ago

Ah, sorry, neither. .prototype on classes, ES6 or web platform, is already immutable in the first sense. Rather, this would be that you couldn't do Object.setPrototypeOf(Text, function(){}). This is relevant because if you do setPrototypeOf on an ES6 derived class, constructing that class subsequently will invoke the new prototype, rather than the original one, which differs from the behavior of web platform classes despite those classes otherwise seeming to be derived.

Making prototypes immutable in this sense wouldn't conflict with polyfills (except insofar as they couldn't replicate this immutability) in the current state of the world, but would mean that if we later wanted to refactor the class hierarchy to add another intermediate class between Text and EventTarget (or above EventTarget), polyfills would not be able to replicate this refactoring on browsers which had shipped the "immutable prototype" change but not the "new intermediate class" change.

littledan commented 5 years ago

There are many differences between ES6 classes+proposed new features, and what's present in the web platform. You can see some discussion about the way classes different in https://github.com/heycam/webidl/issues/226 , but when it comes to this issue, I see two main issues:

In the constructor path, ES6 classes dynamically look up the super class for super() calls, whereas web specifications don't make use of that dynamic lookup (often not explicitly calling out to the static superclass constructor, even). Nevertheless, these subclasses are able to use internal slots (or "associated" values) and get brand checks in WebIDL.
The use of separate "brands" per class execution (and, as a result, per realm), as opposed to a common one shared across realms for web platform objects. See recent discussion at https://github.com/tc39/proposal-class-fields/issues/60 . I think we could work out a solution to this with some kind of next-level decorators, but there's still details to work out. @domenic has been experimenting with web specifications using per-realm brands.

The idea has been raised various times of using private symbols rather than WeakMaps to drive the semantics of private fields and methods. Imagine we made the following designating (let's ignore the concerns about strong encapsulation when examining this issue):

class C extends D {
  #x;
  #m() {}
}
// ===>
const x = PrivateSymbol(), m = PrivateSymbol();
class C extends D {
  [x];
  [m]() {}

Such a class declaration would have similar extra observable dynamic behavior from manipulating prototype chains to the WeakMap semantics.

There was previously an idea for both public and private fields to use a fixed shape, based on the class definition evaluation-time inheritance chain. However, this would lead to its own weird mismatches, requiring multiple prototype chain walks on initialization and expensive new TDZ checks in hazard cases; see https://github.com/tc39/proposal-private-fields/issues/17 .

I think we'd be better off rectifying this either with a way to freeze a prototype chain without freezing the objects, or a syntax to add the fields and methods to an object without it being the super() return value (e.g., a class.initialize pseudo-function?). I don't see how we could solve this issue by changing the data model.

Right now, the web platform lacks any JavaScript-level mixins, in the sense of @justinfagnani's mixins proposal. If we do decide to add them in the future (as @kenchris suggested, I think the class-based super() chain, coupled with how private fields and method brands are added to the return value of super(), could provide a useful guide for the semantics. (Private method brand checks would be used in place of WebIDL "implements" checks for these mixins.)

kaizhu256 commented 5 years ago

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

prototype monkey-patching is routinely done internally by nodejs to coerce Uint8Array <-> Buffer

// https://github.com/nodejs/node/blob/v11.4.0/lib/buffer.js#L246
Object.setPrototypeOf(Buffer, Uint8Array);

// https://github.com/nodejs/node/blob/v11.4.0/lib/buffer.js#L308
Object.setPrototypeOf(SlowBuffer.prototype, Uint8Array.prototype);
Object.setPrototypeOf(SlowBuffer, Uint8Array);

// older nodejs v6 codebase coercing Uint8Array -> Buffer
// https://github.com/nodejs/node/blob/v6.2.1/lib/buffer.js#L246
function createBuffer(size, noZeroFill) {
  flags[kNoZeroFill] = noZeroFill ? 1 : 0;
  try {
    const ui8 = new Uint8Array(size);
    Object.setPrototypeOf(ui8, Buffer.prototype);
    return ui8;
  } finally {
    flags[kNoZeroFill] = 0;
  }
}

which makes them ineligible for private-field brand-checks:

// https://github.com/nodejs/node/blob/v11.4.0/lib/buffer.js#L246
Buffer.isBuffer = function isBuffer(b) {
  return b instanceof Buffer;
};

making me believe this was ok behavior, which i then employed in this real-world example

rdking commented 5 years ago

@littledan I've been reworking proposal-class-members. The version I'm working on contains a potential solution to the problem. A private initializer function gets created from the class definition and attached to the prototype. These changes use Symbol.private as an implementation detail to completely hide the such properties from user-land code.

The point is that with this, at least with class-members, classes meeting the needs described here can be built by somehow calling the private initializers attached to the prototype chain. I'm thinking something like Reflect.init(target_class, instance_object). Something like this function could be created and even made to work class-fields.

rdking commented 5 years ago

An implementation for Reflect.init() might look like this under class-members:

Reflect.init(target, receiver) {
  assert(typeof(target) == "function");
  assert(["function", "object"].includes(typeof(receiver)));

  //Initialize receiver against all classes in the chain.
  while (target) {
    let proto = target.prototype;
    if (proto && (typeof(proto) == "object") &&(Symbol.PrivateInitializer in proto)) {
      let init = proto[Symbol.PrivateInitializer];
      if (typeof(init) == "function") {
        init.call(receiver);
      }
    }
    target = Object.getPrototypeOf(target);
  }
}

Of course, this couldn't be done in user-land ES code since Symbol.PrivateInitializer must not be exposed due to restrictions on private symbols.

littledan commented 5 years ago

@kaizhu256 I'm not suggesting that we prohibit those mutations, but rather the mutations of the prototype of the constructors of built-in classes.

And, yes, that's right, in this case your class would not get the private fields or methods of that other class. I think this is working-as-intended behavior; if a class wants to support this special "become"-type behavior, it can do so as HTMLElement does by choosing to write the super return trick into itself.

domenic commented 5 years ago

There are many differences between ES6 classes+proposed new features, and what's present in the web platform. You can see some discussion about the way classes different in heycam/webidl#226 , but when it comes to this issue, I see two main issues:

I think I made a mistake mentioning web platform classes in the OP. Let's take them out of the picture entirely. You can rephrase all of my concerns, as well as your two main issues, by comparing ES6 classes+proposed features with classes defined in the ES262 (and ES402?) standards.

littledan commented 5 years ago

@domenic Maybe you can be more specific. Is there an example of a subclass that does the equivalent of adding a private field that I could look into more? I was mostly thinking of having a strong analogy to private fields/internal slots defined in base classes, which I think this proposal has.

littledan commented 5 years ago

What's in common about the examples that started this thread is that neither the TypedArray constructors nor platform classes call their super constructor. I'm suggesting that we support this either through better support for immutable constructor chains, or explicit field/private method initialization on any object, or both, so that JS classes can use this sort of behavior too. What do you think of these solutions?

jridgewell commented 5 years ago

I'm suggesting that we support this either through better support for immutable constructor chains

Are you suggesting that we would change these constructors to call their super, given that it would be immutable?

or explicit field/private method initialization on any object

I do like this.

Is there actually a problem with allowing Text's data slot to be installed on a foreign object?

bakkot commented 5 years ago

Are you suggesting that we would change these constructors to call there super, given that it would be immutable?

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.

jridgewell commented 5 years ago

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. "X is my super" is a nebulous concept when there's X itself is mutable.

bakkot commented 5 years ago

Would immutable supers be available to normal ES6 classes (say, either by default or by some opt-in keyword)?

See https://github.com/tc39/ecma262/issues/538, but I'd hope so.

it doesn't erase the attack vector, it pushes it earlier

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.

jridgewell commented 5 years ago

Not entirely sure what you mean by "attack vector" in the context of this thread

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 return foreign constructor in the super class and the subclass will treat it as a true instance.

That makes branding really just as "having one implies having all the others" check.

littledan commented 5 years ago

Is there actually a problem with allowing Text's data slot to be installed on a foreign object?

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.

littledan commented 5 years ago

FWIW I documented the "brand" guarantees of this proposal in this FAQ entry. The next step for me is to follow up with the class.instantiate proposal.

littledan commented 5 years ago

I wrote up a very early proposal draft for class.initialize, as described above.

devsnek commented 5 years ago

x-posting from irc:

With the original example that is roughly class X { #y; constructor() { const O = getObjectSomehow(); O.#y = 5; return O; } }.

i think the fact that getObjectSomehow happens to use the prototype is irrelevant. this seems more like a case for something like private symbols. O has no semantic relationship to the instance of X with a private #y that was created. This is why in the spec, we say ObjectCreate(P, [a bunch of slots]) instead of just ObjectCreate(P).

tc39 / proposal-class-fields

Branding guarantees do not match web platform classes, due to return override #179

for example: