tc39 / proposal-private-declarations

A proposal to allow trusted code _outside_ of the class lexical scope to access private state
https://tc39.es/proposal-private-declarations/
MIT License
25 stars 4 forks source link

What is `typeof private #x`? #4

Open Igmat opened 5 years ago

Igmat commented 5 years ago

Currently it didn't mentioned in spec, but it seems to be very important.

private #x declares a private name. But what is private name in terms of JavaScript. Is typeof #x some kind of primitive value? Is it private symbol? How does it relate to key reification and Membrane pattern?

ljharb commented 5 years ago

Since #x isn’t a first class value, you wouldn’t be able to get the typeof it.

Igmat commented 5 years ago

So,

private #x;

console.log(#x); // we'll have Syntax Error here?

How will we share #x among different modules in this case?

ljharb commented 5 years ago

yes, that would be a syntax error.

you wouldn't share it, just like you can't share a variable - you'd probably share something like o => o.#x instead.

Igmat commented 5 years ago

So it's impossible to create friend classes in different files, isn't it?

ljharb commented 5 years ago

I mean, I think it's impossible to reliably restrict access in JS between constructs (preventing everything else from accessing it) using any means other than lexical scoping, or, relying on being first-run code (so you can do all your public sharing first, and then lock it down afterwards before other code is ran).

jridgewell commented 5 years ago

All of @ljharb's responses are correct.

One of the direction's well explore is reification of the private state into some form of value, maybe a Symbol value. This was requested by @keithamus in order to gain his approval. It's also an interesting idea, because the [#x] = 1; declaration and this[#x] access syntax would make sense without an awkward outer keyword.

So it's impossible to create friend classes in different files, isn't it?

This proposal is not tackling sharing state across modules, that will be another another proposal later on.

Igmat commented 5 years ago

This proposal is not tackling sharing state across modules, that will be another another proposal later on.

Ok, got it.

Still it isn't clear how it relates to brand-checks. What will happen in following sample?

private #id;
const setId = (obj, id) => {
  obj[#id] = id;
};
const obj = {};
setId(obj, 1);
console.log(obj[#id]);
jridgewell commented 5 years ago

Still it isn't clear how it relates to brand-checks. What will happen in following sample?

It will throw during the set. The committee decided explicit initialization is required, and for now you can’t initialize on an object without the super return override trick.

Igmat commented 5 years ago

So I'll need class for that?

jridgewell commented 5 years ago

Yes. Or you can manually reify the private name into a map that uses the super return override trick.

bmeck commented 5 years ago

Another possibility to share things between modules is to adopt something like decorators are doing and exporting and importing via different tokens than identifiers:

import {#foo} from "friend";
export {#bar} for "friend";
rdking commented 5 years ago

The committee decided explicit initialization is required, and for now you can’t initialize on an object without the super return override trick.

Wait, so this wouldn't work?

private #id;
const setId = (obj, id) => {
  obj[#id] = id;
};
const obj = {
  [#id]: undefined
};
setId(obj, 1);
console.log(obj[#id]);
ljharb commented 5 years ago

Nope.

rdking commented 5 years ago

Wow... That drastically decreases the usability of this proposal.

I can understand why explicit initialization is needed, but according to the ES spec, providing an initialization value for an object property in a lexical definition is explicit initialization of that property. So why wouldn't the above be allowed?

bmeck commented 5 years ago

@rdking thats not true of private fields as they exist either:

class X {
  #prop
  static add(o) {
    o.#prop = 1;
  }
}
X.add({}); // fails, #prop is not on {}, isn't initialized
rdking commented 5 years ago

@bmeck Your example is a bit different from mine. Where you're trying to add #prop to o after o's construction, I'm trying to add a private property to an object in its lexical definition, which is effectively during its construction.

bmeck commented 5 years ago

@rdking ah, yes I misunderstood after seeing the setId. If the initialization is in an PropertyDefinitionList and goes through PropertyDefinitionEvaluation it should be ok with the committee under previous discussions.

bakkot commented 5 years ago

@rdking Yeah, I think @ljharb may have misread your example. As written I'd expect that to work.

rdking commented 5 years ago

Ok... Given that alone, I like this proposal. Can a private declaration of this kind be used as a RHS expression?

bmeck commented 5 years ago

@rdking can you clarify "of this kind" / maybe have a minimal example

rdking commented 5 years ago
private #x;
let o = { [#x]: void 0 };

function set(obj, name) {
   //What will happen here?
   obj[name] = 42;
}

set(o, #x); //Is this valid?
bmeck commented 5 years ago

@rdking

You couldn't have set(o, #x) since #x isn't a first class value to pass around (this is a syntax error); due to the same lack of being first class name could not be #x so obj[name] couldn't refer to obj.#x. You would likely have to do something like:

private #x;
let setX = (o, v) => o.#x = v;

let o = { [#x]: void 0 };

function set(obj, setter) {
   setter(obj, 42)
}

set(o, setX);

You can still pass a way to set the value around, but passing around the actual private declaration doesn't work as it isn't a value.

rdking commented 5 years ago

Ok, so never valid as a RHS term. Meaning it is also only valid within the execution context where it was declared.

ljharb commented 5 years ago

oh, my mistake, i was thinking this was a different repo. with this proposal that would work! My apologies.

shannon commented 5 years ago

In my case, I was really hoping something like this would be valid:

//visit.js
private #visited;
export function visit(obj) {
   obj[#visited] = true;
}
//main.js
import { visit } from './visit.js';

const obj = {};
visit(obj);

To provide the ability to tag or attach private data to foreign objects without having to use symbols (that are not private) or weakmaps/weaksets that have a very cumbersome and less readable API.

Is the explicit initialization requirement a limitation of implementation or a conceptual one?

bmeck commented 5 years ago

@shannon It is due to the potential for errors being seen as problematic / telling if something was meant to have the field. Explicit initialization for your example could be done via super override, or someone could propose an alternative syntax instead of requiring super override. I do not think arbitrary assignment without initialization is expected in the near future.

shannon commented 5 years ago

@bmeck I guess I don't really understand the problem then. I explicitly set the field on that object in a clear manner. Why is that considered problematic compared to initializing it in a class declaration or the super override trick?

In my example, nothing should have that field except objects that have passed through visit. If they didn't pass through visit then they don't have it. It seems pretty simple and not problematic to me.

bmeck commented 5 years ago

@shannon see https://github.com/tc39/proposal-class-fields/blob/master/PRIVATE_SYNTAX_FAQ.md#how-does-this-proposal-relate-to-brand-checks for some previous discussion on the topic.

shannon commented 5 years ago

@bmeck Sorry I've seen this many times before but that pertains to the private-class-fields proposal.

It states the following:

if a private field declared in a particular class exists on an instance, then that class's constructor has run against that instance

I guess my hope was that this proposal (proposal-private-declarations) would allow for a more generic way of defining private properties on all objects. Not just a different way of doing the same thing as private-class-fields.

I was hoping that the above quote would become the following for my example:

if a private field exists on an instance, then that function has run against that instance

bmeck commented 5 years ago

@shannon because of accidental assignment being a means to make a brand check "fail". o.#x = foo would essentially make o join the set of objects branded by #x. This is seen as a problem since #x might be used to mark objects as having some particular privilege such as being trusted. If that is the case, then mutating o.#x; could accidentally mark o as trusted, which we don't want to allow naively. There are a variety of cases discussed in https://github.com/rwaldron/tc39-notes if you search around for "brand check" and "private". While workarounds exist, just like converting symbols to strings, this is just setup for a conservative stance on default behavior given some usages that were expected.

shannon commented 5 years ago

@bmeck I strongly urge everyone in tc39 to reconsider trying so desperately to completely block developers from making obvious mistakes at the cost of usability. Accidentally assigning this visited property elsewhere in my example just wouldn't make any sense. It's not a mistake that would be made.

There are many arguments in the class fields proposal against including brand checking in the first place. If this proposal has the same requirements then it becomes significantly less useful, for me anyways.

bmeck commented 5 years ago

@shannon alternatives that do not have the same initialization problems are being considered such as https://github.com/littledan/proposal-new-initialize , however brand checking has remained a pertinent use case over the course over time and is unlikely to be removed as a use case especially since private fields have started shipping.

shannon commented 5 years ago

@bmeck This proposal hasn't started shipping. So why must we limit it in the same way?

Developers can always use class fields if they need brand checks.

bmeck commented 5 years ago

@shannon the value of consistency is great, and the use cases of private fields are not mutually exclusive with other proposals such as this one.

rdking commented 5 years ago

@bmeck I get the branding concerns. However, can't those branding concerns be fielded by modules? The simple reality is that the existence of an IIFE around the private declaration and its uses completely satisfies everything TC39 is worried about. To misquote and further (possibly over-) extend a statement from @ljharb, anything not private is public. So if this name is not kept private (by closure), then it is public and subject to be used in any number of ways. It shouldn't be TC39's responsibility to say that absolutely no one might want to use a private name for something other than brand checking.

bmeck commented 5 years ago

@rdking the branding check is to ensure it remains private and isn't accidentally attached to things. I'm not sure I understand how it is leaking currently (in this discussion or private fields).

jridgewell commented 5 years ago

Wow, this thread was active.

Wow... That drastically decreases the usability of this proposal.

I've left private state in objects as a follow on. There's a clear path forward once we land this proposal.

Ok, so never valid as a RHS term. Meaning it is also only valid within the execution context where it was declared.

It's only valid in property key positions (both computed and regular).

I guess my hope was that this proposal (proposal-private-declarations) would allow for a more generic way of defining private properties on all objects.

Private properties have to be defined on the object during creation, they can't be added later. The committee has been consistent with this requirement. I even tried to change it, but that proposal was rejected.

This doesn't limit use, though, since it can be forced onto an already constructed object via super-return-override. It's just a little cumbersome.

Super Return Override and Private Declarations ```js private #prop; const o = {}; o.prop; // Error force(o); o.#prop; // => undefined function force(object) { class Base { constructor() { return object; } } return new Sub extends Base { [#prop] = undefined; constructor() { super(object); } }; } ```
shannon commented 5 years ago

The committee has been consistent with this requirement. I even tried to change it, but that proposal was rejected.

I thank you for trying @jridgewell. I know from the other proposal that you have at least heard our feedback.

Having said that, if the super return trick is acceptable, and adds private fields to objects, why can't we just add private fields via o[#x] = true. It doesn't seem like we are stopping anything. Just requiring more boilerplate.

The idea that this is somehow less error prone is a bit silly. It's not only more complicated code, it's wasteful.

bmeck commented 5 years ago

@shannon the brand check prevents accidental addition of the field to an unqualified target. Otherwise all branded assignments would need to do a check to ensure that the field exists on the target, of which we don't have a means to do since #x in o doesn't work (currently). Wrapping all assignments with that check isn't possible without adding more semantics to the model and requires more boilerplate to avoid accidents. We can argue on both sides, but the requirement is unlikely to change.

rdking commented 5 years ago

This doesn't limit use, though, since it can be forced onto an already constructed object via super-return-override. It's just a little cumbersome.

The only real limitation I can find in this proposal is the utter inability to share a private name across modules. Does this work?

let name = "question";
eval(`private #${name}`);
let o = { [#question] = "answer" };
jridgewell commented 5 years ago

I don't know what direct eval will do yet. If you intention is to simulate module boundaries, there's been talk of explicit exports:

// a.mjs
private #foo;

export { #foo } fo 'b.mjs';

// b.mjs
import { #foo } from 'a.mjs';

class Bar {
  [#foo] = 1
}

// c.mjs

import { #foo } from 'a.mjs; // Error!
ljharb commented 5 years ago

Note, though, that that direction has the same problems as static decorators and builtin modules, namely that there's no way for it to work in Scripts (there'd need to be a way to reflect on the non-normal exports of a module, too, but that's easily surmountable).

shannon commented 5 years ago

@bmeck I understand what you are saying but you are tying all private fields accesses to brand check in doing so. When in practice it's only the first that matters. In my example I would just add the following at the beginning of my method:

if(!obj[#visited]) throw Error();

I don't need to check any others. That's if I even care about brand checking. But my use cases go something like this:

if(!obj[#sprite]) { obj[#sprite] = await loadSprite(obj.sprite); }
doSomethingWithSprite(obj[#sprite]);

With weakmaps it looks like this

if(!spriteMap.get(obj)) { spriteMap.set(obj, await loadSprite(obj.sprite)); }
doSomethingWithSprite(spriteMap.get(obj));

It is currently impossible for me to do this any other way that isn't more resource intensive in some way. Using super override trick would only make it worse.

And I've tried other ways but in all cases the readability is garbage:

const sprite = spriteMap.get(obj) || spriteMap.set(obj, await loadSprite(obj.sprite)).get(obj);
doSomethingWithSprite(sprite);

Which works but, come on. So I end up with all these silly little ensureSpriteIsLoaded methods for all sorts of things. That gets cumbersome trying to pass around weakmaps or I have to add these methods to different modules so I get lots of duplicates of the same exact code with different variable names. It's now more readabile but a pain to maintain.

I also have no control over the class/prototype structure of these objects and I shouldn't have to care. I just care that it has a sprite field.

So again I understand what you are saying and I can see that there are some use cases that require brand checks on lots of accesses, but we've really maimed my use case by supporting this other one. At the same time there is already another proposal that will support this brand check functionality, So it's a bit frustrating that this one has to follow suit.