Stage 3 Editor Specification Review

[rbuckton]

This is a placeholder task for Stage 3 Specification Review feedback from an ECMAScript Editor.

[ljharb]

just a few early thoughts:

• https://tc39.es/proposal-class-static-block/#table-class-static-block's title is wrong
• https://tc39.es/proposal-class-static-block/#table-class-static-block's [[ThisBindingStatus]] slot should have spec enums, not strings (just like https://tc39.es/ecma262/#table-additional-fields-of-function-environment-records)
• Why is IsStatic false on a class static block? it seems like it'd never be called, but i'm not 100% sure.

[ljharb]

ping @rbuckton there's still a few editorial issues/questions ^

[rbuckton]

Thanks, I'll take a look today.

[rbuckton]

• https://tc39.es/proposal-class-static-block/#table-class-static-block's title is wrong
• https://tc39.es/proposal-class-static-block/#table-class-static-block's [[ThisBindingStatus]] slot should have spec enums, not strings (just like https://tc39.es/ecma262/#table-additional-fields-of-function-environment-records)

I'll have a PR up shortly for these.

• Why is IsStatic false on a class static block? it seems like it'd never be called, but i'm not 100% sure.

I think you are correct. We invoke IsStatic in two places:

• ClassDefinitionEvaluation, but only on the set of methods returned from NonConstructorMethodDefinitions (which will not include static blocks).
• PrototypePropertyNameList, which is evaluated unconditionally against all class elements, but should short-circuit before IsStatic because PropName of a static block will be ~empty~.

I can remove it since it isn't used.

[rbuckton]

You can find the PR for this feedback in #31.

[syg]

• The text for Table 1 reads weird since they're all talking about "this function".
• In https://tc39.es/proposal-class-static-block/#sec-runtime-semantics-classdefinitionevaluation, can step 33 be an assert, since there's already an Early Error covering multiple static blocks?
• Where is NewClassStaticBlockEnvironment?
• F instead of the block is passed to ClassStaticBlockEvaluateBody(F).
• I would prefer a note in https://tc39.es/proposal-class-static-block/#classstaticblockdeclarationinstantiation that says only a single Environment Record is needed for both vars and lexical bindings.

[rbuckton]

• In https://tc39.es/proposal-class-static-block/#sec-runtime-semantics-classdefinitionevaluation, can step 33 be an assert, since there's already an Early Error covering multiple static blocks?

How would you word this assert? Essentially, the list contains zero or one elements. If I have an Assert, I still need the If (even if worded differently):

1. <ins>Assert: _staticBlocks_ is an empty list or contains a single element.</ins>
2. <ins>If _staticBlocks_ is not an empty list, then</ins>
  ...

• Where is NewClassStaticBlockEnvironment?

An oversight, I'll add it shortly.

• F instead of the block is passed to ClassStaticBlockEvaluateBody(F).

Good catch, thanks.

[rbuckton]

• I would prefer a note in https://tc39.es/proposal-class-static-block/#classstaticblockdeclarationinstantiation that says only a single Environment Record is needed for both vars and lexical bindings.

Are you describing a note like the one at the top of this section: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation, or a different note entirely?

[rbuckton]

I think perhaps you are describing something like this note in FunctionDeclarationInstantiation?

As I understand it the rationale behind the extra environment and NOTE in that section has to do with the possibility of parameter initializers performing direct eval that might introduce new bindings. I can add the note, but there are no parameters to a static {} block so there's no opportunity for direct eval to occur within that scope before evaluating the body.

[rbuckton]

@syg - I believe I have addressed your feedback. Please let me know if there's anything I have missed or misunderstood.

[syg]

I can add the note, but there are no parameters to a static {} block so there's no opportunity for direct eval to occur within that scope before evaluating the body.

Fair enough, I retract that suggestion.

I believe I have addressed your feedback. Please let me know if there's anything I have missed or misunderstood.

Where are the changes? I don't see an updated render nor commits.

[rbuckton]

See #31. I've addressed both your and @ljharb's feedback in that PR.

[rbuckton]

Closing this per #31. Feel free to comment/reopen if you feel there is additional feedback you feel needs to be addressed prior to possible Stage 3 advancement. Thank you both @ljharb and @syg for your reviews.

[bakkot]

Couple last things:

• The Early Error forbidding await identifiers says "not crossing function or static initialization block boundaries, excluding arrow function boundaries" [emphasis added]. By my reading, that would mean class { static { let x = async () => { await foo(); } } would be illegal, which sounds wrong. I think you want to include arrow function boundaries for this particular error. (Although that's kind of tricky, because of the cover grammar for arrow parameters - should (await) => 0 be legal inside a class static block?)
• There's a handful of other Early Errors I believe you need.
  • The way I've written those errors, label: { class A { static { label: {} } } } would be legal. (Note the duplicate label.) I assume that's the intent, since it matches how functions work. If you wanted it to be illegal, you'd need other changes.

[rbuckton]

• The Early Error forbidding await identifiers says "not crossing function or static initialization block boundaries, excluding arrow function boundaries" [emphasis added]. By my reading, that would mean class { static { let x = async () => { await foo(); } } would be illegal, which sounds wrong. I think you want to include arrow function boundaries for this particular error. (Although that's kind of tricky, because of the cover grammar for arrow parameters - should (await) => 0 be legal inside a class static block?)

Agreed, this is tricky to word correctly. The other option might be to parse with [+Await] and forbid AwaitExpression and for-await-of the way that ContinueStatement and BreakStatement are written, but if we add any other [+Await]-specific features in the future we'd be chasing those down.

• There's a handful of other Early Errors I believe you need.

I plan to look at these today, thanks!

[ptomato]

• Why is IsStatic false on a class static block? it seems like it'd never be called, but i'm not 100% sure.

I think you are correct. We invoke IsStatic in two places:

* `ClassDefinitionEvaluation`, but only on the set of methods returned from `NonConstructorMethodDefinitions` (which will not include `static` blocks).

* `PrototypePropertyNameList`, which is evaluated unconditionally against all class elements, but should short-circuit before `IsStatic` because `PropName` of a static block will be `~empty~`.

I can remove it since it isn't used.

I agree with this, but when I was reading the spec text I got confused by ClassStaticBlock not being mentioned in IsStatic until I found this discussion. Maybe add a note similar to the one in "Static Semantics: Contains" saying something like "Static semantic rules using this operation do not consider static initialization blocks" or something like that?

[rbuckton]

I agree with this, but when I was reading the spec text I got confused by ClassStaticBlock not being mentioned in IsStatic until I found this discussion. Maybe add a note similar to the one in "Static Semantics: Contains" saying something like "Static semantic rules using this operation do not consider static initialization blocks" or something like that?

I'll defer to @ljharb on whether to reintroduce it with that wording since it was his recommendation to remove it.

[rbuckton]

The reason I had it there in the first place is that we go out of our way to indicate ClassElement: `;` returns false for IsStatic, despite it also never being invoked for that production.

[ljharb]

I was fine with removing it if unused, but mainly i was confused to see IsStatic not be “true” for something named “static”.