Remove per-parameter position var scope

The spec was normatively changed to simplify var scopes for parameter
expressions. Previously there was a per-parameter var scope in sloppy
mode so direct evals could introduce vars that did not escape the
parameter position. That semantics is complex both for the programmer
and implementation and has resulted in bugs in the past. Furthermore, it
has never been fully interoperable (with Safari in particular). The spec
was instead changed to be simpler: to have a single var scope for
sloppy evals in parameters that encloses the parameter scope and body
scope.

This simplification lets us remove expression-scope-reparenter.

Drive-by removal of stale reference to PatternRewriter.

Bug: v8:7532
Change-Id: Iade5594abe0009f7f3f6a1adad18628b17e1e779
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1962471Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65517}

BUILD.gn

@@ -2665,8 +2665,6 @@ v8_source_set("v8_base_without_compiler") {
     "src/objects/value-serializer.h",
     "src/objects/visitors.cc",
     "src/objects/visitors.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/expression-scope-reparenter.cc

-// Copyright 2015 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.
-
-#include "src/parsing/expression-scope-reparenter.h"
-
-#include "src/ast/ast-traversal-visitor.h"
-#include "src/ast/ast.h"
-#include "src/ast/scopes.h"
-#include "src/objects/objects-inl.h"
-
-namespace v8 {
-namespace internal {
-
-namespace {
-
-class Reparenter final : public AstTraversalVisitor<Reparenter> {
- public:
-  Reparenter(uintptr_t stack_limit, Expression* initializer, Scope* scope)
-      : AstTraversalVisitor(stack_limit, initializer), scope_(scope) {}
-
- private:
-  // This is required so that the overriden Visit* methods can be
-  // called by the base class (template).
-  friend class AstTraversalVisitor<Reparenter>;
-
-  void VisitFunctionLiteral(FunctionLiteral* expr);
-  void VisitClassLiteral(ClassLiteral* expr);
-  void VisitVariableProxy(VariableProxy* expr);
-
-  void VisitBlock(Block* stmt);
-  void VisitTryCatchStatement(TryCatchStatement* stmt);
-  void VisitWithStatement(WithStatement* stmt);
-
-  Scope* scope_;
-};
-
-void Reparenter::VisitFunctionLiteral(FunctionLiteral* function_literal) {
-  function_literal->scope()->ReplaceOuterScope(scope_);
-}
-
-void Reparenter::VisitClassLiteral(ClassLiteral* class_literal) {
-  class_literal->scope()->ReplaceOuterScope(scope_);
-  // No need to visit the constructor since it will have the class
-  // scope on its scope chain.
-  DCHECK_EQ(class_literal->constructor()->scope()->outer_scope(),
-            class_literal->scope());
-
-  if (class_literal->static_fields_initializer() != nullptr) {
-    DCHECK_EQ(
-        class_literal->static_fields_initializer()->scope()->outer_scope(),
-        class_literal->scope());
-  }
-#if DEBUG
-  // The same goes for the rest of the class, but we do some
-  // sanity checking in debug mode.
-  for (ClassLiteralProperty* prop : *class_literal->private_members()) {
-    // No need to visit the values, since all values are functions with
-    // the class scope on their scope chain.
-    DCHECK(prop->value()->IsFunctionLiteral());
-    DCHECK_EQ(prop->value()->AsFunctionLiteral()->scope()->outer_scope(),
-              class_literal->scope());
-  }
-  for (ClassLiteralProperty* prop : *class_literal->public_members()) {
-    // No need to visit the values, since all values are functions with
-    // the class scope on their scope chain.
-    DCHECK(prop->value()->IsFunctionLiteral());
-    DCHECK_EQ(prop->value()->AsFunctionLiteral()->scope()->outer_scope(),
-              class_literal->scope());
-  }
-#endif
-}
-
-void Reparenter::VisitVariableProxy(VariableProxy* proxy) {
-  if (!proxy->is_resolved()) {
-    if (scope_->outer_scope()->RemoveUnresolved(proxy)) {
-      scope_->AddUnresolved(proxy);
-    }
-  } else {
-    // Ensure that temporaries we find are already in the correct scope.
-    DCHECK(proxy->var()->mode() != VariableMode::kTemporary ||
-           proxy->var()->scope() == scope_->GetClosureScope());
-  }
-}
-
-void Reparenter::VisitBlock(Block* stmt) {
-  if (stmt->scope())
-    stmt->scope()->ReplaceOuterScope(scope_);
-  else
-    VisitStatements(stmt->statements());
-}
-
-void Reparenter::VisitTryCatchStatement(TryCatchStatement* stmt) {
-  Visit(stmt->try_block());
-  if (stmt->scope()) {
-    stmt->scope()->ReplaceOuterScope(scope_);
-  } else {
-    Visit(stmt->catch_block());
-  }
-}
-
-void Reparenter::VisitWithStatement(WithStatement* stmt) {
-  Visit(stmt->expression());
-  stmt->scope()->ReplaceOuterScope(scope_);
-}
-
-}  // anonymous namespace
-
-void ReparentExpressionScope(uintptr_t stack_limit, Expression* expr,
-                             Scope* scope) {
-  // The only case that uses this code is block scopes for parameters containing
-  // sloppy eval.
-  DCHECK(scope->is_block_scope());
-  DCHECK(scope->is_declaration_scope());
-  DCHECK(scope->AsDeclarationScope()->sloppy_eval_can_extend_vars());
-  DCHECK(scope->outer_scope()->is_function_scope());
-
-  Reparenter r(stack_limit, expr, scope);
-  r.Run();
-}
-
-}  // namespace internal
-}  // namespace v8

src/parsing/expression-scope-reparenter.h

-// Copyright 2015 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.
-
-#ifndef V8_PARSING_EXPRESSION_SCOPE_REPARENTER_H_
-#define V8_PARSING_EXPRESSION_SCOPE_REPARENTER_H_
-
-#include <stdint.h>
-
-namespace v8 {
-namespace internal {
-
-class Expression;
-class Scope;
-
-// When an extra declaration scope needs to be inserted to account for
-// a sloppy eval in a default parameter or function body, the expressions
-// needs to be in that new inner scope which was added after initial
-// parsing.
-//
-// scope is the new inner scope, and its outer_scope() is assumed
-// to be the scope which was used during the initial parse.
-void ReparentExpressionScope(uintptr_t stack_limit, Expression* expr,
-                             Scope* scope);
-
-}  // namespace internal
-}  // namespace v8
-
-#endif  // V8_PARSING_EXPRESSION_SCOPE_REPARENTER_H_

src/parsing/parser.cc

@@ -21,7 +21,6 @@
 #include "src/logging/log.h"
 #include "src/numbers/conversions-inl.h"
 #include "src/objects/scope-info.h"
-#include "src/parsing/expression-scope-reparenter.h"
 #include "src/parsing/parse-info.h"
 #include "src/parsing/rewriter.h"
 #include "src/runtime/runtime.h"
@@ -2653,38 +2652,11 @@ Block* Parser::BuildParameterInitializationBlock(
                                     initial_value, kNoSourcePosition);
     }
 
-    DeclarationScope* param_scope = scope()->AsDeclarationScope();
-    ScopedPtrList<Statement>* param_init_statements = &init_statements;
-
-    base::Optional<ScopedPtrList<Statement>> non_simple_param_init_statements;
-    if (!parameter->is_simple() && param_scope->sloppy_eval_can_extend_vars()) {
-      param_scope = NewVarblockScope();
-      param_scope->set_start_position(parameter->pattern->position());
-      param_scope->set_end_position(parameter->initializer_end_position);
-      param_scope->RecordEvalCall();
-      non_simple_param_init_statements.emplace(pointer_buffer());
-      param_init_statements = &non_simple_param_init_statements.value();
-      // Rewrite the outer initializer to point to param_scope
-      ReparentExpressionScope(stack_limit(), parameter->pattern, param_scope);
-      ReparentExpressionScope(stack_limit(), initial_value, param_scope);
-    }
-
-    BlockState block_state(&scope_, param_scope);
+    BlockState block_state(&scope_, scope()->AsDeclarationScope());
     DeclarationParsingResult::Declaration decl(parameter->pattern,
                                                initial_value);
+    InitializeVariables(&init_statements, PARAMETER_VARIABLE, &decl);
 
-    InitializeVariables(param_init_statements, PARAMETER_VARIABLE, &decl);
-
-    if (param_init_statements != &init_statements) {
-      DCHECK_EQ(param_init_statements,
-                &non_simple_param_init_statements.value());
-      Block* param_block =
-          factory()->NewBlock(true, *non_simple_param_init_statements);
-      non_simple_param_init_statements.reset();
-      param_block->set_scope(param_scope);
-      param_scope = param_scope->FinalizeBlockScope()->AsDeclarationScope();
-      init_statements.Add(param_block);
-    }
     ++index;
   }
   return factory()->NewBlock(true, init_statements);

src/parsing/parser.h

@@ -355,8 +355,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return scope()->GetDeclarationScope()->has_checked_syntax();
   }
 
-  // PatternRewriter and associated methods defined in pattern-rewriter.cc.
-  friend class PatternRewriter;
   void InitializeVariables(
       ScopedPtrList<Statement>* statements, VariableKind kind,
       const DeclarationParsingResult::Declaration* declaration);

test/mjsunit/es6/default-parameters-destructuring.js

@@ -7,16 +7,16 @@
   var x = 1;
 
   function f41({[eval("var x = 2; 'a'")]: w}, z = x) { return z; }
-  assertEquals(1, f41({}));
-  assertEquals(1, f41({a: 0}));
+  assertEquals(2, f41({}));
+  assertEquals(2, f41({a: 0}));
   function f42({[eval("var x = 2; 'a'")]: w}, z = eval("x")) { return z; }
-  assertEquals(1, f42({}));
-  assertEquals(1, f42({a: 0}));
+  assertEquals(2, f42({}));
+  assertEquals(2, f42({a: 0}));
   function f43({a: w = eval("var x = 2")}, z = x) { return z; }
-  assertEquals(1, f43({}));
+  assertEquals(2, f43({}));
   assertEquals(1, f43({a: 0}));
   function f44({a: w = eval("var x = 2")}, z = eval("x")) { return z; }
-  assertEquals(1, f44({}));
+  assertEquals(2, f44({}));
   assertEquals(1, f44({a: 0}));
 
   function f5({a = eval("var x = 2"), b = x}) { return b; }
@@ -39,16 +39,16 @@
   assertEquals(1, f74({a: 0}));
 
   var g41 = ({[eval("var x = 2; 'a'")]: w}, z = x) => { return z; };
-  assertEquals(1, g41({}));
-  assertEquals(1, g41({a: 0}));
+  assertEquals(2, g41({}));
+  assertEquals(2, g41({a: 0}));
   var g42 = ({[eval("var x = 2; 'a'")]: w}, z = eval("x")) => { return z; };
-  assertEquals(1, g42({}));
-  assertEquals(1, g42({a: 0}));
+  assertEquals(2, g42({}));
+  assertEquals(2, g42({a: 0}));
   var g43 = ({a: w = eval("var x = 2")}, z = x) => { return z; };
-  assertEquals(1, g43({}));
+  assertEquals(2, g43({}));
   assertEquals(1, g43({a: 0}));
   var g44 = ({a: w = eval("var x = 2")}, z = eval("x")) => { return z; };
-  assertEquals(1, g44({}));
+  assertEquals(2, g44({}));
   assertEquals(1, g44({a: 0}));
 
   var g5 = ({a = eval("var x = 2"), b = x}) => { return b; };

test/mjsunit/es6/default-parameters.js

@@ -169,23 +169,23 @@
   var x = 1;
 
   function f1(y = eval("var x = 2")) { with ({}) { return x; } }
-  assertEquals(1, f1());
+  assertEquals(2, f1());
   function f2(y = eval("var x = 2"), z = x) { return z; }
-  assertEquals(1, f2());
+  assertEquals(2, f2());
   assertEquals(1, f2(0));
   function f3(y = eval("var x = 2"), z = eval("x")) { return z; }
-  assertEquals(1, f3());
+  assertEquals(2, f3());
   assertEquals(1, f3(0));
   function f8(y = (eval("var x = 2"), x)) { return y; }
   assertEquals(2, f8());
   assertEquals(0, f8(0));
 
   function f11(z = eval("var y = 2")) { return y; }
-  assertThrows(f11, ReferenceError);
-  function f12(z = eval("var y = 2"), b = y) {}
-  assertThrows(f12, ReferenceError);
-  function f13(z = eval("var y = 2"), b = eval("y")) {}
-  assertThrows(f13, ReferenceError);
+  assertEquals(2, f11());
+  function f12(z = eval("var y = 2"), b = y) { return b; }
+  assertEquals(2, f12());
+  function f13(z = eval("var y = 2"), b = eval("y")) { return b; }
+  assertEquals(2, f13());
 
   function f21(f = () => x) { eval("var x = 2"); return f() }
   assertEquals(1, f21());
@@ -195,23 +195,23 @@
   assertEquals(3, f22(() => 3));
 
   var g1 = (y = eval("var x = 2")) => { with ({}) { return x; } };
-  assertEquals(1, g1());
+  assertEquals(2, g1());
   var g2 = (y = eval("var x = 2"), z = x) => { return z; };
-  assertEquals(1, g2());
+  assertEquals(2, g2());
   assertEquals(1, g2(0));
   var g3 = (y = eval("var x = 2"), z = eval("x")) => { return z; };
-  assertEquals(1, g3());
+  assertEquals(2, g3());
   assertEquals(1, g3(0));
   var g8 = (y = (eval("var x = 2"), x)) => { return y; };
   assertEquals(2, g8());
   assertEquals(0, g8(0));
 
   var g11 = (z = eval("var y = 2")) => { return y; };
-  assertThrows(g11, ReferenceError);
-  var g12 = (z = eval("var y = 2"), b = y) => {};
-  assertThrows(g12, ReferenceError);
-  var g13 = (z = eval("var y = 2"), b = eval("y")) => {};
-  assertThrows(g13, ReferenceError);
+  assertEquals(2, g11());
+  var g12 = (z = eval("var y = 2"), b = y) => { return b; };
+  assertEquals(2, g12());
+  var g13 = (z = eval("var y = 2"), b = eval("y")) => { return b; };
+  assertEquals(2, g13());
 
   var g21 = (f = () => x) => { eval("var x = 2"); return f() };
   assertEquals(1, g21());

test/mjsunit/es6/generator-destructuring.js

@@ -162,23 +162,23 @@
   var x = 1;
 
   function* f1(y = eval("var x = 2")) { with ({}) { return x; } }
-  assertEquals(1, f1().next().value);
+  assertEquals(2, f1().next().value);
   function* f2(y = eval("var x = 2"), z = x) { return z; }
-  assertEquals(1, f2().next().value);
+  assertEquals(2, f2().next().value);
   assertEquals(1, f2(0).next().value);
   function* f3(y = eval("var x = 2"), z = eval("x")) { return z; }
-  assertEquals(1, f3().next().value);
+  assertEquals(2, f3().next().value);
   assertEquals(1, f3(0).next().value);
   function* f8(y = (eval("var x = 2"), x)) { return y; }
   assertEquals(2, f8().next().value);
   assertEquals(0, f8(0).next().value);
 
   function* f11(z = eval("var y = 2")) { return y; }
-  assertThrows(() => f11().next(), ReferenceError);
-  function* f12(z = eval("var y = 2"), b = y) {}
-  assertThrows(() => f12().next(), ReferenceError);
-  function* f13(z = eval("var y = 2"), b = eval("y")) {}
-  assertThrows(() => f13().next(), ReferenceError);
+  assertEquals(2, f11().next().value);
+  function* f12(z = eval("var y = 2"), b = y) { return b; }
+  assertEquals(2, f12().next().value);
+  function* f13(z = eval("var y = 2"), b = eval("y")) { return b; }
+  assertEquals(2, f13().next().value);
 
   function* f21(f = () => x) { eval("var x = 2"); return f() }
   assertEquals(1, f21().next().value);

test/mjsunit/es8/async-destructuring.js

@@ -329,23 +329,23 @@ function assertEqualsAsync(expected, run, msg) {
   var x = 1;
 
   async function f1(y = eval("var x = 2")) { with ({}) { return x; } }
-  assertEqualsAsync(1, () => f1());
+  assertEqualsAsync(2, () => f1());
   async function f2(y = eval("var x = 2"), z = x) { return z; }
-  assertEqualsAsync(1, () => f2());
+  assertEqualsAsync(2, () => f2());
   assertEqualsAsync(1, () => f2(0));
   async function f3(y = eval("var x = 2"), z = eval("x")) { return z; }
-  assertEqualsAsync(1, () => f3());
+  assertEqualsAsync(2, () => f3());
   assertEqualsAsync(1, () => f3(0));
   async function f8(y = (eval("var x = 2"), x)) { return y; }
   assertEqualsAsync(2, () => f8());
   assertEqualsAsync(0, () => f8(0));
 
   async function f11(z = eval("var y = 2")) { return y; }
-  assertThrowsAsync(f11, ReferenceError);
-  async function f12(z = eval("var y = 2"), b = y) {}
-  assertThrowsAsync(f12, ReferenceError);
-  async function f13(z = eval("var y = 2"), b = eval("y")) {}
-  assertThrowsAsync(f13, ReferenceError);
+  assertEqualsAsync(2, () => f11());
+  async function f12(z = eval("var y = 2"), b = y) { return b; }
+  assertEqualsAsync(2, () => f12());
+  async function f13(z = eval("var y = 2"), b = eval("y")) { return b; }
+  assertEqualsAsync(2, () => f13());
 
   async function f21(f = () => x) { eval("var x = 2"); return f() }
   assertEqualsAsync(1, () => f21());
@@ -355,23 +355,23 @@ function assertEqualsAsync(expected, run, msg) {
   assertEqualsAsync(3, () => f22(() => 3));
 
   var g1 = async (y = eval("var x = 2")) => { with ({}) { return x; } };
-  assertEqualsAsync(1, () => g1());
+  assertEqualsAsync(2, () => g1());
   var g2 = async (y = eval("var x = 2"), z = x) => { return z; };
-  assertEqualsAsync(1, () => g2());
+  assertEqualsAsync(2, () => g2());
   assertEqualsAsync(1, () => g2(0));
   var g3 = async (y = eval("var x = 2"), z = eval("x")) => { return z; };
-  assertEqualsAsync(1, () => g3());
+  assertEqualsAsync(2, () => g3());
   assertEqualsAsync(1, () => g3(0));
   var g8 = async (y = (eval("var x = 2"), x)) => { return y; };
   assertEqualsAsync(2, () => g8());
   assertEqualsAsync(0, () => g8(0));
 
   var g11 = async (z = eval("var y = 2")) => { return y; };
-  assertThrowsAsync(g11, ReferenceError);
-  var g12 = async (z = eval("var y = 2"), b = y) => {};
-  assertThrowsAsync(g12, ReferenceError);
-  var g13 = async (z = eval("var y = 2"), b = eval("y")) => {};
-  assertThrowsAsync(g13, ReferenceError);
+  assertEqualsAsync(2, () => g11());
+  var g12 = async (z = eval("var y = 2"), b = y) => { return b; };
+  assertEqualsAsync(2, () => g12());
+  var g13 = async (z = eval("var y = 2"), b = eval("y")) => { return b; };
+  assertEqualsAsync(2, () => g13());
 
   var g21 = async (f = () => x) => { eval("var x = 2"); return f() };
   assertEqualsAsync(1, () => g21());

test/mjsunit/harmony/eval-param-scoping.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.
+
+function f0(a = eval("var b"), b) { }
+assertThrows(f0, SyntaxError);
+
+function f1(a = eval("var b = 0"), b) { }
+assertThrows(f1, SyntaxError);
+
+function f2(a = eval("function b(){}"), b) { }
+assertThrows(f2, SyntaxError);
+
+function f3(a = eval("{ function b(){} }"), b) { return b }
+assertEquals(undefined, f3());
+
+function f4(b, a = eval("var b = 0")) { return b }
+assertThrows(f4, SyntaxError);
+
+function f5(b, a = eval("function b(){}")) { return b }
+assertThrows(f5, SyntaxError);
+
+function f6(b, a = eval("{ function b(){} }")) { return b }
+assertEquals(42, f6(42));

test/test262/test262.status

@@ -528,6 +528,40 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=9808
   'built-ins/AggregateError/*': [FAIL],
 
+  # https://bugs.chromium.org/p/v8/issues/detail?id=7532
+  'language/expressions/arrow-function/scope-param-elem-var-close': [FAIL],
+  'language/expressions/arrow-function/scope-param-elem-var-open': [FAIL],
+  'language/expressions/arrow-function/scope-param-rest-elem-var-close': [FAIL],
+  'language/expressions/arrow-function/scope-param-rest-elem-var-open': [FAIL],
+  'language/expressions/function/scope-param-elem-var-close': [FAIL],
+  'language/expressions/function/scope-param-elem-var-open': [FAIL],
+  'language/expressions/function/scope-param-rest-elem-var-close': [FAIL],
+  'language/expressions/function/scope-param-rest-elem-var-open': [FAIL],
+  'language/expressions/generators/scope-param-elem-var-close': [FAIL],
+  'language/expressions/generators/scope-param-elem-var-open': [FAIL],
+  'language/expressions/generators/scope-param-rest-elem-var-close': [FAIL],
+  'language/expressions/generators/scope-param-rest-elem-var-open': [FAIL],
+  'language/expressions/object/scope-gen-meth-param-elem-var-close': [FAIL],
+  'language/expressions/object/scope-gen-meth-param-elem-var-open': [FAIL],
+  'language/expressions/object/scope-gen-meth-param-rest-elem-var-close': [FAIL],
+  'language/expressions/object/scope-gen-meth-param-rest-elem-var-open': [FAIL],
+  'language/expressions/object/scope-meth-param-elem-var-close': [FAIL],
+  'language/expressions/object/scope-meth-param-elem-var-open': [FAIL],
+  'language/expressions/object/scope-meth-param-rest-elem-var-close': [FAIL],
+  'language/expressions/object/scope-meth-param-rest-elem-var-open': [FAIL],
+  'language/function-code/each-param-has-own-non-shared-eval-scope': [FAIL_SLOPPY],
+  'language/function-code/each-param-has-own-scope': [FAIL_SLOPPY],
+  'language/function-code/eval-param-env-with-computed-key': [FAIL_SLOPPY],
+  'language/function-code/eval-param-env-with-prop-initializer': [FAIL_SLOPPY],
+  'language/statements/function/scope-param-elem-var-close': [FAIL],
+  'language/statements/function/scope-param-elem-var-open': [FAIL],
+  'language/statements/function/scope-param-rest-elem-var-close': [FAIL],
+  'language/statements/function/scope-param-rest-elem-var-open': [FAIL],
+  'language/statements/generators/scope-param-elem-var-close': [FAIL],
+  'language/statements/generators/scope-param-elem-var-open': [FAIL],
+  'language/statements/generators/scope-param-rest-elem-var-close': [FAIL],
+  'language/statements/generators/scope-param-rest-elem-var-open': [FAIL],
+
   ######################## NEEDS INVESTIGATION ###########################
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=7833