Unify function parameter checking in PreParser and Parser.

This also makes the "delayed strict mode violations" mechanism in PreParser
unnecessary.

The attached tests didn't pass before this CL but now they do.

BUG=v8:3126
LOG=N
R=mstarzinger@chromium.org

Review URL: https://codereview.chromium.org/157453003

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19197 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/parser.cc

@@ -4068,8 +4068,12 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     //    '(' (Identifier)*[','] ')'
     Expect(Token::LPAREN, CHECK_OK);
     scope->set_start_position(scanner().location().beg_pos);
-    Scanner::Location name_loc = Scanner::Location::invalid();
-    Scanner::Location dupe_loc = Scanner::Location::invalid();
+
+    // We don't yet know if the function will be strict, so we cannot yet
+    // produce errors for parameter names or duplicates. However, we remember
+    // the locations of these errors if they occur and produce the errors later.
+    Scanner::Location eval_args_error_log = Scanner::Location::invalid();
+    Scanner::Location dupe_error_loc = Scanner::Location::invalid();
     Scanner::Location reserved_loc = Scanner::Location::invalid();
 
     bool done = (peek() == Token::RPAREN);
@@ -4079,16 +4083,16 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
           ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
 
       // Store locations for possible future error reports.
-      if (!name_loc.IsValid() && IsEvalOrArguments(param_name)) {
-        name_loc = scanner().location();
-      }
-      if (!dupe_loc.IsValid() && top_scope_->IsDeclared(param_name)) {
-        duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
-        dupe_loc = scanner().location();
+      if (!eval_args_error_log.IsValid() && IsEvalOrArguments(param_name)) {
+        eval_args_error_log = scanner().location();
       }
       if (!reserved_loc.IsValid() && is_strict_reserved) {
         reserved_loc = scanner().location();
       }
+      if (!dupe_error_loc.IsValid() && top_scope_->IsDeclared(param_name)) {
+        duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
+        dupe_error_loc = scanner().location();
+      }
 
       top_scope_->DeclareParameter(param_name, VAR);
       num_parameters++;
@@ -4256,7 +4260,8 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       scope->set_end_position(scanner().location().end_pos);
     }
 
-    // Validate strict mode.
+    // Validate strict mode. We can do this only after parsing the function,
+    // since the function can declare itself strict.
     if (!top_scope_->is_classic_mode()) {
       if (IsEvalOrArguments(function_name)) {
         ReportMessageAt(function_name_location,
@@ -4271,14 +4276,14 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
         *ok = false;
         return NULL;
       }
-      if (name_loc.IsValid()) {
-        ReportMessageAt(name_loc, "strict_eval_arguments",
+      if (eval_args_error_log.IsValid()) {
+        ReportMessageAt(eval_args_error_log, "strict_eval_arguments",
                         Vector<const char*>::empty());
         *ok = false;
         return NULL;
       }
-      if (dupe_loc.IsValid()) {
-        ReportMessageAt(dupe_loc, "strict_param_dupe",
+      if (dupe_error_loc.IsValid()) {
+        ReportMessageAt(dupe_error_loc, "strict_param_dupe",
                         Vector<const char*>::empty());
         *ok = false;
         return NULL;

src/preparser.cc

@@ -75,9 +75,6 @@ PreParser::PreParseResult PreParser::PreParseLazyFunction(
     if (!scope_->is_classic_mode()) {
       int end_pos = scanner()->location().end_pos;
       CheckOctalLiteral(start_position, end_pos, &ok);
-      if (ok) {
-        CheckDelayedStrictModeViolation(start_position, end_pos, &ok);
-      }
     }
   }
   return kPreParseSuccess;
@@ -1295,7 +1292,7 @@ PreParser::Arguments PreParser::ParseArguments(bool* ok) {
 }
 
 PreParser::Expression PreParser::ParseFunctionLiteral(
-    Identifier name,
+    Identifier function_name,
     Scanner::Location function_name_location,
     bool name_is_strict_reserved,
     bool is_generator,
@@ -1314,8 +1311,23 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   int start_position = position();
   bool done = (peek() == Token::RPAREN);
   DuplicateFinder duplicate_finder(scanner()->unicode_cache());
+  // We don't yet know if the function will be strict, so we cannot yet produce
+  // errors for parameter names or duplicates. However, we remember the
+  // locations of these errors if they occur and produce the errors later.
+  Scanner::Location eval_args_error_loc = Scanner::Location::invalid();
+  Scanner::Location dupe_error_loc = Scanner::Location::invalid();
+  Scanner::Location reserved_error_loc = Scanner::Location::invalid();
   while (!done) {
-    ParseIdentifier(kDontAllowEvalOrArguments, CHECK_OK);
+    bool is_strict_reserved = false;
+    Identifier param_name =
+        ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
+    if (!eval_args_error_loc.IsValid() && param_name.IsEvalOrArguments()) {
+      eval_args_error_loc = scanner()->location();
+    }
+    if (!reserved_error_loc.IsValid() && is_strict_reserved) {
+      reserved_error_loc = scanner()->location();
+    }
+
     int prev_value;
     if (scanner()->is_literal_ascii()) {
       prev_value =
@@ -1325,11 +1337,10 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
           duplicate_finder.AddUtf16Symbol(scanner()->literal_utf16_string(), 1);
     }
 
-    if (prev_value != 0) {
-      SetStrictModeViolation(scanner()->location(),
-                             "strict_param_dupe",
-                             CHECK_OK);
+    if (!dupe_error_loc.IsValid() && prev_value != 0) {
+      dupe_error_loc = scanner()->location();
     }
+
     done = (peek() == Token::RPAREN);
     if (!done) {
       Expect(Token::COMMA, CHECK_OK);
@@ -1353,9 +1364,10 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   }
   Expect(Token::RBRACE, CHECK_OK);
 
-  // Validate strict mode.
+  // Validate strict mode. We can do this only after parsing the function,
+  // since the function can declare itself strict.
   if (!scope_->is_classic_mode()) {
-    if (name.IsEvalOrArguments()) {
+    if (function_name.IsEvalOrArguments()) {
       ReportMessageAt(function_name_location, "strict_eval_arguments", NULL);
       *ok = false;
       return Expression::Default();
@@ -1366,10 +1378,27 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
       *ok = false;
       return Expression::Default();
     }
+    if (eval_args_error_loc.IsValid()) {
+      ReportMessageAt(eval_args_error_loc, "strict_eval_arguments",
+                      Vector<const char*>::empty());
+      *ok = false;
+      return Expression::Default();
+    }
+    if (dupe_error_loc.IsValid()) {
+      ReportMessageAt(dupe_error_loc, "strict_param_dupe",
+                      Vector<const char*>::empty());
+      *ok = false;
+      return Expression::Default();
+    }
+    if (reserved_error_loc.IsValid()) {
+      ReportMessageAt(reserved_error_loc, "unexpected_strict_reserved",
+                      Vector<const char*>::empty());
+      *ok = false;
+      return Expression::Default();
+    }
 
     int end_position = scanner()->location().end_pos;
     CheckOctalLiteral(start_position, end_position, CHECK_OK);
-    CheckDelayedStrictModeViolation(start_position, end_position, CHECK_OK);
     return Expression::StrictFunction();
   }
 
@@ -1475,7 +1504,8 @@ PreParser::Identifier PreParser::ParseIdentifier(
     PreParser::Identifier name = GetIdentifierSymbol();
     if (allow_eval_or_arguments == kDontAllowEvalOrArguments &&
         !scope_->is_classic_mode() && name.IsEvalOrArguments()) {
-      StrictModeIdentifierViolation(scanner()->location(), name, ok);
+      ReportMessageAt(scanner()->location(), "strict_eval_arguments", NULL);
+      *ok = false;
     }
     return name;
   } else if (scope_->is_classic_mode() &&
@@ -1490,58 +1520,6 @@ PreParser::Identifier PreParser::ParseIdentifier(
 }
 
 
-void PreParser::SetStrictModeViolation(Scanner::Location location,
-                                       const char* type,
-                                       bool* ok) {
-  if (!scope_->is_classic_mode()) {
-    ReportMessageAt(location, type, NULL);
-    *ok = false;
-    return;
-  }
-  // Delay report in case this later turns out to be strict code
-  // (i.e., for function names and parameters prior to a "use strict"
-  // directive).
-  // It's safe to overwrite an existing violation.
-  // It's either from a function that turned out to be non-strict,
-  // or it's in the current function (and we just need to report
-  // one error), or it's in a unclosed nesting function that wasn't
-  // strict (otherwise we would already be in strict mode).
-  strict_mode_violation_location_ = location;
-  strict_mode_violation_type_ = type;
-}
-
-
-void PreParser::CheckDelayedStrictModeViolation(int beg_pos,
-                                                int end_pos,
-                                                bool* ok) {
-  Scanner::Location location = strict_mode_violation_location_;
-  if (location.IsValid() &&
-      location.beg_pos > beg_pos && location.end_pos < end_pos) {
-    ReportMessageAt(location, strict_mode_violation_type_, NULL);
-    *ok = false;
-  }
-}
-
-
-void PreParser::StrictModeIdentifierViolation(Scanner::Location location,
-                                              Identifier identifier,
-                                              bool* ok) {
-  const char* type = "strict_eval_arguments";
-  if (identifier.IsFutureReserved()) {
-    type = "unexpected_reserved";
-  } else if (identifier.IsFutureStrictReserved() || identifier.IsYield()) {
-    type = "unexpected_strict_reserved";
-  }
-  if (!scope_->is_classic_mode()) {
-    ReportMessageAt(location, type, NULL);
-    *ok = false;
-    return;
-  }
-  strict_mode_violation_location_ = location;
-  strict_mode_violation_type_ = type;
-}
-
-
 // Parses and identifier or a strict mode future reserved word, and indicate
 // whether it is strict mode future reserved.
 PreParser::Identifier PreParser::ParseIdentifierOrStrictReservedWord(

src/preparser.h

@@ -243,8 +243,6 @@ class PreParser : public ParserBase {
       : ParserBase(scanner, stack_limit),
         log_(log),
         scope_(NULL),
-        strict_mode_violation_location_(Scanner::Location::invalid()),
-        strict_mode_violation_type_(NULL),
         parenthesized_function_(false) { }
 
   ~PreParser() {}
@@ -655,20 +653,8 @@ class PreParser : public ParserBase {
 
   bool CheckInOrOf(bool accept_OF);
 
-  void SetStrictModeViolation(Scanner::Location,
-                              const char* type,
-                              bool* ok);
-
-  void CheckDelayedStrictModeViolation(int beg_pos, int end_pos, bool* ok);
-
-  void StrictModeIdentifierViolation(Scanner::Location,
-                                     Identifier identifier,
-                                     bool* ok);
-
   ParserRecorder* log_;
   Scope* scope_;
-  Scanner::Location strict_mode_violation_location_;
-  const char* strict_mode_violation_type_;
   bool parenthesized_function_;
 };
 

test/cctest/test-parsing.cc

@@ -1940,3 +1940,40 @@ TEST(DontRegressPreParserDataSizes) {
     CHECK(!data.has_error());
   }
 }
+
+
+TEST(FunctionDeclaresItselfStrict) {
+  // Tests that we produce the right kinds of errors when a function declares
+  // itself strict (we cannot produce there errors as soon as we see the
+  // offending identifiers, because we don't know at that point whether the
+  // function is strict or not).
+  const char* context_data[][2] = {
+    {"function eval() {", "}"},
+    {"function arguments() {", "}"},
+    {"function yield() {", "}"},
+    {"function interface() {", "}"},
+    {"function foo(eval) {", "}"},
+    {"function foo(arguments) {", "}"},
+    {"function foo(yield) {", "}"},
+    {"function foo(interface) {", "}"},
+    {"function foo(bar, eval) {", "}"},
+    {"function foo(bar, arguments) {", "}"},
+    {"function foo(bar, yield) {", "}"},
+    {"function foo(bar, interface) {", "}"},
+    {"function foo(bar, bar) {", "}"},
+    { NULL, NULL }
+  };
+
+  const char* strict_statement_data[] = {
+    "\"use strict\";",
+    NULL
+  };
+
+  const char* non_strict_statement_data[] = {
+    ";",
+    NULL
+  };
+
+  RunParserSyncTest(context_data, strict_statement_data, kError);
+  RunParserSyncTest(context_data, non_strict_statement_data, kSuccess);
+}