Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Classes are haunted: yield and await probably shouldn't be allowed in field initializers #3333

Open
syg opened this issue May 21, 2024 · 5 comments
Labels

Comments

@syg
Copy link
Contributor

syg commented May 21, 2024

FieldDefinition passes along [?Yield, ?Await], which means the RHS of field initializers inside async functions and generators allow yield and await expressions to be parsed.

Suspending in the middle of construction of a class instance is super cursed!

It is arguably sensible for static initializers, but even that seems gross.

Behavior diverges among browser implementations. JSC parses both yield and await expressions in initializers, and SpiderMonkey and V8 reject yield and await expressions.

$ cat ./test-class-await.js
async function f() {
  class C {
    field = await 42;
  }
}
$ eshost -s ./test-class-await.js 
#### JavaScriptCore


#### SpiderMonkey

SyntaxError: unexpected token: numeric literal:

#### V8

SyntaxError: Unexpected number
$ cat ./test-class-yield.js
function* f() {
  class C {
    field = yield 42;
  }
}
$ eshost -s ./test-class-yield.js 
#### JavaScriptCore


#### SpiderMonkey

SyntaxError: yield is a reserved identifier:

#### V8

SyntaxError: Unexpected strict mode reserved word
$ cat ./test-class-static-await.js
async function f() {
  class C {
    static field = await 42;
  }
}
$ eshost -s ./test-class-static-await.js
#### JavaScriptCore


#### SpiderMonkey

SyntaxError: unexpected token: numeric literal:

#### V8

SyntaxError: Unexpected reserved word
$ cat ./test-class-static-yield.js
function* f() {
  class C {
    static field = yield 42;
  }
}
$ eshost -s ./test-class-static-yield.js
#### JavaScriptCore


#### SpiderMonkey

SyntaxError: yield is a reserved identifier:

#### V8

SyntaxError: Unexpected strict mode reserved word
@syg syg added the spec bug label May 21, 2024
@bakkot
Copy link
Contributor

bakkot commented May 21, 2024

The maximally consistent interpretation is to say that initializer RHSes are basically new function bodies and therefore should unset the yield/await flags, but we could also just unconditionally ban them. We do that for await in static blocks (by unconditionally setting the flags and then having an early error for "contains await").

@bakkot
Copy link
Contributor

bakkot commented May 21, 2024

#2437

@syg
Copy link
Contributor Author

syg commented May 21, 2024

The maximally consistent interpretation is to say that initializer RHSes are basically new function bodies and therefore should unset the yield/await flags, but we could also just unconditionally ban them. We do that for await in static blocks (by unconditionally setting the flags and then having an early error for "contains await").

Agreed. I think these should all be [~Yield, +Await, ~Return] with Early Error ban for await, like static blocks.

@kmiller68
Copy link
Contributor

kmiller68 commented May 21, 2024

While we're at it... can we also ban syntactically

class C {
   #secrets;
   [this.#secrets] = 42;
   static [this.#secrets] = 42;
};

or

class C {
   static #secrets;
   [this.#secrets] = 42;
   static [this.#secrets] = 42;
};

Maybe this should be in a different issue though. Right now I think they're all a runtime error (Although, until recently it was a crash in JSC).

@nicolo-ribaudo
Copy link
Member

dhecht added a commit to dhecht/WebKit that referenced this issue Jul 10, 2024
https://bugs.webkit.org/show_bug.cgi?id=276438
rdar://119044881

Reviewed by NOBODY (OOPS!).

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
webkit-commit-queue pushed a commit to dhecht/WebKit that referenced this issue Jul 10, 2024
https://bugs.webkit.org/show_bug.cgi?id=276438
rdar://119044881

Reviewed by Yusuke Suzuki.

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):

Canonical link: https://commits.webkit.org/280837@main
dhecht added a commit to dhecht/WebKit that referenced this issue Aug 22, 2024
https://bugs.webkit.org/show_bug.cgi?id=276438
rdar://119044881

Reviewed by Yusuke Suzuki.

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):

Canonical link: https://commits.webkit.org/280837@main
dhecht added a commit to dhecht/WebKit that referenced this issue Aug 23, 2024
https://bugs.webkit.org/show_bug.cgi?id=278589
rdar://132338331

Reviewed by NOBODY (OOPS!).

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* JSTests/stress/yield-await-class-field-initializer-expr.js: Added.
(genTestCases):
(genTestScript.append):
(genTestScript):
(expected):
(testNegativeCases):
(testPositiveCases.assertEq):
(testPositiveCases.yieldFn0):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionBody):
(JSC::Parser<LexerType>::parseClass):
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
* Source/JavaScriptCore/parser/Parser.h:
dhecht added a commit to dhecht/WebKit that referenced this issue Aug 25, 2024
https://bugs.webkit.org/show_bug.cgi?id=278589
rdar://132338331

Reviewed by NOBODY (OOPS!).

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* JSTests/stress/yield-await-class-field-initializer-expr.js: Added.
(genTestCases):
(genTestScript.append):
(genTestScript):
(expected):
(testNegativeCases):
(testPositiveCases.assertEq):
(testPositiveCases.yieldFn0):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionBody):
(JSC::Parser<LexerType>::parseClass):
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
* Source/JavaScriptCore/parser/Parser.h:
dhecht added a commit to dhecht/WebKit that referenced this issue Aug 26, 2024
https://bugs.webkit.org/show_bug.cgi?id=278589
rdar://132338331

Reviewed by NOBODY (OOPS!).

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* JSTests/stress/yield-await-class-field-initializer-expr.js: Added.
(genTestCases):
(genTestScript.append):
(genTestScript):
(expected):
(testNegativeCases):
(testPositiveCases.assertEq):
(testPositiveCases.yieldFn0):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionBody):
(JSC::Parser<LexerType>::parseClass):
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
* Source/JavaScriptCore/parser/Parser.h:
dhecht added a commit to dhecht/WebKit that referenced this issue Aug 26, 2024
https://bugs.webkit.org/show_bug.cgi?id=278589
rdar://132338331

Reviewed by NOBODY (OOPS!).

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

However, if the field initializer expression is an async function
expression, then 'await' is allowed within that expression.

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* JSTests/stress/yield-await-class-field-initializer-expr.js: Added.
(genTestCases):
(genTestScript.append):
(genTestScript):
(expected):
(testNegativeCases):
(testPositiveCases.assertEq):
(testPositiveCases.yieldFn0):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionBody):
(JSC::Parser<LexerType>::parseClass):
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
* Source/JavaScriptCore/parser/Parser.h:
webkit-commit-queue pushed a commit to dhecht/WebKit that referenced this issue Aug 27, 2024
https://bugs.webkit.org/show_bug.cgi?id=278589
rdar://132338331

Reviewed by Yusuke Suzuki.

The language spec doesn't explictly disallow yield and await expressions
in class field initializers, however it implicitly does given that
the expression is effectively evaluated as if it's inside a method.
Additionally, the consensus seems to be that these expressions
should not be allowed, see:

tc39/ecma262#3333

The yield and await expressions are now handled differently, but
similar to how they are each handled in other contexts. This also
seems to be the least disruptive way to handle existing scripts,
as well as consistent with other engines:

yield: raise syntax error
await: parse as an identifier

However, if the field initializer expression is an async function
expression, then 'await' is allowed within that expression.

Also adding a new test that generates and verifies scripts containing
a class with a field initializer containing yield and await, where the
class is either not nested or nested inside generator or async functions
to verify the behavior of various combinations.

* JSTests/stress/yield-await-class-field-initializer-expr.js: Added.
(genTestCases):
(genTestScript.append):
(genTestScript):
(expected):
(testNegativeCases):
(testPositiveCases.assertEq):
(testPositiveCases.yieldFn0):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseFunctionBody):
(JSC::Parser<LexerType>::parseClass):
(JSC::Parser<LexerType>::parseYieldExpression):
(JSC::Parser<LexerType>::parseAwaitExpression):
(JSC::Parser<LexerType>::parsePrimaryExpression):
(JSC::Parser<LexerType>::parseUnaryExpression):
* Source/JavaScriptCore/parser/Parser.h:

Canonical link: https://commits.webkit.org/282819@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants