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

Early error for same name for static and instance field? (question about lexical scoping) #256

Closed mheiber closed 5 years ago

mheiber commented 5 years ago

What is the specced behavior in this case?


class C {
    static #foo;
    #foo
}

Is this an [*early* error as covered here](https://tc39.es/ecma262/#sec-block-static-semantics-early-errors)?
ljharb commented 5 years ago

It is a Syntax Error if PrivateBoundIdentifiers of ClassBody contains any duplicate entries.

Does that and https://tc39.es/proposal-class-fields/#sec-private-bound-identifiers cover it?

mheiber commented 5 years ago

thanks!

rdking commented 5 years ago

Just out of curiosity, why is this an error when the two fields would exist in separate namespaces?

ljharb commented 5 years ago

The namespaces you refer to (static vs instance) differentiate objects, but don’t disambiguate the local syntax:

class X {
  static #a;
  #a;
  a(y) {
    return y.#a;
  }
}
var x = new X();
x.a(x); // does this throw, or return undefined?
x.a(X); // what about this one?

Since each syntactic usage of .#a is supposed to unambiguously refer to a single private field, and the notation doesn’t contain a difference for static vs instance, that means that all private fields, static or instance, in fact share a namespace.

rdking commented 5 years ago

I get it. TC39 decided not to allow the same [[PrivateName]] key to exist on both an instance and the constructor, despite the fact that it would not be semantically different from:

class X {
  static _a;
  _a;
  a(y) {
    return y._a;
  }
}
var x = new X();
x.a(x); // does this throw, or return undefined?
x.a(X); // what about this one?

Private or not, they should both return without throwing since the field would exist in both places. There would be no confusion or complication over this issue since it would simply be the same [[PrivateName]] in the lookup for both the constructor and instances. No different than having the same property name on both an instance and the constructor. Is there any particular reason for not allowing this? That's the part I don't get.

WebReflection commented 5 years ago

I think @rdking has a point: the current limitation is an implementation detail that doesn't reason with developers expectations. If specs were saying that y.#a would pick the right private in case y is [class] instead of instance there wouldn't be any ambiguity.

class X {
  static #a;
  #a;
  a(y) {
    return y.#a;
  }
}

pseudo transpilation that works

const a = {class: new WeakMap, instance: new WeakMap};
class X {
  constructor() { a.instance.set(this, void 0) }
  a(y) {
    return a[isClass(y) ? 'class' : 'instance'].get(y);
  }
}
a.class.set(X, void 0);
nicolo-ribaudo commented 5 years ago

If an object has two different private fields with the same name, which do you think should have the precedence?

class Base {
  constructor() {
    return Derived;
  }
}

class Derived extends Base {
  #x = 1;
  static #x = 2;

  static getX(obj) {
    return obj.#x;
  }
}

const result = new Derived();

// result has both a "static #x" and an "instance #x"
console.log(Derived.getX(result)); // 1 or 2? 
mheiber commented 5 years ago

@nicolo-ribaudo , re your comment: In JS, class properties are not accessbile from instances of the class via this.foo. The "class" object is really the constructor function, so this fits with the semantics of desugared classes.

function Konstructor () {}
Konstructor.foo = 3 // static propertiy
new Konstructor().foo // undefined

You can see this behavior if you swap #x for x in your example (try it in Chrome Canary).

bakkot commented 5 years ago

@mheiber You've missed that his code sample using the return override trick to make Derived an instance of itself, I think. (Look at the implementation of Base.)

mheiber commented 5 years ago

@bakkot thanks for pointing that out. I'm not following the relevance. When I try with public fields I get 1 (the value of the instance property), as expected. Is there something special about private fields in the example? To be honest, I'm not following how the override trick is related either.

nicolo-ribaudo commented 5 years ago

The return override trick makes it such that result === Derived.

Desugared, it is something like this:

function Konstructor () { return Konstructor }
Konstructor.foo = 3 // static propertiy
new Konstructor().foo // 3

If in my example you expect it to log 1, then try running the following code (which is exactly the same, but with the instance property commented):

class Base {
  constructor() {
    return Derived;
  }
}

class Derived extends Base {
  //#x = 1;
  static #x = 2;

  static getX(obj) {
    return obj.#x;
  }
}

const result = new Derived();

console.log(Derived.getX(result));
mheiber commented 5 years ago

Asking again (sorry I'm not 100% following)

What is the relationship to private fields? What specifically does this have to do with privacy?

Here's my stab at an answer:

  1. Derived === result. So we're effectively setting #x twice in the first example.
    • This is not actually a problem for public fields (which can be reassigned) or even a problem for private fields, which can also be reassigned.
    • This is a problem for private methods, which (iirc) cannot be reassigned.

Thanks for your time in catching me up!

WebReflection commented 5 years ago

@nicolo-ribaudo not sure that question was for me, but my transpiled code would work the exact same.

class Base {
  constructor() {
    return Derived;
  }
}

const x = {class: new WeakMap, instance: new WeakMap};
class Derived extends Base {
  constructor(...args) {
    const self = super(...args);
    const classy = isClass(self);
    const _x = classy ? x.class : x.instance;
    _x.set(self, classy ? 2 : 1);
    return self;
  }
  static getX(obj) {
    const classy = isClass(obj);
    const _x = classy ? x.class : x.instance;
    return _x.get(obj);
  }
}

I mean ... I've picked a specific example but the concept of distinguishing between [class] and regular instance remains and works regardless, doesn't it? 🤔

WebReflection commented 5 years ago

P.S. the isClass could be something like:

const isClass = ((gOPD, fb) => Class =>
  typeof Class === 'function' && !(gOPD(Class, 'prototype') || fb).writable
)(Object.getOwnPropertyDescriptor, {writable: true});

assuming Babel makes prototype not writable once transpiled

nicolo-ribaudo commented 5 years ago

Oh ok, I had assumed that you would have always fields in the constructor to the x.instance weakmap. In your example, it would be the only case in the language where a value which is set earlier has the precedence over a value which is set later. The only other exception is non-writable properties, but they throw in strict mode.

I have tried to rewrite your desugaring to match the current proposal, only removing the restriction that an instance and public field can't share the same name, and I think that it could work. (I don't see why it would be needed, but it doesn't matter)

const x = new WeakMap();

class Derived extends Base {
  constructor(...args) {
    super(...args);
    if (x.has(this)) throw new Error("#x has already been declared"); // (1)
    x.set(this, 1);
  }
  static getX(obj) {
    return x.get(obj);
  }
}
x.set(Derived, 2);

(1) is because private class fields already throw when re-declared:

let result = {};
class Base { constructor() { return result } }

class Derived extends Base { #i = 0 }

// Add #i to result
new Derived();

// Add #i to result again
new Derived(); // throws
WebReflection commented 5 years ago

@nicolo-ribaudo yup, that's another way to go (probably even better, since a whole WeakMap for a single value was indeed too much).

As we can see, there's no reason to not allow same name between static and instance fields ... we might want to throw in methods if the x.has(obj) returns false, otherwise everything else is really an implementation detail, that shouldn't be spec'd as language limitation (IMHO)

bakkot commented 5 years ago

Incidentally, there's previous discussion of this in https://github.com/tc39/proposal-static-class-features/issues/47.

WebReflection commented 5 years ago

I didn't see any important use cases, and allowing name sharing would increase complexity a lot.

I tend to agree same name is not a great pattern, but then again we have clear examples in the language itself, where String.length and "string".length both exists and have their own meanings.

If it's accepted on the public field level, consistency would be my use case for this on the private fields too.

Since it doesn't look like complexity is really increased neither, at least from transpilation point of view, I wonder what is so hard in the native side of the affair, that we arbitrarily chose to make the private fields inconsistent with the public one.

From a developer point of view, this will be just yet another laughable JS shenanigan :man_shrugging:

Maybe @littledan can share more on this

ljharb commented 5 years ago

I’ll try to restate, since my reading of the above thread does not leave any doubt for me as to why ohh a static and instance field of the same name can’t be allowed.

Using the return override trick, if both a static and instance field of the same name were allowed, you can make an object that has both the static and the instance field.

How would you extract both fields’ values from that object? (with a public field name, like length, there aren’t two fields - there’s only one - but each private field is a distinct value - it’s not like a string name)

nicolo-ribaudo commented 5 years ago

Using the return override trick, if both a static and instance field of the same name were allowed, you can make an object that has both the static and the instance field.

This is the problem I initially found, but then I realized that the proposal already handles this case. Due to the return-override trick, it is already possible to try to define the same private name twice, and it currently throws (you can test the second snippet in https://github.com/tc39/proposal-class-fields/issues/256#issuecomment-511200563 in an engine with private fields support). If it was possible to use the same name for a static and an instance field, it would behave the same: my initial example (https://github.com/tc39/proposal-class-fields/issues/256#issuecomment-511197210) would throw. This is under the assumption that staitic #x; #x wouldn't create two different private names, but reuse the same one.

EDIT: Throw = throw at runtime

WebReflection commented 5 years ago

I don't think you can have that situation at all. Mind showing some code example that would create such scenario?

ljharb commented 5 years ago

this case is why you can’t have a different field with the same name across static vs instance:

class X {
  static #a = 1;
  #a = 2;
  constructor() { return X; }
  static a(y) { return y.#a; }
}
const x = new X();
X.a(x); // 1, 2, throw?
nicolo-ribaudo commented 5 years ago

That example doesn't actually install the instance field on X; you have to use super() for that to happen. Anyway, I would expect it to throw at new X(), like it already does.

ljharb commented 5 years ago

lol I’m on mobile rn so i won’t try to make a fixed example with return override; if someone else can in the meantime please do.

nicolo-ribaudo commented 5 years ago

It's this one: https://github.com/tc39/proposal-class-fields/issues/256#issuecomment-511197210

class Base {
  constructor() {
    return Derived;
  }
}

class Derived extends Base {
  #x = 1;
  static #x = 2;

  static getX(obj) {
    return obj.#x;
  }
}

const result = new Derived();

// result has both a "static #x" and an "instance #x"
console.log(Derived.getX(result)); // 1 or 2? 

If it was allowed to re-use the same private name, I would expect this to throw at new Derived() because it tries to define a private field which is already defined. (Step 4 of https://tc39.es/proposal-class-fields/#sec-privatefieldadd)

nicolo-ribaudo commented 5 years ago

Btw, my position is: I don't think that there should be technical reasons for this to be disallowed, and allowing it would make it more consistent with public fields without modifying the current privacy level of private fields. On the other hand, I can't think of a single good reason for this to be allowed and a class which uses the same name for both a static and an instant field would just cause confusion for who reads the code. If it was allowed, I would need a linting rule to disallow this bad practice. With private fields we have the possibility of disallowing some bad practices at language level, but I don't know it this is "universally bad" and thus should be disallowed by the language.

ljharb commented 5 years ago

There’d be no way to get both fields’ values out, as in your example.

WebReflection commented 5 years ago

Same with public fields, right?

robpalme commented 5 years ago

Private field names, unlike property names, are precise. They have a "provenance restriction" as Shu defined. https://rfrn.org/~shu/2018/05/02/the-semantics-of-all-js-class-elements.html

If the same name were permitted on both the class and the instance, dereferencing that field on an incoming parameter would become ambiguous, i.e. did the user intend to dereference the static field or the instance field?

So the current unambiguous behaviour seems to have value worth retaining.

On Sun, 14 Jul 2019, 17:28 Nicolò Ribaudo, notifications@github.com wrote:

Btw, my position is: I don't think that there should be technical reasons for this to be disallowed, and allowing it would make it more consistent with public fields without modifying the current privacy level of private fields. On the other hand, I can't think of a single good reason for this to be allowed and a class which uses the same name for both a static and an instant field would just cause confusion for who reads the code. If it was allowed, I would need a linting rule to disallow this bad practice. With private fields we have the possibility of disallowing some bad practices at language level, but I don't know it this is "universally bad" and thus should be disallowed by the language.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-class-fields/issues/256?email_source=notifications&email_token=ABU6F5J2PMTD2EEYWFQIE3TP7NAZXA5CNFSM4IC4NKT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ4HTZI#issuecomment-511212005, or mute the thread https://github.com/notifications/unsubscribe-auth/ABU6F5PWPOYT4QV6IVEPQOLP7NAZXANCNFSM4IC4NKTQ .

ljharb commented 5 years ago

@WebReflection no, because in public fields, every “a” is the same a. Private fields are different, more like symbols where each “a” is a distinct one.

WebReflection commented 5 years ago

I'm not sure I am following

class X {
  static _a = 1;
  _a = 2;
  a(y) {
    return y._a;
  }
}
var x = new X();
x.a(x); // 2
x.a(X); // 1

that's what I read on Chromium ... and I believe static fields are always the same field for the class itself defining them. Once again, I don't have a valid use case now but consistency is what's driving the software world (eslint, standard/semi-standard/prettier) and keep having inconsistencies baked in the language seems wrong.

I'd personally rather enable same field name and flag the circular edge case we have here as not-allowed than rule same name private field out everywhere for a single, irrelevant, edge case.

WebReflection commented 5 years ago

Also (maybe) relevant: TypeScript allows this, and as much as I don't care about TS, I can see already people moaning about TS being superior with (fake) private fields too.

Screenshot from 2019-07-14 18-07-52

nicolo-ribaudo commented 5 years ago

If the same name were permitted on both the class and the instance, dereferencing that field on an incoming parameter would become ambiguous, i.e. did the user intend to dereference the static field or the instance field?

Note that is is already possible to copy the private names from instance fields to static fields:

class Base {
  static #class = null;
  constructor() {
    const ret = Base.#class;
    Base.#class = null;
    return ret;
  }

  static defineStaticFields(Class) {
    Base.#class = Class;
    new Class();
  }
}

class Derived extends Base {
  #x;

  set x(v) { this.#x = v }
  get x() { return this.#x }

  static set x(v) { Derived.#x = v }
  static get x() { return Derived.#x }
}
Base.defineStaticFields(Derived);

Derived.x = 1; // sets static #x
const inst = new Derived();
inst.x = 2; // sets instance #x

console.log(Derived.x, inst.x); // logs "1 2"

For this reason, allowing people to explicitly choose which private names they want to re-use in both positions doesn't weaken any guarantee.

no, because in public fields, every “a” is the same a. Private fields are different, more like symbols where each “a” is a distinct one.

This discussion is based on the possibility that, inside the same class, #a and static #a wouldn't create two different private names but reuse the same one.

rdking commented 5 years ago

@ljharb If in a class declaration, every #a is the same #a, then the consistency holds between private names and public names. Installing the same name to 2 different objects is no different whether the object is an instance or the class constructor. As was stated before, the return override trick in such a scenario should just cause an error.

IMHO, it's not for a language to restrict the use of ill-advised code patterns. Leave that to tooling and education. Sometimes, those ill-advised patterns are actually the best and most expedient solution to a problem (even though that's rarely the case). The job of the language is to provide the developer with the ability to be expressive and thorough. I don't see the point of this restriction.

ljharb commented 5 years ago

It’s imo very much for a language to limit ill-advised patterns - passing burden onto developers and the linting community when it’s avoidable is a bad thing.

Note that the way this is specced, it’s an error, so if there actually turn out to be any good or \ solutions that require the duplicates, the restriction could be relaxed.

WebReflection commented 5 years ago

passing burden onto developers and the linting community when it’s avoidable is a bad thing.

but no consistency is worse DX

Note that the way this is specced, it’s an error, so if there actually turn out to be any good or \ solutions that require the duplicates, the restriction could be relaxed.

but if it throws no new pattern/ideal/solution can possibly come out 'cause nobody would throw errors, they need their code to execute 🤔

WebReflection commented 5 years ago

also about that:

It’s imo very much for a language to limit ill-advised patterns

so same name for public fields should throw too, as there's no valid reason for that ... for developers there's no difference, and from the ergonomic point of view, there is no difference indeed.

ljharb commented 5 years ago

but no consistency is worse DX

I'm still not seeing the inconsistency you're referring to.

There's a very valid reason for that - every occurrence of a public property name that's the same string is the same property name, so static length = 3; length = 2 doesn't have the same conflict that private fields do.

rdking commented 5 years ago

It’s imo very much for a language to limit ill-advised patterns

I understand why you might think this. It seems to be a common theme in modern programming. However, that kind of thinking is a trap that leads to dead ends for the language. The reasoning is simply that what is thought of as "bad" today may become tomorrow's new great pattern. It is thus better that a language simplifies the use of "good patterns" without unnecessarily restricting "bad patterns". Doing so limits future innovation in the language.

Note that the way this is specced, it’s an error...

Does this mean that the spec regarding this will be corrected?

ljharb commented 5 years ago

no, because it’s not incorrect. It means it could be amended later to do something instead of being an error.