Fix pre-parsing function declarations.

src/preparser.cc

Index: src/preparser.cc
diff --git a/src/preparser.cc b/src/preparser.cc
index 47d21bac15e8a253829eeddc5a582258618ecd7e..cd8888555deaf419e04a4ac4f8cecc1c3ff3e654 100644
--- a/src/preparser.cc
+++ b/src/preparser.cc
@@ -117,7 +117,18 @@ void PreParser::CheckOctalLiteral(int beg_pos, int end_pos, bool* ok) {
 
 
 PreParser::Statement PreParser::ParseSourceElement(bool* ok) {
+  // (Ecma 262 5th Edition, clause 14):
+  // SourceElement:
+  //    Statement
+  //    FunctionDeclaration
+  //
+  // In harmony mode we allow additionally the following productions
+  // SourceElement:
+  //    LetDeclaration
+
   switch (peek()) {
+    case i::Token::FUNCTION:
+      return ParseFunctionDeclaration(ok);
     case i::Token::LET:
       return ParseVariableStatement(kSourceElement, ok);
     default:
@@ -225,8 +236,19 @@ PreParser::Statement PreParser::ParseStatement(bool* ok) {
     case i::Token::TRY:
       return ParseTryStatement(ok);
 
-    case i::Token::FUNCTION:
-      return ParseFunctionDeclaration(ok);
+    case i::Token::FUNCTION: {
+      i::Scanner::Location start_location = scanner_->peek_location();
+      Statement statement = ParseFunctionDeclaration(CHECK_OK);
+      i::Scanner::Location end_location = scanner_->location();
+      if (strict_mode()) {
+        ReportMessageAt(start_location.beg_pos, end_location.end_pos,
+                        "strict_function", NULL);
+        *ok = false;
+        return Statement::Default();
+      } else {
+        return statement;
+      }
+    }
 
     case i::Token::DEBUGGER:
       return ParseDebuggerStatement(ok);
@@ -271,14 +293,10 @@ PreParser::Statement PreParser::ParseBlock(bool* ok) {
   //
   Expect(i::Token::LBRACE, CHECK_OK);
   while (peek() != i::Token::RBRACE) {
-    i::Scanner::Location start_location = scanner_->peek_location();
-    Statement statement = ParseSourceElement(CHECK_OK);
-    i::Scanner::Location end_location = scanner_->location();
-    if (strict_mode() && statement.IsFunctionDeclaration()) {
-      ReportMessageAt(start_location.beg_pos, end_location.end_pos,
-                      "strict_function", NULL);
-      *ok = false;
-      return Statement::Default();
+    if (harmony_block_scoping_) {
+      ParseSourceElement(CHECK_OK);
+    } else {
+      ParseStatement(CHECK_OK);
     }
   }
   Expect(i::Token::RBRACE, ok);
@@ -375,15 +393,7 @@ PreParser::Statement PreParser::ParseExpressionOrLabelledStatement(bool* ok) {
     if (peek() == i::Token::COLON &&
         (!strict_mode() || !expr.AsIdentifier().IsFutureReserved())) {

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

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?

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

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]

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:

       Consume(i::Token::COLON);
-      i::Scanner::Location start_location = scanner_->peek_location();
-      Statement statement = ParseStatement(CHECK_OK);
-      if (strict_mode() && statement.IsFunctionDeclaration()) {
-        i::Scanner::Location end_location = scanner_->location();
-        ReportMessageAt(start_location.beg_pos, end_location.end_pos,
-                        "strict_function", NULL);
-        *ok = false;
-      }
-      return Statement::Default();
+      return ParseStatement(CHECK_OK);

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

CHECK_OK -> ok
We won't be checking the result after the return anyway.

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

So this is how we allow function declarations in all blocks, but not after a
label in strict mode?

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

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:

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

Done.

     }
     // Preparsing is disabled for extensions (because the extension details
     // aren't passed to lazily compiled functions), so we don't
@@ -513,15 +523,7 @@ PreParser::Statement PreParser::ParseSwitchStatement(bool* ok) {
       Expect(i::Token::DEFAULT, CHECK_OK);
       Expect(i::Token::COLON, CHECK_OK);
     } else {
-      i::Scanner::Location start_location = scanner_->peek_location();
-      Statement statement = ParseStatement(CHECK_OK);
-      if (strict_mode() && statement.IsFunctionDeclaration()) {
-        i::Scanner::Location end_location = scanner_->location();
-        ReportMessageAt(start_location.beg_pos, end_location.end_pos,
-                        "strict_function", NULL);
-        *ok = false;
-        return Statement::Default();
-      }
+      ParseStatement(CHECK_OK);
     }
     token = peek();
   }