Should accessing a private name of incorrect type be an early error?

[gsathya]

We throw an early error if a private name is accessed but not declared,

class A {
  foo() {
    this.#a = 1; // this throws an early error
  }
}

For private accessors, we don't throw an early error for the following:

class A {
  get #a() {}
  foo() {
    this.#a = 1; // this is a runtime error
  }
}

The engine statically knows whether a private setter or getter is available during parsing so this could be an early error. Should it?

Reasons for why it should be an early error:

• If it can be an early error, it should be as it helps developers catch these bugs sooner
• Consistent with private fields

Reasons for not having it be an early error:

• Not consistent with public accessors

Any other reasons?

[joyeecheung]

To make it an early error, implementations need to keep track of the private accesses during parsing and validate them against the kind of private names that they resolve to after scope analysis (since the private names can be shadowed and declared after the code that contains the access), even if the access is never executed, so I believe there is some inevitable overhead to make it an early error.

If it can be an early error, it should be as it helps developers catch these bugs sooner

We won't completely lose the opportunity for catching bugs even if it's not an early error in the implementations though, as tooling like linters can still statically analyze the code and warn developers.

Setting overhead and developer experience aside, this would be mostly about consistency with other features.

[gsathya]

To make it an early error, implementations need to keep track of the private accesses during parsing and validate them against the kind of private names that they resolve to after scope analysis (since the private names can be shadowed and declared after the code that contains the access), even if the access is never executed, so I believe there are some inevitable overhead here.

Isn't this overhead present even without changing this to be an early error? We have to distinguish between setters and getters to emit different code in any case.

[joyeecheung]

Isn't this overhead present even without changing this to be an early error? We have to distinguish between setters and getters to emit different code in any case.

If the implementation does not emit code for access that is never executed, this can be avoided.

[gsathya]

If the implementation does not emit code for access that is never executed, this can be avoided.

I don't understand what this means. Can you elaborate? How is your parser going to do dead code elimination?

[joyeecheung]

I don't understand what this means. Can you elaborate? How is your parser going to do dead code elimination?

Taking the example from the OP:

class A {
  get #a() {}
  foo() {
    this.#a = 1; // this is a runtime error
  }
}

If this needs to be an early error, the implementation needs to remember the kind of all declared private names and the kind of all private name accesses, then perform type checks with them after the scope analysis resolve the acesses to the correct private names.

If this is only a runtime error, since foo() itself parses (we know that #a resolves to some private name), the implementation only needs to remember the kind of all declared private names (but not necessarily the kind of accesses - it just need to know that they exist). It does not need to validate the kind of access of this.#a = 1 (a write) against the kind of private name that it resolves to (getter-only) until foo() is executed, and can validate it on-demand.

I suppose this is more about generating/interpreting code lazily, rather than eliminating code that won't be executed.

[gsathya]

You'll still have to lazy parse the function foo to check for early errors of private fields. You will also have to store what kind of private name a is during parsing of the class body. The only thing you're saving is a single branch (to check the type). This branch isn't in generated code or in the builtins, it's in the parser. I think having this one branch is worth it to have better developer experience.

[joyeecheung]

The only thing you're saving is a single branch (to check the type). This branch isn't in generated code or in the builtins, it's in the parser. I think having this one branch is worth it to have better developer experience.

Hm, that's fair. I guess the uncertain bit is still mostly about consistency, then.

If this is an early error, what type of error would this be? It seems a bit odd to make it a SyntaxError, rather than e.g. a TypeError. Can TypeError be early errors?

[gsathya]

@rkirsling just made all early errors be SyntaxErrors so I don't think we'd want to deviate from that.

[caiolima]

I don't have a strong opinion about this, but I think it would be better if this is considered an early error. It is not clear to me the motivation to decide to throw a runtime TypeError instead.

[joyeecheung]

I tweaked with the idea a bit and found a case where we still need to throw a runtime error:

class C {
  set #a(val) {}
  setA(obj, val) { obj.#a = val; }

  constructor() {
    class D {
      get #a() {}
    }
    this.setA(new D(), 1);
  }
}

So the question would be..should we throw early SyntaxError for the ones that can be checked as early as possible and throw runtime TypeError for the ones that can only be checked during runtime?

[joyeecheung]

hm, although in the example above, the error thrown is for the incorrect brand, instead of incorrect kind of the private name.

[caiolima]

hm, although in the example above, the error thrown is for the incorrect brand, instead of incorrect kind of the private name.

I think you can solve this by adding class D extends C.

However, it is not clear to me why should this error happen during runtime. The getter #a defined on D doesn't change anything about the setter #a defined in class C, since they are different PrivateNames.

[joyeecheung]

However, it is not clear to me why should this error happen during runtime. The getter #a defined on D doesn't change anything about the setter #a defined in class C, since they are different PrivateNames.

This can be viewed this way:

class C {
  set #a1(val) {}
  setA(obj, val) { obj.#a1 = val; }

  constructor() {
    class D {
      get #a2() {}
    }
    this.setA(new D(), 1);
  }
}

I guess the #a getter in class D here is just a red herring.

[caiolima]

However, it is not clear to me why should this error happen during runtime. The getter #a defined on D doesn't change anything about the setter #a defined in class C, since they are different PrivateNames.

This can be viewed this way:

class C {
  set #a1(val) {}
  setA(obj, val) { obj.#a1 = val; }

  constructor() {
    class D {
      get #a2() {}
    }
    this.setA(new D(), 1);
  }
}

I guess the #a getter in class D here is just a red herring.

So, in this last case, what's the issue of throwing an early error?

[ljharb]

In that case you wouldn't; because the set matches the setter.

[bakkot]

If we do this I think we should also throw early for assignment to methods, as in:

class A {
  #m(){}
  foo(){
    this.#m = whatever; // should be an Early Error
  }
}

(I'm in favor.)

[caiolima]

In that case you wouldn't; because the set matches the setter.

I think my question was not clear enough. I don't understand how @joyeecheung's example is a blocker to throw early errors.

[joyeecheung]

I don't understand how @joyeecheung's example is a blocker to throw early errors.

I didn’t mean to say it is a blocker, sorry about the confusion. But I suppose if it’s throwing for better developer experience, the engine needs to be clever about the error message so that it’s clear which #a the object is lacking at runtime. (However this is already the case if the wrong access is not an early error since it happens later than the brand checking)

[littledan]

The use of runtime, rather than early, errors for these cases was deliberate in this proposal. My apologies for not documenting this better. My preference is to stick with runtime errors for these sorts of cases, to enable flexibility in the design space for decorators.

When I think about early errors in JavaScript, I think about errors that we can give reliably, under all circumstances. As an example, ES6 removed the strict mode prohibition on duplicate property keys in object literals, because computed property names made that check impossible to do consistently.

We've been careful to not give the decorators proposal the ability to subvert important properties of private names. For example, a decorator cannot introduce a new private name to the class scope (although there were several requests)--the private name scope is purely lexical, and requires the use of a declaration that's lexically present.

However, within the frame of lexical scoping, there's potential flexibility for how private fields and methods are used. The 2018-Jan 2019 decorators proposal included the capability of decorators to turn a private field into a private accessor or private method, or vice versa. This is useful, for example, if you want to observe writes to a private field in a @tracked decorator. You could also picture a @nonwritable decorator applied to a private field. To enable this, the writability of a private field or method is a runtime, not static, attribute.

In this context, it's only possible to do the appropriate checks at early error time if no decorator is used, which would be an unusual discontinuity. The newer "static decorators" proposal does not include these capabilities in decorators. However, I'd like to leave the door open for a potential future built-in decorator to add this capability. Note that, even with static decorators, we still don't know what the decorator does at "early error time", since we don't have access to imports at that time.

[bakkot]

@littledan If the only reason to make these cases runtime errors is for the sake of consistency when decorators are introduced, it seems like we ought to make them early errors now, and relax them to runtime errors as part of decorators. By analogy to the duplicate property name error you mention, I think it was good that the error was early prior to the introduction of computed property names.

[littledan]

@bakkot I've been trying to design decorators together with private fields and methods, to identify cross-cutting concerns, at the request of a number of TC39 delegates. The runtime nature of these errors is one property of a unified design that takes all of these aspects into account. I don't share the view that it makes sense to have a time-limited early error that we will later remove. This will needlessly add to the diversity in observable behavior among JS engines that are trying to be conformant, and result in specification, implementation and testing work that is short-lived. I think the duplicate property name case was quite different, as computed property names were not forseen at that time, and there was a very long gap between those two JavaScript versions. TC39 has long affirmed that it will add decorators to JavaScript--this is what Stage 2 means--so I'd like to proceed with private methods keeping that in mind, when these sorts of cross-cutting issues come up.

[littledan]

I'm concluding "Wont fix" on this, based on the reasoning at https://github.com/tc39/proposal-private-methods/issues/69#issuecomment-532163694 and the fact that no one else raised this in the October 2019 TC39 meeting.