[parser] Use cached kDynamic variable for eval-introduced vars

That makes the declaration in sync with how dynamic references are resolved,
avoiding duplicate variable creation in the likely case that the variable is
also referenced within the eval.

Bug: v8:5112, v8:5135, v8:8693
Change-Id: I0c55495f573fe8b5076b1627c139ff72d1adda74
Also-by: leszeks@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1408890
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58850}

src/ast/scopes.cc

@@ -1022,8 +1022,7 @@ void Scope::DeclareVariable(Declaration* declaration, VariableProxy* proxy,
       // The proxy is bound to a lookup variable to force a dynamic declaration
       // using the DeclareEvalVar or DeclareEvalFunction runtime functions.
       DCHECK_EQ(NORMAL_VARIABLE, kind);
-      var = new (zone()) Variable(this, name, mode, kind, init, kMaybeAssigned);
-      var->AllocateTo(VariableLocation::LOOKUP, -1);
+      var = NonLocal(proxy->raw_name(), VariableMode::kDynamic);
     } else {
       // Declare the name.
       var = DeclareLocal(name, mode, kind, init);
@@ -1162,7 +1161,8 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
     // are lexical vs nested var.
     if (decl->IsVariableDeclaration() &&
         decl->AsVariableDeclaration()->AsNested() != nullptr) {
-      DCHECK_EQ(decl->var()->mode(), VariableMode::kVar);
+      DCHECK(decl->var()->mode() == VariableMode::kVar ||
+             decl->var()->mode() == VariableMode::kDynamic);
       Scope* current = decl->AsVariableDeclaration()->AsNested()->scope();
       // Iterate through all scopes until and including the declaration scope.
       while (true) {
@@ -1715,7 +1715,7 @@ void Scope::CheckZones() {
 Variable* Scope::NonLocal(const AstRawString* name, VariableMode mode) {
   // Declare a new non-local.
   DCHECK(IsDynamicVariableMode(mode));
-  Variable* var = variables_.Declare(zone(), nullptr, name, mode);
+  Variable* var = variables_.Declare(zone(), this, name, mode);
   // Allocate it by giving it a dynamic lookup.
   var->AllocateTo(VariableLocation::LOOKUP, -1);
   return var;

src/contexts.cc

@@ -168,13 +168,14 @@ static PropertyAttributes GetAttributesForMode(VariableMode mode) {
   return mode == VariableMode::kConst ? READ_ONLY : NONE;
 }
 
-Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
-                               int* index, PropertyAttributes* attributes,
+// static
+Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
+                               ContextLookupFlags flags, int* index,
+                               PropertyAttributes* attributes,
                                InitializationFlag* init_flag,
                                VariableMode* variable_mode,
                                bool* is_sloppy_function_name) {
-  Isolate* isolate = GetIsolate();
-  Handle<Context> context(*this, isolate);
+  Isolate* isolate = context->GetIsolate();
 
   bool follow_context_chain = (flags & FOLLOW_CONTEXT_CHAIN) != 0;
   bool failed_whitelist = false;
@@ -365,9 +366,10 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
       // Check the original context, but do not follow its context chain.
       Object obj = context->get(WRAPPED_CONTEXT_INDEX);
       if (obj->IsContext()) {
+        Handle<Context> context(Context::cast(obj), isolate);
         Handle<Object> result =
-            Context::cast(obj)->Lookup(name, DONT_FOLLOW_CHAINS, index,
-                                       attributes, init_flag, variable_mode);
+            Context::Lookup(context, name, DONT_FOLLOW_CHAINS, index,
+                            attributes, init_flag, variable_mode);
         if (!result.is_null()) return result;
       }
       // Check whitelist. Names that do not pass whitelist shall only resolve

src/contexts.h

@@ -635,11 +635,12 @@ class Context : public HeapObject {
   // 4) result.is_null():
   //    There was no binding found, *index is always -1 and *attributes is
   //    always ABSENT.
-  Handle<Object> Lookup(Handle<String> name, ContextLookupFlags flags,
-                        int* index, PropertyAttributes* attributes,
-                        InitializationFlag* init_flag,
-                        VariableMode* variable_mode,
-                        bool* is_sloppy_function_name = nullptr);
+  static Handle<Object> Lookup(Handle<Context> context, Handle<String> name,
+                               ContextLookupFlags flags, int* index,
+                               PropertyAttributes* attributes,
+                               InitializationFlag* init_flag,
+                               VariableMode* variable_mode,
+                               bool* is_sloppy_function_name = nullptr);
 
   static inline int FunctionMapIndex(LanguageMode language_mode,
                                      FunctionKind kind, bool has_prototype_slot,

src/interpreter/bytecode-generator.cc

@@ -1249,7 +1249,7 @@ void BytecodeGenerator::VisitVariableDeclaration(VariableDeclaration* decl) {
       }
       break;
     case VariableLocation::LOOKUP: {
-      DCHECK_EQ(VariableMode::kVar, variable->mode());
+      DCHECK_EQ(VariableMode::kDynamic, variable->mode());
       DCHECK(!variable->binding_needs_init());
 
       Register name = register_allocator()->NewRegister();
@@ -1273,7 +1273,8 @@ void BytecodeGenerator::VisitVariableDeclaration(VariableDeclaration* decl) {
 void BytecodeGenerator::VisitFunctionDeclaration(FunctionDeclaration* decl) {
   Variable* variable = decl->var();
   DCHECK(variable->mode() == VariableMode::kLet ||
-         variable->mode() == VariableMode::kVar);
+         variable->mode() == VariableMode::kVar ||
+         variable->mode() == VariableMode::kDynamic);
   switch (variable->location()) {
     case VariableLocation::UNALLOCATED: {
       FeedbackSlot slot =

src/runtime/runtime-scopes.cc

@@ -242,10 +242,12 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   VariableMode mode;
 
   // Check for a conflict with a lexically scoped variable
+  // TODO(verwaest): We should be able to do this at compile-time of the eval
+  // and drop these flags.
   const ContextLookupFlags lookup_flags = static_cast<ContextLookupFlags>(
       FOLLOW_CONTEXT_CHAIN | STOP_AT_DECLARATION_SCOPE | SKIP_WITH_CONTEXT);
-  context_arg->Lookup(name, lookup_flags, &index, &attributes, &init_flag,
-                      &mode);
+  Context::Lookup(context_arg, name, lookup_flags, &index, &attributes,
+                  &init_flag, &mode);
   if (attributes != ABSENT && IsLexicalVariableMode(mode)) {
     // ES#sec-evaldeclarationinstantiation 5.a.i.1:
     // If varEnvRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
@@ -256,8 +258,9 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
                                    RedeclarationType::kSyntaxError);
   }
 
-  Handle<Object> holder = context->Lookup(name, DONT_FOLLOW_CHAINS, &index,
-                                          &attributes, &init_flag, &mode);
+  Handle<Object> holder =
+      Context::Lookup(context, name, DONT_FOLLOW_CHAINS, &index, &attributes,
+                      &init_flag, &mode);
   DCHECK(holder.is_null() || !holder->IsModule());
   DCHECK(!isolate->has_pending_exception());
 
@@ -777,8 +780,9 @@ RUNTIME_FUNCTION(Runtime_DeleteLookupSlot) {
   PropertyAttributes attributes;
   InitializationFlag flag;
   VariableMode mode;
-  Handle<Object> holder = isolate->context()->Lookup(
-      name, FOLLOW_CHAINS, &index, &attributes, &flag, &mode);
+  Handle<Context> context(isolate->context(), isolate);
+  Handle<Object> holder = Context::Lookup(context, name, FOLLOW_CHAINS, &index,
+                                          &attributes, &flag, &mode);
 
   // If the slot was not found the result is true.
   if (holder.is_null()) {
@@ -813,8 +817,9 @@ MaybeHandle<Object> LoadLookupSlot(Isolate* isolate, Handle<String> name,
   PropertyAttributes attributes;
   InitializationFlag flag;
   VariableMode mode;
-  Handle<Object> holder = isolate->context()->Lookup(
-      name, FOLLOW_CHAINS, &index, &attributes, &flag, &mode);
+  Handle<Context> context(isolate->context(), isolate);
+  Handle<Object> holder = Context::Lookup(context, name, FOLLOW_CHAINS, &index,
+                                          &attributes, &flag, &mode);
   if (isolate->has_pending_exception()) return MaybeHandle<Object>();
 
   if (!holder.is_null() && holder->IsModule()) {
@@ -905,19 +910,17 @@ RUNTIME_FUNCTION_RETURN_PAIR(Runtime_LoadLookupSlotForCall) {
 namespace {
 
 MaybeHandle<Object> StoreLookupSlot(
-    Isolate* isolate, Handle<String> name, Handle<Object> value,
-    LanguageMode language_mode,
+    Isolate* isolate, Handle<Context> context, Handle<String> name,
+    Handle<Object> value, LanguageMode language_mode,
     ContextLookupFlags context_lookup_flags = FOLLOW_CHAINS) {
-  Handle<Context> context(isolate->context(), isolate);
-
   int index;
   PropertyAttributes attributes;
   InitializationFlag flag;
   VariableMode mode;
   bool is_sloppy_function_name;
   Handle<Object> holder =
-      context->Lookup(name, context_lookup_flags, &index, &attributes, &flag,
-                      &mode, &is_sloppy_function_name);
+      Context::Lookup(context, name, context_lookup_flags, &index, &attributes,
+                      &flag, &mode, &is_sloppy_function_name);
   if (holder.is_null()) {
     // In case of JSProxy, an exception might have been thrown.
     if (isolate->has_pending_exception()) return MaybeHandle<Object>();
@@ -977,32 +980,37 @@ RUNTIME_FUNCTION(Runtime_StoreLookupSlot_Sloppy) {
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
+  Handle<Context> context(isolate->context(), isolate);
   RETURN_RESULT_OR_FAILURE(
-      isolate, StoreLookupSlot(isolate, name, value, LanguageMode::kSloppy));
+      isolate,
+      StoreLookupSlot(isolate, context, name, value, LanguageMode::kSloppy));
 }
 
-// Store into a dynamic context for sloppy-mode block-scoped function hoisting
-// which leaks out of an eval. In particular, with-scopes are be skipped to
-// reach the appropriate var-like declaration.
-RUNTIME_FUNCTION(Runtime_StoreLookupSlot_SloppyHoisting) {
+RUNTIME_FUNCTION(Runtime_StoreLookupSlot_Strict) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
-  const ContextLookupFlags lookup_flags = static_cast<ContextLookupFlags>(
-      FOLLOW_CONTEXT_CHAIN | STOP_AT_DECLARATION_SCOPE | SKIP_WITH_CONTEXT);
+  Handle<Context> context(isolate->context(), isolate);
   RETURN_RESULT_OR_FAILURE(
-      isolate, StoreLookupSlot(isolate, name, value, LanguageMode::kSloppy,
-                               lookup_flags));
+      isolate,
+      StoreLookupSlot(isolate, context, name, value, LanguageMode::kStrict));
 }
 
-RUNTIME_FUNCTION(Runtime_StoreLookupSlot_Strict) {
+// Store into a dynamic declaration context for sloppy-mode block-scoped
+// function hoisting which leaks out of an eval.
+RUNTIME_FUNCTION(Runtime_StoreLookupSlot_SloppyHoisting) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
+  const ContextLookupFlags lookup_flags =
+      static_cast<ContextLookupFlags>(DONT_FOLLOW_CHAINS);
+  Handle<Context> declaration_context(isolate->context()->declaration_context(),
+                                      isolate);
   RETURN_RESULT_OR_FAILURE(
-      isolate, StoreLookupSlot(isolate, name, value, LanguageMode::kStrict));
+      isolate, StoreLookupSlot(isolate, declaration_context, name, value,
+                               LanguageMode::kSloppy, lookup_flags));
 }
 
 }  // namespace internal

test/mjsunit/es6/block-sloppy-function.js

@@ -620,15 +620,12 @@ eval(`
       return 4;
     } }`);
 
-    // assertEquals(0, f);
-    assertEquals(4, f());
+    assertEquals(0, f);
   }
 
-  // assertEquals(4, f());
-  assertEquals(undefined, f);
+  assertEquals(4, f());
 })();
 
-// This test is incorrect BUG(v8:5168). The commented assertions are correct.
 (function evalHoistingThroughWith() {
   with ({f: 0}) {
     eval(`{ function f() {

test/mjsunit/regress/regress-sloppy-block-function-hoisting-dynamic.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.
+
+with({}) { eval("{function f(){f}}") };
+f()

test/test262/test262.status

@@ -388,16 +388,6 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=5116
   'built-ins/TypedArray/prototype/fill/fill-values-conversion-operations-consistent-nan': [PASS, FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5135
-  'annexB/language/eval-code/direct/func-block-decl-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-decl-a-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-decl-b-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-stmt-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-no-else-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-if-stmt-else-decl-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-switch-case-eval-func-block-scoping': [FAIL],
-  'annexB/language/eval-code/direct/func-switch-dflt-eval-func-block-scoping': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=5139
   'annexB/built-ins/Date/prototype/setYear/year-number-relative': [FAIL],
 
@@ -447,16 +437,6 @@
   'built-ins/TypedArrayConstructors/internals/Set/key-is-out-of-bounds': [FAIL],
   'built-ins/TypedArrayConstructors/internals/Set/BigInt/key-is-out-of-bounds': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5112
-  'annexB/language/eval-code/direct/func-block-decl-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-decl-a-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-decl-b-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-else-stmt-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-if-decl-no-else-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-if-stmt-else-decl-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-switch-case-eval-func-no-skip-try': [FAIL],
-  'annexB/language/eval-code/direct/func-switch-dflt-eval-func-no-skip-try': [FAIL],
-
   # SharedArrayBuffer tests that require flags
   'built-ins/SharedArrayBuffer/*': ['--harmony-sharedarraybuffer'],
   'built-ins/Atomics/*': ['--harmony-sharedarraybuffer'],