tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Private property that shadows public property #147

Closed shannon closed 5 years ago

shannon commented 5 years ago

I'm concerned about this proposal and, as others have expressed, how the TC39 have handled community feedback. So I've been thinking a lot about the problem and the various different proposals and syntaxes and I just can't help but think that the JS "engine" should be able to do this the way I expected private properties to work before I knew this proposal existed. Even if crudely at first, it should be able to be optimized later. But then I don't know if what I expect is really what other developers expected so I figured I would ask while the discussion is still hot.

Anyways, I'm just wondering, if hypothetically there were no technical limitations about property access and property access performance, would the below syntax and mental model be desirable to most developers.

It's described simply as you can declare a private property that shadows public properties when accessed from inside the class.

class MyClass {
    private x = 'private x';

    getPrivateX() {
        return this.x;
    }
}

const myClass = new MyClass();

assert(myClass.x === undefined);
assert(myClass.getPrivateX() === 'private x');

myClass.x = 'public x';

assert(myClass.x === 'public x');
assert(myClass.getPrivateX() === 'private x');

I understand that there may be some situations where you need to access the public property from inside the class but this would align with any variable scope shadowing and would generally be a known mental model. I also understand that you may want to access private properties of other objects of the same class (obj.x). So let's just ignore the technical limitation for a moment and assume that worked as expected (obj.x === 'private x' when inside the class). Would that mental model also be desirable?

shannon commented 5 years ago

@rdking and @hax I'm curious what your thoughts are on this based on your various discussions in other threads.

bakkot commented 5 years ago
class A {
  private x;
  method(obj) {
    return obj.x;
  }
}

There is no way for the engine to know if obj.x in the above code is intended by the programmer to access private field x of A instances or public field x of generic objects.

Edit:

That is to say,

So let's just ignore the technical limitation for a moment and assume that worked as expected (obj.x === 'private x' when inside the class).

It is not true that this is necessarily what is expected, especially in deeply nested classes. See the FAQ.

shannon commented 5 years ago

@bakkot again ignoring technical limitations, I can read that and make a decision it means they are referencing the private x. As long as it's defined that way. So I'm not sure how there would be absolutely no way for the engine to also decide this.

shannon commented 5 years ago

@bakkot sorry I kind of get what you are saying but I guess because it's shadowed I would expect it to check if it's an instance of A and if private x is defined. Otherwise just return public x of object.

jridgewell commented 5 years ago

So does every .x property reference mean "private" now? That's a huge break in the language. How do I access the public .x on a passed in object?

Now if every .x has to be decided on the object, it's easy to extract private state by passing in foreign objects instead of instances of the class. That breaks the whole point of hard encapsulation.

I'm concerned about this proposal and, as others have expressed, how the TC39 have handled community feedback.

We've discussed these ideas over and over again. That the issues in this repo seem to go nowhere is because the ideas aren't tenable. The best alternatives have been the private symbols proposal, and classes 1.1, and the committee still feels this is the best option.

bakkot commented 5 years ago

@shannon

@bakkot sorry I kind of get what you are saying but I guess because it's shadowed I would expect it to check if it's an instance of A and if private x is defined. Otherwise just return public x of object.

That means that x is now part of the class's public API, because someone outside of the class can pass in a non-instance with an x field to get the class to operate on it.

This is also in the FAQ.

shannon commented 5 years ago

@bakkot @jridgewell with all due respect this use case is far less common than you seem to be letting on. In this case if I was accepting objects as parameters I would use a brand check. I would expect that shadow to behave this way based on what I know of JS and I would work around the other constraints accordingly. A brand check is easy. If I'm accepting objects, that's part of my public API anyways. I would verify my input.

We've discussed these ideas over and over again. That the issues in this repo seem to go nowhere is because the ideas aren't tenable. The best alternatives have been the private symbols proposal, and classes 1.1, and the committee still feels this is the best option.

Tenable to who? As others have said all of the proposals have limitations and for some reason the TC39 committee has a list of hard requirements that they haven't shared with the rest of the world. And they have decided that this proposal is the only way forward despite all of it's own shortcomings. I would say this proposal is untenable, yet here we are.

hax commented 5 years ago

@shannon Unfortunately , as some requirements which TC39 already has consensus, like hard private, undetectable, no runtime-check for performance... private access can never be this.x.

@bakkot @jridgewell

Sigh... You already see, whenever you use syntax this.#x, there will be always a similar questions, because they want to eliminate # or want private keyword. Even your this._x to this.#x is a good way to educate, such question will never stopped...

That's why I think classes 1.1 is better, it just have no such pain: no #, have keyword. No questions.

shannon commented 5 years ago

@hax that's sort of my question. Ignoring those requirements. Would this be something that developers expect or want? Hard private and undetectable are debatable. But no runtime-check is not something I expected to be part of the language here. I do expect the engine to optimize this mostly away so I don't have to think about it. I just want this to behave like I expect. And it seems like there is not much that behaves the way I would expect. I do think the classes 1.1 is a better approach but I expected let and const, not var. And just function instead of hidden. At this point it's just another lexical scoped block with a special perceived instance level block.

hax commented 5 years ago

@shannon

The problem is you can't ignore these requirements...

Of coz we could always argue for undetectable and debatable. But I don't think it's possible to change the semantic requirements.

So the only part left is syntax. And classes 1.1 mostly about syntax and mental model.

Because the semantic requirements makes Java/C# like private style impossible, so in my opinion, we should choose a syntax/terminology keep far distance to Java/C#, so programmers won't always ask why not Java-style πŸ˜‚

About you specific syntax preference, var is chose because it just tell you there is a instance var, if you use let you need to check whether you are in class body or not, and const do not very useful (only mutable state make sense). I think function was discussed, don't remember why they choose hidden, but it's just bikeshed issue.

Igmat commented 5 years ago

@hax I've seen you pointed this in several threads:

Unfortunately , as some requirements which TC39 already has consensus, like hard private, undetectable, no runtime-check for performance... private access can never be this.x.

Generally it's wrong. We could use Symbol.private that provides hard private, undectable and no runtime-check + syntax sugar on top of it (described in #134) to get private x + this.x.

rdking commented 5 years ago

@shannon The truth is that there are no technical limitations within ES engines to prevent this syntax. However, most of the objection you're going to receive is due to the oddly skittish nature of developers expecting the language to protect them from making mistakes. I don't know what happened, but somewhere along the way developers have shifted from expecting their tools to enable possibilities to expecting their tools to limit possibilities. This is disheartening to me.

But back to the issue at hand, I can and have enabled almost exactly that syntax just by using a Proxy. Even inheritance continued to work as I expected. So there really is no technical limitation preventing the private x/this.x syntax. With that having been said, there are plenty of concerns:

Using that syntax:

Would it be something developers want? Therein lies the rub. Want the syntax? Yes! Want the extra considerations and limitations? No-one's really done a good public test to figure that out, but I would wager "probably not". The same kind of trade-off exists in all of the proposals. The problem is whether or not the particular trade is a fair and acceptable one. The reason the current proposal has received so much discussion even into the 3rd stage is because many of us find the trade-offs require to be too extreme. Would the same be true for this syntax? I don't know.

shannon commented 5 years ago

@hax const I get, and it could be added later with initializes. I don't quite follow this part though:

if you use let you need to check whether you are in class body or not

I think you would have the same issue with var. Yea it's mostly bikeshedding but I only say this because of how I expect it to look and work. And that means looking like other existing parts of JS.

The problem is you can't ignore these requirements...

I just don't know where these requirements came from. I'm really curious how many developers have asked for them and what types of use cases need them. It seems like every time a new proposal pops up it gets debated by the same two or three people in GitHub and then the committee goes and discusses it offline and then says it didn't meet the requirements.

I have my own "requirements":

  1. It should make my code easier to read
  2. It should make my code easier to write
  3. It should make my code easier to understand

So far this proposal has failed at all of these.

shannon commented 5 years ago

@rdking

However, most of the objection you're going to receive is due to the oddly skittish nature of developers expecting the language to protect them from making mistakes. I don't know what happened, but somewhere along the way developers have shifted from expecting their tools to enable possibilities to expecting their tools to limit possibilities. This is disheartening to me.

I agree with this statement. I don't know what happened either.

Would it be something developers want? Therein lies the rub. Want the syntax? Yes! Want the extra considerations and limitations? No-one's really done a good public test to figure that out, but I would wager "probably not". The same kind of trade-off exists in all of the proposals. The problem is whether or not the particular trade is a fair and acceptable one. The reason the current proposal has received so much discussion even into the 3rd stage is because many of us find the trade-offs require to be too extreme. Would the same be true for this syntax? I don't know.

Does the TC39 have a mechanism for doing this public test? I see the nodejs modules team doing something like a questionnaire for the minimum implementation of the ES modules in Node. It's not a vote but if you ask the right questions you can get good feedback on what the larger developer community actually wants. Instead of all this anecdotal conjecture. I would highly recommend that the TC39 do something similar here.

jridgewell commented 5 years ago

@shannon:

this use case is far less common than you seem to be letting on. In this case if I was accepting objects as parameters I would use a brand check.

Even if we ignore the surprising changes in semantics, I don't think you realize how many property accesses are in JS code. Imagine slowing down every one of them by even 1% just to add a runtime check if for "is this a private property?"

I do expect the engine to optimize this mostly away so I don't have to think about it.

That's a core issue, it's not possible to just "optimize" this away. It will affect every property access in all code, even where private state is not used at all.

I just want this to behave like I expect.

Coming from JS? I already expect .x to be the public property on the object.

Other languages (any of which you're thinking of) have a compile time type checker to enforce property access, so there's no ambiguity. That's not possible in JS because there is not type checker.

Tenable to who? As others have said all of the proposals have limitations and for some reason the TC39 committee has a list of hard requirements that they haven't shared with the rest of the world.

To the committee. We have already discussed runtime semantics like this, and it is unworkable.


@rdking:

The truth is that there are no technical limitations within ES engines to prevent this syntax.

This is half true. Yes, we could add runtime checks and do exactly as this issue suggest. But the performance difference is a technical limitation. The breaking of hard-encapsulation this allows is a technical limitation. Etc.

Igmat commented 5 years ago

@jridgewell, no there is an option to make private x and this.x work properly without breaking hard privacy and without adding any redundant runtime-checks, so no performance difference will appear.

shannon commented 5 years ago

@jridgewell

That's a core issue, it's not possible to just "optimize" this away. It will affect every property access in all code, even where private state is not used at all.

I don't think this is true. It would only apply to classes that have private fields and only when accessing a private field. It is pretty straightforward to manage this in a crude way with get/set and analyzing the stack. The engine can optimize more beyond that.

I don't see how this really breaks hard encapsulation. Mistakenly accessing a property of a foreign object is not about encapsulation as I understand it in JS terms.

shannon commented 5 years ago

Or as @Igmat has suggested with private symbols. It's not technically impossible to do. And this is something I would expect the engine to be able to do under the hood.

jridgewell commented 5 years ago

@Igmat:

private x and this.x work properly without breaking hard privacy and without adding any redundant runtime-checks, so no performance difference will appear.

The only solution I'm aware of is changing all .x property accesses in the scope of the private field to being private property accesses. That means there's no way to access the someone else's public property .x in the same scope.

This was also discussed when we talked about access semantics, and it was flatly rejected.


@shannon:

It would only apply to classes that have private fields and only when accessing a private field.

Unfortunately, it is true. There is no way to do runtime checking of the receiver (to decide if the property is public or private) without affect all public property accesses.

I don't see how this really breaks hard encapsulation. Mistakenly accessing a property of a foreign object is not about encapsulation as I understand it in JS terms.

See https://github.com/tc39/proposal-private-fields/issues/14#issuecomment-153050837, from the FAQ.

Or as @Igmat has suggested with private symbols. It's not technically impossible to do. And this is something I would expect the engine to be able to do under the hood.

Private symbols are a different proposal (and one I actually support) than this issue's proposal. Doing this[privateSymbol] is not ambiguous, because the privateSymbol binding carries the "this is always private" marker on it.

shannon commented 5 years ago

@jridgewell

The only solution I'm aware of is changing all .x property accesses in the scope of the private field to being private property accesses. That means there's no way to access the someone else's public property .x in the same scope.

This was also discussed when we talked about access semantics, and it was flatly rejected.

This could be done with an API similar to Reflect (or even just Reflect.get itself) if this uncommon use case was important enough. Not sure why it would be flatly rejected.

Unfortunately, it is true. There is no way to do runtime checking of the receiver (to decide if the property is public or private) without affect all public property accesses.

I'm sorry I just can't accept this as truth. There must be some way to branch the accessor code in a way that is equivalent to the this.#foo access in terms of performance.

rdking commented 5 years ago

@jridgewell What's with this scare tactic?

Even if we ignore the surprising changes in semantics, I don't think you realize how many property accesses are in JS code. Imagine slowing down every one of them by even 1% just to add a runtime check if for "is this a private property?"

Right now, while no such feature exists, we as developers are using things like Proxy, closures, WeakMaps, and other such things to achieve data privacy and incurring far, far greater time penalties in the process. And yet, we're not complaining. Now if we say we incur a 1% time penalty for every private access, a language based solution would reduce that to a 0.001% time penalty scattered across every access. Would we even notice such a small penalty? Even if we did, would it be worth the trade-off to gain a reasonable, non-future blocking, non-feature breaking version of private data that's easier to use than what we're currently doing? I think most developers would say yes.

jridgewell commented 5 years ago

@shannon:

Not sure why it would be flatly rejected.

Just imagine:

class Example {
  test(obj) {
    return obj.name;
  }
}

Now if I introduce a private name field, and I have to refactor every .name access? Or I have to either wrap every input object in a proxy to access it's public field, or make sure I never name my private field the same as any other public property access in my scope. I don't think this will work for normal developers.


@rdking:

we as developers are using things like Proxy, closures, WeakMaps, and other such things to achieve data privacy and incurring far, far greater time penalties in the process

Which only affects the private properties you're using. It does not affect the public properties external to your code.

Now if we say we incur a 1% time penalty for every private access, a language based solution would reduce that to a 0.001% time penalty scattered across every access.

Ok. My project has 199614 public property accesses, before transpiling and not counting spec algorithms. And then there are hot-for loops, which access properties on sequential objects, etc. It doesn't scare you to slow down every property access to make support for some private properties?

Would we even notice such a small penalty?

Yes, I think you would absolutely notice it. And so did every one of the implementers in the room.

Even if we did, would it be worth the trade-off to gain a reasonable, non-future blocking, non-feature breaking version of private data that's easier to use than what we're currently doing?

Per my above above reply in this comment, I don't think this is easier to use at all.

shannon commented 5 years ago

@jridgewell

just imagine:

class Example { test(obj) { return obj.name; } } Now if I introduce a private name field, and I have to refactor every .name access? Or I have to either wrap every input object in a proxy to access it's public field, or make sure I never name my private field the same as any other public property access in my scope. I don't think this will work for normal developers.

I disagree with this logic. If I'm adding a private field I almost always want to be then accessing the private field internally so no refactor is required. A refactor is only required on the off chance I sill want to access the public property of obj which sounds perfectly reasonable to me. I want to explicitly access the public property, not the private property declared here. If the obj is not instance of Example, then again no refactor and nothing changes in the lookup semantics.

I don't think this will work for normal developers.

I think this will work just fine for normal developers.

jridgewell commented 5 years ago

A refactor is only required on the off chance I sill want to access the public property of obj which sounds perfectly reasonable to me... If the obj is not instance of Example, then again no refactor and nothing changes in the lookup semantics.

Please refer back to the slowdown and encapsulation break in https://github.com/tc39/proposal-class-fields/issues/147#issuecomment-430001469. Having .x mean both public and private access suffers both problems.

rdking commented 5 years ago

@jridgewell

The only solution I'm aware of is changing all .x property accesses in the scope of the private field to being private property accesses. That means there's no way to access the someone else's public property .x in the same scope.

Want other solutions?

Those are just 2 of the ideas I came up with in the last 30 seconds.

Now if I introduce a private name field, and I have to refactor every .name access?

In that case, I'd just look at you funny, wondering why you did that. With this version of the syntax, private doesn't give you the right to have multiple properties with the same name. That's no different than other languages supporting private. It's also no different than the current condition of ES. An object's property names are always mutually exclusive. There's no reason requiring this to be any different due to access levels being applied to those properties.

My project has 199614 public property accesses....

That sounds really scary.... wait.... no it doesn't. I would have to somehow run every possible pathway through your code to incur that many accesses. Even if I did, 200,000 * 10 = 2Million extra clock ticks. So that weird process running through every possible path which would take probably about a minute or 2 now, would take an extra 2 seconds. Justin, computers are scary fast.

shannon commented 5 years ago

@jridgewell

Please refer back to the slowdown and encapsulation break in #147 (comment). Having .x mean both public and private access suffers both problems.

Please refer to the rest of this thread where these issues are discussed.

shannon commented 5 years ago

@rdking @jridgewell

That sounds really scary.... wait.... no it doesn't. I would have to somehow run every possible pathway through your code to incur that many accesses. Even if I did, 200,000 * 10 = 2Million extra clock ticks. So that weird process running through every possible path which would take probably about a minute or 2 now, would take an extra 2 seconds. Justin, computers are scary fast.

And then compare this to the extra time it takes to compile the extra this.#x syntax semantics. I suspect the performance would even out as the engine optimized at runtime, which V8 is pretty great at.

jridgewell commented 5 years ago

@rdking:

Total cost? An insignificant 10 cpu cycles per access.

Every single implementer in the room balked. I can't stress that enough, every single one of them doesn't want to make access more complicated than it already is.

Given those are both the same solution, I'll leave it at that.

With this version of the syntax, private doesn't give you the right to have multiple properties with the same name.

I'm not talking about multiple private names on my class, that would be silly. I'm talking about property access on objects that may not be instances of my class. Why should the .name access on an foreign object have to change because I introduce a private name field on my class?

That's no different than other languages supporting private.

What?

I would have to somehow run every possible pathway through your code to incur that many accesses.

Not really, just any one of them in a loop.


@shannon

Please refer to the rest of this thread where these issues are discussed.

I cannot be more explicit in my rebuttal of them every time it's brought up.

shannon commented 5 years ago

@jridgewell

I would have to somehow run every possible pathway through your code to incur that many accesses.

Not really, just any one of them in a loop.

Then why mention you how many you have? This would almost certainly be optimized away if you ran it in a loop.

Every single implementer in the room balked. I can't stress that enough, every single one of them doesn't want to make access more complicated than it already is.

No one wants to make things more complicated, but if that's what it takes to make the syntax/mental model clean and simple then that's what it takes. That's the job of the engine. Are there any bugs / issue trackers / forums with the various engines that track this discussion or was it just in a closed room with TC39?

jridgewell commented 5 years ago

Then why mention you how many you have? This would almost certainly be optimized away if you ran it in a loop.

Because it gives a perspective on the number of areas this will impact. Property access is by far the most common operation in JS (I can't back that up, but it is my strong intuition knowing all of the codebases I've contributed to). Now any loop that has any property access (nearly all of them, I'd wager) are going to be impacted. Multiply that by the number of accesses (of which there are many, which was my point).

This would almost certainly be optimized away if you ran it in a loop

"Optimized" down to what, two instructions if we're consistently iterating the same type. That's still far too much of a hit. Good luck to any code that's polymorphic, of which you're guaranteed to be writing.

but if that's what it takes to make the syntax/mental model clean and simple then that's what it takes.

Again, it's not simpler. Either we've broken encapsulation and we've slowed down access (conditional receiver semantics), or we've every private name we add to a class changes the semantics of public .property accesses in the scope (always private semantics). I don't recall any committee member arguing that those are simpler mental models.

Are there any bugs / issue trackers / forums with the various engines that track this discussion or was it just in a closed room with TC39?

Basically everything is on Github. You just have to know the handles of people who work on whichever implementation you care about. The "closed room" meetings have public notes.

Igmat commented 5 years ago

@jridgewell, you don't have to change any code for access to public property x of foreign object according to my proposal in #134. You can't access private x of foreign object using obj.x syntax either. But if you want, you may do it using obj[x], please read carefully that proposal. It actually doesn't have performance problems, since it uses existing lookup mechanism.

shannon commented 5 years ago

@jridgewell

Multiply that by the number of accesses (of which there are many, which was my point).

I would say that if we do our job right it would not be O(n) but more likely O(logn) or better.

"Optimized" down to what, two instructions if we're consistently iterating the same type. That's still far too much of a hit. Good luck to any code that's polymorphic, of which you're guaranteed to be writing.

I'm not an engine contributor but I can at least imagine a way to optimize all difference away from private vs public property access so that it doesn't need to do this logic every single lookup. IIRC V8 already caches property look ups in certain scenarios. Private properties would actually make this slightly simpler and more resistant to change thus extending the life of the cache.

Again, it's not simpler. Either we've broken encapsulation and we've slowed down access (conditional receiver semantics), or we've every private name we add to a class changes the semantics of public .property accesses in the scope (always private semantics). I don't recall any committee member arguing that those are simpler mental models.

Again, I would argue that there is no broken encapsulation and there is no always private semantics, so I'm not sure why you state this as if this is something I would say is simpler.

Basically everything is on Github. You just have to know the handles of people who work on whichever implementation you care about. The "closed room" meetings have public notes.

Thanks, I guess I will be searching through quite a bit of notes over the last couple of years.

@Igmat I would be ok with obj[x] as a lookup of a foreign object. Seems pretty reasonable to me. I don't want to take away from your proposal there. My main point of starting this thread is that the justifications because of technical limitations seems really weak and I'm just trying to point out that if we work through those limitations we might arrive at a better solution. Instead of just saying that's too hard (in so many words).

bakkot commented 5 years ago

@Igmat your proposal gives the this keyword even more special semantics, which I really, really do not think is a good idea (see FAQ).

shannon commented 5 years ago

@jridgewell At there very least you could JIT compile away an static reference (this.x). And then it would only be dynamic references for this. That's a significant chunk. And I've really just a random developer think of things up right now. I'm sure there a tons of other areas for optimization.

jridgewell commented 5 years ago

you don't have to change any code for access to public property x of foreign object according to my proposal in #134.

Ah, I didn't realize you were talking about a different issue's proposal. Yes, we can treat this as a special property reference. There's a bit too much in that thread for me to read through at this point.

Given your this.privateField syntax, how would methodThatAccessPrivateField.call(forignObject) behave? Does it interact with the public, or private? It's it a TypeError?

I would say that if we do our job right it would not be O(n) but more likely O(logn) or better. I'm not an engine contributor but I can at least imagine a way to optimize all difference away from private vs public property access so that it doesn't need to do this logic every single lookup.

This is coming down to a failure to understand how engines handle monomorphic and polymorphic code.

In a monomorphic case, every object is of exactly the same "shape" (has same exact properties, properties have same type, etc). The engine has to gate this monomorphic code with an if statement: "if not expected shape, deoptimize". Even if that boils down to 2 CPU instructions, we're gonna hit slowdowns.

In polymorphic cases, there are multiple successive if statements. Even more slowdown.

In megamorphic cases, there won't be any JIT. You're likely writing megamorphic code without even realizing. If you've ever used lodash/underscore/higher-order functions, I guarantee you're writing megamorphic code.

Again, I would argue that there is no broken encapsulation

And again, please see https://github.com/tc39/proposal-private-fields/issues/14#issuecomment-153050837 for how this breaks encapsulation.

shannon commented 5 years ago

@jridgewell I understand how this is supposed to work and I would assume that the lookup logic for property access would be of the monomorphic form. All fields have descriptors of a defined form. If your point is that there is one more flag on the field descriptor, then sure, but there other flags added to descriptors for other langauge features (writable, getters). I think it would be more than justified here. So I would say it's at most 1 check difference. So if it was worth it for getters, it's worth it for private fields.

Also I wouldn't make any assumptions about code that I write when I want to optimize. This tone is rather condescending.

Again mistaken property lookups on foreign objects is not breaking encapsulation. It's just bad code if you do this.

shannon commented 5 years ago

@jridgewell I'd also just like to point out that loops could really optimize this away even though you seem to disagree. Depending on the case of course.

Simple real world use case that I need in my project.

for(const prop of ['x', 'y', z'']){
  sendOverWire(this.id, prop, this[prop]);
}

If prop is always a private property it doesn't have to do this private property check after a while. V8 does micro optimizations like this all the time.

jridgewell commented 5 years ago

I would assume that the lookup logic for property access would be of the monomorphic form.

All property access are monomorphic? Because it's affecting all of them.

Also I wouldn't make any assumptions about code that I write when I want to optimize.

But you're not considering that is is all property access that are affected. Even the code you didn't write and didn't optimize.

This tone is rather condescending.

Apologies, that's not my intention. Megamorphic code is not bad code, and you are not a bad programmer for writing it. I certainly write code that's megamorphic. It just means it fell off a very narrow hot-path the engines like to take.

Again mistaken property lookups on foreign objects is not breaking encapsulation. It's just bad code if you do this.

Did you read the following example from the link?

let x1 = new X(1);
let obj = { a: 3 };
x1.swap(obj); // --> TypeError or 3?

If it's 3 (like you've suggested based on it being a foreign object, so it should use public field), then encapsulation is broken. I don't understand why that's even up for debate. I have now exposed that a public field a behaves a certain way in my "encapsulated" code. If I change the name of my private field, it'll change the public .a's behavior.

If prop is always a private property it doesn't have to do this private property check after a while. V8 does micro optimizations like this all the time.

Incorrect. It has to do the "is it same shape" check on every iteration.

shannon commented 5 years ago

All property access are monomorphic? Because it's affecting all of them.

Yes the portion of the code that does the access logic should always be the same. Not that the code surrounding the access has to be monomorphic. That surrounding code could be optimized around other heuristics.

Incorrect. It has to do the "is it same shape" check on every iteration.

No because the properties are static string literals. It should know that the entire loop can be unrolled and it will only reference private properties. The engine can do this under the hood and does do similar things like this all the time.

Did you read the following example from the link?

Yes I read it and my comment stands. If you're public API accepts foreign objects it's already part of your public API. If you care about this you brand check. The act of accessing the field does not constitute breaking encapsulation. It just means you accept bad input. I don't really see why this proposal is trying to solve this issue as I see it as a completely separate issue that shouldn't be conflated with encapsulation.

bakkot commented 5 years ago

If you're public API accepts foreign objects it's already part of your public API.

I think it would be very bad for private fields to be specified in such a way that the natural way of using them makes the names of those private fields part of the public API of the class. We should not do that.

shannon commented 5 years ago

@bakkot it wouldn't. Writing code that acts in private names for foreign objects would make it part of your public API. The same way the current proposal throwing TypeError makes it part of the public API.

hax commented 5 years ago

Generally it's wrong. We could use Symbol.private that provides hard private, undectable and no runtime-check + syntax sugar on top of it (described in #134) to get private x + this.x.

@Igmat Yes, your proposal may work, but there is a new pain: resolution rule is more complex,this.priv and other.priv is not consistent. Personally I think I can accept that just like I can accept this.#x (if ignore other issues of public field), but I'm afraid it's very hard to convince others πŸ˜‚

bakkot commented 5 years ago

@shannon Code that I write in Java all the time:

class Point {
  // constructor omitted
  private int x;
  private int y;
  boolean equal(Point other) {
    return this.x == other.x && this.y == other.y;
  }
}

If the analogous JS code, replacing private by whatever private field mechanism we end up with and removing the type annotations, allows someone to craft a non-instance of Point which will .equal a Point, for example as ({x: 0, y: 1}), then the natural way of using them makes the names of those private fields part of the public API of the class.

Keep in mind that you don't even need an explicit parameter to observe this, since you can .call a method from the class on any object.

jridgewell commented 5 years ago

It should know that the entire loop can be unrolled and it will only reference private properties. The engine can do this under the hood and does do similar things like this all the time.

I will guarantee that there is a "is this the same shape" check at every monomorphic site, because it has to ensure it is monomorphic. Even in the current #prop proposal, it's there.

If you care about this you brand check. The act of accessing the field does not constitute breaking encapsulation.

We disagree at this point.

Accessing a public field when it's meant to private is breaking encapsulation. There is a strong difference between what you are proposing and what private symbols does. In private symbols, it is always a private access, and it's thus impossible to leak the private property afterwards.

Guarding with branding fixes this code example, but does not change the semantics of "it's sometimes private and sometimes public". If the proposal doesn't guarantee always private, it's not encapsulated.

I don't really see why this proposal is trying to solve this issue as I see it as a completely separate issue that shouldn't be conflated with encapsulation.

Because it is encapsulation.

hax commented 5 years ago

@shannon

I think you would have the same issue with var.

If we just jump into a method (for example, when debugging), and try to find whether x in a method is a instance var, yes, they are same (first check method scope, then class scope). But in most cases, you reading code in linear order, and you remember some essential info (like the name/type of instance vars, the name and signature of methods...), skip some details. You don't need to check whether x is instance var every time. It's like there is a "cache" in our brain.

Assume we are not using var in ES6+ anymore, so we can salvage for only instance var usage. πŸ˜‚

That means, when you see var declaration, it's a instance var. So you glance the code, and gather all vars. But in let case, you need to make sure you are in the classbody. It's easy, but just a extra step. I would prefer no extra step if possible.

And, var for instance var is a very lucky concept/terminology consistency.

hax commented 5 years ago

@shannon

I just don't know where these requirements came from.

I don't know too. But I guess some come from library/framework authors, some come from the engine vendors, and some from the the preference of the TC39 members as how they understand the language in a language designer perspective. The last is not necessarily bad, most time they are good I believe.

I'm really curious how many developers have asked for them and what types of use cases need them.

While as a developer I don't have these requirements in most case, but I understand they may all have some good reason to have. I just don't want to deny the requirements from others, just like I don't want other deny mine. πŸ˜›

In the worst case, like how I am against public field, I never say public field is a false requirement, but: you don't have enough evidence to prove it's a must to have in current status, and even you want it anyway, you should not land it in such broken form.

hax commented 5 years ago

@shannon

then the committee goes and discusses it offline and then says it didn't meet the requirements.

Of coz it's a communication problem.

But, @littledan @bakkot and others who love current proposal, what I am really worrying about is, you guys talked to many others, and convinced some it's a good enough proposal. And when you do more, you see more agree with you, less disagree with you, so you just believe, yes, we're ok! And for those still make a noise (like me πŸ˜‰ or some others here), you just think ok, we can't satisfy everyone, we already did our best to "communicate" with the objectors.

But it's probably only a self-reinforcement, not a truth. Many who disagree just disappear because they just tired. And someone who disagree will never come here, they just choose use their own private solution and don't care anymore. So for a controversial proposal like this, the risk of community break will always there, we need to step back and find some root cause of the controversial parts, and try to eliminate or at least mitigate it.

hax commented 5 years ago

@shannon

I have my own "requirements":

  1. It should make my code easier to read

this.#x may be ok if you accept this.#x is new this._x

  1. It should make my code easier to write

this.#x not too bad, if again, you consider it's a new this._x

  1. It should make my code easier to understand

Again, not too bad if you think in this._x way...

So far this proposal has failed at all of these.

No, I'm not trying to say current proposal is ok (you will not believe me 😘)! I just try to take the position of the supporters of this proposal.

If you just think in a way like they want, yes, it seems not too bad. (at least in the private syntax part)

Unfortunately , the "when you learned, you will accommodate it" assumption is only partly true. The full chains are "only when you learned, agreed, or at least compromised, you may accept it, and accommodate it".

So you (@bakkot @littledan ) see the problem, there are many (like us? πŸ˜‰), who will hardly to agree it, and some of them don't want to compromise, so they never accept it, don't mention accommodate it.

hax commented 5 years ago

@shannon

It's not a vote but if you ask the right questions you can get good feedback on what the larger developer community actually wants. Instead of all this anecdotal conjecture. I would highly recommend that the TC39 do something similar here.

Actually they did, I truely believe, but the problem is... see my previous comment: https://github.com/tc39/proposal-class-fields/issues/147#issuecomment-430089075

hax commented 5 years ago

@bakkot

@Igmat your proposal gives the this keyword even more special semantics, which I really, really do not think is a good idea.

Though I agree with you in this case (see my previous comment: https://github.com/tc39/proposal-class-fields/issues/147#issuecomment-430076754) , you know what, as I always say you should think about why there are too many such tryings? You always think your solution is good enough, and you always think they eventually have to accept it and accommodate it, but the statistics in #100 and all these hopeless tryings must tell you something.