Preventing name collisions between public and private properties

[mbrowne]

Please correct me if I'm wrong, but it looks like the following would be valid in the current proposal and would actually create 3 separate properties:

class Demo {
  private #x = 1
  
  method() {
    this.x = 2
    this['#x'] = 3
  }
}

This seems unnecessarily confusing. While I certainly agree with the point in the FAQ about being able to create a property in a subclass even if its name collides with the name of a private property in a parent class, I see no good reason to allow such a name collision within the same class, especially not this.x (I think this['#x'] is more debatable since # isn't really part of the property name).

BTW, I also can't think of a good design in which you'd need to redeclare public properties, which seems to also be possible in the current proposal:

class Base {
  x = 1
}

class Derived extends Base {
  x = 2
}

My concern with all of the above is code readability, especially when inheritance is present which already increases mental overhead. The fewer cases of ambiguity, the better.

[loganfsmyth]

This seems like something that could be easily covered with a linting rule, and probably one I'd use too, but I'm not sure it feels necessary as a language restriction since there isn't a technical reason for it.

[ljharb]

You may be forgetting that the following code must also work:

const classes = [
  class Demo {
  },
  class Demo {
    #x;
  },
  class Demo {
    #x = 1;
  },
  class Demo {
    #x = undefined;
  },
];

classes.forEach((C) => {
  const o = new C();
  assert.equal('x' in o, false);
  assert.equal('#x' in o, false);
  assert.equal(o.x, undefined);
  assert.deepEqual([...Reflect.ownKeys(o)], []);
  assert.deepEqual([...Reflect.ownKeys(Object.getPrototypeOf(o))], ['constructor']);
  assert.deepEqual([...Reflect.ownKeys(C.prototype)], ['constructor']);

  o.x = 1;
  o['#x'] = 2;
  assert.deepEqual([...Reflect.ownKeys(o)], ['x', '#x']);
});

In other words, if this.#x and this.x both can't coexist, then "private" is not assured.

[littledan]

The FAQ also explains that we deliberately support public and private fields with the same name, for strong privacy reasons. I don't see a language fix that we could make here.

[mbrowne]

@ljharb @littledan I'm not following...if this.x and this['x'] are both undefined, how does that break the privacy of this.#x? To be clear, I'm proposing that:

1. If a private field #x already exists on an object, declared in class C, it would be illegal to make an assignment to this.x or this['#x'] within the scope of C (for inner classes, etc. the same principle would hold for aliases to the instance of C).

2. If a public field x already exists on an object higher up the prototype chain, then it would be illegal to create a new private field #x.

3. Ideally, given an instance o, it would be illegal to make an assignment to o.x if a private field #x already exists somewhere in the object's prototype chain and a public field x was never declared in any of the subclasses....with of course a good error message explaining that there's already a private field # and that's why it's illegal. But I realize this last point may be impractical.

Note that I did not say anything about this.x or this['#x'] actually returning the value of the private field #x or referring to it in any way (although perhaps there could be a warning in addition to just returning undefined, assuming outside awareness of a private field is deemed safe as long as there's no way to access it). Also, this example would still be perfectly valid:

class Base {
  #x = 0
}

class Derived extends Base {
  x = 0
}

I think at least #1 and #2 would be practical to implement at runtime without any negative effect on performance, since if I understand correctly, the engine is always aware of the scope of any assignments it's evaluating.

[littledan]

This proposal would break the goal of the private fields proposal that no operation for reading or writing private fields should trigger meta-object operations such as checking up the prototype chain for conflicts.

[mbrowne]

Doesn't the ES engine currently have to check if a slot for a (public) property already exists before creating a new slot? Let's just consider #2 in my proposal for the moment, and suppose I amended it to a simpler version that doesn't check the prototype chain:

If a public field x already exists on the instance, then it would be illegal to create a new private field #x

Just checking for conflicts within the same class would still be better than nothing. And now that I think about it, I recall that this proposal always puts fields on the instance, not the prototype, so I think that all you'd have to do to prevent conflicts with a same-named public property in a parent class would be to check the current instance. So I'm struggling to understand why this would be bad for performance, assuming that's the reason for wanting to avoid "triggering meta-object operations".

It's probably obvious that I have a rudimentary understanding of the ES engine and not in-depth knowledge, so I apologize if I'm wasting anyone's time. But based on my current understanding, I think I still have a valid point here.

[mbrowne]

@loganfsmyth a linting rule is a good fallback if the language can't enforce this. But as a general principle, I would ask: why should a language allow a bad practice simply because it doesn't cause any technical issues? I would say only if there are compelling exceptions to the rule where a usually bad practice makes sense in a particular situation. I can't think of any such exceptions here.

[ephys]

If a public field x already exists on an object higher up the prototype chain, then it would be illegal to create a new private field #x.

Part of the appeal of this proposal for me is the impossibility to have a name collision when using private fields. I wouldn't expect my code to crash because a class I'm extending (eg from a library) decided to add a public field that happens to share its name with one of my private fields.

[littledan]

Closing this issue as the subject is addressed by the FAQ and comments on the thread.

[bakkot]

@mbrowne

To give a non-technical reason for it - I actually find it really nice to be able to do

class C {
  get foo() {
    return this.#foo;
  }
  #foo = 0;
}

or similar. I don't think it's actually desirable to stop me from doing that!

Similarly, redeclaring public properties can be useful:

class Base {
  get x(){ throw new TypeError('must be implemented in a subclass'); }
  type = 'base';
}
class Derived extends Base {
  x = 42;
  type = 'derived';
}

In any case, allowing these collisions is consistent with the rest of the language, where ({x:0, x:1}) is legal. (Which is a recent change: this was only made legal with ES2015 and the introduction of computed property names, when it become impossible to prohibit all name collisions statically.)

More generally, I think we usually feel that "we can't think of a good reason to want this" is not a good enough reason to ban a thing, if it's something which falls out naturally from the rest of the semantics of the language.

[mbrowne]

@bakkot I have to admin, that looks nicer than:

class C {
  get foo() {
    return this.#_foo;
  }
  #_foo = 0;
}

I suppose if you consider the # as part of the field name in a way (in a similar way as you might use an underscore prefix in the analogous situation in other languages, say C#), then it makes sense.

I still think there could be confusion over this, but admittedly my thinking is biased by the fact that such name collisions are impossible in other languages (at least, if you consider # to be just a syntactic necessity and not part of the field name).

Thanks for the replies everyone.

[mbrowne]

I still think it's concerning that you can have a private field #x and a public field '#x' in the same class. But I guess most of the confusion caused by that would be in the category of beginner's mistakes as people learn the syntax, since there are very few cases where someone would do that deliberately (hopefully none if they care about other people being able to read their code). So I guess it's ok if the language won't prevent this particular case if only because it's an edge case after the initial learning curve. But I hope this is regarded as an acceptable downside of the current proposal, given the tradeoffs, rather than something that simply doesn't matter.

Also, @bakkot, I still think redeclaring public fields on subclasses should be avoided... Granted, in ES the same syntax can be used to assign a new value to a property as to declare it for the first time and set its initial value, but semantically I think the constructor would be a better place for setting a new initial value of an inherited field. Allowing some field declarations to be new declarations and others to be re-declarations is just unnecessary mental overhead. And this is assuming you really need an instance field for this sort of thing rather than a static field (the latter seems to be more common for such cases). Having said all that, I consider this a more minor point and won't be pursuing that suggestion further. Just wanted to at least explain my reasoning.