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

Symbol.private vs WeakMap semantics #183

Closed Igmat closed 5 years ago

Igmat commented 5 years ago

Originally I posted following messages in #149 answering to @erights concern about Membrane pattern implementation with Symbol.private approach. I've decided to create new issue here for several reasons:

  1. Previous thread become too big to follow it
  2. While this topic is related to original discussion it's actually important enough to be separated
  3. 21 days passed from the moment I posted it, 16 from @erights promise to review it - and NO response so far (even though he was active in https://github.com/rdking/proposal-class-members/issues/10#issuecomment-445988975)
  4. According to this: https://github.com/tc39/proposal-class-fields/issues/175#issuecomment-441698604, https://github.com/tc39/proposal-class-fields/issues/149#issuecomment-431527298 and September meeting it seems to be a major concern against Symbol.private

@ljharb, @erights, @littledan, @zenparsing could you, please answer to this:

  1. Is Membrane pattern a major concern?
  2. Did I address Membrane+Symbol.private issues with following solution?
  3. Is lack of ergonomic syntax is also a major concern?
  4. Should I summarize syntax solutions for Symbol.private (including my yet unpublished ideas)?

Copied from https://github.com/tc39/proposal-class-fields/issues/149#issuecomment-441057730


@erights, as I said before there is a solution that makes your test reachable using Symbol.private, here's a sample:

function isPrimitive(obj) {
    return obj === undefined
        || obj === null
        || typeof obj === 'boolean'
        || typeof obj === 'number'
        || typeof obj === 'string'
        || typeof obj === 'symbol'; // for simplicity let's treat symbols as primitives
}

function createWrapFn(originalsToProxies, proxiesToOriginals, unwrapFn) {
    // `privateHandlers` are special objects dedicated to keep invariants built
    // on top of exposing private symbols via public API
    // we also need one-to-one relation between `privateHandler` and `original`
    const privateHandlersOriginals = new WeakMap();
    // we're keep track of created handlers, so we'll be able to adjust them with
    // newly exposed private symbols
    const allHandlers = new Set();

    // just simple helper that creates getter/setter pair for specific
    // private symbol and object that gets through membrane
    function handlePrivate(handler, privateSymbol) {
        const original = privateHandlersOriginals.get(handler);
        Object.defineProperty(handler, privateSymbol, {
            get() {
                return wrap(original[privateSymbol]);
            },
            set(v) {
                original[privateSymbol] = unwrapFn(v);
            }
        })
    }

    function wrap(original) {
        // we don't need to wrap any primitive values
        if (isPrimitive(original)) return original;
        // we also don't need to wrap already wrapped values
        if (originalsToProxies.has(original)) return originalsToProxies.get(original);
        const privateHandler = {};
        privateHandlersOriginals.set(privateHandler, original);
        allHandlers.add(privateHandler);

        //       note that we don't use `original` here as proxy target
        //                      ↓↓↓↓↓↓↓↓↓↓↓↓↓↓
        const proxy = new Proxy(privateHandler, {
            apply(target, thisArg, argArray) {
                thisArg = unwrapFn(thisArg);
                for (let i = 0; i < argArray; i++) {
                    if (!isPrimitive(argArray[i])) {
                        argArray[i] = unwrapFn(argArray[i]);
                    }
                }

                //          but we use `original` here instead of `target`
                //                           ↓↓↓↓↓↓↓↓
                const retval = Reflect.apply(original, thisArg, argArray);

                // in case when private symbols is exposed via some part of public API
                // we have to add such symbol to all possible targets where it could appear
                if (typeof retval === 'symbol' && retval.private) {
                    allHandlers.forEach(handler => handlePrivate(handler, retval));
                }

                return wrap(retval);
            },
            get(target, p, receiver) {
                receiver = unwrapFn(receiver);
                //       but we use `original` here instead of `target`
                //                         ↓↓↓↓↓↓↓↓
                const retval = Reflect.get(original, p, receiver);

                // in case when private symbols is exposed via some part of public API
                // we have to add such symbol to all possible targets where it could appear
                if (typeof retval === 'symbol' && retval.private) {
                    allHandlers.forEach(handler => handlePrivate(handler, retval));
                }

                return wrap(retval);
            },
            // following methods also should be implemented,
            // but it they are skipped for simplicity
            getPrototypeOf(target) { },
            setPrototypeOf(target, v) { },
            isExtensible(target) { },
            preventExtensions(target) { },
            getOwnPropertyDescriptor(target, p) { },
            has(target, p) { },
            set(target, p, value, receiver) { },
            deleteProperty(target, p) { },
            defineProperty(target, p, attributes) { },
            enumerate(target) { },
            ownKeys(target) { },
            construct(target, argArray, newTarget) { },
        });

        originalsToProxies.set(original, proxy);
        proxiesToOriginals.set(proxy, original);

        return proxy;
    }

    return wrap;
}

function membrane(obj) {
    const originalProxies = new WeakMap();
    const originalTargets = new WeakMap();
    const outerProxies = new WeakMap();

    const wrap = createWrapFn(originalProxies, originalTargets, unwrap);
    const wrapOuter = createWrapFn(outerProxies, originalProxies, wrap)

    function unwrap(proxy) {
        return originalTargets.has(proxy)
            ? originalTargets.get(proxy)
            : wrapOuter(proxy);
    }

    return wrap(obj);
}

const privateSymbol = Symbol.private();
const Left = {
    base: {
        [privateSymbol]: ''
    },
    value: '',
    field: privateSymbol,
};
const Right = membrane(Left);

const { base: bT, field: fT, value: vT } = Left;
const { base: bP, field: fP, value: vP } = Right;

// # set on left side of membrane
// ## set using left side field name
bT[fT] = vT;
assert(bP[fP] === vP);

bT[fT] = vP;
assert(bP[fP] === vT);

// ## set using right side field name
bT[fP] = vT;
assert(bP[fT] === vP);

bT[fP] = vP;
assert(bP[fT] === vT);

// # set on right side of membrane
// ## set using left side field name
bP[fT] = vT;
assert(bT[fP] === vP);

bP[fT] = vP;
assert(bT[fP] === vT);

// ## set using right side field name
bP[fP] = vT;
assert(bT[fT] === vP);

bP[fP] = vP;
assert(bT[fT] === vT);

Copied from https://github.com/tc39/proposal-class-fields/issues/149#issuecomment-441075202


I skipped fP->fT and fT->fP transformations in previous example for simplicity. But I'll mention such transformation here, since they are really easy handled when private symbols crosses boundary using public API of membraned object

set on left side of membrane

set using left side field name

bT[fT] = vT;

goes as usual, since happens on one (left) side of membrane

assert(bP[fP] === vP);
bT[fT] = vP;

set using right side field name

bT[fP] = vT;
bT[fP] = vP;

set on right side of membrane

set using left side field name

bP[fT] = vT;
bP[fT] = vP;

set using right side field name

bP[fP] = vT;
bP[fP] = vP;
jridgewell commented 5 years ago

Eventually, we (including you) agreed that the original proposal should go back to Stage 3. I'm surprised that, after all that, still saying here that it's a problem.

This was under the assumption that weakmaps where the only possibility. I had never even thought of using private symbols until @zenparsing brought it up in the Sept 2018 meeting. Now that I know of it, I'm pushing them as the better idea.

littledan commented 5 years ago

Several reasons why private symbols are not feasible are discussed in this thread and other in-committee and offline discussions. I was aware of the private symbol idea years earlier, and I didn't push it because of those reasons. We are going in circles.

jridgewell commented 5 years ago

Several reasons why private symbols are not feasible are discussed in this thread and other in-committee and offline discussions... We are going in circles.

We're not. As I said with you before, the main hard objection has been membranes. I'm putting in the legwork to prove that they're not incompatible. Even @erights thinks we're making progress here.

The last objection will be branding. Should it be done be default, or is a manual check satisfactory? But this isn't a technical requirement (encapsulation doesn't care either way), it's a subjective one. My opinion is that by-default is the wrong choice, so I'm making my case.

kaizhu256 commented 5 years ago

-> is commonly used in comments to describe baton-passing and conversion between data "types" like these real-world examples:

// https://github.com/kaizhu256/node-utility2/blob/2018.12.30/lib.utility2.js#L3449
local.bufferValidateAndCoerce = function (bff, mode) {
/*
 * this function will validate and coerce/convert <bff> -> Buffer
 * (or String if <mode> = "string")
 */
    ...
    // convert utf8 -> Uint8Array
    if (typeof bff === "string") {
    ...
    }
    // convert Uint8Array -> utf8
    if (mode === "string") {
        return new TextDecoder().decode(bff);
    }
    // coerce Uint8Array -> Buffer
    if (!local.isBrowser && !Buffer.isBuffer(bff)) {
        Object.setPrototypeOf(bff, Buffer.prototype);
    }
    return bff;
};

// https://github.com/kaizhu256/node-utility2/blob/2018.12.30/lib.utility2.js#L2173
local._http.ServerResponse.prototype.end = function (data) {
    ...
    // asynchronously send response from server -> client
    setTimeout(function () {
        that.onResponse(that);
        that.emit("data", local.bufferConcat(that.chunkList));
        that.emit("end");
    });
};
erights commented 5 years ago

At https://github.com/erights/PLNSOWNSF/tree/master/examples are variations of @zenparsing 's example at https://github.com/tc39/proposal-class-fields/issues/183#issuecomment-451733300 , to compare for readability and clarity. What is your preference ranking for

Be prepared to be surprised by your own choices.

ljharb commented 5 years ago

@igmat what about if someone passes a thunk for a private Symbol across a membrane? Like () => privateSymbol - will you wrap every function’s return to ensure that there’s no way to get at an unauthorized private symbol?

erights commented 5 years ago

@ljharb Membranes already do all that wrapping for you. If it doesn't, it is not a working membrane.

Igmat commented 5 years ago

@erights, @littledan I understand that it's long https://github.com/tc39/proposal-class-fields/issues/183#issuecomment-451788529, but could you please pay attention to and answer few questions from it?

  1. Do you need more explenations for this question?

  2. Does it make any sense for you?

  3. I agree that membranes are no longer the main objection.

    Is it possible to use this quote of yours as an argument in favor of Symbol.private proposal?

  4. Do you agree with this statement?

  5. And did @jridgewell adressed requirement for brand-checking when need in his https://github.com/tc39/proposal-class-fields/issues/183#issuecomment-451719147?

  6. Do you think that it's reasonable trade-off?

erights commented 5 years ago

@erights, @littledan I understand that it's long #183 (comment), but could you please pay attention to and answer few questions from it?

  1. Do you need more explenations for this question?

It is no longer a central concern. I can see that this is possible, so I do not currently need to understand it in detail.

  1. Does it make any sense for you?

No. I don't understand the point.

  1. I agree that membranes are no longer the main objection. Is it possible to use this quote of yours as an argument in favor of Symbol.private proposal?

Yes, absolutely. Compared to our previous understanding, it is definitely an argument in favor, and a strong one. However, IMO the remaining arguments against remain fatally strong.

  1. Do you agree with this statement?

No. The GC issue is only because everyone but Chakra ignored my advice about how to implement WeakMaps. PrivateName fixes that.

  1. And did @jridgewell adressed requirement for brand-checking when need in his #183 (comment)?

@rdking's answer at https://github.com/tc39/proposal-class-fields/issues/183#issuecomment-451718871 is much better that @jridgewell's, but IMO still fatally bad.

  1. Do you think that it's reasonable trade-off?

Rereading, I am unclear on which position is the "it" you are asking about. If you mean not having integrity by default, then no, I do not think it is a reasonable tradeoff.

rdking commented 5 years ago

@littledan Isn't a "foot-gun" just a part of the language that does not behave in an intuitive fashion? A part that's simply easy to forget it behaves differently due to how other similar patterns work? If so, then private fields does indeed introduce foot-guns. That you find this to be "strong language" is ... interesting. As a reminder, while the WeakMap story exists for clarification, it should also be noted that Babel's implementation (an instantiation of the WeakMap Story) can be augmented to remove some of the foot-guns while private-fields cannot.

I'm not trying to raise an argument on this. I just want to point out that your downplay over the use of the term "foot-gun" might be misconstrued by readers as possibly a bit disingenuous. Since I know that's not your intent...

rdking commented 5 years ago

@erights 2 questions:

  1. Why do you believe that no one would choose to follow my example or similar where brand-checking is needed? Is not something similar to this already current practice for such scenarios?
  2. Why is confused deputy such an issue when it will inherently exist for all subclasses? Long and short? The confused deputy problem is not an issue where private fields is concerned, regardless of how they are implemented. In the end, the only thing that can access the data stored against the private field is the class that wrote the data. Further, the data is only available via the instance object. It is simply not possible to construct code that performs some undesirable action as a result of a "confused deputy" action unless both the private key and instance object are simultaneously available. In the case of private symbols, if the private symbol is available, then it is generally safe to assume that communication was intended.
littledan commented 5 years ago

As I said with you before, the main hard objection has been membranes.

I think there may be something funny about how we communicate in TC39. Concerns raised which don't use the "hard objection" phrasing are still important. We've discussed many of these concerns, not just membranes.

zenparsing commented 5 years ago

@erights

For me, the "hidden" ones (both with and without arrow) win hands down. It's totally obvious what's going on given basic understandings of variable scope. It's completely obvious what the difference is between hidden state and normal properties. And there is no parallel lexical namespace to work around with decorators.

You?

littledan commented 5 years ago

Would the "hidden" proposal solve any of the problems this thread set out to solve originally, with an examination private symbols? The ideas seem to be going in very different directions.

zenparsing commented 5 years ago

Well, the "hidden" thing isn't really a proposal; it already works (albiet slower than string properties because WeakMaps are slow, which we should fix). The -> is just there for chaining/readability sugar.

To answer your question, I would say that it rather avoids the problem by not leading the user to expect property-like behavior.

Also, apologies for contributing to the shift of focus in this thread, but it's where the action is at the moment! : )

Igmat commented 5 years ago

@erights,

No. The GC issue is only because everyone but Chakra ignored my advice about how to implement WeakMaps. PrivateName fixes that.

Ok, it's my fault that I shifted the focus. Don't you see pattern (e.g. using Symbol.private instead of WeakMap in some cases) I provided as useful one?

Rereading, I am unclear on which position is the "it" you are asking about. If you mean not having integrity by default, then no, I do not think it is a reasonable tradeoff.

I meant does "avoiding some errors" is more valuable then "having a huge set of useful" patterns?

syg commented 5 years ago

I think there may be something funny about how we communicate in TC39. Concerns raised which don't use the "hard objection" phrasing are still important. We've discussed many of these concerns, not just membranes.

This bears repeating for folks who don't have full context and history paged in. It is not the case that membranes is the only thing that's keeping private symbols from enjoying committee consensus.

Igmat commented 5 years ago

@syg, so why do you refuse to give us more context?

From what I've seen so far, main concerns are:

  1. Issue with Membrane pattern
  2. Lack of branding
  3. Doesn't work the same way as [[InternalSlot]]
  4. Lack of ergonomic syntax

1 - solved here 2 - debatable, since could be easily achieved without being default 3 - why it should work like [[InternalSlot]] at first place? 4 - we already have quite a few different proposal for it, and I'm going to provide one more (I hope it would be last one), but they was rejected mostly because Symbol.private is no-go at all

rdking commented 5 years ago

@syg

This bears repeating for folks who don't have full context and history paged in. It is not the case that membranes is the only thing that's keeping private symbols from enjoying committee consensus.

Since it bears repeating, please repeat the list of issues holding back private symbols.

syg commented 5 years ago

@Igmat It was not my intention to withhold context. (I did not want to derail conversation away from membranes, but I suppose that the issue is titled "Symbol.private vs WeakMap semantics", this is still on topic.) For instance, private symbols' trading of what's been called "hard private" with going up the prototype chain (orthogonal to membrane concerns) is not something that has consensus.

rdking commented 5 years ago

For instance, private symbols' trading of what's been called "hard private" with going up the prototype chain (orthogonal to membrane concerns) is not something that has consensus.

Why is that an issue? From the point of view of any Proxy in the prototype chain, for an object that looks like this:

//PS is a private Symbol instance
let a = {__proto__: {__proto__: {__proto__: {__proto__: {__proto__: {[PS]: Math.PI}}}}}};

attempts to access a[PS] looks no different than an attempt to access a.__proto__.__proto__.__proto__.__proto__.__proto__. Nothing about the private symbol ever gets leaked. So what's the problem?

syg commented 5 years ago

@rdking Please don't shoot the messenger. I'm stating a fact that membranes is not the only thing keeping private symbols from committee consensus. I am not looking to reargue the full contents of those points in this particular thread, nor exhaustively enumerate them.

I understand this answer feels very unsatisfactory and you do wish the debate the full contents of non-consensus points in the hopes of replacing private fields with private symbols, but relitigation should not happen in this thread. I'm sorry about this.

Again, I wished to only clarify that membranes are not the "last hurdle", as it were.

rdking commented 5 years ago

@syg

Please don't shoot the messenger.

😆 Sorry if that came across aggressively. I really do want to know what the outstanding list of issues is that is holding back private symbols. The one issue you pointed out is one that I don't understand, and I would like to know why it is considered an issue. I'm not interested in arguing the points either. I've already learned that it's mostly fruitless, but I still want to understand.

Igmat commented 5 years ago

@syg,

For instance, private symbols' trading of what's been called "hard private" with going up the prototype chain (orthogonal to membrane concerns) is not something that has consensus.

I can't agree with this point. Symbol.private is still hard-private even though it goes up the prototype chain. If you won't provide any clarification for this statement (ideally with an example), then it's not a real fact. Obviously, I can take all reflect-related APIs (e.g. Reflect.has, defineProperty, etc.) and syntaxes (e.g. in class extends, etc.) and show that Symbol.private doesn't leak anywhere, but it would be a huge time investment, which doesn't seem to be needed (am I wrong?), because, most probably, you have some particular example/use-case in mind when talking about breaking hard-privacy, do you?

Also, you might be interested in my answer to most complete list of questions to Symbol.private in @littledan's list.

littledan commented 5 years ago

@jridgewell Walking up the prototype chain on access to private is something we discussed the last time we looked into "static private" solutions. See the section of that repository for a summary of why we didn't select that option. @ljharb puts it succinctly in https://github.com/tc39/proposal-class-fields/issues/43#issuecomment-348051232 :

it must be possible (and the common, default behavior) that any code whatsoever outside the class declaration, including superclasses or subclasses, can not observe anything about private fields; including their names, their values, their absence, or their mere existence.

ljharb commented 5 years ago

To clarify further, if the word "private" is associated with it, I would expect that statement to apply - thus "private symbols walk the prototype chain" imo is a contradictory sentence.

rdking commented 5 years ago

@ljharb That statement "private symbols walk the prototype chain" isn't a contradiction but a misnomer. Private symbols don't walk at all. Property access walks the prototype chain. But since [[Get]] never fires, you have no clue if the goal is to get a property or just a prototype object.

zenparsing commented 5 years ago

To clarify further, if the word "private" is associated with it, I would expect that statement to apply - thus "private symbols walk the prototype chain" imo is a contradictory sentence.

Well said @ljharb, and I think this is a reasonable statement of position (esp. for those that are interested doing internal-slot like things).

But if you take that stance, then "private class methods" don't make sense, either, since class instance methods by definition utilize the prototype chain.

Yes, you can by fiat say that things are "just different" for hash-things, but hard cases make bad law, as Brendan is fond of saying. And the consequences of that bad law are staring us in the face: the proxy problem, the static private problem, the lack of destructuring support, the per-instance memory issues with private methods, the over-reliance on meta-programming to solve essential problems like friendship.

The realization I've had is that if you want to do internal slot stuff, then we already have (almost) everything we need to do that, and in a way that is sufficiently ergonomic, more obvious, and works swimmingly well with existing features. We don't need hash-things and the complexity that they create. We don't need to fight the community.

Syntax is great, but do you know what's better? Realizing you didn't need it in the first place.

bakkot commented 5 years ago

then "private class methods" don't make sense, either, since class instance methods by definition utilize the prototype chain.

I don't think that's actually true according to the definition implied by custom usage? A lot of people would say that function Factory(){ let x = Math.random(); this.m = () => x; } is a "class" with a "class instance method" called m, but obj.m doesn't use the prototype chain at all.

I think a "class instance method" would generally be understood to be nothing more or less than a method which instances of the class all have. Which private methods as currently proposed are. That's why they are the way they are: it's not that we are redefining a term by fiat, it's that we looked at the term and chose something which fit naturally with it. I dispute your definition.

jridgewell commented 5 years ago

To clarify further, if the word "private" is associated with it, I would expect that statement to apply - thus "private symbols walk the prototype chain" imo is a contradictory sentence.

I could call it "encapsulated symbols" instead, but it just doesn't have the same ring to it.

I don't think that's actually true according to the definition implied by custom usage?

Let's not forget that they're neither prototype methods, nor are they like instance fields, because they're non-writable by default. Another thing that bugs me.

littledan commented 5 years ago

We discussed private method semantics in the July and September 2017 TC39 meetings, and continued to revisit them when looking into the "static private" issue; the conclusion of that investigation was along the lines of @bakkot's comment https://github.com/tc39/proposal-class-fields/issues/183#issuecomment-452100642 .

@jridgewell You've mentioned previously that you think private methods should be writable. In response to your inquiries, I discussed this with a number of TC39 delegates, and the general conclusion has been that non-writability is important to make them all equal, in a way that makes it irrelevant whether it's on the prototype chain or not (also it's nice for efficient implementation). This all happened several months ago, and I thought you were OK with the conclusion.

jridgewell commented 5 years ago

I've raised in Jan and March 2018 and once in 2017. Small inconsistencies bug me, making this implementation all the more non-property like. But I'm not going to block over it because it's changeable in the end.

also it's nice for efficient implementation

Wouldn't be necessary with a symbol implementation. Yet another consistency win.

Igmat commented 5 years ago

@bakkot, no, you changed the meaning of method word in JS context, because all other methods in classes (e.g. public one) lives on the prototype. Even in pre-ESnext era, we were installing shared methods to prototype by following syntax:

var SomeClass = function () {}
SomeClass.prototype.method = function () {}

I'm sure you know that. While thing that you've shown isn't methods in JS terms, but rather closure scoped arrow function setted to objects m property. In JS as multipardigmal language those terms a different. Not only in declaring, but also in a lot of their characteristics and semantics.

To clarify further, if the word "private" is associated with it, I would expect that statement to apply - thus "private symbols walk the prototype chain" imo is a contradictory sentence.

@jridgewell, it's not true at all. Access to property keyed with Symbol.private can (and must) walk up the prototype chain, but still be the private one. I don't know why do you have such an opinion, but I'll try to explain the only case where you may worry about hard-private in terms of Symbol.private (especially if some kind of shorthand syntax are presented) that I can imagine. For example:

class A {
    private [#x]() { return 1; }
    printFirstX() {
        console.log(this[#x]());
    }
}
class B extends A {
    private [#x]() { return 2; }
    printSecondX() {
        console.log(this[#x]());
    }
}
const b = new B();
b.printFirstX(); // prints 1
b.printSecondX(); // prints 2

private [#x] = 1; is a declaration of lexically scoped Symbol.private property accessed using #x constant. #x in A and #x in B are two different Symbols, so they are still hard-private even though access to such Symbol-keyed property is walking up the prototype chain.

littledan commented 5 years ago

@jridgewell What's the purpose of continuing to raise these same points repeatedly? We've spent a long time looking into private lookup coming from the prototype chain, as well as writable private methods, in response to your previous interest in them. Is the analysis in the committee's response no longer valid?

jridgewell commented 5 years ago

I haven't continued to raise them? I mentioned it in an offhand comment.

ljharb commented 5 years ago

@Igmat imo a method in JS, colloquially, is any function-valued property on an object - in const o = { f() {} };, f is a method on o. Method syntax in class, certainly, is always on the prototype.

Igmat commented 5 years ago

@ljharb, ok. Let's try to summarize the definition of method term. Method is function that exists (both as own property or found via prototype chain lookup) on the instance. Is it correct?

ljharb commented 5 years ago

@Igmat sounds right to me, in a colloquial/general sense.

Igmat commented 5 years ago

@ljharb great, at least one term, that probably caused misunderstood is clarified. So continuing: We have few ways already to define public methods:

class A {
    constructor() {
        this._anotherMethod = () => { // this lives at instance };
    }
    _method() {
        // this lives at prototype
    }
    changeBehaviorForSomeReason() {
        this._method = () => { // shadow method from prototype for some reason };
        this._anotherMethod = () => { // change instance method for something else };
    }
}

Refactoring from private by convention to hard private using your own slogan # is the new _, we'll get unexpected exception that will be discovered only much later (there won't be any early SyntaxError) after changeBehaviorForSomeReason will be called (and you know that it could be rare and hard-to-detect thing). So we again faced the situation when so BIG difference in semantic and so SMALL difference in syntax will lead to inconsistency and footguns. And I shown only one example, but I'm pretty sure that I can found more, becuase keeping syntax similarity while breaking semantics similarity creates a HUGE amount of false assumptions from developers that aren't aware of in-depth stuff.

ljharb commented 5 years ago

@Igmat certainly making a change from public to private might have some bumps, but that's always going to be the case. In your example, I'm not sure what problems would be caused by changing _ to # (along with declaring #method and #anotherMethod) - can you elaborate?

bakkot commented 5 years ago

Since you will get an error on this.#method = whatever as soon as the code is run, I don't think that would be particularly hard to detect. (I actually do think we should have introduced an early error for code of that form, but a runtime error is almost as good.)

I also expect code of that form to be quite rare. Moreover, I think it's actually good to encourage people to declare as fields things which are intended to be different per instance - code will tend to be more readable when it's clear ahead of time which methods are universally shared and which might differ per instance.

littledan commented 5 years ago

An early error would be hard to preserve with decorators.

bakkot commented 5 years ago

Ah, right, that's true.

Igmat commented 5 years ago

@ljharb, private methods aren't writable and live at instance side not prototype, so this.#method = () => {} will throw at runtime, because it can't even shadow original method, while this.#anotherMethod = () => {} will just work.

Also the only difference between #anotherMethod = () => {} and #method() {} is fact that first is writable and second is not, while both are bound to this. It's just one more inconsistency between them.

Igmat commented 5 years ago

I don't think that would be particularly hard to detect

@bakkot, it could be hard to detect, because such pattern (appears for example in game development) whe we have default in prototype and override it rarely at instance levels, often used in way when changeBehaviorForSomeReason called rarely, so it could be not caught while testing, but happen already in production.

But whatever, the main point is that such big semantic difference for quite similar things leads to inconsistent mental model.

One more example. Refactoring from private to public:

class A {
    #method() {
    }
    someOtherMethod() {
        setTimeout(this.#method, 1000);
    }
}

At some point we decided that #method should be part of public API, because we want our users to be able to override #method for some reason. We just dropped # and may even not notice the problem, unless we replace method with something that uses this.

jridgewell commented 5 years ago

I also expect code of that form to be quite rare. Moreover, I think it's actually good to encourage people to declare as fields things which are intended to be different per instance

Personal opinions are the wrong way to justify this one. It's a pattern I use in personal code, enough that I've tried to have the spec changed.

This was done because you can't have mutable methods and use weakmap semantics without either memory bloat (1 slot per method are on every instance), or implementation difficulty (have to store the default method some shared place and maybe an overridden method on the instance).

Igmat commented 5 years ago

This was done because you can't have mutable methods and use weakmap semantics without either memory bloat (1 slot per method are on every instance), or implementation difficulty (have to store the default method some shared place and maybe an overridden method on the instance).

@jridgewell, but we can use Symbol.private that doesn't have such issue. The only thing we need is to provide shorthand syntax for Symbols, so ergonomic will be at least at the same level as in existing proposal.

bakkot commented 5 years ago

@Igmat, I don't understand your refactoring example; can you say more about what the problem is in that case?

Igmat commented 5 years ago

#method is bind to proper this receiver, so invoking setTimeout(this.#method, 1000); will just work as expected, whether or not #method uses this inside. Refactoring #method to method just by dropping # sigil, will result in that this.method will be called with undefined or Window receiver (depending on platform used), which isn't intended and hard-to-detect.

ljharb commented 5 years ago

@Igmat this.#method isn't bound to the receiver? You'd have to do this.#method.bind(this) for that (also, undefined vs the global isn't based on "platform", it's based on "strict mode")