Bug 201615 - [JSC] Add missing syntax errors for await in function parameter default expressions
Summary: [JSC] Add missing syntax errors for await in function parameter default expre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-09 14:52 PDT by Ross Kirsling
Modified: 2019-09-16 15:57 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.37 KB, patch)
2019-09-09 14:56 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2019-09-11 16:02 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2019-09-16 12:59 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2019-09-16 13:07 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (13.16 KB, patch)
2019-09-16 13:09 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2019-09-09 14:52:36 PDT
Arrow function parameter default expressions should not permit await keyword
Comment 1 Ross Kirsling 2019-09-09 14:56:50 PDT
Created attachment 378411 [details]
Patch
Comment 2 Ross Kirsling 2019-09-10 16:48:06 PDT
Comment on attachment 378411 [details]
Patch

Ugh, guess this solution is too naive since it breaks, e.g., `await => {}` at the top level (which evidently neither we nor test262 have tests for...).

Also the problem description is a bit off.

We need to prohibit await in default expressions of:
- async arrow and non-arrow functions
- non-async arrow functions in an async context

So these need to become errors:
    async function f(x = await => {}) {}
    async (x = await => {}) => {};

    async function f() { (x = await => {}) => {}; }
    async () => { (x = await => {}) => {}; };

And these must not become errors:
    async function f() { function g(x = await => {}) {} }
    (x = await => {}) => {};
Comment 3 Ross Kirsling 2019-09-11 16:02:28 PDT
Created attachment 378591 [details]
Patch
Comment 4 Yusuke Suzuki 2019-09-13 02:15:26 PDT
Comment on attachment 378591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378591&action=review

> Source/JavaScriptCore/parser/Parser.cpp:350
> +        semanticFailIfTrue(!m_parserState.allowAwait && match(AWAIT), "Cannot use 'await' as a parameter name in an async function");

What is the difference between this check and isDisallowedIdentifierAwait?
Comment 5 Ross Kirsling 2019-09-13 11:37:33 PDT
(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 378591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378591&action=review
> 
> > Source/JavaScriptCore/parser/Parser.cpp:350
> > +        semanticFailIfTrue(!m_parserState.allowAwait && match(AWAIT), "Cannot use 'await' as a parameter name in an async function");
> 
> What is the difference between this check and isDisallowedIdentifierAwait?

Hmm, I (In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 378591 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378591&action=review
> 
> > Source/JavaScriptCore/parser/Parser.cpp:350
> > +        semanticFailIfTrue(!m_parserState.allowAwait && match(AWAIT), "Cannot use 'await' as a parameter name in an async function");
> 
> What is the difference between this check and isDisallowedIdentifierAwait?

Hmm, I didn't consciously avoid `isDisallowedIdentifierAwait` here, but the observable difference I can see would be in module code: namely, that `await => 3` in a module would change from `Cannot use 'await' as a parameter name in a module.` to `Cannot use 'await' as a parameter name in an async function.`, which would be incorrect.
Comment 6 Ross Kirsling 2019-09-16 12:59:39 PDT Comment hidden (obsolete)
Comment 7 Ross Kirsling 2019-09-16 13:00:55 PDT
Comment on attachment 378883 [details]
Patch

Added even more tests (for all the things I was implicitly expecting).

Any further concerns here?
Comment 8 Ross Kirsling 2019-09-16 13:07:42 PDT Comment hidden (obsolete)
Comment 9 Ross Kirsling 2019-09-16 13:09:12 PDT
Created attachment 378888 [details]
Patch
Comment 10 WebKit Commit Bot 2019-09-16 15:56:15 PDT
Comment on attachment 378888 [details]
Patch

Clearing flags on attachment: 378888

Committed r249925: <https://trac.webkit.org/changeset/249925>
Comment 11 WebKit Commit Bot 2019-09-16 15:56:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-09-16 15:57:18 PDT
<rdar://problem/55418472>