Closed Bug 1315815 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file, 1 obsolete file)

|\u0061sync function f() {}| should be an error.  Likewise for a few other async/await constructs.  They aren't right now.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8808400 - Flags: review?(andrebargull)
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);
```
Attachment #8808400 - Flags: review?(andrebargull) → review+
Attached patch v2Splinter Review
(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.
Attachment #8808439 - Flags: review?(andrebargull)
Attachment #8808400 - Attachment is obsolete: true
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.
Attachment #8808439 - Flags: review?(andrebargull) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/862b57ae9f38
Don't treat async or await as a keyword when they contain escapes.  r=anba
https://hg.mozilla.org/mozilla-central/rev/862b57ae9f38
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.