1315815 - Don't treat async or await as a keyword when they contain escapes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

|\u0061sync function f() {}| should be an error.  Likewise for a few other async/await constructs.  They aren't right now.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[André Bargull [:anba]]

Comment on attachment 8808400 [details] [diff] [review]
Patch

Review of attachment 8808400 [details] [diff] [review]:
-----------------------------------------------------------------

`\u0061sync()=>{}` doesn't report a syntax error. Apart from that r=me.


I don't think it's important to change {Full,Syntax}ParseHandler::nameIsUnparenthesizedAsync() to check for escape sequences, because being able to differentiate between "async" and "\u0061sync" only changes the error message in some cases, it's not required for spec compliance.

For example, it would change the error message/location in:
```
js> \u0061sync ({a=0}, delete) => {}
typein:1:25 SyntaxError: expected expression, got ')':
typein:1:25 \u0061sync ({a=0}, delete) => {}
typein:1:25 .........................^
```

To be the same as in:
```
js> \u0061ync ({a=0}, delete) => {}  
typein:2:13 SyntaxError: missing : after property id:
typein:2:13 \u0061ync ({a=0}, delete) => {}
typein:2:13 .............^
```

::: js/src/frontend/Parser.cpp
@@ +7579,5 @@
>      bool maybeAsyncArrow = false;
> +    if (tt == TOK_NAME &&
> +        tokenStream.currentName() == context->names().async &&
> +        !tokenStream.currentToken().nameContainsEscape())
> +    {

Also add `MOZ_ASSERT(!tokenStream.currentToken().nameContainsEscape())` after `MOZ_ASSERT(tokenStream.currentName() == context->names().async);` in the maybeAsyncArrow if-block.

::: js/src/tests/ecma_7/AsyncFunctions/async-contains-unicode-escape.js
@@ +1,1 @@
> +var BUGNUMBER = 9999999;

s/9999999/1315815/

@@ +2,5 @@
> +var summary = "async/await syntax";
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +function test(code, eval)

Nit: Maybe rename "eval" to "indirectEval".

@@ +16,5 @@
> +test("var x = ### function f() {}", eval);
> +test("### x => {};", eval);
> +test("var x = ### x => {}", eval);
> +test("({ ### x() {} })", eval);
> +test("var x = ### function f() {}", eval);

Can you also add tests where it doesn't matter if or if not escapes are used.

For example:
```
var async = 0;
assertEq(\u0061sync, 0);
```

And:
```
var obj = {\u0061sync() { return 1; }};
assertEq(obj.async(), 1);
```

Comment 3

[Jeff Walden [:Waldo]]

Attachment: v2

(In reply to André Bargull from comment #2)
> `\u0061sync()=>{}` doesn't report a syntax error.

Fixt.

> I don't think it's important to change
> {Full,Syntax}ParseHandler::nameIsUnparenthesizedAsync() to check for escape
> sequences, because being able to differentiate between "async" and
> "\u0061sync" only changes the error message in some cases, it's not required
> for spec compliance.

Possibly.  But it's more faithful to the grammar, and it's more consistent with the requirement that keywords/bolded terms in the spec not contain escapes.  This version of the patch renames and reorganizes a little bit of this so that detecting async is more clearly a standalone operation for the exact purpose of detecting "async(" and that only.

> For example, it would change the error message/location in:

This is enough in the weeds I'm not even considering the effect on error messages, only on readability of the code implementing the spec.

> Nit: Maybe rename "eval" to "indirectEval".

Added a comment to explain why not.

> Can you also add tests where it doesn't matter if or if not escapes are used.

Done.  If you want more, say so.

Comment 4

[André Bargull [:anba]]

Comment on attachment 8808439 [details] [diff] [review]
v2

Review of attachment 8808439 [details] [diff] [review]:
-----------------------------------------------------------------

Still good! :-)

::: js/src/frontend/FullParseHandler.h
@@ +9,5 @@
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/PodOperations.h"
>  
> +#include <string.h>

Also add #include to SyntaxParseHandler.h for consistency?

@@ +875,5 @@
>  
> +    bool isAsyncKeyword(ParseNode* node, ExclusiveContext* cx) {
> +        return node->isKind(PNK_NAME) &&
> +               node->pn_pos.begin + strlen("async") == node->pn_pos.end &&
> +               node->pn_atom == cx->names().async;

Maybe use the same order when checking for unescaped async in FullParseHandler and SyntaxParseHandler. But up to you, I don't mind keeping it as is.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/862b57ae9f38
Don't treat async or await as a keyword when they contain escapes.  r=anba

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/862b57ae9f38