[preparser] Remove ExpressionClassifier error tracking in the PreParser.

PreParser now does not longer track which kind of error occurred.
If we see an error we reparse with the parser and report the error.
Furthermore, this fixes tests in test-parsing.

Change-Id: I1860949fab4d65ff4a5a1b63796c7574494f9d50
Reviewed-on: https://chromium-review.googlesource.com/1231173
Commit-Queue: Florian Sattler <sattlerf@google.com>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56281}

src/parsing/parser-base.h

@@ -945,7 +945,11 @@ class ParserBase {
 
   void ReportClassifierError(
       const typename ExpressionClassifier::Error& error) {
-    impl()->ReportMessageAt(error.location, error.message, error.arg);
+    if (classifier()->does_error_reporting()) {
+      impl()->ReportMessageAt(error.location, error.message, error.arg);
+    } else {
+      impl()->ReportUnidentifiableError();
+    }
   }
 
   void ValidateExpression(bool* ok) {
@@ -4393,17 +4397,29 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
         int dummy_num_parameters = -1;
         DCHECK_NE(kind & FunctionKind::kArrowFunction, 0);
         FunctionLiteral::EagerCompileHint hint;
-        bool parse_result = impl()->SkipFunction(
+        bool did_preparse_successfully = impl()->SkipFunction(
             nullptr, kind, FunctionLiteral::kAnonymousExpression,
             formal_parameters.scope, &dummy_num_parameters,
             &produced_preparsed_scope_data, false, false, &hint, CHECK_OK);
-        DCHECK(parse_result);
-        USE(parse_result);
+
         DCHECK_NULL(produced_preparsed_scope_data);
-        // Discard any queued destructuring assignments which appeared
-        // in this function's parameter list, and which were adopted
-        // into this function state, above.
-        function_state.RewindDestructuringAssignments(0);
+
+        if (did_preparse_successfully) {
+          // Discard any queued destructuring assignments which appeared
+          // in this function's parameter list, and which were adopted
+          // into this function state, above.
+          function_state.RewindDestructuringAssignments(0);
+        } else {
+          // In case we did not sucessfully preparse the function because of an
+          // unidentified error we do a full reparse to return the error.
+          Consume(Token::LBRACE);
+          body = impl()->NewStatementList(8);
+          ParseFunctionBody(body, impl()->NullIdentifier(), kNoSourcePosition,
+                            formal_parameters, kind,
+                            FunctionLiteral::kAnonymousExpression, ok);
+          CHECK(!*ok);
+          return impl()->NullExpression();
+        }
       } else {
         Consume(Token::LBRACE);
         body = impl()->NewStatementList(8);

src/parsing/parser.cc

@@ -2553,12 +2553,12 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   // abort lazy parsing if it suspects that wasn't a good idea. If so (in
   // which case the parser is expected to have backtracked), or if we didn't
   // try to lazy parse in the first place, we'll have to parse eagerly.
-  bool did_preparse =
+  bool did_preparse_successfully =
       should_preparse &&
       SkipFunction(function_name, kind, function_type, scope, &num_parameters,
                    &produced_preparsed_scope_data, is_lazy_inner_function,
                    is_lazy_top_level_function, &eager_compile_hint, CHECK_OK);
-  if (!did_preparse) {
+  if (!did_preparse_successfully) {
     body = ParseFunction(
         function_name, pos, kind, function_type, scope, &num_parameters,
         &function_length, &has_duplicate_parameters, &expected_property_count,
@@ -2577,7 +2577,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
         reinterpret_cast<const char*>(function_name->raw_data()),
         function_name->byte_length());
   }
-  if (V8_UNLIKELY(FLAG_runtime_stats) && did_preparse) {
+  if (V8_UNLIKELY(FLAG_runtime_stats) && did_preparse_successfully) {
     const RuntimeCallCounterId counters[2][2] = {
         {RuntimeCallCounterId::kPreParseBackgroundNoVariableResolution,
          RuntimeCallCounterId::kPreParseNoVariableResolution},
@@ -2691,6 +2691,14 @@ bool Parser::SkipFunction(
     // Propagate stack overflow.
     set_stack_overflow();
     *ok = false;
+  } else if (pending_error_handler()->ErrorUnidentifiableByPreParser()) {
+    // If we encounter an error that the preparser can not identify we reset to
+    // the state before preparsing. The caller may then fully parse the function
+    // to identify the actual error.
+    bookmark.Apply();
+    function_scope->ResetAfterPreparsing(ast_value_factory(), true);
+    pending_error_handler()->ResetUnidentifiableError();
+    return false;
   } else if (pending_error_handler()->has_pending_error()) {
     *ok = false;
   } else {

src/parsing/parser.h

@@ -144,6 +144,8 @@ struct ParserTypes<Parser> {
 
   typedef ParserTarget Target;
   typedef ParserTargetScope TargetScope;
+
+  static constexpr bool ExpressionClassifierReportErrors = true;
 };
 
 class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
@@ -179,7 +181,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
 
  private:
   friend class ParserBase<Parser>;
-  friend class v8::internal::ExpressionClassifier<ParserTypes<Parser>>;
+  friend class v8::internal::ExpressionClassifierErrorTracker<
+      ParserTypes<Parser>>;
   friend bool v8::internal::parsing::ParseProgram(ParseInfo*, Isolate*);
   friend bool v8::internal::parsing::ParseFunction(
       ParseInfo*, Handle<SharedFunctionInfo> shared_info, Isolate*);
@@ -448,6 +451,13 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   // by parsing the function with PreParser. Consumes the ending }.
   // If may_abort == true, the (pre-)parser may decide to abort skipping
   // in order to force the function to be eagerly parsed, after all.
+  // In case the preparser detects an error it cannot identify, it resets the
+  // scanner- and preparser state to the initial one, before PreParsing the
+  // function.
+  // SkipFunction returns true if it correctly parsed the function, including
+  // cases where we detect an error. It returns false, if we needed to stop
+  // parsing or could not identify an error correctly, meaning the caller needs
+  // to fully reparse. In this case it resets the scanner and preparser state.
   bool SkipFunction(const AstRawString* function_name, FunctionKind kind,
                     FunctionLiteral::FunctionType function_type,
                     DeclarationScope* function_scope, int* num_parameters,
@@ -782,6 +792,10 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
                                              arg, error_type);
   }
 
+  // Dummy implementation. The parser should never have a unidentifiable
+  // error.
+  V8_INLINE void ReportUnidentifiableError() { UNREACHABLE(); }
+
   void ReportMessageAt(Scanner::Location source_location,
                        MessageTemplate::Template message,
                        const AstRawString* arg,

src/parsing/preparser.cc

@@ -128,7 +128,6 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   function_scope->set_is_being_lazily_parsed(true);
 #endif
 
-  DCHECK(!track_unresolved_variables_);
   track_unresolved_variables_ =
       ShouldTrackUnresolvedVariables(is_inner_function);
 
@@ -168,7 +167,11 @@ PreParser::PreParseResult PreParser::PreParseFunction(
     formals_classifier.reset(new ExpressionClassifier(this, &duplicate_finder));
     // We return kPreParseSuccess in failure cases too - errors are retrieved
     // separately by Parser::SkipLazyFunctionBody.
-    ParseFormalParameterList(&formals, CHECK_OK_VALUE(kPreParseSuccess));
+    ParseFormalParameterList(
+        &formals,
+        CHECK_OK_VALUE(pending_error_handler()->ErrorUnidentifiableByPreParser()
+                           ? kPreParseNotIdentifiableError
+                           : kPreParseSuccess));
     Expect(Token::RPAREN, CHECK_OK_VALUE(kPreParseSuccess));
     int formals_end_position = scanner()->location().end_pos;
 
@@ -221,10 +224,15 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   track_unresolved_variables_ = false;
 
   if (result == kLazyParsingAborted) {
+    DCHECK(!pending_error_handler()->ErrorUnidentifiableByPreParser());
     return kPreParseAbort;
   } else if (stack_overflow()) {
+    DCHECK(!pending_error_handler()->ErrorUnidentifiableByPreParser());
     return kPreParseStackOverflow;
   } else if (!*ok) {
+    if (pending_error_handler()->ErrorUnidentifiableByPreParser()) {
+      return kPreParseNotIdentifiableError;
+    }
     DCHECK(pending_error_handler()->has_pending_error());
   } else {
     DCHECK_EQ(Token::RBRACE, scanner()->peek());
@@ -235,19 +243,25 @@ PreParser::PreParseResult PreParser::PreParseFunction(
       const bool allow_duplicate_parameters =
           is_sloppy(function_scope->language_mode()) && formals.is_simple &&
           !IsConciseMethod(kind);
-      ValidateFormalParameters(function_scope->language_mode(),
-                               allow_duplicate_parameters,
-                               CHECK_OK_VALUE(kPreParseSuccess));
+      ValidateFormalParameters(
+          function_scope->language_mode(), allow_duplicate_parameters,
+          CHECK_OK_VALUE(
+              pending_error_handler()->ErrorUnidentifiableByPreParser()
+                  ? kPreParseNotIdentifiableError
+                  : kPreParseSuccess));
 
       *produced_preparsed_scope_data = ProducedPreParsedScopeData::For(
           preparsed_scope_data_builder_, main_zone());
     }
+    DCHECK(!pending_error_handler()->ErrorUnidentifiableByPreParser());
 
     if (is_strict(function_scope->language_mode())) {
       int end_pos = scanner()->location().end_pos;
       CheckStrictOctalLiteral(function_scope->start_position(), end_pos, ok);
     }
   }
+
+  DCHECK(!pending_error_handler()->ErrorUnidentifiableByPreParser());
   return kPreParseSuccess;
 }
 

src/parsing/preparser.h

@@ -953,6 +953,7 @@ struct ParserTypes<PreParser> {
   typedef PreParserFuncNameInferrer FuncNameInferrer;
   typedef PreParserSourceRange SourceRange;
   typedef PreParserSourceRangeScope SourceRangeScope;
+  static constexpr bool ExpressionClassifierReportErrors = false;
 };
 
 
@@ -980,6 +981,7 @@ class PreParser : public ParserBase<PreParser> {
   enum PreParseResult {
     kPreParseStackOverflow,
     kPreParseAbort,
+    kPreParseNotIdentifiableError,
     kPreParseSuccess
   };
 
@@ -1557,6 +1559,10 @@ class PreParser : public ParserBase<PreParser> {
                                              arg, error_type);
   }
 
+  V8_INLINE void ReportUnidentifiableError() {
+    pending_error_handler()->SetUnidentifiableError();
+  }
+
   V8_INLINE void ReportMessageAt(Scanner::Location source_location,
                                  MessageTemplate::Template message,
                                  const PreParserIdentifier& arg,

src/pending-compilation-error-handler.h

@@ -62,6 +62,12 @@ class PendingCompilationErrorHandler {
 
   Handle<String> FormatErrorMessageForTest(Isolate* isolate) const;
 
+  bool SetUnidentifiableError() { return unidentifiable_error_ = true; }
+
+  bool ResetUnidentifiableError() { return unidentifiable_error_ = false; }
+
+  bool ErrorUnidentifiableByPreParser() { return unidentifiable_error_; }
+
  private:
   class MessageDetails {
    public:
@@ -97,6 +103,7 @@ class PendingCompilationErrorHandler {
 
   bool has_pending_error_;
   bool stack_overflow_;
+  bool unidentifiable_error_ = false;
 
   MessageDetails error_details_;
   ParseErrorType error_type_;

test/cctest/test-parsing.cc

@@ -1550,8 +1550,10 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
           "However, the preparser succeeded",
           source->ToCString().get(), message_string->ToCString().get());
     }
-    // Check that preparser and parser produce the same error.
-    if (test_preparser && !ignore_error_msg) {
+    // Check that preparser and parser produce the same error, except for cases
+    // where we do not track errors in the preparser.
+    if (test_preparser && !ignore_error_msg &&
+        !pending_error_handler.ErrorUnidentifiableByPreParser()) {
       i::Handle<i::String> preparser_message =
           pending_error_handler.FormatErrorMessageForTest(CcTest::i_isolate());
       if (!i::String::Equals(isolate, message_string, preparser_message)) {
@@ -2041,7 +2043,7 @@ TEST(ErrorsFutureStrictReservedWords) {
                                           {"() => {", "}"},
                                           {nullptr, nullptr}};
   const char* invalid_statements[] = {
-      FUTURE_STRICT_RESERVED_LEX_BINDINGS("let") nullptr};
+      FUTURE_STRICT_RESERVED_LEX_BINDINGS(let) nullptr};
 
   RunParserSyncTest(non_strict_contexts, invalid_statements, kError);
 }