Issue 117 - strict mode and future reserved words

Issue 117 - strict mode and future reserved words

Adds a new token value for all future reserved words. The scanner
is not strict mode aware and always returns the new
IDENTIFIER_OR_FUTURE_RESERVED_WORD token. The parser and preparser
check for the new token type based on the strict mode context.

BUG=
TEST=

[Peter Hallam, 2011-02-03 03:39:30]

Issue 117 - strict mode and future reserved words 

Adds a new token value for all future reserved words. The scanner 
is not strict mode aware and always returns the new 
IDENTIFIER_OR_FUTURE_RESERVED_WORD token. The parser and preparser 
check for the new token type based on the strict mode context.

[Martin Maly, 2011-02-03 03:50:08]

Adding Lasse.

[Lasse Reichstein, 2011-02-03 11:42:47]

LGTM

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

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode753
src/parser.cc:753: result = ParseFunctionLiteral(name, false, // strict mode
identifier already checked???
The SharedFunctionInfo should (eventually) contain the strict-mode bit of the
surrounding context.
It might even know whether the function contains the "use strict" declarative if
it was provided by the preparser/parser that first scanned the function source.

So, definitely inherit strictness from the context, and probably have the body
already checked.

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode2829
src/parser.cc:2829: case Token::IDENTIFIER_OR_FUTURE_RESERVED_WORD: {
This seems odd, since both tokens seem to be able to represent an identifier. If
IDENTIFIER_OR_FUTURE_RESERVED_WORD only represent future reserved words, it
should be named just FUTURE_RESERVED_WORD. The parser can then choose to handle
a future reserved word as an identifier.

On a side note, they seem to have removed "native" as a reserved word. Which is
annoying, since we are actually using it in our natives files to declare
extension backed functions.

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode3767
src/parser.cc:3767: !Token::IsKeyword(next)) {
Maybe future reserved words should be considered keywords, so you only need to
check IDENTIFIER or IsKeyword.
We only use IsKeyword to check for something that is a sequence of letters, but
which isn't scanned as an identifier. Future reserved words would match that
categorization. 
(But we need to check that we don't assume that IsKeyword(t) => Token::String(t)
works).

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode3809
src/parser.cc:3809: // we can't do during preparsing.
This code isn't used for preparsing any more, so the comment is out of date.
(The identical comment in perparser.cc is also out of date, for the opposite
reason).

http://codereview.chromium.org/6246064/diff/1/src/preparser.cc
File src/preparser.cc (right):

http://codereview.chromium.org/6246064/diff/1/src/preparser.cc#newcode952
src/preparser.cc:952: case i::Token::IDENTIFIER_OR_FUTURE_RESERVED_WORD: {
If it's a future reserved word, we know that it's not "get" or "set". We should
be able to make something simpler in that case.

[Lasse Reichstein, 2011-02-03 11:43:28]

But remember to add some tests!
We don't want any non-strict code to be affected by the new reserved words.

[Lasse Reichstein, 2011-02-03 11:55:07]

Ignore previous comment, the tests are fine (and this is just suggesting an
extra level of paranoia-testing).

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

http://codereview.chromium.org/6246064/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:322: eval("var x = { set " + word + " (value) { 'use
strict'; } };");
Try with the word as a string literal as well. It should pass in both normal and
strict mode.

[Peter Hallam, 2011-02-04 18:32:41]

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

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode753
src/parser.cc:753: result = ParseFunctionLiteral(name, false, // strict mode
identifier already checked???
If I understand correctly, we can only get here after the function to be lazy
parsed has successfully passed a regular full parse. In that case there can be
no strict mode errors left in the function when we get here. Passing false
disables the future reserved word checking for the function name, which should
be ok because it has been checked in the earlier full parse.

On 2011/02/03 11:42:48, Lasse Reichstein wrote:
> The SharedFunctionInfo should (eventually) contain the strict-mode bit of the
> surrounding context.
> It might even know whether the function contains the "use strict" declarative
if
> it was provided by the preparser/parser that first scanned the function
source.
> 
> So, definitely inherit strictness from the context, and probably have the body
> already checked.

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode2829
src/parser.cc:2829: case Token::IDENTIFIER_OR_FUTURE_RESERVED_WORD: {
On 2011/02/03 11:42:48, Lasse Reichstein wrote:
> This seems odd, since both tokens seem to be able to represent an identifier.
If
> IDENTIFIER_OR_FUTURE_RESERVED_WORD only represent future reserved words, it
> should be named just FUTURE_RESERVED_WORD. The parser can then choose to
handle
> a future reserved word as an identifier.

Done.

> On a side note, they seem to have removed "native" as a reserved word. Which
is
> annoying, since we are actually using it in our natives files to declare
> extension backed functions.

Done - added issue 1097.

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode3809
src/parser.cc:3809: // we can't do during preparsing.
On 2011/02/03 11:42:48, Lasse Reichstein wrote:
> This code isn't used for preparsing any more, so the comment is out of date.
> (The identical comment in perparser.cc is also out of date, for the opposite
> reason).

Done.

http://codereview.chromium.org/6246064/diff/1/src/preparser.cc
File src/preparser.cc (right):

http://codereview.chromium.org/6246064/diff/1/src/preparser.cc#newcode952
src/preparser.cc:952: case i::Token::IDENTIFIER_OR_FUTURE_RESERVED_WORD: {
True, however this way the code in the preparser is smaller and matches the code
in the parser more closely.

On 2011/02/03 11:42:48, Lasse Reichstein wrote:
> If it's a future reserved word, we know that it's not "get" or "set". We
should
> be able to make something simpler in that case.

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

http://codereview.chromium.org/6246064/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:322: eval("var x = { set " + word + " (value) { 'use
strict'; } };");
On 2011/02/03 11:55:07, Lasse Reichstein wrote:
> Try with the word as a string literal as well. It should pass in both normal
and
> strict mode.

Done.

[Lasse Reichstein, 2011-02-07 08:05:36]

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

http://codereview.chromium.org/6246064/diff/1/src/parser.cc#newcode753
src/parser.cc:753: result = ParseFunctionLiteral(name, false, // strict mode
identifier already checked???
True. Maybe at some point add an ASSERT that the name isn't reserved, and give
the false value a name (e.g., is_reserved_name).