[webkit-reviews] review denied: [Bug 62613] No context for javascript parse errors. : [Attachment 97382] proposed patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 62613: No context for javascript parse errors.
https://bugs.webkit.org/show_bug.cgi?id=62613

Attachment 97382: proposed patch
https://bugs.webkit.org/attachment.cgi?id=97382&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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"