[parser] Replacing ExpressionClassifier with ExpressionScope that knows what it's tracking

Since it's explicit what we're tracking, we can immediately throw errors in
certain cases, and ignore irrelevant errors. We don't need to use the
classifier itself to track "let let", since we know whether we're parsing a
"let". Errors that were previously (almost) always accumulated are now
immediately pushed to the scopes that care (parameter initialization errors).

This CL drops avoiding allocation of classified errors, at least for now, but
that doesn't affect performance anymore since we don't aggressively blacklist
anymore. Classified errors are even less likely with the more precise approach.

ParseAssignmentExpression doesn't introduce its own scope immediately, but
reuses the outer scope.

Rather than using full ExpressionClassifiers + Accumulate to separate
expressions/patterns from each other while keeping track of the overall error
state, this now uses an explicit AccumulationScope.

When we parse (async) arrow functions we introduce new scopes
that track that they may be (async) arrow functions.

We track StrictModeFormal parameters in 2 different ways if it isn't
immediately certain that it is a strict-mode formal error: Either directly on
the (Pre)ParserFormalParameters, or on the NextArrowFunctionInfo in the case
we're not yet certain that we'll have an arrow function. In the latter case we
don't have a FormalParameter object yet, and we'll copy it over once we know
we're parsing an arrow function. The latter works because it's not allowed to
change strictness of a function with non-simple parameters.

Design doc:
https://docs.google.com/document/d/1FAvEp9EUK-G8kHfDIEo_385Hs2SUBCYbJ5H-NnLvq8M/

Change-Id: If4ecd717c9780095c7ddc859c8945b3d7d268a9d
Reviewed-on: https://chromium-review.googlesource.com/c/1367809
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58307}

BUILD.gn

@@ -2433,9 +2433,9 @@ v8_source_set("v8_base") {
     "src/optimized-compilation-info.h",
     "src/ostreams.cc",
     "src/ostreams.h",
-    "src/parsing/expression-classifier.h",
     "src/parsing/expression-scope-reparenter.cc",
     "src/parsing/expression-scope-reparenter.h",
+    "src/parsing/expression-scope.h",
     "src/parsing/func-name-inferrer.cc",
     "src/parsing/func-name-inferrer.h",
     "src/parsing/parse-info.cc",

src/ast/scopes.cc

@@ -844,6 +844,7 @@ void DeclarationScope::AddLocal(Variable* var) {
 }
 
 void Scope::Snapshot::Reparent(DeclarationScope* new_parent) {
+  DCHECK(!IsCleared());
   DCHECK_EQ(new_parent, outer_scope_and_calls_eval_.GetPointer()->inner_scope_);
   DCHECK_EQ(new_parent->outer_scope_, outer_scope_and_calls_eval_.GetPointer());
   DCHECK_EQ(new_parent, new_parent->GetClosureScope());

src/parsing/parser.cc

@@ -758,12 +758,13 @@ FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
       SetLanguageMode(scope, info->language_mode());
 
       scope->set_start_position(info->start_position());
-      ExpressionClassifier formals_classifier(this);
       ParserFormalParameters formals(scope);
       // The outer FunctionState should not contain destructuring assignments.
       DCHECK_EQ(0,
                 function_state.destructuring_assignments_to_rewrite().size());
       {
+        DeclarationParsingScope formals_scope(
+            this, ExpressionScope::kParameterDeclaration);
         // Parsing patterns as variable reference expression creates
         // NewUnresolved references in current scope. Enter arrow function
         // scope for formal parameter parsing.
@@ -774,6 +775,7 @@ FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
           Expect(Token::RPAREN);
         } else {
           // BindingIdentifier
+          ParameterParsingScope scope(impl(), &formals);
           ParseFormalParameter(&formals);
           DeclareFormalParameters(&formals);
         }
@@ -1131,10 +1133,8 @@ Statement* Parser::ParseExportDefault() {
 
     default: {
       int pos = position();
-      ExpressionClassifier classifier(this);
       AcceptINScope scope(this, true);
       Expression* value = ParseAssignmentExpression();
-      ValidateExpression();
       SetFunctionName(value, ast_value_factory()->default_string());
 
       const AstRawString* local_name =
@@ -2319,6 +2319,17 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
   return outer_block;
 }
 
+void ParserFormalParameters::ValidateDuplicate(Parser* parser) const {
+  if (has_duplicate()) {
+    parser->ReportMessageAt(duplicate_loc, MessageTemplate::kParamDupe);
+  }
+}
+void ParserFormalParameters::ValidateStrictMode(Parser* parser) const {
+  if (strict_error_loc.IsValid()) {
+    parser->ReportMessageAt(strict_error_loc, strict_error_message);
+  }
+}
+
 void Parser::AddArrowFunctionFormalParameters(
     ParserFormalParameters* parameters, Expression* expr, int end_pos) {
   // ArrowFunctionFormals ::
@@ -2941,8 +2952,6 @@ void Parser::ParseFunction(
 
   bool is_wrapped = function_type == FunctionLiteral::kWrapped;
 
-  ExpressionClassifier formals_classifier(this);
-
   int expected_parameters_end_pos = parameters_end_pos_;
   if (expected_parameters_end_pos != kNoSourcePosition) {
     // This is the first function encountered in a CreateDynamicFunction eval.
@@ -2971,6 +2980,8 @@ void Parser::ParseFunction(
   } else {
     // For a regular function, the function arguments are parsed from source.
     DCHECK_NULL(arguments_for_wrapped_function);
+    DeclarationParsingScope formals_scope(
+        this, ExpressionScope::kParameterDeclaration);
     ParseFormalParameterList(&formals);
     if (expected_parameters_end_pos != kNoSourcePosition) {
       // Check for '(' or ')' shenanigans in the parameter string for dynamic

src/parsing/parser.h

@@ -113,13 +113,23 @@ struct ParserFormalParameters : FormalParametersBase {
     Parameter* const* next() const { return &next_parameter; }
   };
 
-  Scanner::Location duplicate_location() const { return duplicate_loc; }
+  void set_strict_parameter_error(const Scanner::Location& loc,
+                                  MessageTemplate message) {
+    strict_error_loc = loc;
+    strict_error_message = message;
+  }
+
   bool has_duplicate() const { return duplicate_loc.IsValid(); }
+  void ValidateDuplicate(Parser* parser) const;
+  void ValidateStrictMode(Parser* parser) const;
 
   explicit ParserFormalParameters(DeclarationScope* scope)
       : FormalParametersBase(scope) {}
+
   base::ThreadedList<Parameter> params;
   Scanner::Location duplicate_loc = Scanner::Location::invalid();
+  Scanner::Location strict_error_loc = Scanner::Location::invalid();
+  MessageTemplate strict_error_message = MessageTemplate::kNone;
 };
 
 template <>
@@ -155,8 +165,6 @@ struct ParserTypes<Parser> {
   typedef v8::internal::SourceRangeScope SourceRangeScope;
   typedef ParserTarget Target;
   typedef ParserTargetScope TargetScope;
-
-  static constexpr bool ExpressionClassifierReportErrors = true;
 };
 
 class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
@@ -192,8 +200,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
 
  private:
   friend class ParserBase<Parser>;
-  friend class v8::internal::ExpressionClassifierErrorTracker<
-      ParserTypes<Parser>>;
+  friend struct ParserFormalParameters;
+  friend class i::ExpressionScope<ParserTypes<Parser>>;
   friend bool v8::internal::parsing::ParseProgram(ParseInfo*, Isolate*);
   friend bool v8::internal::parsing::ParseFunction(
       ParseInfo*, Handle<SharedFunctionInfo> shared_info, Isolate*);
@@ -915,7 +923,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   }
 
   V8_INLINE void DeclareFormalParameters(ParserFormalParameters* parameters) {
-    ValidateFormalParameterInitializer();
     bool is_simple = parameters->is_simple;
     DeclarationScope* scope = parameters->scope;
     if (!is_simple) scope->SetHasNonSimpleParameters();
@@ -955,11 +962,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   void SetFunctionNameFromIdentifierRef(Expression* value,
                                         Expression* identifier);
 
-  V8_INLINE ZoneList<typename ExpressionClassifier::Error>*
-  GetReportedErrorList() const {
-    return function_state_->GetReportedErrorList();
-  }
-
   V8_INLINE void CountUsage(v8::Isolate::UseCounterFeature feature) {
     ++use_counts_[feature];
   }

src/parsing/preparser.cc

@@ -96,6 +96,14 @@ PreParser::PreParseResult PreParser::PreParseProgram() {
   return kPreParseSuccess;
 }
 
+void PreParserFormalParameters::ValidateDuplicate(PreParser* preparser) const {
+  if (has_duplicate_) preparser->ReportUnidentifiableError();
+}
+
+void PreParserFormalParameters::ValidateStrictMode(PreParser* preparser) const {
+  if (strict_parameter_error_) preparser->ReportUnidentifiableError();
+}
+
 PreParser::PreParseResult PreParser::PreParseFunction(
     const AstRawString* function_name, FunctionKind kind,
     FunctionLiteral::FunctionType function_type,
@@ -131,12 +139,12 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   FunctionState function_state(&function_state_, &scope_, function_scope);
 
   PreParserFormalParameters formals(function_scope);
-  std::unique_ptr<ExpressionClassifier> formals_classifier;
 
   // Parse non-arrow function parameters. For arrow functions, the parameters
   // have already been parsed.
   if (!IsArrowFunction(kind)) {
-    formals_classifier.reset(new ExpressionClassifier(this));
+    DeclarationParsingScope formals_scope(
+        this, ExpressionScope::kParameterDeclaration);
     // We return kPreParseSuccess in failure cases too - errors are retrieved
     // separately by Parser::SkipLazyFunctionBody.
     ParseFormalParameterList(&formals);
@@ -291,13 +299,16 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
     }
 
     FunctionState function_state(&function_state_, &scope_, function_scope);
-    ExpressionClassifier formals_classifier(this);
 
     Expect(Token::LPAREN);
     int start_position = position();
     function_scope->set_start_position(start_position);
     PreParserFormalParameters formals(function_scope);
-    ParseFormalParameterList(&formals);
+    {
+      DeclarationParsingScope formals_scope(
+          this, ExpressionScope::kParameterDeclaration);
+      ParseFormalParameterList(&formals);
+    }
     Expect(Token::RPAREN);
     int formals_end_position = scanner()->location().end_pos;
 
@@ -406,7 +417,7 @@ bool PreParser::IdentifierEquals(const PreParserIdentifier& identifier,
 PreParserExpression PreParser::ExpressionFromIdentifier(
     const PreParserIdentifier& name, int start_position, InferName infer) {
   VariableProxy* proxy = nullptr;
-  DCHECK_EQ(name.string_ == nullptr, has_error());
+  DCHECK_IMPLIES(name.string_ == nullptr, has_error());
   if (name.string_ == nullptr) return PreParserExpression::Default();
   proxy = scope()->NewUnresolved(factory()->ast_node_factory(), name.string_,
                                  start_position, NORMAL_VARIABLE);

src/parsing/preparser.h

@@ -832,21 +832,28 @@ class PreParserFactory {
   Zone* zone_;
 };
 
+class PreParser;
+
 class PreParserFormalParameters : public FormalParametersBase {
  public:
   explicit PreParserFormalParameters(DeclarationScope* scope)
       : FormalParametersBase(scope) {}
 
-  Scanner::Location duplicate_location() const { UNREACHABLE(); }
-  bool has_duplicate() const { return has_duplicate_; }
   void set_has_duplicate() { has_duplicate_ = true; }
+  bool has_duplicate() { return has_duplicate_; }
+  void ValidateDuplicate(PreParser* preparser) const;
+
+  void set_strict_parameter_error(const Scanner::Location& loc,
+                                  MessageTemplate message) {
+    strict_parameter_error_ = loc.IsValid();
+  }
+  void ValidateStrictMode(PreParser* preparser) const;
 
  private:
   bool has_duplicate_ = false;
+  bool strict_parameter_error_ = false;
 };
 
-class PreParser;
-
 class PreParserTarget {
  public:
   PreParserTarget(ParserBase<PreParser>* preparser,
@@ -935,8 +942,6 @@ struct ParserTypes<PreParser> {
   typedef PreParserSourceRangeScope SourceRangeScope;
   typedef PreParserTarget Target;
   typedef PreParserTargetScope TargetScope;
-
-  static constexpr bool ExpressionClassifierReportErrors = false;
 };
 
 
@@ -954,7 +959,6 @@ struct ParserTypes<PreParser> {
 // it is used) are generally omitted.
 class PreParser : public ParserBase<PreParser> {
   friend class ParserBase<PreParser>;
-  friend class v8::internal::ExpressionClassifier<ParserTypes<PreParser>>;
 
  public:
   typedef PreParserIdentifier Identifier;
@@ -1016,6 +1020,8 @@ class PreParser : public ParserBase<PreParser> {
   }
 
  private:
+  friend class i::ExpressionScope<ParserTypes<PreParser>>;
+  friend class PreParserFormalParameters;
   // These types form an algebra over syntactic categories that is just
   // rich enough to let us recognize and propagate the constructs that
   // are either being counted in the preparser data, or is important
@@ -1677,14 +1683,12 @@ class PreParser : public ParserBase<PreParser> {
 
   V8_INLINE void DeclareFormalParameters(
       const PreParserFormalParameters* parameters) {
-    ValidateFormalParameterInitializer();
     if (!parameters->is_simple) parameters->scope->SetHasNonSimpleParameters();
   }
 
   V8_INLINE void DeclareArrowFunctionFormalParameters(
       PreParserFormalParameters* parameters, const PreParserExpression& params,
       const Scanner::Location& params_loc) {
-    ValidateFormalParameterInitializer();
     if (params.variables_ != nullptr) {
       Scope* scope = parameters->scope;
       for (auto variable : *params.variables_) {
@@ -1709,11 +1713,6 @@ class PreParser : public ParserBase<PreParser> {
       const PreParserExpression& value, const PreParserExpression& identifier) {
   }
 
-  V8_INLINE ZoneList<typename ExpressionClassifier::Error>*
-  GetReportedErrorList() const {
-    return function_state_->GetReportedErrorList();
-  }
-
   V8_INLINE void CountUsage(v8::Isolate::UseCounterFeature feature) {
     if (use_counts_ != nullptr) ++use_counts_[feature];
   }

test/message/regress/fail/regress-903874.out

-*%(basename)s:6: RangeError: Maximum call stack size exceeded
-eval(code);
-     ^
-RangeError: Maximum call stack size exceeded
-    at *%(basename)s:6:6

test/mjsunit/es8/async-arrow-default-function-await.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+async(o = (function(await) {})) => 0

test/message/regress/fail/regress-903874.js → test/mjsunit/regress/regress-903874.js

@@ -3,4 +3,4 @@
 // found in the LICENSE file.
 
 var code = "function f(" + ("{o(".repeat(10000));
-eval(code);
+assertThrows(code, SyntaxError);