420857 - Syntax error for missing brace should mention the line number of the opening brace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Hubert Kauker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

When there is a JavaScript error such that a closing brace is missing,
error console only reports the location of the line number where the
closing brace is expected.
Usually this is the very last line of source code, which is quite useless.

Better: Report the line number where that improper block was STARTED, or both.

This would particularly be quite helpful in long code, where the end of file
is far away from the location where the actual error occurs.

Reproducible: Always

Steps to Reproduce:
function test1() {
        // This function is ok.
}

// This function is not ok.
function test2() {
    if(true) {
        // bla bla
        // 100 lines of some complex source code
                
    // forget to close the opening }
// Apparently close the function body
}

function test3() {
        // This function is ok.
}

Actual Results:  
Missing } after function body. Line: 18.

Expected Results:  
Improper use of { ... }. Lines: 6, 18.

or:

Opening { is not closed, missing } after function body. Lines: 6,18.

Comment 1

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7afbd9e37e72098abe275afd4b41e48b3b2a0a16

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Report the position of opening brace for missing brace error in function body.

just like bug 104442, added a note for the place the "{" is opened, for missing "}" error.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Report the position of opening brace for missing brace error in block.

added the note also for block statement.

this time the check is done by MUST_MATCH_TOKEN_MOD.

added MUST_MATCH_TOKEN_MOD_WITH_REPORT, that receives expression that reports error if the token doesn't match.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Report the position of opening brace for missing brace error in object literal.

also for object literal.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Report the position of opening bracket for missing bracket error in array literal.

and array literal

Comment 6

[André Bargull [:anba]]

Comment on attachment 8843549 [details] [diff] [review]
Part 1: Report the position of opening brace for missing brace error in function body.

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

Cool!

::: js/src/frontend/Parser.cpp
@@ +1014,5 @@
> +    SprintfLiteral(columnNumber, "%" PRIu32, column);
> +    char lineNumber[MaxWidth];
> +    SprintfLiteral(lineNumber, "%" PRIu32, line);
> +
> +    if (!notes->addNoteASCII(pc->sc()->context,

Parser<ParseHandler>::reportRedeclaration(...) uses addNoteLatin1() for the same kind of parameters, shouldn't they match (so either both addNoteLatin1 or both addNoteASCII)?

@@ +3660,5 @@
>      }
>  
>      if (bodyType == StatementListBody) {
>          bool matched;
>          if (!tokenStream.matchToken(&matched, TOK_RC, TokenStream::Operand))

Pre-existing: When at EOF, matchToken() will call ungetToken() and push back the last token before EOF. This results in a different error location for missing "}" in functions when compared to blocks. But I'm not sure this is worth fixing.

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to André Bargull from comment #6)
> ::: js/src/frontend/Parser.cpp
> @@ +1014,5 @@
> > +    SprintfLiteral(columnNumber, "%" PRIu32, column);
> > +    char lineNumber[MaxWidth];
> > +    SprintfLiteral(lineNumber, "%" PRIu32, line);
> > +
> > +    if (!notes->addNoteASCII(pc->sc()->context,
> 
> Parser<ParseHandler>::reportRedeclaration(...) uses addNoteLatin1() for the
> same kind of parameters, shouldn't they match (so either both addNoteLatin1
> or both addNoteASCII)?

thanks, the redeclaration's one should use ASCII.
I was using Latin1 because it once contained variable name that can be latin1,
and now it only contains numbers, that should fit into ASCII.


> @@ +3660,5 @@
> >      }
> >  
> >      if (bodyType == StatementListBody) {
> >          bool matched;
> >          if (!tokenStream.matchToken(&matched, TOK_RC, TokenStream::Operand))
> 
> Pre-existing: When at EOF, matchToken() will call ungetToken() and push back
> the last token before EOF. This results in a different error location for
> missing "}" in functions when compared to blocks. But I'm not sure this is
> worth fixing.

yeah, it looks like we just need to use MUST_MATCH_TOKEN_MOD_WITH_REPORT here too.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd217c511e1ea49d924f6eb8d255b15781ec940
Bug 420857 - Part 1: Report the position of opening brace for missing brace error in function body. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb042254ca987be5af5c81fcd99ad5ca087b41b7
Bug 420857 - Part 2: Report the position of opening brace for missing brace error in block. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b077fcd66f03dc2370b3eb1ea8a282ad1b1a278
Bug 420857 - Part 3: Report the position of opening brace for missing brace error in object literal. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/81fd3c22387cac585f2242e28edc1dac9e131d0f
Bug 420857 - Part 4: Report the position of opening bracket for missing bracket error in array literal. r=anba

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecd217c511e1
https://hg.mozilla.org/mozilla-central/rev/fb042254ca98
https://hg.mozilla.org/mozilla-central/rev/4b077fcd66f0
https://hg.mozilla.org/mozilla-central/rev/81fd3c22387c