Closed Bug 1345960 Opened 3 years ago Closed 3 years ago

TOK_COMMA should be excluded from possible async method definition

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: arai, Assigned: arai)

References

()

Details

Attachments

(4 files, 1 obsolete file)

code:
  const { async, } = { };

actual result
  syntax error

expected result:
  pass

"," is considered as a part of method definition, and just throws syntax error.

this breaks existing code, per https://discourse.mozilla-community.org/t/anyone-else-has-ff-52-issues/14253


[Tracking Requested - why for this release]: breaks existing valid JS code
Parser::propertyName treats "async" as non-keyword if it won't become an async function syntax and become a valid property syntax.
we should exclude TOK_COMMA there too.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8845553 - Flags: review?(shu)
Comment on attachment 8845553 [details] [diff] [review]
Handle shorthand property and destructuring with async keyword properly.

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

::: js/src/frontend/Parser.cpp
@@ +9304,5 @@
>          TokenKind tt;
>          if (!tokenStream.getToken(&tt))
>              return null();
> +        if (tt != TOK_LP && tt != TOK_COLON && tt != TOK_RC && tt != TOK_ASSIGN &&
> +            tt != TOK_COMMA)

I don't understand why this check is in the negative. That is, looking at the |switch (ltok)| below, we can handle the case of the next token being TOK_NUMBER, TOK_LB, TOK_STRING, or TokenKindIsPossibleIdentifierName(ltok). ISTM we should check those directly.
Attachment #8845553 - Flags: review?(shu)
Comment on attachment 8845553 [details] [diff] [review]
Handle shorthand property and destructuring with async keyword properly.

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

::: js/src/frontend/Parser.cpp
@@ +9310,5 @@
>              isAsync = true;
>              ltok = tt;
>          } else {
>              tokenStream.ungetToken();
>          }

To fix bug 1345968 at the same time for property names, we can do something like:

        if (tt == TOK_NUMBER || tt == TOK_LB || tt == TOK_STRING ||
            TokenKindIsPossibleIdentifierName(tt))
        {
            isAsync = true;
            tokenStream.consumeKnownToken(tt);
            ltok = tt;
        }
Thanks :)
fixed
Attachment #8845553 - Attachment is obsolete: true
Attachment #8845722 - Flags: review?(shu)
Comment on attachment 8845722 [details] [diff] [review]
Handle shorthand property and destructuring with async keyword properly.

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

Great, thanks!
Attachment #8845722 - Flags: review?(shu) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/60728dc4728b6ec0eea3be80ed285f53b67fab61
Bug 1345960 - Handle shorthand property and destructuring with async keyword properly. r=shu
almost same patch is applicable for mozilla-aurora

Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1185106

> [User impact if declined]:
Breaks existing valid JS code
(throws syntax error for unrelated syntax to bug 1185106)

> [Is this code covered by automated tests?]:
Yes

> [Has the fix been verified in Nightly?]:
Not yet

> [Needs manual test from QE? If yes, steps to reproduce]:
No

> [List of other uplifts needed for the feature/fix]:
No

> [Is the change risky?]:
Less risky

> [Why is the change risky/not risky?]:
Allows a syntax that was rejected wrongly,
and it was working with the expected path before the patch.

> [String changes made/needed]:
None
Attachment #8845733 - Flags: review+
Attachment #8845733 - Flags: approval-mozilla-aurora?
There's some difference in beta, because of the change in bug 1336783 (Removes KeywordIsName and adds TokenKindIsPossibleIdentifierName).
can you review that part?
Attachment #8845745 - Flags: review?(shu)
https://hg.mozilla.org/mozilla-central/rev/60728dc4728b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Tracking for all branches based on the comment that this breaks existing valid JS code.
Attachment #8845745 - Flags: review?(shu) → review+
Comment on attachment 8845745 [details] [diff] [review]
(mozilla-beta) Handle shorthand property and destructuring with async keyword properly.

beta needs different patch.
I'll post a patch for release/esr52 that is also a bit different

Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1185106

> [User impact if declined]:
Breaks existing valid JS code
(throws syntax error for unrelated syntax to bug 1185106)

> [Is this code covered by automated tests?]:
Yes

> [Has the fix been verified in Nightly?]:
Yes

> [Needs manual test from QE? If yes, steps to reproduce]:
No

> [List of other uplifts needed for the feature/fix]:
None

> [Is the change risky?]:
Less risky

> [Why is the change risky/not risky?]:
Allows a syntax that was rejected wrongly,
and it was working with the expected path before the patch.

> [String changes made/needed]:
None
Attachment #8845745 - Flags: approval-mozilla-beta?
for release

Approval Request Comment
> [Feature/Bug causing the regression]:
Bug 1185106

> [User impact if declined]:
Breaks existing valid JS code
(throws syntax error for unrelated syntax to bug 1185106)

> [Is this code covered by automated tests?]:
Yes

> [Has the fix been verified in Nightly?]:
Yes

> [Needs manual test from QE? If yes, steps to reproduce]:
No

> [List of other uplifts needed for the feature/fix]:
None

> [Is the change risky?]:
Less risky

> [Why is the change risky/not risky?]:
Allows a syntax that was rejected wrongly,
and it was working with the expected path before the patch.

> [String changes made/needed]:
None

====

for esr52

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
not security, but breaks the web and extensions.

> User impact if declined:
Breaks existing valid JS code.

> Fix Landed on Version:
55

> Risk to taking this patch (and alternatives if risky):
Less risky, this allows a syntax that was rejected wrongly,
and it was working with the expected path before the patch.

> String or UUID changes made by this patch:
None
Attachment #8846231 - Flags: review+
Attachment #8846231 - Flags: approval-mozilla-release?
Attachment #8846231 - Flags: approval-mozilla-esr52?
Comment on attachment 8845733 [details] [diff] [review]
(mozilla-aurora) Handle shorthand property and destructuring with async keyword properly. r=shu

Fix a JS issue. Aurora54+.
Attachment #8845733 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8845745 [details] [diff] [review]
(mozilla-beta) Handle shorthand property and destructuring with async keyword properly.

webcompat fix for beta53
Attachment #8845745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8846231 [details] [diff] [review]
(mozilla-release/mozilla-esr52) Handle shorthand property and destructuring with async keyword properly. r=shu

webcompat fix for release and esr52
Attachment #8846231 - Flags: approval-mozilla-release?
Attachment #8846231 - Flags: approval-mozilla-release+
Attachment #8846231 - Flags: approval-mozilla-esr52?
Attachment #8846231 - Flags: approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/3833e945e3cd (FIREFOX_ESR_52_0_X_RELBRANCH - 52.0.2)
You need to log in before you can comment on or make changes to this bug.