Fix pre-parsing function declarations.

src/preparser.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 99 matching lines @@
 
 
 #define CHECK_OK  ok);                      \
   if (!*ok) return kUnknownSourceElements;  \
   ((void)0
 #define DUMMY )  // to make indentation work
 #undef DUMMY
 
 
 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:
       return ParseStatement(ok);
   }
 }
 
 
 PreParser::SourceElements PreParser::ParseSourceElements(int end_token,
                                                          bool* ok) {
@@ skipping 87 matching lines @@
 
     case i::Token::SWITCH:
       return ParseSwitchStatement(ok);
 
     case i::Token::THROW:
       return ParseThrowStatement(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);
 
     default:
       return ParseExpressionOrLabelledStatement(ok);
   }
 }
 
 
@@ skipping 24 matching lines @@
 
 PreParser::Statement PreParser::ParseBlock(bool* ok) {
   // Block ::
   //   '{' Statement* '}'
 
   // Note that a Block does not introduce a new execution scope!
   // (ECMA-262, 3rd, 12.2)
   //
   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);
   return Statement::Default();
 }
 
 
 PreParser::Statement PreParser::ParseVariableStatement(
     VariableDeclarationContext var_context,
     bool* ok) {
@@ skipping 74 matching lines @@
 
 
 PreParser::Statement PreParser::ParseExpressionOrLabelledStatement(bool* ok) {
   // ExpressionStatement | LabelledStatement ::
   //   Expression ';'
   //   Identifier ':' Statement
 
   Expression expr = ParseExpression(true, CHECK_OK);
   if (expr.IsRawIdentifier()) {
     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
     // accept "native function" in the preparser.
   }
   // Parsed expression statement.
   ExpectSemicolon(CHECK_OK);
   return Statement::ExpressionStatement(expr);
 }
 
@@ skipping 109 matching lines @@
   i::Token::Value token = peek();
   while (token != i::Token::RBRACE) {
     if (token == i::Token::CASE) {
       Expect(i::Token::CASE, CHECK_OK);
       ParseExpression(true, CHECK_OK);
       Expect(i::Token::COLON, CHECK_OK);
     } else if (token == i::Token::DEFAULT) {
       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();
   }
   Expect(i::Token::RBRACE, ok);
   return Statement::Default();
 }
 
 
 PreParser::Statement PreParser::ParseDoWhileStatement(bool* ok) {
   // DoStatement ::
@@ skipping 1135 matching lines @@
       backing_store_.Add(static_cast<byte>((ascii_length >> 14) | 0x80u));
     }
     backing_store_.Add(static_cast<byte>((ascii_length >> 7) | 0x80u));
   }
   backing_store_.Add(static_cast<byte>(ascii_length & 0x7f));
 
   backing_store_.AddBlock(bytes);
   return backing_store_.EndSequence().start();
 }
 } }  // v8::preparser