Stage 3 Specification Review: Philip Chimento

[rbuckton]

This is a placeholder task for Stage 3 Specification Review feedback from Philip Chimento (@ptomato), per https://github.com/tc39/notes/blob/master/meetings/2020-09/sept-22.md#class-static-initialization-block-for-stage-2

[rbuckton]

@ptomato: have you had an opportunity to review the specification text?

[ptomato]

I intend to do it today and Monday 😄

[ptomato]

I've completed my review today, I had the advantage of going last after others had gone over the spec text before me 😄

Here are my comments, all of them quite minor:

• It's not clear to me where this paragraph is intended to be inserted into the spec.
• Is this missing a description? https://github.com/tc39/proposal-class-static-block/blob/master/spec/sec-executable-code-and-execution-contexts-patch.html#L39
• It is a Syntax Error if the code matched by this production is nested, directly or indirectly (but not crossing function or static initialization block boundaries, excluding arrow function boundaries), within a ClassStaticBlock and the StringValue of Identifier is "await". - agree with @bakkot's question about the exclusion of arrow function boundaries. But also, is it necessary to explicitly specify static initialization block boundaries here? If an await identifier is contained within another nested class static initialization block, it's still an error nonetheless, but this way of wording it could imply that it isn't an error.
• HasDirectSuper is not cross-referenced, is it missing an entry in the bibliography? Some other operations are missing cross-references as well.
• PropName should be under "Syntax-Directed Operations" → "Miscellaneous", not "ECMAScript Language: Functions and Classes" → "Method Definitions"?
• Likewise, LexicallyDeclaredNames, VarDeclaredNames, and VarScopedDeclarations should be under "Syntax-Directed Operations" → "Scope Analysis"?
• Maybe add for clarity to https://github.com/tc39/proposal-class-static-block/blob/master/spec/sec-class-definitions-patch.html#L212 "Assert: staticBlocks contains zero or one element"
• PrivateNameEnvironment seems to have been renamed to PrivateEnvironment in the class fields proposal, rename it throughout, and consider adding it to biblio.json to cross-reference it for convenience?

[bakkot]

• HasDirectSuper is not cross-referenced, is it missing an entry in the bibliography? Some other operations are missing cross-references as well.

Probably, but it's not this project's fault; the biblio is quite stale. It will link properly in the actual spec when this is upstreamed.

• PropName should be under "Syntax-Directed Operations" → "Miscellaneous", not "ECMAScript Language: Functions and Classes" → "Method Definitions"?
• Likewise, LexicallyDeclaredNames, VarDeclaredNames, and VarScopedDeclarations should be under "Syntax-Directed Operations" → "Scope Analysis"?

Yeah, sorry, that changed literally yesterday. I wouldn't worry about fixing up this proposal; I'll make sure it's right as part of the upstream PR.

[rbuckton]

• It's not clear to me where this paragraph is intended to be inserted into the spec.

I'll have to look into that. I believe the spec text in that section has changed since I first wrote the spec text for the proposal. I believe its intended to go between "function" and "module" environment records here:

• Is this missing a description? https://github.com/tc39/proposal-class-static-block/blob/master/spec/sec-executable-code-and-execution-contexts-patch.html#L39

Not precisely. The class static block environment record is modeled after a function environment record, which only includes a note on what "lexical" means:

• It is a Syntax Error if the code matched by this production is nested, directly or indirectly (but not crossing function or static initialization block boundaries, excluding arrow function boundaries), within a ClassStaticBlock and the StringValue of Identifier is "await". - agree with @bakkot's question about the exclusion of arrow function boundaries. But also, is it necessary to explicitly specify static initialization block boundaries here? If an await identifier is contained within another nested class static initialization block, it's still an error nonetheless, but this way of wording it could imply that it isn't an error.

The wording is based on the same wording we use for break and continue in illegal places. The reason for the wording is that the Static Semantic rule applies to any BindingIdentifier : Identifier, and indicates that await as an identifier is illegal inside of a static {} block, but not illegal inside of a function in a static block:

function f() { // function boundary
  let await; // legal here
  class C {
    static { // static block boundary, do not cross
      let await; // illegal here, directly nested
      {
        let await; // illegal here, indirectly nested
      }
      function g() { // function boundary, do not cross
        let await; // legal here
      }
      class D {
        static { // static block boundary, do not cross
          let await; // illegal here, directly nested
        }
      }
    }
  }
}

• HasDirectSuper is not cross-referenced, is it missing an entry in the bibliography? Some other operations are missing cross-references as well.

IIRC, cross references are only defined when an algorithm as an aoid, but HasDirectSuper is not defined with one in the ECMA262 spec. It looks like a new feature was added for ecmarkup called <emu-see-also-para />, so I may need to update the copy of ecmarkup I'm using. Its also unclear how something like HasDirectSuper should cross-reference when it exists both in the core spec and in a proposal.

• PrivateNameEnvironment seems to have been renamed to PrivateEnvironment in the class fields proposal, rename it throughout, and consider adding it to biblio.json to cross-reference it for convenience?

I actually fixed that a few hours ago, you can see it updated in the current version of the proposal spec text: https://tc39.es/proposal-class-static-block/#sec-runtime-semantics-classdefinitionevaluation. I was planning on rebasing that algorithm on the "class static features" proposal (which is supposed to be a diff from the class fields proposal), but I believe the spec text in that proposal is stale or out of sync.

[bakkot]

IIRC, cross references are only defined when an algorithm as an aoid, but HasDirectSuper is not defined with one in the ECMA262 spec

That was true up until last night, but it does now! (To be clear, I'm mentioning this because I'm excited about the change in ecma262, not because I think this proposal needs to do anything about it.)

It looks like a new feature was added for ecmarkup called <emu-see-also-para />

AFAICT ecmarkup never actually had support for that. ecma262 contained such elements, but they didn't do anything, and I got rid of all of them in the above-mentioned PR.

[rbuckton]

That was true up until last night, but it does now! (To be clear, I'm mentioning this because I'm excited about the change in ecma262, not because I think this proposal needs to do anything about it.)

How does this affect the readability of the spec? Previously the operations were adjacent to their syntax definitions in the spec. It seems like it would make it harder to follow. Previously I would have looked for how identifiers are handled under https://tc39.es/ecma262/#sec-identifiers, but now they're moved to another part of the spec entirely (with no, "this can be found here"-like mention, kind of like https://tc39.es/ecma262/#sec-constructor-properties-of-the-global-object). I'm not entirely sure I'm a fan of that change, to be honest.

[rbuckton]

For ecmarkup, I would been happy with multiple algorithms having the same AOID show up as a list of references when you click the link (like we do for the "References" link when hovering over a definition). Even if the spec has been reorganized to avoid this, I think it still might be valuable. Regardless, both this and my last comment are off topic for this discussion.

[bakkot]

How does this affect the readability of the spec?

I think it's a massive improvement. Having the definition for each SDO in a single place makes it much easier to see what they do, and matches the conceptual model: generally the question one has is "what does this SDO do?", not "what are all of the various ways that SDOs are defined over this syntactic form?". We've described this planned change at each of the last four meetings, as well as in an issue which has been open since April, to exclusively positive feedback, so I think that's the common sentiment.

Previously I would have looked for how identifiers are handled under https://tc39.es/ecma262/#sec-identifiers

Yeah, we intend to have links from the definition to all of the SDOs defined over it. The work in ecmarkup is done, but it's blocked by a PR to grammarkdown.

Anyway, happy to continue this conversation in tc39/ecma262#1950 or in a new issue.

[rbuckton]

I'll look at the grammarkdown PR. I don't recall seeing it in my notifications, but its been a busy two months.

[rbuckton]

@ptomato I addressed your first comment (about the dangling paragaph) in #34. Please let me know if my explanations for the other comments are sufficient, or if you feel there is more that should be updated.

[ptomato]

Thanks! The #34 PR looks like it ought to clear things up for me. I think the only unresolved point is this:

• It is a Syntax Error if the code matched by this production is nested, directly or indirectly (but not crossing function or static initialization block boundaries, excluding arrow function boundaries), within a ClassStaticBlock and the StringValue of Identifier is "await". - agree with @bakkot's question about the exclusion of arrow function boundaries. But also, is it necessary to explicitly specify static initialization block boundaries here? If an await identifier is contained within another nested class static initialization block, it's still an error nonetheless, but this way of wording it could imply that it isn't an error.

The wording is based on the same wording we use for break and continue in illegal places. The reason for the wording is that the Static Semantic rule applies to any BindingIdentifier : Identifier, and indicates that await as an identifier is illegal inside of a static {} block, but not illegal inside of a function in a static block:

function f() { // function boundary
  let await; // legal here
  class C {
    static { // static block boundary, do not cross
      let await; // illegal here, directly nested
      {
        let await; // illegal here, indirectly nested
      }
      function g() { // function boundary, do not cross
        let await; // legal here
      }
      class D {
        static { // static block boundary, do not cross
          let await; // illegal here, directly nested
        }
      }
    }
  }
}

What I meant was that the let await; inside the static block of D is illegal whether you explicitly specify "without crossing static initialization block boundaries" or not. As far as I could see, that language doesn't make anything else illegal that wasn't already illegal without it, so it's unnecessary. (I agree that's not the case for break and continue.) If you think it's clearer to keep it, then I'll defer to your judgement.

[rbuckton]

Since (hopefully) the intent is clear, its more of an editorial discussion that (also hopefully) shouldn't be a stage 3 blocker.

[rbuckton]

@ptomato - Thanks for your review!