[Webkit-unassigned] [Bug 62613] No context for javascript parse errors.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62613


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97382|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Oliver Hunt <oliver at apple.com>  2011-06-15 22:39:48 PST ---
(From update of attachment 97382)
View in context: https://bugs.webkit.org/attachment.cgi?id=97382&action=review

The bulk of the patch is looking good, but still needs a bit of tidying in the logic and structure but not too much (see my comments below), but the error messages need a bit more work.

We should just file a bug on the lexer errors and work on that once we've got the parser errors sorted out and landed.

> Source/JavaScriptCore/parser/JSParser.cpp:135
> +    ALWAYS_INLINE String getToken() {

In JSC we use UString rather than String (yes i realise there both essentially RefPtr<StringImpl>, but while a foolish consistency may be the hobgoblin of a little mind, code consistency is awesome!)

> Source/JavaScriptCore/parser/JSParser.cpp:184
> +                break;

Incorrect indenting

> Source/JavaScriptCore/parser/JSParser.cpp:328
> +        case LastUntaggedToken: 
> +            ASSERT_NOT_REACHED();
> +            break;

I'd put this one at the end

> Source/JavaScriptCore/parser/JSParser.cpp:346
> +        case AUTOPLUSPLUS: 
> +            name = "auto ++"; 
> +            break;
> +        case AUTOMINUSMINUS: 
> +            name = "auto --"; 
> +            break;

These are internal concepts, you should just make the above PLUSPLUS logic also handle AUTOPLUSPLUS, eg.
case AUTOPLUSPLUS:
case PLUSPLUS:
   ...

(and the same for --)

> Source/JavaScriptCore/parser/JSParser.cpp:426
> +        return name;

Given we're just returning name, it would probably be better to replace the

   name = ...;
   break;

pattern with

    return ...;

And then put
    ASSERT_NOT_REACHED();
    return "internal error";

at the end.

> Source/JavaScriptCore/parser/JSParser.cpp:450
> +            errorMessage = "Invalid token character sequence: ";

I think "Unrecognised token: " but i'm notoriously bad with such things so we may want to ask for input from geoff (in future the lexer should provide a meaningful message)

> Source/JavaScriptCore/parser/JSParser.cpp:481
> +            m_errorMessage = UString(String::format("Expected token: %s", name).impl());

We may want this message to be:

Expected <token that we want> but got <token that we got> instead.

> LayoutTests/fast/js/basic-strict-mode-expected.txt:31
> +PASS (function eval(){'use strict';}) threw exception SyntaxError: Unexpected token: }.

looking at these i wonder if we want quotes around the symbol tokens?

> LayoutTests/fast/js/basic-strict-mode-expected.txt:79
> +PASS 'use strict'; return threw exception SyntaxError: Unexpected token: return.

Better error would be
"return is only inside of a function"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:81
> +PASS 'use strict'; break threw exception SyntaxError: Unexpected token 'break'.
> +PASS (function(){'use strict'; break}) threw exception SyntaxError: Unexpected token 'break'.

Better error would be
"break is only valid inside a switch or loop statement"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:83
> +PASS 'use strict'; continue threw exception SyntaxError: Unexpected token 'continue'.
> +PASS (function(){'use strict'; continue}) threw exception SyntaxError: Unexpected token 'continue'.

Better error would be
"continue is only valid inside a loop statement"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:88
> +PASS (function(){'use strict'; for(;;)continue missingLabel}) threw exception SyntaxError: Unexpected identifier: missingLabel (in strict mode).

Better error here is:
"Label '<label name>' is not defined"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:94
> +PASS 'use strict'; 007; threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode).
> +PASS (function(){'use strict'; 007;}) threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode).
> +PASS 'use strict'; '\007'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS (function(){'use strict'; '\007';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS '\007'; 'use strict'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS (function(){'\007'; 'use strict';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).

These will be fixed when we make the lexer errors better

> LayoutTests/fast/js/basic-strict-mode-expected.txt:100
> +PASS 'use strict'; delete aDeletableProperty; threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){'use strict'; delete aDeletableProperty;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS 'use strict'; (function (){ delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){'use strict'; (function (){ delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function (){ 'use strict'; delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){(function (){ 'use strict'; delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;.

These errors aren't very good -- we're saying that the semicolon is unexpected but it's actually strict mode disallowing deletion of an unqualified property

> LayoutTests/fast/js/basic-strict-mode-expected.txt:114
> +PASS 'use strict'; ++eval threw exception SyntaxError: Unexpected EOF (in strict mode).
> +PASS (function(){'use strict'; ++eval}) threw exception SyntaxError: Unexpected token: }.
> +PASS 'use strict'; eval++ threw exception SyntaxError: Unexpected token: ++.
> +PASS (function(){'use strict'; eval++}) threw exception SyntaxError: Unexpected token: ++.
> +PASS 'use strict'; --eval threw exception SyntaxError: Unexpected EOF (in strict mode).
> +PASS (function(){'use strict'; --eval}) threw exception SyntaxError: Unexpected token: }.
> +PASS 'use strict'; eval-- threw exception SyntaxError: Unexpected token: --.

These errors need to be improved:
"Can't assign to eval in strict mode"
and
"Can't assign to arguments in strict mode"

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.