Add tests for "Class Static Init. Blocks" proposal #2968
Conversation
|
@rbuckton I need you to review these. Thanks! |
|
@rbuckton ping |
|
These look great! I just tested in v8/d8 with
|
|
@jugglinmike could you rebase your fixups and push back to the branch? Thanks!! |
This proposal is currently at "stage 3" in TC39's standardization process.
jugglinmike
force-pushed
the
class-static-initialization-blocks
branch
from
6ed4671
to
ff9be07
Compare
What service! Good news, V8 has tickets for these:
Certainly. That's done, and the original version of this branch is available here in case that's interesting to anybody. |
|
@rbuckton mentioned in tc39/proposal-class-static-block#27 (comment) that another way to handle If so, the following examples would throw, they are marked as valid as of ff9be07.
I agree with tc39/proposal-class-static-block#43 (comment), either we come up a formal definition of function boundaries or we do in another way. |
| let test262 = 'first block'; | ||
| } | ||
| static { | ||
| probe = test262; |
There was a problem hiding this comment.
I couldn't figure out how we're using probe - should there be an assertion for ti?
| var test262 = 'first block'; | ||
| } | ||
| static { | ||
| probe = test262; |
There was a problem hiding this comment.
Similar comment to my other comment above - should we be using probe in an assertion?
|
Great catch, @alflennik! Fixed in another fixup commit |
|
I'm confused about some of the For class expression, ClassExpression's BindingIdentifier doesn't have [~Await], why should await be allowed? For arrow parameters, ArrowFunction's ArrowParameters also doesn't have [~Await]. By comparison, FunctionExpression's FormalParameters has [~Await]. |
|
|
||
| class C { | ||
| static { | ||
| (class await {}); |
There was a problem hiding this comment.
This shouldn't be allowed if the intention is that class static blocks are like async function bodies for the purposes of [Await].
async function f() { (class await {}) } doesn't parse.
|
|
||
| class C { | ||
| static { | ||
| (await => 0); |
There was a problem hiding this comment.
Similarly here and in other arrow function tests, await is never allowed as an arrow parameter in async function bodies, so shouldn't here either.
There was a problem hiding this comment.
As mentioned by @syg, await should be disallowed here, per tc39/proposal-class-static-block#46
|
@syg,
Currently, the proposal's restrictions on |
Right enough. The editors would really like the draft spec to be reworked so that the await-as-identifier restrictions are expressed here exactly as they would be for async function bodies so the two align. We can wait for @rbuckton to do that. |
|
@alflennik thanks for taking time to review this! Syntax testing is not for the faint of heart, so I commend your willingness to dive right in to a review. That aside, we (which includes you) would benefit from the champion of this proposal being actively engaged. My understanding is that the proposal itself is now stale (that is, it does not reflect the intended semantics of the feature), which is not good for anyone. I also commend @jugglinmike for sticking to a disciplined approach and only testing the feature as it's currently written, not as its "intended". I'm going to attempt to escalate attention to this PR. |
|
Just want to leave a comment here: SpiderMonkey's prototype implmentation, landed under Bug 1712138 passes all these tests except some of the the await static semantic tests, so we're definitely interested in seeing more clarification around We currently fail: |
|
@mgaudet Thanks for reporting in, that's actually really helpful. I've made several attempts to reach @rbuckton both here and via IRC (before and during the recent TC39 meeting). I'm not sure what to do when a champion refuses to engage with a required step in the process. Can someone else take over the proposal? |
|
I'd like to apologize for the delay on this, I've been out of the office this last week. I'm hoping to get caught up early next week. |
|
I put up a PR for I plan to fully review the test-262 tests and will start working on a PR for ECMA-262 this upcoming week. |
There was a problem hiding this comment.
This looks good, though all of the esid: sec-identifier cases will need to be updated once tc39/proposal-class-static-block#46 has merged. That should help to clarify the intended await behavior.
Also, while I saw tests for SuperProperty and ThisExpression in a static block, I did not see any for accessing the bound name of the class, i.e.:
var value;
class C { static { value = C; } }
// or
export default class C { static { value = C; } }
// or
(class C { static { value = C; } });This is permitted in static fields today (i.e., class C { static x = C; })
| BindingIdentifier : Identifier | ||
|
|
||
| [...] | ||
| - It is a Syntax Error if the code matched by this production is nested, |
There was a problem hiding this comment.
As of tc39/proposal-class-static-block#46, this is changing such that the block is parsed in +Await and we issue an error if ContainsAwait returns true, so this section has been removed.
| // Copyright (C) 2021 the V8 project authors. All rights reserved. | ||
| // This code is governed by the BSD license found in the LICENSE file. | ||
| /*--- | ||
| esid: sec-identifiers |
There was a problem hiding this comment.
As of tc39/proposal-class-static-block#46, there are no longer any changes to sec-identifiers, these tests will likely need to be moved to sec-class-definitions (due to the change to use +Await), or to sec-class-definitions-static-semantics-early-errors (due to the new early error when ContainsAwait is true).
| [...] | ||
|
|
||
| ClassStaticBlockStatementList : | ||
| StatementList[~Yield, ~Await, ~Return]opt |
There was a problem hiding this comment.
Per tc39/proposal-class-static-block#46, this changes to:
| StatementList[~Yield, ~Await, ~Return]opt | |
| StatementList[~Yield, +Await, ~Return]opt |
However AwaitExpression and for await are both early errors in static semantics.
| [...] | ||
|
|
||
| ClassStaticBlockStatementList : | ||
| StatementList[~Yield, ~Await, ~Return]opt |
There was a problem hiding this comment.
| StatementList[~Yield, ~Await, ~Return]opt | |
| StatementList[~Yield, +Await, ~Return]opt |
| } | ||
| } | ||
|
|
||
| assert.sameValue(probe262, 'outer scope'); |
There was a problem hiding this comment.
| assert.sameValue(probe262, 'outer scope'); | |
| assert.sameValue(probe, 'outer scope'); |
|
@rbuckton thanks for the review! |
|
A few updates regarding the proposal:
tc39/ecma262#2440 will be the definitive source for the specification going forward, though I'll try to keep tc39/proposal-class-static-block#47 up to date with any changes for now. |
|
Thanks for those updates, @rbuckton --with other proposals, I've sometimes been confused about where development was happening. Anyway, I'll be updating this patch as per the latest corrections today. |
|
@rbuckton regarding access to the bound names of the class, I decided to express that as a new kind of scope-probing test--one focused on the environment record's derivation (that also highlighted the need for a corresponding test for the variable lexical environment). This patch is ready for another review. |
…k r=arai This has been verified with the current version of tc39/test262#2968, rev a502d69 Differential Revision: https://phabricator.services.mozilla.com/D119793
…k r=arai This has been verified with the current version of tc39/test262#2968, rev a502d69 Differential Revision: https://phabricator.services.mozilla.com/D119793
This proposal is currently at "stage 3" in TC39's standardization
process.