Error for incorrect usage of await keyword is not helpful

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kdzwinel, Assigned: arai)

Tracking

52 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8810205 [details]
Screen Shot 2016-11-12 at 11.40.15.png

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

a year 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

a year ago
Created attachment 8811581 [details] [diff] [review]
Provide better error message when errornous syntax possibly match "await SOMETHING" outside async function.

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

a year 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 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be5959030d5a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 7

a year ago
Thank you, it will make common async/await errors much easier to debug 
(Reporter)

Comment 8

a year 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

a year 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.