1041426 - Syntax errors in function call parameters unclear

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sebastian Zartner [:sebo]]

Attachment: Test case

When the syntax of parameters within a function call is incorrect, the provided error message is pretty unclear what's wrong. It just says:

SyntaxError: syntax error

See the attached test case for an example.

The message should clarify what's actually wrong with this construct. It should tell the user that the token 'if' is unexpected.

See this related Firebug discussion group thread:

https://groups.google.com/d/topic/firebug/VlPNkegOB8M/discussion

Sebastian

Comment 1

[Sebastian Zartner [:sebo]]

Attachment: Test case

Comment 2

[j.j.]

Result in other browsers:

Opera 12
Syntax error at line 10 while loading: expected expression, got keyword 'if'

Safari
SyntaxError: Unexpected token 'if'

Chrome
Uncaught SyntaxError: Unexpected token if

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Make error message for unexpected token more clear.

With this patch, Parser generates error message just like Opera 12 does (in comment #2), such as following:

  SyntaxError: expected expression, got keyword 'if'
  SyntaxError: expected expression, got token ';'
  SyntaxError: expected EOF, got token '}'

in following conditions (where "syntax error" was thrown before, except template literal, which is fixed in patch Part 2):

  1. wrong token at primary expression (including the case in comment #1)
  2. EOF, EOL, ';' or ')' after 'throw'
  3. non-EOF after function body
  4. non-EOF after entire source (?)

Then, I have a question about 4th case. When the condition `if (!tokenStream.matchToken(TOK_EOF))` in `Parser<ParseHandler>::parse(JSObject *chain)` match? I'd like to make a testcase for it.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=99cc222af86f

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Make error message for syntax error in template literal more clear.

In addition to Part 1, here is a fix for template literal.

JSMSG_SYNTAX_ERROR is no more used, and removed.
If it's better to leave it for future use, please tell me.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8463155 [details] [diff] [review]
Part 1: Make error message for unexpected token more clear.

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

Sorry for the delay here.  Overall this looks good, but I'd like a little extra polish before declaring go on it.

::: js/src/frontend/Parser.cpp
@@ +70,5 @@
>  
>  static const unsigned BlockIdLimit = 1 << ParseNode::NumBlockIdBits;
>  
> +const char *
> +TokenKindToDesc(TokenKind tt) {

Let's run with this to get this in the tree, but we should add a higher-order macro for tokens, like we have for ParseNodeKinds, so we don't have this separate-location stuff here.  Mind filing the followup bug and maybe fixing it?

@@ +72,5 @@
>  
> +const char *
> +TokenKindToDesc(TokenKind tt) {
> +    switch (tt) {
> +      case TOK_ERROR:           return "error token";

Please assert we never hit this.  TOK_ERROR is really a sentinel to indicate an error state, not something to inquire into.  :-)  If someone's passing in TOK_ERROR (which doesn't happen after the changes I request), it's a bug.

@@ +73,5 @@
> +const char *
> +TokenKindToDesc(TokenKind tt) {
> +    switch (tt) {
> +      case TOK_ERROR:           return "error token";
> +      case TOK_EOF:             return "EOF";

Mild preference for "end of file" to be more readable.

@@ +75,5 @@
> +    switch (tt) {
> +      case TOK_ERROR:           return "error token";
> +      case TOK_EOF:             return "EOF";
> +      case TOK_EOL:             return "line terminator";
> +      case TOK_SEMI:            return "token ';'";

"token" is compiler-speak (notwithstanding that I expect many readers would understand it, or at least gloss over it), so I'd remove it from all of these.

@@ +749,1 @@
>              return null();

This error message isn't quite accurate, see the comment on the one test.  Rather than double down on JSMSG_UNEXPECTED_TOKEN, could we add another error specifically for when we get trailing garbage where "EOF" (really EOF or statement) was expected?  Something like this, maybe:

MSG_DEF(JSMSG_GARBAGE_AFTER_INPUT, ###, 2, JSEXN_SYNTAXERR, "unexpected garbage after {0}, starting with {1}")

with "function body" passed in for the first parameter here.

@@ +1020,1 @@
>          return null();

Same JSMSG_GARBAGE_AFTER_INPUT comment applies here.

@@ +5201,5 @@
>      if (tt == TOK_ERROR)
>          return null();
>      if (tt == TOK_EOF || tt == TOK_EOL || tt == TOK_SEMI || tt == TOK_RC) {
> +        report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "expression",
> +               TokenKindToDesc(tt));

So this is technically all fine as it goes, but we could/should make this a little nicer.  The throw<EOF>, throw;, and throw} cases are one sort of error: missing a thrown value entirely.  The throw<EOL> case is *potentially* the same thing, but it's more likely that someone didn't know line breaks weren't permitted between throw and expression.

So for the first three cases, we should add an error like "throw statement is missing an expression".  And for the last case, maybe "no line break is allowed between 'throw' and its expression"?

@@ +7588,5 @@
>              // It doesn't matter what; when we reach the =>, we will rewind and
>              // reparse the whole arrow function. See Parser::assignExpr.
>              return handler.newNullLiteral(pos());
>          }
> +        goto unexpected_token;

In this case (unlike the next one), because only one token of lookahead is needed for sanity, "expected expression, got token ')'" is a totally reasonable error message, no changes needed.

@@ +7595,5 @@
>          // Not valid expression syntax, but this is valid in an arrow function
>          // with a rest param: `(a, b, ...rest) => body`.
>          if (tokenStream.matchToken(TOK_NAME) &&
>              tokenStream.matchToken(TOK_RP) &&
> +            tokenStream.peekToken() == TOK_ARROW) {

This (the old code too) would double-report, or erase the better error message, if an error is encountered in prior code when reached via the goto.  Consider parsing something like |(...@|, say.  The resulting error isn't in the ... but in the @.  But we don't point at the @ because of this error-overwriting problem.

Considering this triple-dot code more closely, it's clear the error messages generated -- even with this patch -- are not quite right.  For example (eyeballing), I think you'd get this behavior with your patch:

js> (a, ...b);
typein:1:8 SyntaxError: expected expression, got token ')':
typein:1:8 (a, ...b);
typein:1:8 ........^

One, we don't expect an expression.  Two, we haven't hit an error where we're pointing!  (Or fudged it with a vaguer error message.)  This is nonsense.  :-)  Let's expand/rewrite the whole thing to be clearer and more targeted in errors:

      case TOK_TRIPLEDOT: {
        TokenKind next;

        // This isn't valid expression syntax, but it's valid in an arrow
        // function as a trailing rest param: `(a, b, ...rest) => body`.  Check
        // for a name, closing parenthesis, and arrow, and allow it only if all
        // are present.
        next = tokenStream.getToken();
        if (next == TOK_ERROR)
            return null();
        if (next != TOK_NAME) {
            report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "rest argument name",
                   TokenKindToDesc(next));
            return null();
        }

        next = tokenStream.getToken();
        if (next == TOK_ERROR)
            return null();
        if (next != TOK_RP) {
            report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "closing parenthesis",
                   TokenKindToDesc(next));
            return null();
        }

        next = tokenStream.peekToken();
        if (next != TOK_ARROW) {
            report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN,
                   "token '=>' after argument list",
                   TokenKindToDesc(next));
            return null();
        }

        tokenStream.ungetToken();  // put back right paren

        // Return an arbitrary expression node. See case TOK_RP above.
        return handler.newNullLiteral(pos());
      }

Even this is problematic in that two-pass arrow-function parsing means we don't even know that we're actually parsing an arrow function's arguments, versus just an expression, as in |var x = ...y);|.  But such code's off in the weeds, so as long as our messages make sense assuming we actually *are* in arrow-like code, I guess it doesn't really matter.

It'd be nice to have tests for these more-precise error messages, when those changes have been made.

@@ -7478,5 @@
>          // with a rest param: `(a, b, ...rest) => body`.
>          if (tokenStream.matchToken(TOK_NAME) &&
>              tokenStream.matchToken(TOK_RP) &&
> -            tokenStream.peekToken() == TOK_ARROW)
> -        {

The old whitespace here was correct -- we don't allow condition to blend into if-body, as occurs when the brace is at the end of the line, because it severely hinders readability.

@@ +7612,1 @@
>        default:

Minor nit, but could you switch these lines so that the the switch/case/default structure is simpler (that is, switch body directly contains only "case"/"default" "blocks")?  I know C+ lets us put cases and defaults anywhere within that we want, but I don't think it helps to have to think about that complexity when we don't have to.

::: js/src/jit-test/tests/basic/syntax-error-function-body-eof.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  new Function("switch (x) {} }");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: expected EOF, got token '}'") == -1, false);

Hmm.  "expected EOF" is overly strong -- we're really okay with either a statement or EOF.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> Then, I have a question about 4th case. When the condition `if
> (!tokenStream.matchToken(TOK_EOF))` in `Parser<ParseHandler>::parse(JSObject
> *chain)` match? I'd like to make a testcase for it.

I think an unadorned } will do the trick.

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Let's run with this to get this in the tree, but we should add a
> higher-order macro for tokens, like we have for ParseNodeKinds, so we don't
> have this separate-location stuff here.  Mind filing the followup bug and
> maybe fixing it?

Filed as bug 1053240.
I'll post a patch for it in a few days.

> @@ +749,1 @@
> >              return null();
> 
> This error message isn't quite accurate, see the comment on the one test. 
> Rather than double down on JSMSG_UNEXPECTED_TOKEN, could we add another
> error specifically for when we get trailing garbage where "EOF" (really EOF
> or statement) was expected?  Something like this, maybe:
> 
> MSG_DEF(JSMSG_GARBAGE_AFTER_INPUT, ###, 2, JSEXN_SYNTAXERR, "unexpected
> garbage after {0}, starting with {1}")
> 
> with "function body" passed in for the first parameter here.
> 
> @@ +1020,1 @@
> >          return null();
> 
> Same JSMSG_GARBAGE_AFTER_INPUT comment applies here.
> 
> @@ +5201,5 @@
> >      if (tt == TOK_ERROR)
> >          return null();
> >      if (tt == TOK_EOF || tt == TOK_EOL || tt == TOK_SEMI || tt == TOK_RC) {
> > +        report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "expression",
> > +               TokenKindToDesc(tt));
> 
> So this is technically all fine as it goes, but we could/should make this a
> little nicer.  The throw<EOF>, throw;, and throw} cases are one sort of
> error: missing a thrown value entirely.  The throw<EOL> case is
> *potentially* the same thing, but it's more likely that someone didn't know
> line breaks weren't permitted between throw and expression.
> 
> So for the first three cases, we should add an error like "throw statement
> is missing an expression".  And for the last case, maybe "no line break is
> allowed between 'throw' and its expression"?

That's great, I agree, they are more user-friendly error messages :)

> @@ -7478,5 @@
> >          // with a rest param: `(a, b, ...rest) => body`.
> >          if (tokenStream.matchToken(TOK_NAME) &&
> >              tokenStream.matchToken(TOK_RP) &&
> > -            tokenStream.peekToken() == TOK_ARROW)
> > -        {
> 
> The old whitespace here was correct -- we don't allow condition to blend
> into if-body, as occurs when the brace is at the end of the line, because it
> severely hinders readability.

Sorry, it's my mistake.

> ::: js/src/jit-test/tests/basic/syntax-error-function-body-eof.js
> @@ +1,5 @@
> > +var caught = false;
> > +try {
> > +  new Function("switch (x) {} }");
> > +} catch (e) {
> > +  assertEq(e.toString().indexOf("SyntaxError: expected EOF, got token '}'") == -1, false);
> 
> Hmm.  "expected EOF" is overly strong -- we're really okay with either a
> statement or EOF.

After the change above applied, it will be "unexpected garbage after function body, starting with '}'".

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> (In reply to Tooru Fujisawa [:arai] from comment #3)
> > Then, I have a question about 4th case. When the condition `if
> > (!tokenStream.matchToken(TOK_EOF))` in `Parser<ParseHandler>::parse(JSObject
> > *chain)` match? I'd like to make a testcase for it.
> 
> I think an unadorned } will do the trick.

|Reflect.parse("}")| hit it, thanks :)


I'll post an updated patch after bug 1053240 is fixed.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Make error message for unexpected token more clear

Thank you for landing the patch in bug 1053240.
Here is updated patch, using FOR_EACH_TOKEN_KIND macro.

Part 2 is no more needed because there is no JSMSG_SYNTAX_ERROR in template literal related code.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=36effa81bdde

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8482190 [details] [diff] [review]
Make error message for unexpected token more clear

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

::: js/src/frontend/Parser.cpp
@@ +633,5 @@
>      Node pn = statements();
>      if (pn) {
>          if (!tokenStream.matchToken(TOK_EOF)) {
> +            report(ParseError, false, null(), JSMSG_GARBAGE_AFTER_INPUT, "script",
> +                   TokenKindToDesc(tokenStream.peekToken()));

Minor readability nitpick, but it's probably good to keep all the error message parameters together, not broken across two lines, even if one of them fits on the previous line without exceeding 99ch.

            report(ParseError, false, null(), JSMSG_GARBAGE_AFTER_INPUT,
                   "script", TokenKindToDesc(tokenStream.peekToken()));

@@ +752,5 @@
>          return null();
>  
>      if (!tokenStream.matchToken(TOK_EOF)) {
> +        report(ParseError, false, null(), JSMSG_GARBAGE_AFTER_INPUT, "function body",
> +               TokenKindToDesc(tokenStream.peekToken()));

Same nitpick here.

@@ +7416,5 @@
> +        if (next == TOK_ERROR)
> +            return null();
> +        if (next != TOK_NAME) {
> +            report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "rest argument name",
> +                   TokenKindToDesc(next));

Same line-breaking nitpick.

@@ +7430,5 @@
> +            return null();
> +        }
> +
> +        next = tokenStream.peekToken();
> +        if (next != TOK_ARROW) {

I imagine this was probably what I typed in my last comment, but it's not what should actually be here.  We also need to detect if TOK_ERROR was hit, like in the previous two bits, else we'll hit this behavior:

js> (a, b, ...rest) @
Assertion failure: false (MOZ_ASSERT_UNREACHABLE: TOK_ERROR should not be passed.), at /home/jwalden/moz/slots/js/src/frontend/TokenStream.cpp:1821

...which is funny, because I wasn't actually trying to hit the "<bad token>" exception-case flagged elsewhere in this patch.  :-)

More generally, the whole idea of peekToken seems not quite right.  It's immediately okay to do something like this:

    if (tokenStream.peekToken(TOK_FOOBAR))
        return parseAsFoobar();

where parseAsFoobar() assumes it can consume a TOK_FOOBAR.  But any code written this way, in the case where that wasn't hit, can't correctly handle the error case, for such correct handling must happen only if peekToken would return TOK_ERROR.  So it seems the only safe way is to assign peekToken() to a local, compare against TOK_ERROR and return null() if so, and only then deal with the if-it's-a-particular-token case.  My quick skim of the parser shows a bunch of places that mishandle this.  We should fix those, change this API, or something.  Please file a followup bug on this.

@@ +7433,5 @@
> +        next = tokenStream.peekToken();
> +        if (next != TOK_ARROW) {
> +            report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN,
> +                   "'=>' after argument list",
> +                   TokenKindToDesc(next));

Line break nit.

@@ +7450,5 @@
>  
>        default:
> +      unexpected_token:
> +        report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "expression",
> +               TokenKindToDesc(tt));

Line break nit.

::: js/src/frontend/TokenStream.cpp
@@ +1819,5 @@
> +#undef EMIT_CASE
> +      case TOK_ERROR:
> +        MOZ_ASSERT_UNREACHABLE("TOK_ERROR should not be passed.");
> +        break;
> +      case TOK_LIMIT: break;

I'd just combine TOK_ERROR/TOK_LIMIT into a single thing.  If we're going to be nice about totally bogus token kinds in general, having an exception only for TOK_ERROR doesn't make sense.

::: js/src/frontend/TokenStream.h
@@ +788,5 @@
>  extern JS_FRIEND_API(int)
>  js_fgets(char *buf, int size, FILE *file);
>  
> +extern const char *
> +TokenKindToDesc(js::frontend::TokenKind tt);

Let's move this function inside js::frontend.  That also (incidentally) lets us get rid of the qualification on |tt| here.

::: js/src/jit-test/tests/basic/syntax-error-function-body-eof.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  new Function("switch (x) {} }");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: unexpected garbage after function body, starting with '}'") == -1, false);

assertEq(e instanceof SyntaxError, true);
assertEq(e.message.startsWith("unexpected..."), true);

or something that works about the same, for the second line -- don't care about particulars.

::: js/src/jit-test/tests/basic/syntax-error-primary.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  new Function(")");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: expected expression, got ')'") == -1, false);

Same comments about checking for SyntaxError, checking the message separately.

@@ +10,5 @@
> +caught = false;
> +try {
> +  new Function("...;");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: expected rest argument name, got ';'") == -1, false);

This isn't much of an error message, but the code's not even *wrong*, really, so good enough.

@@ +28,5 @@
> +caught = false;
> +try {
> +  new Function("...a);");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: expected '=>' after argument list, got ';'") == -1, false);

Please add a test for "...a) @" to exercise the TOK_ERROR case that currently hits an assertion.

@@ +46,5 @@
> +caught = false;
> +try {
> +  new Function("(");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: expected expression, got end of file") == -1, false);

So as I look at this, I realize that it's not really end of *file*.  It's end of *script*, which only sometimes is also end of file (and isn't, here).  Could you change the description to end of script, updating all the tests you added here accordingly?

::: js/src/jit-test/tests/basic/syntax-error-throw.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  new Function("throw;");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: throw statement is missing an expression") == -1, false);

assertEq(e instanceof SyntaxError, true);
assertEq(e.message, "throw statement...");

@@ +10,5 @@
> +caught = false;
> +try {
> +  new Function("throw\n1;");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: no line break is allowed between 'throw' and its expression") == -1, false);

Same test-type-and-message-separately nit.

@@ +19,5 @@
> +caught = false;
> +try {
> +  new Function("throw}");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: throw statement is missing an expression") == -1, false);

Again.

@@ +28,5 @@
> +caught = false;
> +try {
> +  new Function("throw");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: throw statement is missing an expression") == -1, false);

Again.

::: js/src/jit-test/tests/basic/syntax-error-toplevel-eof.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  Reflect.parse("}");
> +} catch (e) {
> +  assertEq(e.toString().indexOf("SyntaxError: unexpected garbage after script, starting with '}'") == -1, false);

Note (referring back to the "end of file" thing) that we're referring to the entity as a *script*, not a *file*, here.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Make error message for unexpected token more clear

Thank you again!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> @@ +7430,5 @@
> > +            return null();
> > +        }
> > +
> > +        next = tokenStream.peekToken();
> > +        if (next != TOK_ARROW) {
> 
> I imagine this was probably what I typed in my last comment, but it's not
> what should actually be here.  We also need to detect if TOK_ERROR was hit,
> like in the previous two bits, else we'll hit this behavior:
> 
> js> (a, b, ...rest) @
> Assertion failure: false (MOZ_ASSERT_UNREACHABLE: TOK_ERROR should not be
> passed.), at /home/jwalden/moz/slots/js/src/frontend/TokenStream.cpp:1821
> 
> ...which is funny, because I wasn't actually trying to hit the "<bad token>"
> exception-case flagged elsewhere in this patch.  :-)

Thanks, I added TOK_ERROR check and testcase for it.

> More generally, the whole idea of peekToken seems not quite right.  It's
> immediately okay to do something like this:
> 
>     if (tokenStream.peekToken(TOK_FOOBAR))
>         return parseAsFoobar();
> 
> where parseAsFoobar() assumes it can consume a TOK_FOOBAR.  But any code
> written this way, in the case where that wasn't hit, can't correctly handle
> the error case, for such correct handling must happen only if peekToken
> would return TOK_ERROR.  So it seems the only safe way is to assign
> peekToken() to a local, compare against TOK_ERROR and return null() if so,
> and only then deal with the if-it's-a-particular-token case.  My quick skim
> of the parser shows a bunch of places that mishandle this.  We should fix
> those, change this API, or something.  Please file a followup bug on this.

Sorry if I'm wrong, you mean that whenever peekToken returns TOK_ERROR, it should be reported as "illegal character" error, but it's overwritten by other error message in some case, right?

for example:
  js> eval("function *f() { f(yield\n@ )}");
  typein:2:0 SyntaxError: missing ) after argument list:
  typein:2:0 @ )}
  typein:2:0 ^

If so, I'll file a bug about it.
If not, would you tell me what the problem is, and what I should file about?


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=4053f36a7e2e

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #10)
> Sorry if I'm wrong, you mean that whenever peekToken returns TOK_ERROR, it
> should be reported as "illegal character" error, but it's overwritten by
> other error message in some case, right?
> 
> for example:
>   js> eval("function *f() { f(yield\n@ )}");
>   typein:2:0 SyntaxError: missing ) after argument list:
>   typein:2:0 @ )}
>   typein:2:0 ^

When peekToken returns TOK_ERROR, we've already reported the error.  We shouldn't overwrite it by reporting *again* -- rather we should return null() or false or whatever the error-case return value should be.

As for "should be".  It's arguable that both "illegal character" and a situation-specific "didn't get this thing we wanted" are reasonable behaviors.  Because of how tokenization and parsing intermix, tho, it's easiest to only implement the first.  So that's what we should do.

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> When peekToken returns TOK_ERROR, we've already reported the error.  We
> shouldn't overwrite it by reporting *again* -- rather we should return
> null() or false or whatever the error-case return value should be.
> 
> As for "should be".  It's arguable that both "illegal character" and a
> situation-specific "didn't get this thing we wanted" are reasonable
> behaviors.  Because of how tokenization and parsing intermix, tho, it's
> easiest to only implement the first.  So that's what we should do.

Thanks, I filed it as bug 1066827.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8487411 [details] [diff] [review]
Make error message for unexpected token more clear

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

A couple little nit-ish things still, but few enough I'll land with those tweaks done myself.  Thanks!

::: js/src/frontend/Parser.cpp
@@ +5009,5 @@
>          return null();
> +    if (tt == TOK_EOF || tt == TOK_SEMI || tt == TOK_RC) {
> +        report(ParseError, false, null(), JSMSG_MISSING_EXPR_AFTER_THROW);
> +        return null();
> +    } else if (tt == TOK_EOL) {

No else after return:

if (tt == TOK_EOF ...) {
    ...
}
if (tt == TOK_EOL) {
    ...
}

@@ +7447,5 @@
>              // It doesn't matter what; when we reach the =>, we will rewind and
>              // reparse the whole arrow function. See Parser::assignExpr.
>              return handler.newNullLiteral(pos());
>          }
> +        goto unexpected_token;

The next token could be TOK_ERROR, so we need to check for that, not go and report possibly again.

This is definitely a messy spot.  We hit it for something even more nonsensical, like

js> ) @
typein:8:0 SyntaxError: expected expression, got ')':
typein:8:0 ) @
typein:8:0 ^

where the ')' is really where the error first happens.  But this is a necessary consequence of our current arrow function parsing tactic of interleaving it with normal parsing, without introducing some sort of arrow-parsing-specific mode.  Oh well.  It's not very likely people will hit this in any event.

::: js/src/jit-test/tests/basic/syntax-error-primary.js
@@ +42,5 @@
> +try {
> +  new Function("...a) @");
> +} catch (e) {
> +  assertEq(e instanceof SyntaxError, true);
> +  assertEq(e.message.startsWith("expected '=>' after argument list, got '@'") == -1, false);

This is where the rub about what error message is best arises.  This error message makes sense, but perhaps equally (or more) strongly, @ is not a valid character.

This is the aforementioned situation where we're doing |tokenStream.peekToken() == TOK_ARROW| and failing to detect that TOK_ERROR was returned, and so no error should be reported (because one already was).

::: js/src/js.msg
@@ +228,1 @@
>  MSG_DEF(JSMSG_DUPLICATE_FORMAL,        1, JSEXN_SYNTAXERR, "duplicate formal argument {0}")

One thing I've been forgetting to mention: because we include JSMSG_* directly into self-hosted code now, every time we insert a new error message in this file, or remove an existing one, we need to bump the bytecode version number in vm/Xdr.h.  So that needs to happen.

Comment 14

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/400209a093d2

Comment 15

[Sebastian Zartner [:sebo]]

Two days ago I got an email that this issue got fixed (Status=>RESOLVED, Resolution=>FIXED), though the bug history doesn't reflect that. Did you get that mail, too? Is that a bug in Bugzilla?

Sebastian

Comment 16

[Till Schneidereit [:till]]

I at least didn't get such an email, no.

Comment 17

[Tooru Fujisawa [:arai]]

I recevied mail for bug 1066827, which says "Bug 1066827 depends on bug 1041426, which changed state."
But no mail for this bug.

Comment 18

[Tooru Fujisawa [:arai]]

So, can I change status to RESOLVED FIXED, or should leave this for debugging?
Commit in mozilla-central is http://hg.mozilla.org/mozilla-central/rev/400209a093d2

Comment 19

[Till Schneidereit [:till]]

Yes, seems like the sheriffs forgot to update the bug when merging to central.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Till Schneidereit [:till] from comment #19)
> Yes, seems like the sheriffs forgot to update the bug when merging to
> central.

Comment 15 seems to suggest something weird happening on the bmo end of things, no?

Comment 21

[Till Schneidereit [:till]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Comment 15 seems to suggest something weird happening on the bmo end of
> things, no?

Ooh, of course. Hmm, I wonder if that's something that Operations would be interested in.

Comment 22

[Sebastian Zartner [:sebo]]

(In reply to Till Schneidereit [:till] from comment #21)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> > Comment 15 seems to suggest something weird happening on the bmo end of
> > things, no?
> 
> Ooh, of course. Hmm, I wonder if that's something that Operations would be
> interested in.

I can create a bug for that. Where should it go then? By "Operations" I assume you mean "Mozilla Foundation Operations"?

Sebastian

Comment 23

[Till Schneidereit [:till]]

(In reply to Sebastian Zartner from comment #22)
> I can create a bug for that. Where should it go then? By "Operations" I
> assume you mean "Mozilla Foundation Operations"?

Much appreciated. The right component, I think, is "bugzilla.mozilla.org :: Administration".

Comment 24

[Sebastian Zartner [:sebo]]

(In reply to Till Schneidereit [:till] from comment #23)
> (In reply to Sebastian Zartner from comment #22)
> > I can create a bug for that. Where should it go then? By "Operations" I
> > assume you mean "Mozilla Foundation Operations"?
> 
> Much appreciated. The right component, I think, is "bugzilla.mozilla.org ::
> Administration".

For reference: bug 1071059.

Regarding this issue I can say that it's working fine for me now. Thanks!

Sebastian