[parser] Delete unresolved variables created for labels

This deletes unresolved VariableProxy objects created for labels in the
preparser which prevents shadowed variables in enclosing scopes from
being context-allocated.

Previously this was only done in the full parser, which leads to
bytecode mismatches with lazy source positions.

Bug: chromium:1009728, v8:8510
Change-Id: If2d0c345346116a7f5aacbcd0cf3638e9f7e04cc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1836258Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64104}

src/ast/scopes.cc

@@ -1402,6 +1402,7 @@ void Scope::AnalyzePartially(DeclarationScope* max_outer_scope,
 
     for (VariableProxy* proxy = scope->unresolved_list_.first();
          proxy != nullptr; proxy = proxy->next_unresolved()) {
+      if (proxy->is_removed_from_unresolved()) continue;
       DCHECK(!proxy->is_resolved());
       Variable* var =
           Lookup<kParsedScope>(proxy, scope, max_outer_scope->outer_scope());

src/parsing/expression-scope.h

@@ -548,6 +548,10 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
     return end;
   }
 
+  ScopedList<std::pair<VariableProxy*, int>>* variable_list() {
+    return &variable_list_;
+  }
+
  protected:
   bool is_verified() const {
 #ifdef DEBUG
@@ -559,10 +563,6 @@ class ExpressionParsingScope : public ExpressionScope<Types> {
 
   void ValidatePattern() { Validate(kPatternIndex); }
 
-  ScopedList<std::pair<VariableProxy*, int>>* variable_list() {
-    return &variable_list_;
-  }
-
  private:
   friend class AccumulationScope<Types>;
 

src/parsing/parser-base.h

@@ -5135,13 +5135,29 @@ ParserBase<Impl>::ParseExpressionOrLabelledStatement(
   }
 
   bool starts_with_identifier = peek_any_identifier();
-  ExpressionT expr = ParseExpression();
+
+  ExpressionT expr;
+  {
+    // Effectively inlines ParseExpression, so potential labels can be extracted
+    // from expression_scope.
+    ExpressionParsingScope expression_scope(impl());
+    AcceptINScope scope(this, true);
+    expr = ParseExpressionCoverGrammar();
+    expression_scope.ValidateExpression();
+
     if (peek() == Token::COLON && starts_with_identifier &&
         impl()->IsIdentifier(expr)) {
       // The whole expression was a single identifier, and not, e.g.,
       // something starting with an identifier or a parenthesized identifier.
-    impl()->DeclareLabel(&labels, &own_labels,
-                         impl()->AsIdentifierExpression(expr));
+      DCHECK_EQ(expression_scope.variable_list()->length(), 1);
+      VariableProxy* label = expression_scope.variable_list()->at(0).first;
+      impl()->DeclareLabel(&labels, &own_labels, label->raw_name());
+
+      // Remove the "ghost" variable that turned out to be a label from the top
+      // scope. This way, we don't try to resolve it during the scope
+      // processing.
+      this->scope()->DeleteUnresolved(label);
+
       Consume(Token::COLON);
       // ES#sec-labelled-function-declarations Labelled Function Declarations
       if (peek() == Token::FUNCTION && is_sloppy(language_mode()) &&
@@ -5150,6 +5166,7 @@ ParserBase<Impl>::ParseExpressionOrLabelledStatement(
       }
       return ParseStatement(labels, own_labels, allow_function);
     }
+  }
 
   // If we have an extension, we allow a native function declaration.
   // A native function declaration starts with "native function" with

src/parsing/parser.cc

@@ -1523,15 +1523,11 @@ Statement* Parser::DeclareNative(const AstRawString* name, int pos) {
 
 void Parser::DeclareLabel(ZonePtrList<const AstRawString>** labels,
                           ZonePtrList<const AstRawString>** own_labels,
-                          VariableProxy* var) {
-  DCHECK(IsIdentifier(var));
-  const AstRawString* label = var->raw_name();
-
-  // TODO(1240780): We don't check for redeclaration of labels
-  // during preparsing since keeping track of the set of active
-  // labels requires nontrivial changes to the way scopes are
-  // structured.  However, these are probably changes we want to
-  // make later anyway so we should go back and fix this then.
+                          const AstRawString* label) {
+  // TODO(1240780): We don't check for redeclaration of labels during preparsing
+  // since keeping track of the set of active labels requires nontrivial changes
+  // to the way scopes are structured.  However, these are probably changes we
+  // want to make later anyway so we should go back and fix this then.
   if (ContainsLabel(*labels, label) || TargetStackContainsLabel(label)) {
     ReportMessage(MessageTemplate::kLabelRedeclaration, label);
     return;
@@ -1549,11 +1545,6 @@ void Parser::DeclareLabel(ZonePtrList<const AstRawString>** labels,
   }
   (*labels)->Add(label, zone());
   (*own_labels)->Add(label, zone());
-
-  // Remove the "ghost" variable that turned out to be a label
-  // from the top scope. This way, we don't try to resolve it
-  // during the scope processing.
-  scope()->DeleteUnresolved(var);
 }
 
 bool Parser::ContainsLabel(ZonePtrList<const AstRawString>* labels,

src/parsing/parser.h

@@ -8,6 +8,7 @@
 #include <cstddef>
 
 #include "src/ast/ast-source-ranges.h"
+#include "src/ast/ast-value-factory.h"
 #include "src/ast/ast.h"
 #include "src/ast/scopes.h"
 #include "src/base/compiler-specific.h"
@@ -274,7 +275,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   Statement* BuildInitializationBlock(DeclarationParsingResult* parsing_result);
   void DeclareLabel(ZonePtrList<const AstRawString>** labels,
                     ZonePtrList<const AstRawString>** own_labels,
-                    VariableProxy* expr);
+                    const AstRawString* label);
   bool ContainsLabel(ZonePtrList<const AstRawString>* labels,
                      const AstRawString* label);
   Expression* RewriteReturn(Expression* return_value, int pos);

src/parsing/preparser.h

@@ -5,6 +5,7 @@
 #ifndef V8_PARSING_PREPARSER_H_
 #define V8_PARSING_PREPARSER_H_
 
+#include "src/ast/ast-value-factory.h"
 #include "src/ast/ast.h"
 #include "src/ast/scopes.h"
 #include "src/parsing/parser-base.h"
@@ -1071,9 +1072,8 @@ class PreParser : public ParserBase<PreParser> {
 
   V8_INLINE void DeclareLabel(ZonePtrList<const AstRawString>** labels,
                               ZonePtrList<const AstRawString>** own_labels,
-                              const PreParserExpression& expr) {
-    DCHECK(!parsing_module_ || !expr.AsIdentifier().IsAwait());
-    DCHECK(IsIdentifier(expr));
+                              const AstRawString* label) {
+    DCHECK(!parsing_module_);
   }
 
   // TODO(nikolaos): The preparser currently does not keep track of labels.

test/mjsunit/regress/regress-crbug-1009728.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: --stress-lazy-source-positions
+
+function foo(x) {
+  (function bar() {
+    {
+      x: 1
+    }
+    function f() {}
+  });
+}
+foo();