-
Notifications
You must be signed in to change notification settings - Fork 113
Should private fields be per class execution, or based on source position? #60
Comments
It sounds like you’re suggesting a change that would allow me to write code whose behavior changed based on the existence or content of a private field in a different class, merely by extending it. If so, that violates “hard private” - the practical use case being “a change to, removal of, or addition of a private field should never be visible outside the |
Upon rereading, it sounds more like the suggestion involves source position (and that i was misunderstanding). This seems very weird to me - the template literal caching by source position, for example, is weird - and I’m not sure i understand the implications, but given that a common pattern in React is an “HOC” - a higher-order component - and that it would be important to ensure that each newly created class had a distinct identity (and couldn’t access the private fields of the other returns from the HOC), it seems like the current design is necessary for that. |
@ljharb With or without this change, you can develop code which depends on the existence of a private field. The code samples are valid in the current proposal. |
Also, with or without this change, higher-order classes/components will each have distinct identity. The question here is, should the private field identities also be different? Keep in mind that, even if they are the same, only code which textually sits inside the class will be able to read or write the private field. |
Yes; |
OK, thanks for the feedback about this use case, @ljharb . Does anyone have any other use cases that would benefit or be hurt by this level of privacy? |
@littledan FWIW, after we spoke the other day, I don’t think either implementation is harder in any meaningful way than the other. If week keep the identity of the private field tied to the dynamic instatiation of the class instead of the to the textual location inside the class, the main thing I think we should consider are the performance implications. Some thoughts on what it’d imply (at least in JSC, but probably similar in other engines as well):
|
@saambarati Interesting, those performance properties are more or less similar to what @verwaest and @bmeurer said would be the performance impact for V8. |
Yes, that is very similar to what we expect for V8. As discussed before, I'd vote for the probably less confusing path of having each |
I wouldn't find that less confusing, since there's no reified symbol value to work with. Private symbols aren't a thing; I think it would be more confusing to try to explain private fields as if they were private symbols (rather than the WeakMap desugaring, which I find quite easy to explain to people) |
While I don't have a use case which would work with one design but not the other, I would be very surprised by the change proposed here. |
On Sun, Nov 19, 2017 at 10:00 AM, Kevin Gibbons ***@***.***> wrote:
While I don't have a use case which would work with one design but not the
other,
function Nat(allegedNat) {
if (Number.isSafeInteger(allegedNat) && allegedNat >= 0) {
return allegedNat;
}
throw new Error('Natural number expected');
}
class Mint {
#Purse = void 0;
constructor() {
class Purse {
#balance = 0;
constructor(balance) { this.#balance = Nat(balance); }
balance() { return this.#balance; }
makePurse() { return new Purse(0); }
deposit(amount, src) {
Nat(this.#balance + Nat(amount));
src.balance = Nat(src.#balance - amount);
this.#balance += amount;
}
}
// constructor not available from instances
Purse.prototype.constructor = void 0;
this.#Purse = Purse;
}
makePurse(balance) { return new this.#Purse(balance); }
}
I would be *very* surprised by the change proposed here.
So would the author of the above code ;)
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAQtzCZolz8L2wSsuQQ06UHQW3t1OBFRks5s4GzRgaJpZM4QjA89>
.
--
Cheers,
--MarkM
|
When I wrote:
I was only speaking with respect to implementation concerns. FWIW, if I put my language design hat on, I think having the private field tied to the dynamic instatation of a class provides better semantics. |
The counter-claim, as @verwaest explained to me, was something like, if you have a class nested in something, you might have done that sort of by accident, and you'd expect them to be able to access each others' private fields. (Please correct me if that's not what was intended.) |
This; is actually something I have thought quite a bit about this over the years. It turns out there are use case pros and cons for both approaches.
First I want to propose some terminology for for the two alternatives.
Let’s call what is currently specified “lexical private”. It works like every other lexically scoped declaration or lexically captured reference in the language. Each evaluation of the source code declarative construct produces a new distint binding and exact reference binds in the current lexical environment. One of the arguments for this approach is its consistency with all other binding forms.
Let’s call the the other alternative position private. A evaluation of a private field/method declaration at a specific source code position share the same private access key. This is similar to how template objects are currently handled are will have similar issues.
MarkM gave a good example of a situation where position private would be undesirable, from a use case perspective.
Here is an example where lexical private would be undesirable from a use case perspective:
```js
const TimeStamped = S => class extends S {
//this is a mixin that timestamps the creation of its associated class
#created = new Date();
sameYear(aTimeStampedObj) {return this.#created.getFullYear() == aTimeStampedObj.#created.getFullYear()}
};
class My1stClass extends TimeStamped(Object) {
sameOperaton(obj) {
//obj must be a timestamped object
If (this.sameYear(obj)) DoSomething(this, obj);
else DoSomethingElse(this, obj}
}
}
(new My1stClass).someOperation(new My2ndClass); // fails if #create is lexical private
```
Position private also seems to be useful if we ever added private fields to object literals:
```js
function makePoint(x,y) {
return {
#x: x,
#y: y,
add(pt) {return makePoint(this.#x+pt.#x,this.#y+pt.#y)}
}
}
```
While there are use cases for both, I've always came to the conclusion that lexical private is the semantics that is most consistent with the rest of the language. The implement argument with respect to current implementation does carry much weight with me. As a former language implementor I know that implementation can and will improve one time is there are observed bottlenecks. We should focus more on what we believe is the most consistent design for the long term consistency of the language.
Finally, past discussion have identified an alternative way we could support position private use cases. The lexically scope private key declaration:
```js
private #created;
const TimeStamped = S => class extends S {
//this is a mixin that timestamps the creation of its associated class
#created = new Date();
sameYear(aTimeStampedObj) {return this.#created.getFullYear() == aTimeStampedObj.#created.getFullYear()}
};
...
```
|
Based on the above feedback, as well as discussion around the November 2017 TC39 meeting, it seems to me like we should stay with the "per class execution" semantics. |
So far I've only had the need for position privates in my projects. I've been using my own class lib in my projects. I've been making class-factory mixins using it, and I've ran into issues where classes from different mixins unintuitively don't work with each other. It's happened too often. When working with Custom Elements, for example, an author of a re-usable component or framework may want to let the base class be chosen by end users: // author:
const AwesomeMixin = (Base = HTMLElement) => {
return class Awesome extends Base {
// has private fields, but does not know which base class the end user will want to use.
}
}
// user:
class AwesomeButton extends AwesomeMixin(HTMLButtonElement) { ... } ^ The author expects the privates to work with all I am hypothesizing that position-private use cases like my example will be much more common than use cases like Mark's above lexical-private Purse example. In those lexical-private cases someone can easily create a |
After using I am right now implementing a simple "class branding" mechanism (I don't know if it's the same as those "branch check" phrases I've seen thrown around) that will solve my problem: to use it I will define a import Mixin from 'lowclass/Mixin'
import Class from 'lowclass'
// can't use Symbol because they aren't allowed as WeakMap keys. :(
// const FooBrand = Symbol('FooBrand')
// so we use a dummy object as a key
const FooBrand = { brand: 'Foo' }
export default Mixin(Base => {
return Class('Foo').extends(Base, {
// ... define private props and methods ...
}, FooBrand) // passing the key turns on "position private"
}) Note, if we make the symbol in a sub-scope, then it is "position private" only inside that scope, or perhaps "scope private". The nice thing this is that without passing a class brand Symbol, it falls back to "lexical private" It bothers me that the official language feature will have this same problem that I keep encountering repeatedly. I am betting that many people will be frustrated to shoot themselves in the foot with this once they use native privates fields in the wild for long enough. As much as I'd like to have a native private (and protected) feature, I'll be happier using my own implementation because it is more flexible (f.e. I will be able to easily choose between "position" or "lexical" privates) and won't have a bunch of the other issues. |
Unfortunately Symbols can't be used as WeakMap keys, I had to change the example above from doing const FooBrand = Symbol('FooBrand') to const FooBrand = { brand: 'Foo' } // used only as a WeakMap key internally |
…ate/protected access. This let's us choose to have "positional privacy" as described in tc39/proposal-class-fields#60
* feature/class-branding: add tests for class branding allow access to work across instances of the same-brand class Class() can now accept three args (added the brand) Add a class branding mechanism for specifying lexical scoping of private/protected access. This let's us choose to have "positional privacy" as described in tc39/proposal-class-fields#60
@rdking Hey, thought you might be interested, I added method for specifying "positional privacy" and "lexical privacy" in classes made with lowclass. Here's the two test files showing how it works: class-branding.test.js (shows the mixin class problem above solved with positional privacy) and friends.test.js (shows "friends", AKA "package protected" or "module protected"). Really, it's more of an "access scope sharing" feature (for lack of a name), where "positional privacy" and "lexical privacy" are optional subsets of what the user can achieve, other things being friend classes, etc, depending on how they use the "class brand" key. (Is "class brand" the right name to use? Is there a better name for that?) |
If you're talking about a unique value used to identify instances of a class, I tend to call that a "class signature".
Until now, I've been trying to understand what you've meant by "positional privacy". I get it now. You're saying: regardless of the inheritance chain, the set of fields associated with a given class signature is both unique and self consistent. That means regardless of the inheritance chain, a declaration of a class is itself a singleton so that re-declaration doesn't change the signature/field pairing for any of the actual composited classes. That's a curious thing to do. Now I want to know what the scenario was that you keep running into that makes you want this. In 35 years of programming, I've never run into a need for something like that. |
Simply put, the scenario is usage of mixin classes, as in the above examples. I'm taking advantage of mixin classes (producing multiples of the same class, with differing base classes). That's the only time I've ran into this problem so far. I think mixin classes are a common case compared to more obscure cases like the one @erights depicted above. Especially with Custom Elements, where a lib author tends to let the user decide which |
That's the part I don't get. Why are you trying to produce a batch of classes with the same name and same subclass structure but differing ancestry? The normal procedure would be to invert what you're doing. What is the benefit to inverting the inheritance? |
@rdking Have you tried them yet? They are useful! Here's an article by @justinfagnani on them. (Justin, lexical privacy as defined above may cause problems if your mixin classes use the new private fields.) Here's an example, a web component author makes a lib for web app developers. The lib author makes a conveniently-named base class for baked-in custom element functionality which includes template rendering, Shadow DOM, reactive state management, etc: let counter = 0
// The default element base class of this mixin is HTMLElement
const WebComponent = (Base = HTMLElement) => {
return class WebComponent extends Base {
// The class has private fields, and does not know which base class the end user will want to use.
#foo = ++counter
doSomethingWithOther(other) {
other.#foo // doesn't work!
}
// ...
}
} But the user wants to make the new custom element inherit from class NeuButton extends WebComponent(HTMLButtonElement) {
// ...
render() {
return <img src={this.icon} /><span>{this.label}</span>
}
}
customElements.define('neu-button', NeuButton) Then the user can use it in the app as HTML. Depending on the underlying implementation of custom elements: <form>
<!-- ... -->
<neu-button type="submit" label="Submit" icon="/path/to/submit-icon.svg"></neu-button>
</form> or <form>
<!-- ... -->
<button is="neu-button" type="submit" label="Submit" icon="/path/to/submit-icon.svg"></button>
</form> Now here's the problem:
let counter = 0
let privates = new WeakMap
const _ = instance => {
let priv = privates.get(instance)
if (!priv) privates.set(instance, priv = {})
return priv
}
// The default element base class of this mixin is HTMLElement
const WebComponent = (Base = HTMLElement) => {
return class WebComponent extends Base {
constructor() {
super()
// The class has private fields, and does not know which base class the end user will want to use.
_(this).foo = ++counter
}
doSomethingWithOther(other) {
_(other).foo // now it works!
}
// ...
}
} Now do you see the usefulness of mixins in at least one scenario, and the "lexical privacy" problem that comes with private fields, Ranando? |
I wish I had seen this issue earlier, so thanks for tagging me @trusktr, but I don't think the situation is actually that bad. My experience from my days on Dart, which has private fields, implicit interfaces, and mixins, is that the real problem here is with In the cases where you do need to access private state on instances other than |
@justinfagnani @trusktr
....but only next to impossible. The trick is to not redeclare WebComponent. let counter = 0
// The default element base class of this mixin is HTMLElement
const WebComponent = (() => {
class Base { constructor(userBase, ...args) { return new userBase(...args); } }
class WebComponent extends Base {
// The class has private fields, and does not know which base class the end user will want to use.
#foo = ++counter
constructor(userBase, ...args) {
//`this` is going to be an instance of userBase
super(userBase, ...args);
}
doSomethingWithOther(other) {
other.#foo // doesn't work!
}
// ...
}
function getHandler(proto) {
return {
proto,
getPrototypeOf(target) { return this.proto; },
setPrototypeOf(target, p) { this.proto = p; }
};
}
return (userBase = HTMLElement, ...args) => {
const proto = new Proxy(WebComponent.prototype, getHandler(userBase.prototype));
return class extends WebComponent {
constructor() {
super(userBase, ...args);
this.prototype = proto;
}
};
};
})(); It's a little convoluted, but it should do the job. WebComponent, done this way, extends a stable base who's instance type is always userBase, and always has the appropriate private fields. Edit: Corrected issue with absence of dynamic prototypes. |
@rdking The code inside of
@justinfagnani (and @rdking because you agreed) Why not, if the language enforces it? Can you depict how it may not be? What I mean is, obviously we can type check at runtime if we wish, using With a If we really are using WeakMap semantics to define native private fields, then in my opinion the implementation of the
Mixins help make code more re-usable and less coupled. The class-factory mixin paradigm is very useful, and I wish private fields would work with this paradigm.
Not sure what you mean. There's valid reasons to extend from a mixin: f.e. one library author might be making a set of custom elements that all inherit from another lib author's mixin applied onto a base class.
As Justin mentioned and everyone knows, we can use WeakMap to achieve what we want, which is what I'm currently doing, but I was hoping I could've used the language feature instead of tossing it aside for my own implementation. I want to have consistency: I don't want to use the builtin private feature, then have to switch to WeakMaps in cases where the builtin privates don't work. This is why I'll stick to my own private/protected implementation, at least until the feature gets changed.
Not sure what you mean, as
If userBase is But in a non Custom Element case, where Emulating what you've got in your example: |
True, but The only way to get around this is to stabilize your definition of WebComponent so it doesn't get re-defined each time a different class is inherited from. Note: The original example I gave above has a flaw. It doesn't include a proper prototype chain. I'll fix it momentarily. |
Yep, and if it is not
In my example yes, but not in my actual Mixin implementation. (Thanks @justinfagnani for some inspiration there)
It can be spec'd how we want: we can have it not throw and return Plus, type systems (like TypeScript) already enforce type requirements on Honestly saying, having a
With I suppose if it is abstracted away in a |
@rdking Wait a second, if |
@trusktr That's the magic. With a proxy to lord over the prototype chain, we can return what we want to return. Assuming a bit of memoization on top of my edited example above, for any given base class, we'll have an identifiable fixed prototype chain. 2 different base classes will still result in 2 different prototype chains (unless we get really fancy with memoization, but despite that, the name of the private field will always be the same for all variations of WebComponent. |
I thought about a Proxy, keeping track of each instance's base class/proto, so that we thus know which prototype to perform a lookup on when we reach the WebComponent proto, instead of having a single linked prototype. But why should we have to go through that sort of complexity? Positional privacy is obviously the better default here, and I'm sad that the decision to not change it was one based on "because it is easier not to modify the existing spec". That is so sad. @justinfagnani Why not leave We have a feature (dynamic |
@trusktr While I get where you're coming from, I just can't agree. The consequences of the internal changes required to implement positional privacy would break existing code in a myriad of ways. It would basically mean that any class can only ever be created once, regardless of scope. In fact, if you're certain this is something you want, try writing a proposal for it. Then try to write a pseudo-spec that doesn't interfere with anything else in the language. I think you'll find it more difficult than you anticipate. |
… for these classes (for info on "positional privacy" see tc39/proposal-class-fields#60)
Which code, exactly? The proposal is at stage 3. Anyone using such code should be wary of breaking changes (f.e. remember what happened to decorators and
Or rather, the static definition of the class is unique (a mixin is still creating new prototypes containing new props and functions). Anyways, someone who needs the lexical privacy can use a WeakMap inside their scope, but likewise someone who needs positional privacy can use a WeakMap in an outer scope. I'm putting my money on positional privacy being more often and naturally the thing people expect, but who knows, we'll just have to see! I like an idea more similar to Allen's (@allenwb) where we can have the power to choose which type of privacy we want: private #created; // similar to "positional privacy"
const TimeStamped = S => class extends S {
#created = new Date();
sameYear(aTimeStampedObj) {return this.#created.getFullYear() == aTimeStampedObj.#created.getFullYear()}
}; vs const TimeStamped = S => {
private #created; // similar to "lexical privacy"
return class extends S {
#created = new Date();
sameYear(aTimeStampedObj) {return this.#created.getFullYear() == aTimeStampedObj.#created.getFullYear()}
}
}; ^ That's valuable! And it leads towards privacy with object literals, and thus makes the feature more inline with the existing pre-ES6 language (plus WeakMaps). |
@allenwb I really like an idea like your Here's what it could look like with // Engine.js
// a circular dependency required for this privacy sharing to work
import {getNodeProtectedKeys} from './Node'
const nodeKeys = getNodePrivateKeys()
export default new (class Engine {
renderNode(node) {
node.#[nodeKeys.render]() // call the private #render method on the node.
// ^ or some other syntax
}
}) // Node.js
// a circular dependency required for this privacy sharing to work
import Engine from './Engine'
// important, these need to be `var`s
var nodeKeys
var nodeKeysAccessCount
var maxAllowedNodeKeysAccessCount
// this is hoisted above all modules
export function getNodePrivateKeys() {
maxAllowedNodeKeysAccessCount = maxAllowedNodeKeysAccessCount || 1
nodeKeysAccessCount = (nodeKeysAccessCount || 0) + 1
if (nodeKeysAccessCount > maxAllowedNodeKeysAccessCount)
throw new Error("Unauthorized attempt to access nodeKeys")
return nodeKeys || nodeKeys = {
render: #render
}
}
private #render // assume it is hoisted to the top of the current scope (module scope)
export default class Node extends Transformable {
constructor() {
if (nodeKeysAccessCount > maxAllowedNodeKeysAccessCount)
throw new Error("Unauthorized attempt to access nodeKeys")
super()
}
#render() {
// ... update graphics related stuff ...
}
set position(value) {
// ...
Engine.renderNode(this)
}
} // valid application code:
import Node from 'some-lib/Node'
const n = new Node
n.position = [30, 40, 50] // invalid application code, breaks because of the error:
import Node, {getNodePrivateKeys} from 'some-lib/Node'
const nodeKeys = getNodePrivateKeys() // Error: Unauthorized attempt to access nodeKeys
// if they catch or defer the error, they still can't use the lib:
const n = new Node // Error: Unauthorized attempt to access nodeKeys
n.position = [30, 40, 50] It'd be super sweet if new features in JS are radically better than existing languages, rather than just similar. |
I'm using the above "gateway" pattern in practice, to allow friend modules to access protected members of instances of a class defined in another module, here for example: https://github.com/trusktr/infamous/blob/master/src/core/Motor.js#L6 |
Last thing: @allenwb's privacy keys references idea (or however they'd be called) plus ability to pass them around allow for implementing "package private" with "gateways", meaning a private member from one class can be shared with subclasses to make them similar to "protected", while still preventing unauthorized code from extending the classes to gain access to those members. |
Pretty much every piece of code out there that contains a class-generating function. Your scenario is exactly that. By changing the semantics such that the WebComponent class's definition is fixed regardless of how many times the containing function is called and the base changed, every class-generating function would be impacted, and the coded expectation that each iteration of the function produces a new class instance would be broken. Several of my code libraries would die in that fire.
Suppose we use class-fields syntax. I can rewrite your code like this and completely remove the need for "friend" semantics. What's more is that the error case you drew up is no longer valid or necessary, the circular imports are gone, the dependency between the node and the engine has been broken making the code more flexible, the node will only operate on the engine that created it (though that can be adjusted with more code), and the engine has the privilege of potentially knowing every node ot will work with without having to give up any of it's internal functionality. // Engine.js
export default new (class Engine {
#signature = Symbol("Engine Signature");
createNode(clazz, ...args) {
return new clazz(this, signature, ...args);
}
renderNode(node) {
node.render(this.#signature);
}
}) export default class Node extends Transformable {
#engine;
#engineSig;
constructor(engine, sig) {
this.#engine = engine;
this.#engineSig = sig;
}
#render() {
// ... update graphics related stuff ...
}
render(sig) {
if (sig === this.#engineSig)
this.#render();
}
set position(value) {
// ...
this.#renderNode(this)
}
} // valid application code:
import Engine from 'some-lib/Engine'
import Node from 'some-lib/Node'
let engine = new Engine;
const n = engine.createNode(Node);
n.position = [30, 40, 50] |
@justinfagnani I know it's too late to change anything (or is it?), that's only better because we have no choice with the current design. In my opinion it would have been better to allow regular people to write better-organized code more easily (f.e. private fields along with mixins), letting someone wanting to write complex less-practical code with particular guarantees (like @erights showed above) use WeakMaps to achieve whatever they want. This sort of design choice would make the language more practical for average use cases.
@rdking I wasn't trying to do that. I was trying to decouple my hierarchies and make them composable via multiple-inheritance (using mixin factories) so as to have better code organization. The fact that new classes are being created in the mixin functions is only a side-effect of the higher-level idea. It isn't my goal to simply create multiple of (practically) the same class.
That is less practical for what average applications need when they want to use patterns like mixins for code organization and especially code re-use. From a security standpoint, someone writing any code like @erights's example can use WeakMaps, and those people would be a minority. Why not satisfy the majority of JavaScript (web) developers, and let security experts use WeakMaps when they need to? Please, anyone who is an official TC39 language designer, please consider what most people need and not restrict new language features based on rare forms of programming that can already be satisfied with WeakMaps. |
Seriously, why does it have to be "either/or" for this? There's absolutely no reason why @erights can't have the feature complexity he needs while everyone else gets the flexibility they want. The problem is that the proposal is broken. PERIOD. If it breaks existing usage paradigms unintentionally, then it should not be considered a good proposal. Out of utter dismay for the limited understanding shown in the creation of this proposal, I wrote this: ClassicJS. It is essentially a proof of concept on how ES is capable of supporting nearly every feature of a Java class without sacrificing anything in the way of security or functionality. I invite all members of TC39 to have a look and see if this approach satisfies whatever technical desires (specified or not) you had when agreeing to the unfortunate present proposal. There's obviously a few limitations to this approach. To that end, please recognize that if it's about the syntax or speed, there's little I can do about that since Proxy is cripplingly slow while being the best way to emulate what the engine needs to do, and there's absolutely nothing I can do about the syntax without making or modifying something like Babel. For those of you that think like @ljharb and don't want the mere existence of "protected" support, I can only say "I disagree". I suggest you test that as well and see if your concerns are actually well founded. I'm interested in hearing anything any of you have to say about it. |
The current private fields specification would create multiple different field names
#f
in a case like thisFeedback from several engineers in JSC (e.g., @saambarati) and V8 (e.g., @verwaest) has been that the implementation of the current design
willcould besignificantlymore complex to implement and possibly be slower while the JIT is warming up (read: longer startup time) than if each textual#f
had just one private name for it. (Spec text in progress for what "textual" means there: tc39/ecma262#890 . Still some issues to work out.)My question here: What do we think of the motivation for private fields being this amount of private, as they currently are in this proposal? The motivation, as I understand it, doesn't come from brand checking applications as I listed above (which you could also solve, with possibly worse or possibly better performance, using a WeakSet), but rather:
Here, the purpose of the class is to take the given function and wrap it in something which will only let it be called in certain ways. If we let different return values of SafeWrapper be "friends" with each other just for using the same syntax, they'll be able to call each others' unguarded underlying function, which would be inappropriate.
Where I'm stuck is, I can't think of any practical use cases where this particular notion of defensibility is important. Does anyone have an example here where it's come up, either in JS or other programming languages?
cc @allenwb @erights @dtribble
The text was updated successfully, but these errors were encountered: