Proper handling of future reserved words in strict and normal mode.

Proper handling of future reserved words in strict and normal mode.

BUG=86442
TEST=mjsunit/keywords-and-reserved_words.js

Committed: http://code.google.com/p/v8/source/detail?r=8421

[Steven, 2011-06-22 15:06:37]

Some parser patches for you to review. I changed both the parser and the
preparser. PTAL.

[Lasse Reichstein, 2011-06-22 20:29:32]

I like the consistent renaming of functions and variables.
I think the code works.

However, I'm not sure I'd recommend committing it before we see how Webkit fares
with their change. 
This change will make reserved words be invalid as identifiers, where we have
accepted them so far. I.e., it can (and probably will) break real pages that
have used those identifiers. That's the reason we have only prohibited the
reserved words in strict mode so far.

http://codereview.chromium.org/7207007/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode2921
src/parser.cc:2921: Handle<String> name = ParseIdentifierName(CHECK_OK);
It seems ParseIdentifier would be more correct here (since it doesn't allow
everything in the IdentifierName category.
On the other hand, it's just using a more general function for a sub-case that
has already been identified.
Maybe make a comment to that effect.

http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode3861
src/parser.cc:3861: Handle<String> Parser::ParseIdentifier(bool* ok) {
This also (like the next function) parse identifier and strict reserved words.
Why are they named differently?

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode791
src/scanner-base.cc:791: KeywordMatcher::FirstState
KeywordMatcher::first_states_[] = {
The declaration of KeywordMatcher in scanner-base.h has a comment listing the
handled keywords. That should be updated to say that we distinguish future
reserved and future strict-mode reserved words.

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode903
src/scanner-base.cc:903: Token::FUTURE_STRICT_RESERVED_WORD )) return;
Indent to align with "input". Ditto for other cases below.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
File test/mjsunit/keywords-and-reserved_words.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:38: eval("var "+x+";");
Spaces around '+'. 
Even though I think readability can be argued either way.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:61: if (!isStrictKeyword(x))
 Use '{' and '}' when the if statement is not on one line.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:62: return "ERROR";
and indent by 2.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:67: if (isStrictKeyword(x))
ditto here.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:75: assertEquals ("keyword",
classifyIdentifier(word));
Extra space after "assertEquals".

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:79: if(word != "this")
Space after "if", '{' and '}' around body.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:101: assertThrows("var x = { get
foo(" + word + ") { } };", SyntaxError);
This is a syntax error for any word. Literal getters cannot have a formal
parameter.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:117: "double",       "volatile" ];
Great that you check the ES3 keywords that are not in ES5!
Worth making a comment about.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:128: "continue",     "return",
"const".
It's an ES5 future reserved word, so it should at least be mentioned there, but
we really treat it as a keyword (not that there is any way to distinguish the
two).

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:137: "if",           "with" ];
Currently we consider  \u0069f  as a valid identifier (even though it really
isn't according to the specification).
Maybe we should also test that it works.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:169: assertEquals ("strict",
classifyIdentifier(future_strict_reserved_words[i]));
Make a note that the strict mode tests are in strict-mode.js.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:362: assertThrows("function foo (" + word + ")  'use
strict'; {}", SyntaxError);
This is just a plain syntax error, unrelated to 'word'.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:374: eval("var x = { set " + word + " (value) { 'use
strict'; } };");
Duplicates line 348 and 349.

[MarkM, 2011-06-22 21:26:07]

On 2011/06/22 20:29:32, Lasse Reichstein wrote:
> However, I'm not sure I'd recommend committing it before we see how Webkit
fares
> with their change. 
> This change will make reserved words be invalid as identifiers, where we have
> accepted them so far. I.e., it can (and probably will) break real pages that
> have used those identifiers. That's the reason we have only prohibited the
> reserved words in strict mode so far.


From my own informal testing using http://www.erights.org/tests/testjs/ (see
http://codereview.appspot.com/4182070/ ) it seems the treatment of Future
Reserved Words and strict-only Future Reserved Words by Safari WebKit Nightly
Version 5.0.5 (5533.21.1, r89332) fully conforms to the ES5.1 spec. Released
Firefox 5 fully conforms to this as well. And the violations by released Safari
Version 5.0.5 (5533.21.1) are disjoint from v8's violations. 

So no reason to wait on account of this concern.

[Lasse Reichstein, 2011-06-23 07:57:50]

The CL description is a little off. We are already handling strict mode future
reserved words correctly. It's the (non-strict mode) future reserved words that
we deviate on.

[Lasse Reichstein, 2011-06-23 08:07:28]

Ok, we have the canary channel for a reason, so let's do this. It's probably
easier to see what breaks that way than by monitoring the Mozilla bugtracker.

LGTM, and let's see if my concern was unwarranted.

[Steven, 2011-06-24 11:34:59]

PTAL

http://codereview.chromium.org/7207007/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode2921
src/parser.cc:2921: Handle<String> name = ParseIdentifierName(CHECK_OK);
Better still: revert the change and just replace FUTURE_RESERVED_WORD by
FUTURE_STRICT_RESERVED_WORD.

On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> It seems ParseIdentifier would be more correct here (since it doesn't allow
> everything in the IdentifierName category.
> On the other hand, it's just using a more general function for a sub-case that
> has already been identified.
> Maybe make a comment to that effect.

http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode3861
src/parser.cc:3861: Handle<String> Parser::ParseIdentifier(bool* ok) {
Because they implement a different behavior. And the naming is consistent:
* ParseIdentifier parses only identifiers that are valid for the current scope
(i.e. it fails on strict reserved words in a strict scope).
* ParseIdentifierOrStrictReservedWord parses a valid identifiers for the current
scope or it parses a strict reserved word.

On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> This also (like the next function) parse identifier and strict reserved words.
> Why are they named differently?

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode791
src/scanner-base.cc:791: KeywordMatcher::FirstState
KeywordMatcher::first_states_[] = {
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> The declaration of KeywordMatcher in scanner-base.h has a comment listing the
> handled keywords. That should be updated to say that we distinguish future
> reserved and future strict-mode reserved words.

Done.

http://codereview.chromium.org/7207007/diff/1/src/scanner-base.cc#newcode903
src/scanner-base.cc:903: Token::FUTURE_STRICT_RESERVED_WORD )) return;
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Indent to align with "input". Ditto for other cases below.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
File test/mjsunit/keywords-and-reserved_words.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:38: eval("var "+x+";");
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Spaces around '+'. 
> Even though I think readability can be argued either way.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:61: if (!isStrictKeyword(x))
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
>  Use '{' and '}' when the if statement is not on one line.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:62: return "ERROR";
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> and indent by 2.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:67: if (isStrictKeyword(x))
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> ditto here.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:75: assertEquals ("keyword",
classifyIdentifier(word));
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Extra space after "assertEquals".

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:79: if(word != "this")
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Space after "if", '{' and '}' around body.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:101: assertThrows("var x = { get
foo(" + word + ") { } };", SyntaxError);
This was copied from strict-mode.js. Before I delete it,
what is the reason it was added there in the first place?
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> This is a syntax error for any word. Literal getters cannot have a formal
> parameter.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:117: "double",       "volatile" ];
Done. (That's from the WebKit test. Can't take the credit.)
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Great that you check the ES3 keywords that are not in ES5!
> Worth making a comment about.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:128: "continue",     "return",
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> "const".
> It's an ES5 future reserved word, so it should at least be mentioned there,
but
> we really treat it as a keyword (not that there is any way to distinguish the
> two).

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:137: "if",           "with" ];
This one is nasty. 'var \u0069f;' works but 'eval("var \u0069f;");' does not.
What exactly would you like to test here?
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Currently we consider  \u0069f  as a valid identifier (even though it really
> isn't according to the specification).
> Maybe we should also test that it works.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:169: assertEquals ("strict",
classifyIdentifier(future_strict_reserved_words[i]));
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Make a note that the strict mode tests are in strict-mode.js.

Done.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:362: assertThrows("function foo (" + word + ")  'use
strict'; {}", SyntaxError);
Moved it outside of the loop.
On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> This is just a plain syntax error, unrelated to 'word'.

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:374: eval("var x = { set " + word + " (value) { 'use
strict'; } };");
Lines 348-349 are slightly different because they test
property names given by string literals. I would keep both but move 373-374 up.

On 2011/06/22 20:29:33, Lasse Reichstein wrote:
> Duplicates line 348 and 349.

[Lasse Reichstein, 2011-06-24 12:38:04]

LGTM

http://codereview.chromium.org/7207007/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7207007/diff/1/src/parser.cc#newcode3861
src/parser.cc:3861: Handle<String> Parser::ParseIdentifier(bool* ok) {
Makes sense when explained :)

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
File test/mjsunit/keywords-and-reserved_words.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:101: assertThrows("var x = { get
foo(" + word + ") { } };", SyntaxError);
Most likely a bad cut-n-paste :).

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/keywords-and-reser...
test/mjsunit/keywords-and-reserved_words.js:137: "if",           "with" ];
On 2011/06/24 11:34:59, Steven wrote:
> This one is nasty. 'var \u0069f;' works but 'eval("var \u0069f;");' does not.
> What exactly would you like to test here?

You have string-escaping to account for, so it should be eval("var \\u0069f").

Perhaps check that 
  "\\u" + ToHexQuad(keyword.charCodeAt(0)) + keyword.substring(1)
(with a suitable implementation of ToHexQuad) is an identifier, not a keyword.

(But it's not important to do it now, save it for a different CL).

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/7207007/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:374: eval("var x = { set " + word + " (value) { 'use
strict'; } };");
Sounds fine.