Unify function parameter checking in PreParser and Parser.
authormarja@chromium.org <marja@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Fri, 7 Feb 2014 12:44:45 +0000 (12:44 +0000)
committermarja@chromium.org <marja@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Fri, 7 Feb 2014 12:44:45 +0000 (12:44 +0000)
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
src/preparser.cc
src/preparser.h
test/cctest/test-parsing.cc

index 21beacc..54a83e8 100644 (file)
@@ -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;
index 4caf906..49d6cd2 100644 (file)
@@ -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(
index 0f417f1..bcaab74 100644 (file)
@@ -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_;
 };
 
index 4dd7971..85b2f3c 100644 (file)
@@ -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);
+}