1317153 - Error for incorrect usage of await keyword is not helpful

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Konrad Dzwinel]

Attachment: 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

Comment 1

[Tooru Fujisawa [:arai]]

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

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: 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.

Comment 3

[Tooru Fujisawa [:arai]]

(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

[Till Schneidereit [:till]]

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"

Comment 5

[Tooru Fujisawa [:arai]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be5959030d5a

Comment 7

[Konrad Dzwinel]

Thank you, it will make common async/await errors much easier to debug 

Comment 8

[Konrad Dzwinel]

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?

Comment 9

[Tooru Fujisawa [:arai]]

(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