[parsing] Improve elision of hole checks for default parameters

Use the position of commas in arrow expressions to mark the initializer
position of any parameters that might have been set in the preceding
parameter.

To enable this, this makes variable_list_ in ExpressionParsingScope a
ScopedList<pair<VariableProxy*, int>> and changes ScopedList::at to
return references so its elements can be modified in place.

This fixes a source of bytecode mismatches when collecting source
positions lazily and is a second attempt at fixing this after
https://chromium-review.googlesource.com/c/v8/v8/+/1683267 introduced
problems due to destructuring.

Bug: chromium:980422, chromium:981701, v8:8510
Change-Id: I948f89f34fb75d7463a13183e363f7f96ad09d13
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1710671Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62936}

src/parsing/expression-scope.h

@@ -5,6 +5,8 @@
 #ifndef V8_PARSING_EXPRESSION_SCOPE_H_
 #define V8_PARSING_EXPRESSION_SCOPE_H_
 
+#include <utility>
+
 #include "src/ast/scopes.h"
 #include "src/common/message-template.h"
 #include "src/objects/function-kind.h"
@@ -173,6 +175,14 @@ class ExpressionScope {
     return IsInRange(type_, kParameterDeclaration, kLexicalDeclaration);
   }
 
+  int SetInitializers(int variable_index, int peek_position) {
+    if (CanBeExpression()) {
+      return AsExpressionParsingScope()->SetInitializers(variable_index,
+                                                         peek_position);
+    }
+    return variable_index;
+  }
+
  protected:
   enum ScopeType : uint8_t {
     // Expression or assignment target.
@@ -458,8 +468,8 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
       ExpressionScopeT::Report(Scanner::Location(begin, end),
                                MessageTemplate::kInvalidDestructuringTarget);
     }
-    for (VariableProxy* proxy : variable_list_) {
-      proxy->set_is_assigned();
+    for (auto& variable_initializer_pair : variable_list_) {
+      variable_initializer_pair.first->set_is_assigned();
     }
   }
 
@@ -475,14 +485,30 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
     if (!this->CanBeDeclaration()) {
       this->parser()->scope()->AddUnresolved(variable);
     }
-    variable_list_.Add(variable);
+    variable_list_.Add({variable, kNoSourcePosition});
   }
 
   void MarkIdentifierAsAssigned() {
     // It's possible we're parsing a syntax error. In that case it's not
     // guaranteed that there's a variable in the list.
     if (variable_list_.length() == 0) return;
-    variable_list_.at(variable_list_.length() - 1)->set_is_assigned();
+    variable_list_.at(variable_list_.length() - 1).first->set_is_assigned();
+  }
+
+  int SetInitializers(int first_variable_index, int position) {
+    int len = variable_list_.length();
+    if (len == 0) return 0;
+
+    int end = len - 1;
+    // Loop backwards and abort as soon as we see one that's already set to
+    // avoid a loop on expressions like a,b,c,d,e,f,g (outside of an arrowhead).
+    // TODO(delphick): Look into removing this loop.
+    for (int i = end; i >= first_variable_index &&
+                      variable_list_.at(i).second == kNoSourcePosition;
+         --i) {
+      variable_list_.at(i).second = position;
+    }
+    return end;
   }
 
  protected:
@@ -496,7 +522,9 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
 
   void ValidatePattern() { Validate(kPatternIndex); }
 
-  ScopedPtrList<VariableProxy>* variable_list() { return &variable_list_; }
+  ScopedList<std::pair<VariableProxy*, int>>* variable_list() {
+    return &variable_list_;
+  }
 
  private:
   friend class AccumulationScope<Types>;
@@ -542,7 +570,7 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
   bool verified_ = false;
 #endif
 
-  ScopedPtrList<VariableProxy> variable_list_;
+  ScopedList<std::pair<VariableProxy*, int>> variable_list_;
   MessageTemplate messages_[kNumberOfErrors];
   Scanner::Location locations_[kNumberOfErrors];
   bool has_async_arrow_in_scope_chain_;
@@ -665,7 +693,8 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
     // references.
     this->parser()->next_arrow_function_info_.ClearStrictParameterError();
     ExpressionParsingScope<Types>::ValidateExpression();
-    for (VariableProxy* proxy : *this->variable_list()) {
+    for (auto& proxy_initializer_pair : *this->variable_list()) {
+      VariableProxy* proxy = proxy_initializer_pair.first;
       this->parser()->scope()->AddUnresolved(proxy);
     }
   }
@@ -683,21 +712,24 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
     VariableKind kind = PARAMETER_VARIABLE;
     VariableMode mode =
         has_simple_parameter_list_ ? VariableMode::kVar : VariableMode::kLet;
-    for (VariableProxy* proxy : *this->variable_list()) {
+    for (auto& proxy_initializer_pair : *this->variable_list()) {
+      VariableProxy* proxy = proxy_initializer_pair.first;
+      int initializer_position = proxy_initializer_pair.second;
       bool was_added;
-      this->parser()->DeclareAndBindVariable(
-          proxy, kind, mode, Variable::DefaultInitializationFlag(mode), result,
-          &was_added, proxy->position());
+      this->parser()->DeclareAndBindVariable(proxy, kind, mode, result,
+                                             &was_added, initializer_position);
       if (!was_added) {
         ExpressionScope<Types>::Report(proxy->location(),
                                        MessageTemplate::kParamDupe);
       }
     }
 
-    int initializer_position = this->parser()->end_position();
+#ifdef DEBUG
     for (auto declaration : *result->declarations()) {
-      declaration->var()->set_initializer_position(initializer_position);
+      DCHECK_NE(declaration->var()->initializer_position(), kNoSourcePosition);
     }
+#endif  // DEBUG
+
     if (uses_this_) result->UsesThis();
     return result;
   }

src/parsing/parser-base.h

@@ -6,6 +6,7 @@
 #define V8_PARSING_PARSER_BASE_H_
 
 #include <stdint.h>
+#include <utility>
 #include <vector>
 
 #include "src/ast/ast-source-ranges.h"
@@ -1019,7 +1020,8 @@ class ParserBase {
   ExpressionT ParseAssignmentExpressionCoverGrammar();
 
   ExpressionT ParseArrowParametersWithRest(ExpressionListT* list,
-                                           AccumulationScope* scope);
+                                           AccumulationScope* scope,
+                                           int seen_variables);
 
   ExpressionT ParseArrayLiteral();
 
@@ -1365,7 +1367,9 @@ class ParserBase {
   };
 
   std::vector<void*>* pointer_buffer() { return &pointer_buffer_; }
-  std::vector<void*>* variable_buffer() { return &variable_buffer_; }
+  std::vector<std::pair<VariableProxy*, int>>* variable_buffer() {
+    return &variable_buffer_;
+  }
 
   // Parser base's protected field members.
 
@@ -1390,7 +1394,7 @@ class ParserBase {
   ExpressionScope* expression_scope_;
 
   std::vector<void*> pointer_buffer_;
-  std::vector<void*> variable_buffer_;
+  std::vector<std::pair<VariableProxy*, int>> variable_buffer_;
 
   Scanner* scanner_;
 
@@ -1688,6 +1692,7 @@ ParserBase<Impl>::ParsePrimaryExpression() {
       ClassifyParameter(name, beg_pos, end_position());
       ExpressionT result =
           impl()->ExpressionFromIdentifier(name, beg_pos, InferName::kNo);
+      parsing_scope.SetInitializers(0, peek_position());
       next_arrow_function_info_.scope = parsing_scope.ValidateAndCreateScope();
       return result;
     }
@@ -1825,9 +1830,11 @@ ParserBase<Impl>::ParseExpressionCoverGrammar() {
   ExpressionListT list(pointer_buffer());
   ExpressionT expression;
   AccumulationScope accumulation_scope(expression_scope());
+  int variable_index = 0;
   while (true) {
     if (V8_UNLIKELY(peek() == Token::ELLIPSIS)) {
-      return ParseArrowParametersWithRest(&list, &accumulation_scope);
+      return ParseArrowParametersWithRest(&list, &accumulation_scope,
+                                          variable_index);
     }
 
     int expr_pos = peek_position();
@@ -1836,6 +1843,9 @@ ParserBase<Impl>::ParseExpressionCoverGrammar() {
     ClassifyArrowParameter(&accumulation_scope, expr_pos, expression);
     list.Add(expression);
 
+    variable_index =
+        expression_scope()->SetInitializers(variable_index, peek_position());
+
     if (!Check(Token::COMMA)) break;
 
     if (peek() == Token::RPAREN && PeekAhead() == Token::ARROW) {
@@ -1863,7 +1873,7 @@ template <typename Impl>
 typename ParserBase<Impl>::ExpressionT
 ParserBase<Impl>::ParseArrowParametersWithRest(
     typename ParserBase<Impl>::ExpressionListT* list,
-    AccumulationScope* accumulation_scope) {
+    AccumulationScope* accumulation_scope, int seen_variables) {
   Consume(Token::ELLIPSIS);
 
   Scanner::Location ellipsis = scanner()->location();
@@ -1885,6 +1895,8 @@ ParserBase<Impl>::ParseArrowParametersWithRest(
     return impl()->FailureExpression();
   }
 
+  expression_scope()->SetInitializers(seen_variables, peek_position());
+
   // 'x, y, ...z' in CoverParenthesizedExpressionAndArrowParameterList only
   // as the formal parameters of'(x, y, ...z) => foo', and is not itself a
   // valid expression.
@@ -3048,6 +3060,7 @@ ParserBase<Impl>::ParseLeftHandSideContinuation(ExpressionT result) {
     ParseArguments(&args, &has_spread, kMaybeArrowHead);
     if (V8_LIKELY(peek() == Token::ARROW)) {
       fni_.RemoveAsyncKeywordFromEnd();
+      expression_scope()->SetInitializers(0, peek_position());
       next_arrow_function_info_.scope = maybe_arrow.ValidateAndCreateScope();
       scope_snapshot.Reparent(next_arrow_function_info_.scope);
       // async () => ...

src/parsing/parser.cc

@@ -1377,11 +1377,12 @@ VariableProxy* Parser::DeclareBoundVariable(const AstRawString* name,
 }
 
 void Parser::DeclareAndBindVariable(VariableProxy* proxy, VariableKind kind,
-                                    VariableMode mode, InitializationFlag init,
-                                    Scope* scope, bool* was_added, int begin,
-                                    int end) {
-  Variable* var = DeclareVariable(proxy->raw_name(), kind, mode, init, scope,
-                                  was_added, begin, end);
+                                    VariableMode mode, Scope* scope,
+                                    bool* was_added, int initializer_position) {
+  Variable* var = DeclareVariable(
+      proxy->raw_name(), kind, mode, Variable::DefaultInitializationFlag(mode),
+      scope, was_added, proxy->position(), kNoSourcePosition);
+  var->set_initializer_position(initializer_position);
   proxy->BindTo(var);
 }
 

src/parsing/parser.h

@@ -387,9 +387,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   VariableProxy* DeclareBoundVariable(const AstRawString* name,
                                       VariableMode mode, int pos);
   void DeclareAndBindVariable(VariableProxy* proxy, VariableKind kind,
-                              VariableMode mode, InitializationFlag init,
-                              Scope* declaration_scope, bool* was_added,
-                              int begin, int end = kNoSourcePosition);
+                              VariableMode mode, Scope* declaration_scope,
+                              bool* was_added, int initializer_position);
   V8_WARN_UNUSED_RESULT
   Variable* DeclareVariable(const AstRawString* name, VariableKind kind,
                             VariableMode mode, InitializationFlag init,

src/parsing/preparser.h

@@ -1093,10 +1093,11 @@ class PreParser : public ParserBase<PreParser> {
   }
 
   void DeclareAndBindVariable(const VariableProxy* proxy, VariableKind kind,
-                              VariableMode mode, InitializationFlag init,
-                              Scope* scope, bool* was_added, int position) {
-    DeclareVariableName(proxy->raw_name(), mode, scope, was_added, position,
-                        kind);
+                              VariableMode mode, Scope* scope, bool* was_added,
+                              int initializer_position) {
+    Variable* var = DeclareVariableName(proxy->raw_name(), mode, scope,
+                                        was_added, proxy->position(), kind);
+    var->set_initializer_position(initializer_position);
     // Don't bother actually binding the proxy.
   }
 

src/zone/zone.h

@@ -354,11 +354,19 @@ class ScopedList final {
   }
 
   int length() const { return static_cast<int>(end_ - start_); }
-  T at(int i) const {
+
+  const T& at(int i) const {
+    size_t index = start_ + i;
+    DCHECK_LE(start_, index);
+    DCHECK_LT(index, buffer_.size());
+    return *reinterpret_cast<T*>(&buffer_[index]);
+  }
+
+  T& at(int i) {
     size_t index = start_ + i;
     DCHECK_LE(start_, index);
     DCHECK_LT(index, buffer_.size());
-    return static_cast<T>(buffer_[index]);
+    return *reinterpret_cast<T*>(&buffer_[index]);
   }
 
   void CopyTo(ZoneList<T>* target, Zone* zone) const {

test/mjsunit/regress/regress-crbug-980422.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.
+
+// Flags: --enable-lazy-source-positions --stress-lazy-source-positions
+(function () {
+  ((d, e = d) => {
+    return d * e;
+  })();
+})();
+
+try {
+  (function () {
+    ((d, e = f, f = d) => {
+      // Won't get here as the initializers will cause a ReferenceError
+    })();
+  })();
+  assertUnreachable();
+} catch (ex) {
+  assertInstanceof(ex, ReferenceError);
+  // Not using assertThrows because we need to access ex.stack to force
+  // collection of source positions.
+  print(ex.stack);
+}
+
+// Check that spreads in arrow functions work
+(function () {
+  ((...args) => args)();
+})();

test/mjsunit/regress/regress-crbug-981701.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.
+
+// Flags: --throws --enable-lazy-source-positions --stress-lazy-source-positions
+((...{a: [b, ...{b: [] = b, a: c}] = b}) => b)(-2);