Fix pre-parsing function declarations.

Fix pre-parsing function declarations.

The preparser has been out of sync with the parser. As a reminder, we have the
following grammer for harmony mode

 Block ::
   { SourceElement* }
 SourceElement :: 
   Statement
   FunctionDeclaration
   LetDeclaration

instead of

 Block ::
   { Statement* }
 SourceElement ::
   Statement
   FunctionDeclaration

The extension to allow FunctionDeclarations in statement positions in
non-strict code is still active.

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

[Steven, 2011-09-21 09:00:04]

PTAL.

[Lasse Reichstein, 2011-09-21 10:29:34]

LGTM

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

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode396
src/preparser.cc:396: return ParseStatement(CHECK_OK);
CHECK_OK -> ok
We won't be checking the result after the return anyway.

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode396
src/preparser.cc:396: return ParseStatement(CHECK_OK);
So this is how we allow function declarations in all blocks, but not after a
label in strict mode?

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
File test/mjsunit/harmony/block-let-declaration.js (right):

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:51: assertThrows("(function(){" +
str + "})()", expect);
If this is early errors (and I guess it is), you shouldn't call the generated
function. It should fail even if the function is never compiled.

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:55:
assertDoesNotThrow("(function(){" + str + "})()");
But keep the call here. It should never fail, not even if compiled lazily.

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:79: // non-strict statement
positions.
Can you have a non-strict statement position in harmony-block-scoping mode?
I would assume that any harmony enabling flag would make everything strict mode
code (or if not, perhaps it should?)

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:103: }
Call f here, to ensure that it isn't lazily compiled and throws at runtime.

[Steven, 2011-09-21 12:03:28]

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

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode394
src/preparser.cc:394: (!strict_mode() ||
!expr.AsIdentifier().IsFutureReserved())) {
While we are already here. This check is off right? In normal mode we should
disallow
  FutureReserved
  Keywords
but allow all other identifiers and further in strict mode disallow
  FutureStrictReserved
In particular eval and arguments are allowed in both modes. Do you agree?

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode396
src/preparser.cc:396: return ParseStatement(CHECK_OK);
Yes. This is closely following the spec. We have
LabelledStatement ::
  Identifer : Statement
and we disallow function declarations as statements in strict mode.
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> So this is how we allow function declarations in all blocks, but not after a
> label in strict mode?

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode396
src/preparser.cc:396: return ParseStatement(CHECK_OK);
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> CHECK_OK -> ok
> We won't be checking the result after the return anyway.

Done.

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
File test/mjsunit/harmony/block-let-declaration.js (right):

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:51: assertThrows("(function(){" +
str + "})()", expect);
Yes they are early errors. Done.
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> If this is early errors (and I guess it is), you shouldn't call the generated
> function. It should fail even if the function is never compiled.

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:55:
assertDoesNotThrow("(function(){" + str + "})()");
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> But keep the call here. It should never fail, not even if compiled lazily.

Done.

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:79: // non-strict statement
positions.
Indeed the harmony proposals actually take the ES5 strict mode as a base, so in
general yes it should be all strict mode code and ultimately we should do that.
For now I have refrained from doing that yet and just added it orthogonally so
that I can use some features from ES5 normal mode for test cases (notably
forcing dynamic lookups using 'with').
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> Can you have a non-strict statement position in harmony-block-scoping mode?
> I would assume that any harmony enabling flag would make everything strict
mode
> code (or if not, perhaps it should?)

http://codereview.chromium.org/7983006/diff/1/test/mjsunit/harmony/block-let-...
test/mjsunit/harmony/block-let-declaration.js:103: }
On 2011/09/21 10:29:34, Lasse Reichstein wrote:
> Call f here, to ensure that it isn't lazily compiled and throws at runtime.

Done.

[Lasse Reichstein, 2011-09-21 20:11:45]

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

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode394
src/preparser.cc:394: (!strict_mode() ||
!expr.AsIdentifier().IsFutureReserved())) {
We "should" disallow FutureReserved in non-strict mode, but we don't. It's a
legacy thing - we can't start making future-reserved words an error now without
breaking code that uses them.
We block keywords in both modes (expr.IsRawIdentifier() returns false on
keywords), and future reserved words in strict mode.
Both "eval" and "arguments" are allowed in both modes, since they are raw
identifiers and not future reserved words.
I.e., I believe the code does the right thing.

There is an extensive unit-test suite for the preparser. Do check if this needs
to be covered better.

[Steven, 2011-09-21 22:42:07]

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

http://codereview.chromium.org/7983006/diff/1/src/preparser.cc#newcode394
src/preparser.cc:394: (!strict_mode() ||
!expr.AsIdentifier().IsFutureReserved())) {
Actually we do follow ES5 already and for labelled statements disallow the
correct sets of FutureReservedWords in both modes as specified in clause
7.6.1.2. I was just confused by this check because it did not make the
distinction between always FutureReservedWords and the ones that are only
reserved in strict mode. The check is actually redundant because
Parse(Primary)Expression will already disallow the usage. In fact I can use the
following assertions
if (expr.IsRawIdentifier()) {
  ASSERT(!expr.AsIdentifier().IsFutureReserved());
  ASSERT(!strict_mode() ||
         !expr.AsIdentifier().IsFutureStrictReserved());
  [...]

The file preparser/strict-identifiers.pyt does not yet include tests for
checking the identifier used in labelled statements, break or continue. And
indeed the implementations of break and continue both do not check the parsed
identifier, because clearly they use ParseIdentifier directly and not
ParseExpression.

On 2011/09/21 20:11:45, Lasse Reichstein wrote:
> We "should" disallow FutureReserved in non-strict mode, but we don't. It's a
> legacy thing - we can't start making future-reserved words an error now
without
> breaking code that uses them.
> We block keywords in both modes (expr.IsRawIdentifier() returns false on
> keywords), and future reserved words in strict mode.
> Both "eval" and "arguments" are allowed in both modes, since they are raw
> identifiers and not future reserved words.
> I.e., I believe the code does the right thing.
> 
> There is an extensive unit-test suite for the preparser. Do check if this
needs
> to be covered better.