Closed
Bug 1317153
Opened 8 years ago
Closed 8 years ago
Error for incorrect usage of await keyword is not helpful
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: kdzwinel, Assigned: arai)
Details
Attachments
(2 files)
30.43 KB,
image/png
|
Details | |
17.67 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce: Run the scripts below in the DevTools console: #1 async function test(){ return Promise.resolve(); } await test(); #2 async function test(){ return Promise.resolve(); } (function() { await test(); })() Actual results: There should be an error explaining that `await` cannot be used in the top-level scope (#1) and that the wrapper function needs an 'async' keyword (#2) Expected results: Currently, an error being displayed, for both cases, is 'Syntax Error: missing ";" before statement'. The "Learn more" link that follows the error message, in this case, is not helpful either. Google Chrome shows a different error, but also not a useful one: https://crbug.com/664751
Assignee | ||
Comment 1•8 years ago
|
||
since "await" is not keyword outside async function, the situation is almost same as foo test(); so, the error is that "test" appears where operator or semicolon or something was expected. maybe we could detect the case that the statement starts from await, or previous token was await, but I'm not sure how widely it can cover...
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Assignee | ||
Comment 2•8 years ago
|
||
When await expression is placed outside of async function, "await" is parsed as an identifier and parser tries to insert semicolon after that. So, added the code that checks current token to MatchOrInsertSemicolonHelper. Then, to check Parser.pc, changed the following functions to Parser method: * MatchOrInsertSemicolonHelper * MatchOrInsertSemicolonAfterExpression * MatchOrInsertSemicolonAfterNonExpression Additional code is placed only on error path, so this won't harm normal execution.
Assignee: nobody → arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8811581 -
Flags: review?(till)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #2) > When await expression is placed outside of async function, > "await" is parsed as an identifier and parser tries to insert semicolon > after that. to be clear, this happens only if the next token doesn't match to any other syntax. so, the following cases will work: await new Promise(...); await f(); await promise; but the following cases won't work: await +10; await [10]; await new Promise(...); of course, await expression should be used mostly for promise, and the former cases should cover almost cases. the remaining cases is the last example above. if there's newline before operand, it's not a syntax error, and there's almost nothing we can do in parser. (maybe we could add some hint when throwing ReferenceError for await tho...)
Comment 4•8 years ago
|
||
Comment on attachment 8811581 [details] [diff] [review] Provide better error message when errornous syntax possibly match "await SOMETHING" outside async function. Review of attachment 8811581 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, much better. ::: js/src/frontend/Parser.cpp @@ +2621,5 @@ > return false; > if (tt != TOK_EOF && tt != TOK_EOL && tt != TOK_SEMI && tt != TOK_RC) { > + /* > + * When current token is `await` and it's outside of async function, > + * it's possibly intended to be an await expression. Can you expand on this comment to explain why we have to handle this in a special way? Something like "Detect this situation and throw an understandable error. Otherwise we'd throw a confusing "missing ; before statement" error." ::: js/src/js.msg @@ +185,5 @@ > MSG_DEF(JSMSG_AS_AFTER_IMPORT_STAR, 0, JSEXN_SYNTAXERR, "missing keyword 'as' after import *") > MSG_DEF(JSMSG_AS_AFTER_RESERVED_WORD, 1, JSEXN_SYNTAXERR, "missing keyword 'as' after reserved word '{0}'") > MSG_DEF(JSMSG_ASYNC_GENERATOR, 0, JSEXN_SYNTAXERR, "generator function or method can't be async") > MSG_DEF(JSMSG_AWAIT_IN_DEFAULT, 0, JSEXN_SYNTAXERR, "await can't be used in default expression") > +MSG_DEF(JSMSG_AWAIT_OUTSIDE_ASYNC, 0, JSEXN_SYNTAXERR, "await can't be used outside async function") I think this would be slightly better if it were aligned with JSMSG_BAD_SUPERCALL: "await is only valid in async functions"
Attachment #8811581 -
Flags: review?(till) → review+
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be5959030d5a4fd3d37d68091d5b68362704c3bf Bug 1317153 - Provide better error message when errornous syntax possibly match "await SOMETHING" outside async function. r=till
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be5959030d5a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter | ||
Comment 7•8 years ago
|
||
Thank you, it will make common async/await errors much easier to debug
Reporter | ||
Comment 8•8 years ago
|
||
Hm, my previous comment was truncated because I used an emoji (even though the preview was OK). Anyway, I wanted to ask about the `[Learn More]` link in the DevTools console. How can we make sure that this new error message links to the article on MDN? Should I report another issue for that?
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to kdzwinel from comment #8) > Hm, my previous comment was truncated because I used an emoji (even though > the preview was OK). > > Anyway, I wanted to ask about the `[Learn More]` link in the DevTools > console. How can we make sure that this new error message links to the > article on MDN? Should I report another issue for that? Yes, to link to MDN documentation, an entry should be added to the following file: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/errordocs.js
You need to log in
before you can comment on or make changes to this bug.
Description
•