[parser] Track names in VariableDeclarationParsingScope

This allows us to stop tracking variables_ in the preparser.

This currently makes us track slightly more variables than neccessary in the
case `for (var ...` since `var ... of` needs to check conflicts with out simple
catch variables. We should probably track the names through a ScopedPtrList
instead of a ZonePtrList anyway. Then it won't matter anymore.

Change-Id: I64e3f9ab13af8269456439cf15b0bc4d5b9e5380
Reviewed-on: https://chromium-review.googlesource.com/c/1421360Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58960}

src/ast/ast.h

@@ -1585,18 +1585,7 @@ class VariableProxy final : public Expression {
     bit_field_ = IsRemovedFromUnresolvedField::update(bit_field_, true);
   }
 
-  // Provides an access type for the ThreadedList used by the PreParsers
-  // expressions, lists, and formal parameters.
-  struct PreParserNext {
-    static VariableProxy** next(VariableProxy* t) {
-      return t->pre_parser_expr_next();
-    }
-
-    static VariableProxy** start(VariableProxy** head) { return head; }
-  };
-
-  // Provides an access type for the ThreadedList used by the PreParsers
-  // expressions, lists, and formal parameters.
+  // Provides filtered access to the unresolved variable proxy threaded list.
   struct UnresolvedNext {
     static VariableProxy** filter(VariableProxy** t) {
       VariableProxy** n = t;
@@ -1621,8 +1610,7 @@ class VariableProxy final : public Expression {
                 int start_position)
       : Expression(start_position, kVariableProxy),
         raw_name_(name),
-        next_unresolved_(nullptr),
-        pre_parser_expr_next_(nullptr) {
+        next_unresolved_(nullptr) {
     bit_field_ |= IsThisField::encode(variable_kind == THIS_VARIABLE) |
                   IsAssignedField::encode(false) |
                   IsResolvedField::encode(false) |
@@ -1651,9 +1639,6 @@ class VariableProxy final : public Expression {
   V8_INLINE VariableProxy** next() { return &next_unresolved_; }
   VariableProxy* next_unresolved_;
 
-  VariableProxy** pre_parser_expr_next() { return &pre_parser_expr_next_; }
-  VariableProxy* pre_parser_expr_next_;
-
   friend base::ThreadedListTraits<VariableProxy>;
 };
 

src/parsing/expression-scope.h

@@ -263,11 +263,13 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
   typedef class ExpressionScope<Types> ExpressionScopeT;
   typedef typename ExpressionScopeT::ScopeType ScopeType;
 
-  VariableDeclarationParsingScope(ParserT* parser, VariableMode mode)
+  VariableDeclarationParsingScope(ParserT* parser, VariableMode mode,
+                                  ZonePtrList<const AstRawString>* names)
       : ExpressionScopeT(parser, IsLexicalVariableMode(mode)
                                      ? ExpressionScopeT::kLexicalDeclaration
                                      : ExpressionScopeT::kVarDeclaration),
-        mode_(mode) {}
+        mode_(mode),
+        names_(names) {}
 
   VariableProxy* Declare(VariableProxy* proxy) {
     VariableKind kind = NORMAL_VARIABLE;
@@ -275,6 +277,7 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
     this->parser()->DeclareVariable(
         proxy, kind, mode_, Variable::DefaultInitializationFlag(mode_),
         this->parser()->scope(), &added, proxy->position());
+    if (names_) names_->Add(proxy->raw_name(), this->parser()->zone());
     if (!this->IsLexicalDeclaration()) {
       if (this->parser()->loop_nesting_depth() > 0) {
         // Due to hoisting, the value of a 'var'-declared variable may actually
@@ -310,6 +313,7 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
 
  private:
   VariableMode mode_;
+  ZonePtrList<const AstRawString>* names_;
 
   DISALLOW_COPY_AND_ASSIGN(VariableDeclarationParsingScope);
 };

src/parsing/parser-base.h

@@ -1062,7 +1062,8 @@ class ParserBase {
                               int formals_end_pos);
 
   void ParseVariableDeclarations(VariableDeclarationContext var_context,
-                                 DeclarationParsingResult* parsing_result);
+                                 DeclarationParsingResult* parsing_result,
+                                 ZonePtrList<const AstRawString>* names);
   StatementT ParseAsyncFunctionDeclaration(
       ZonePtrList<const AstRawString>* names, bool default_export);
   StatementT ParseFunctionDeclaration();
@@ -1157,6 +1158,39 @@ class ParserBase {
       ZonePtrList<const AstRawString>* labels,
       ZonePtrList<const AstRawString>* own_labels);
 
+  void DesugarBindingInForEachStatement(ForInfo* for_info, BlockT* body_block,
+                                        ExpressionT* each_variable) {
+    // Annex B.3.5 prohibits the form
+    // `try {} catch(e) { for (var e of {}); }`
+    // So if we are parsing a statement like `for (var ... of ...)`
+    // we need to walk up the scope chain and look for catch scopes
+    // which have a simple binding, then compare their binding against
+    // all of the names declared in the init of the for-of we're
+    // parsing.
+    bool is_for_var_of =
+        for_info->mode == ForEachStatement::ITERATE &&
+        for_info->parsing_result.descriptor.mode == VariableMode::kVar;
+
+    if (is_for_var_of) {
+      Scope* scope = this->scope();
+      while (scope != nullptr && !scope->is_declaration_scope()) {
+        if (scope->is_catch_scope()) {
+          auto name = scope->catch_variable()->raw_name();
+          // If it's a simple binding and the name is declared in the for loop.
+          if (name != ast_value_factory()->dot_catch_string() &&
+              for_info->bound_names.Contains(name)) {
+            impl()->ReportMessageAt(for_info->parsing_result.bindings_loc,
+                                    MessageTemplate::kVarRedeclaration, name);
+          }
+        }
+        scope = scope->outer_scope();
+      }
+    }
+
+    impl()->DesugarBindingInForEachStatement(for_info, body_block,
+                                             each_variable);
+  }
+
   bool IsNextLetKeyword();
 
   // Checks if the expression is a valid reference expression (e.g., on the
@@ -3409,7 +3443,8 @@ void ParserBase<Impl>::ParseFormalParameterList(FormalParametersT* parameters) {
 template <typename Impl>
 void ParserBase<Impl>::ParseVariableDeclarations(
     VariableDeclarationContext var_context,
-    DeclarationParsingResult* parsing_result) {
+    DeclarationParsingResult* parsing_result,
+    ZonePtrList<const AstRawString>* names) {
   // VariableDeclarations ::
   //   ('var' | 'const' | 'let') (Identifier ('=' AssignmentExpression)?)+[',']
   //
@@ -3442,28 +3477,27 @@ void ParserBase<Impl>::ParseVariableDeclarations(
       break;
   }
 
+  VariableDeclarationParsingScope declaration(
+      impl(), parsing_result->descriptor.mode, names);
+
   int bindings_start = peek_position();
   do {
     // Parse binding pattern.
     FuncNameInferrerState fni_state(&fni_);
 
-    ExpressionT pattern = impl()->NullExpression();
     int decl_pos = peek_position();
-    {
-      VariableDeclarationParsingScope declaration(
-          impl(), parsing_result->descriptor.mode);
-      pattern = ParseBindingPattern();
-
-      if (IsLexicalVariableMode(parsing_result->descriptor.mode)) {
-        if (impl()->IsIdentifier(pattern)) {
-          if (impl()->IsLet(impl()->AsIdentifier(pattern))) {
-            impl()->ReportMessageAt(
-                Scanner::Location(bindings_start, end_position()),
-                MessageTemplate::kLetInLexicalBinding);
-          }
+    ExpressionT pattern = ParseBindingPattern();
+
+    if (IsLexicalVariableMode(parsing_result->descriptor.mode)) {
+      if (impl()->IsIdentifier(pattern)) {
+        if (impl()->IsLet(impl()->AsIdentifier(pattern))) {
+          impl()->ReportMessageAt(
+              Scanner::Location(bindings_start, end_position()),
+              MessageTemplate::kLetInLexicalBinding);
         }
       }
     }
+
     Scanner::Location variable_loc = scanner()->location();
 
     ExpressionT value = impl()->NullExpression();
@@ -4681,9 +4715,9 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseVariableStatement(
   // is inside an initializer block, it is ignored.
 
   DeclarationParsingResult parsing_result;
-  ParseVariableDeclarations(var_context, &parsing_result);
+  ParseVariableDeclarations(var_context, &parsing_result, names);
   ExpectSemicolon();
-  return impl()->BuildInitializationBlock(&parsing_result, names);
+  return impl()->BuildInitializationBlock(&parsing_result);
 }
 
 template <typename Impl>
@@ -5166,8 +5200,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseTryStatement() {
             } else {
               catch_info.variable = catch_info.scope->DeclareCatchVariableName(
                   ast_value_factory()->dot_catch_string());
-              VariableDeclarationParsingScope destructuring(impl(),
-                                                            VariableMode::kLet);
+              VariableDeclarationParsingScope destructuring(
+                  impl(), VariableMode::kLet, nullptr);
               catch_info.pattern = ParseBindingPattern();
               RETURN_IF_PARSE_ERROR;
               catch_statements.Add(impl()->RewriteCatchPattern(&catch_info));
@@ -5259,7 +5293,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
     Scope* inner_block_scope = NewScope(BLOCK_SCOPE);
     {
       BlockState inner_state(&scope_, inner_block_scope);
-      ParseVariableDeclarations(kForStatement, &for_info.parsing_result);
+      ParseVariableDeclarations(kForStatement, &for_info.parsing_result,
+                                &for_info.bound_names);
     }
     DCHECK(IsLexicalVariableMode(for_info.parsing_result.descriptor.mode));
     for_info.position = position();
@@ -5279,8 +5314,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
     inner_block_scope->set_start_position(scope()->start_position());
     {
       BlockState inner_state(&scope_, inner_block_scope);
-      StatementT init = impl()->BuildInitializationBlock(
-          &for_info.parsing_result, &for_info.bound_names);
+      StatementT init =
+          impl()->BuildInitializationBlock(&for_info.parsing_result);
 
       result = ParseStandardForLoopWithLexicalDeclarations(
           stmt_pos, init, &for_info, labels, own_labels);
@@ -5293,7 +5328,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
 
   StatementT init = impl()->NullStatement();
   if (peek() == Token::VAR) {
-    ParseVariableDeclarations(kForStatement, &for_info.parsing_result);
+    ParseVariableDeclarations(kForStatement, &for_info.parsing_result,
+                              &for_info.bound_names);
     DCHECK_EQ(for_info.parsing_result.descriptor.mode, VariableMode::kVar);
     for_info.position = scanner()->location().beg_pos;
 
@@ -5302,7 +5338,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
                                                    own_labels, scope());
     }
 
-    init = impl()->BuildInitializationBlock(&for_info.parsing_result, nullptr);
+    init = impl()->BuildInitializationBlock(&for_info.parsing_result);
   } else if (peek() != Token::SEMICOLON) {
     // The initializer does not contain declarations.
     int lhs_beg_pos = peek_position();
@@ -5407,8 +5443,7 @@ ParserBase<Impl>::ParseForEachStatementWithDeclarations(
     }
     impl()->RecordIterationStatementSourceRange(loop, body_range);
 
-    impl()->DesugarBindingInForEachStatement(for_info, &body_block,
-                                             &each_variable);
+    DesugarBindingInForEachStatement(for_info, &body_block, &each_variable);
     body_block->statements()->Add(body, zone());
 
     if (IsLexicalVariableMode(for_info->parsing_result.descriptor.mode)) {
@@ -5599,7 +5634,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForAwaitStatement(
 
     {
       BlockState inner_state(&scope_, inner_block_scope);
-      ParseVariableDeclarations(kForStatement, &for_info.parsing_result);
+      ParseVariableDeclarations(kForStatement, &for_info.parsing_result,
+                                &for_info.bound_names);
     }
     for_info.position = scanner()->location().beg_pos;
 
@@ -5663,8 +5699,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForAwaitStatement(
 
     if (has_declarations) {
       BlockT body_block = impl()->NullBlock();
-      impl()->DesugarBindingInForEachStatement(&for_info, &body_block,
-                                               &each_variable);
+      DesugarBindingInForEachStatement(&for_info, &body_block, &each_variable);
       body_block->statements()->Add(body, zone());
       body_block->set_scope(scope()->FinalizeBlockScope());
       body = body_block;

src/parsing/parser.cc

@@ -1407,12 +1407,11 @@ void Parser::Declare(Declaration* declaration, VariableProxy* proxy,
 }
 
 Statement* Parser::BuildInitializationBlock(
-    DeclarationParsingResult* parsing_result,
-    ZonePtrList<const AstRawString>* names) {
+    DeclarationParsingResult* parsing_result) {
   ScopedPtrList<Statement> statements(pointer_buffer());
   for (const auto& declaration : parsing_result->declarations) {
     InitializeVariables(&statements, parsing_result->descriptor.kind,
-                        &declaration, names);
+                        &declaration);
   }
   return factory()->NewBlock(true, statements);
 }
@@ -1603,7 +1602,7 @@ Block* Parser::RewriteCatchPattern(CatchInfo* catch_info) {
       factory()->NewVariableProxy(catch_info->variable));
 
   ScopedPtrList<Statement> init_statements(pointer_buffer());
-  InitializeVariables(&init_statements, NORMAL_VARIABLE, &decl, nullptr);
+  InitializeVariables(&init_statements, NORMAL_VARIABLE, &decl);
   return factory()->NewBlock(true, init_statements);
 }
 
@@ -1825,42 +1824,8 @@ void Parser::DesugarBindingInForEachStatement(ForInfo* for_info,
       for_info->parsing_result.declarations[0];
   Variable* temp = NewTemporary(ast_value_factory()->dot_for_string());
   ScopedPtrList<Statement> each_initialization_statements(pointer_buffer());
-  {
-    decl.initializer = factory()->NewVariableProxy(temp);
-
-    bool is_for_var_of =
-        for_info->mode == ForEachStatement::ITERATE &&
-        for_info->parsing_result.descriptor.mode == VariableMode::kVar;
-    bool collect_names =
-        IsLexicalVariableMode(for_info->parsing_result.descriptor.mode) ||
-        is_for_var_of;
-
-    InitializeVariables(&each_initialization_statements, NORMAL_VARIABLE, &decl,
-                        collect_names ? &for_info->bound_names : nullptr);
-
-    // Annex B.3.5 prohibits the form
-    // `try {} catch(e) { for (var e of {}); }`
-    // So if we are parsing a statement like `for (var ... of ...)`
-    // we need to walk up the scope chain and look for catch scopes
-    // which have a simple binding, then compare their binding against
-    // all of the names declared in the init of the for-of we're
-    // parsing.
-    if (is_for_var_of) {
-      Scope* catch_scope = scope();
-      while (catch_scope != nullptr && !catch_scope->is_declaration_scope()) {
-        if (catch_scope->is_catch_scope()) {
-          auto name = catch_scope->catch_variable()->raw_name();
-          // If it's a simple binding and the name is declared in the for loop.
-          if (name != ast_value_factory()->dot_catch_string() &&
-              for_info->bound_names.Contains(name)) {
-            ReportMessageAt(for_info->parsing_result.bindings_loc,
-                            MessageTemplate::kVarRedeclaration, name);
-          }
-        }
-        catch_scope = catch_scope->outer_scope();
-      }
-    }
-  }
+  decl.initializer = factory()->NewVariableProxy(temp);
+  InitializeVariables(&each_initialization_statements, NORMAL_VARIABLE, &decl);
 
   *body_block = factory()->NewBlock(3, false);
   (*body_block)
@@ -2658,8 +2623,7 @@ Block* Parser::BuildParameterInitializationBlock(
     DeclarationParsingResult::Declaration decl(
         parameter->pattern, parameter->initializer_end_position, initial_value);
 
-    InitializeVariables(param_init_statements, PARAMETER_VARIABLE, &decl,
-                        nullptr);
+    InitializeVariables(param_init_statements, PARAMETER_VARIABLE, &decl);
 
     if (param_init_statements != &init_statements) {
       DCHECK_EQ(param_init_statements,

src/parsing/parser.h

@@ -315,8 +315,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
           location(location) {}
   };
   ZonePtrList<const NamedImport>* ParseNamedImports(int pos);
-  Statement* BuildInitializationBlock(DeclarationParsingResult* parsing_result,
-                                      ZonePtrList<const AstRawString>* names);
+  Statement* BuildInitializationBlock(DeclarationParsingResult* parsing_result);
   void DeclareLabel(ZonePtrList<const AstRawString>** labels,
                     ZonePtrList<const AstRawString>** own_labels,
                     VariableProxy* expr);
@@ -381,8 +380,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   friend class PatternRewriter;
   void InitializeVariables(
       ScopedPtrList<Statement>* statements, VariableKind kind,
-      const DeclarationParsingResult::Declaration* declaration,
-      ZonePtrList<const AstRawString>* names);
+      const DeclarationParsingResult::Declaration* declaration);
 
   Block* RewriteForVarInLegacy(const ForInfo& for_info);
   void DesugarBindingInForEachStatement(ForInfo* for_info, Block** body_block,
@@ -742,13 +740,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   void ReportMessageAt(Scanner::Location source_location,
                        MessageTemplate message, const char* arg = nullptr,
                        ParseErrorType error_type = kSyntaxError) {
-    if (stack_overflow()) {
-      // Suppress the error message (syntax error or such) in the presence of a
-      // stack overflow. The isolate allows only one pending exception at at
-      // time
-      // and we want to report the stack overflow later.
-      return;
-    }
     pending_error_handler()->ReportMessageAt(source_location.beg_pos,
                                              source_location.end_pos, message,
                                              arg, error_type);
@@ -762,13 +753,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   void ReportMessageAt(Scanner::Location source_location,
                        MessageTemplate message, const AstRawString* arg,
                        ParseErrorType error_type = kSyntaxError) {
-    if (stack_overflow()) {
-      // Suppress the error message (syntax error or such) in the presence of a
-      // stack overflow. The isolate allows only one pending exception at at
-      // time
-      // and we want to report the stack overflow later.
-      return;
-    }
     pending_error_handler()->ReportMessageAt(source_location.beg_pos,
                                              source_location.end_pos, message,
                                              arg, error_type);

src/parsing/pattern-rewriter.cc

@@ -40,17 +40,14 @@ class PatternRewriter final : public AstVisitor<PatternRewriter> {
 
   static void InitializeVariables(
       Parser* parser, VariableKind kind,
-      const Parser::DeclarationParsingResult::Declaration* declaration,
-      ZonePtrList<const AstRawString>* names);
+      const Parser::DeclarationParsingResult::Declaration* declaration);
 
  private:
   PatternRewriter(Parser* parser, VariableKind kind,
-                  ZonePtrList<const AstRawString>* names,
                   int initializer_position = kNoSourcePosition,
                   bool declares_parameter_containing_sloppy_eval = false)
       : parser_(parser),
         kind_(kind),
-        names_(names),
         initializer_position_(initializer_position),
         declares_parameter_containing_sloppy_eval_(
             declares_parameter_containing_sloppy_eval) {}
@@ -90,7 +87,6 @@ class PatternRewriter final : public AstVisitor<PatternRewriter> {
 
   Parser* const parser_;
   VariableKind kind_;
-  ZonePtrList<const AstRawString>* names_;
   const int initializer_position_;
   const bool declares_parameter_containing_sloppy_eval_;
 
@@ -99,10 +95,9 @@ class PatternRewriter final : public AstVisitor<PatternRewriter> {
 
 void Parser::InitializeVariables(
     ScopedPtrList<Statement>* statements, VariableKind kind,
-    const DeclarationParsingResult::Declaration* declaration,
-    ZonePtrList<const AstRawString>* names) {
+    const DeclarationParsingResult::Declaration* declaration) {
   if (has_error()) return;
-  PatternRewriter::InitializeVariables(this, kind, declaration, names);
+  PatternRewriter::InitializeVariables(this, kind, declaration);
 
   if (declaration->initializer) {
     int pos = declaration->value_beg_position;
@@ -117,10 +112,9 @@ void Parser::InitializeVariables(
 
 void PatternRewriter::InitializeVariables(
     Parser* parser, VariableKind kind,
-    const Parser::DeclarationParsingResult::Declaration* declaration,
-    ZonePtrList<const AstRawString>* names) {
+    const Parser::DeclarationParsingResult::Declaration* declaration) {
   PatternRewriter rewriter(
-      parser, kind, names, declaration->initializer_position,
+      parser, kind, declaration->initializer_position,
       kind == PARAMETER_VARIABLE && parser->scope()->is_block_scope());
 
   rewriter.RecurseIntoSubpattern(declaration->pattern);
@@ -154,9 +148,6 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* proxy) {
     parser_->ReportMessage(MessageTemplate::kTooManyVariables);
     return;
   }
-  if (names_) {
-    names_->Add(proxy->raw_name(), zone());
-  }
 }
 
 // When an extra declaration scope needs to be inserted to account for

src/parsing/preparser.cc

@@ -419,18 +419,5 @@ PreParserExpression PreParser::ExpressionFromIdentifier(
   return PreParserExpression::FromIdentifier(name, proxy, zone());
 }
 
-void PreParser::InitializeVariables(
-    const DeclarationParsingResult::Declaration* declaration,
-    ZonePtrList<const AstRawString>* names) {
-  if (names && declaration->pattern.variables_ != nullptr) {
-    for (auto variable : *(declaration->pattern.variables_)) {
-      // This is only necessary if there is an initializer, but we don't have
-      // that information here.  Consequently, the preparser sometimes says
-      // maybe-assigned where the parser (correctly) says never-assigned.
-      names->Add(variable->raw_name(), zone());
-    }
-  }
-}
-
 }  // namespace internal
 }  // namespace v8

src/zone/zone-list-inl.h

@@ -128,14 +128,6 @@ void ZoneList<T>::Iterate(Visitor* visitor) {
   for (int i = 0; i < length_; i++) visitor->Apply(&data_[i]);
 }
 
-template <typename T>
-bool ZoneList<T>::Contains(const T& elm) const {
-  for (int i = 0; i < length_; i++) {
-    if (data_[i] == elm) return true;
-  }
-  return false;
-}
-
 template <typename T>
 template <typename CompareFunction>
 void ZoneList<T>::Sort(CompareFunction cmp) {

src/zone/zone.h

@@ -277,7 +277,12 @@ class ZoneList final {
   // Drops all but the first 'pos' elements from the list.
   V8_INLINE void Rewind(int pos);
 
-  inline bool Contains(const T& elm) const;
+  inline bool Contains(const T& elm) const {
+    for (int i = 0; i < length_; i++) {
+      if (data_[i] == elm) return true;
+    }
+    return false;
+  }
 
   // Iterate through all list entries, starting at index 0.
   template <class Visitor>

test/mjsunit/for-of-in-catch-duplicate-decl.js

+// Copyright 2019 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.
+
+assertThrows("try { } catch (e) { var e; for (var e of []) {} }")