Sloppy eval declarations should not shadow lexical function declarations

This was being allowed due to the use of BindingFlags instead of VariableMode
to determine whether a looked-up binding was lexical. Because function
declarations are hoisted, they never need hole checks, and so were being
miscategorized as non-lexical.

This patch augments Context::Lookup with a VariableMode out param, which
allows this check to determine precisely whether the binding is lexical.

BUG=v8:4454, v8:5256

Review-Url: https://codereview.chromium.org/2206483004
Cr-Commit-Position: refs/heads/master@{#38260}

src/contexts.cc

@@ -218,12 +218,10 @@ static void GetAttributesAndBindingFlags(VariableMode mode,
   }
 }
 
-
-Handle<Object> Context::Lookup(Handle<String> name,
-                               ContextLookupFlags flags,
-                               int* index,
-                               PropertyAttributes* attributes,
-                               BindingFlags* binding_flags) {
+Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
+                               int* index, PropertyAttributes* attributes,
+                               BindingFlags* binding_flags,
+                               VariableMode* variable_mode) {
   Isolate* isolate = GetIsolate();
   Handle<Context> context(this, isolate);
 
@@ -232,6 +230,7 @@ Handle<Object> Context::Lookup(Handle<String> name,
   *index = kNotFound;
   *attributes = ABSENT;
   *binding_flags = MISSING_BINDING;
+  *variable_mode = VAR;
 
   if (FLAG_trace_contexts) {
     PrintF("Context::Lookup(");
@@ -270,6 +269,7 @@ Handle<Object> Context::Lookup(Handle<String> name,
                    r.context_index, reinterpret_cast<void*>(*c));
           }
           *index = r.slot_index;
+          *variable_mode = r.mode;
           GetAttributesAndBindingFlags(r.mode, r.init_flag, attributes,
                                        binding_flags);
           return ScriptContextTable::GetContext(script_contexts,
@@ -339,6 +339,7 @@ Handle<Object> Context::Lookup(Handle<String> name,
                  slot_index, mode);
         }
         *index = slot_index;
+        *variable_mode = mode;
         GetAttributesAndBindingFlags(mode, init_flag, attributes,
                                      binding_flags);
         return context;
@@ -358,6 +359,7 @@ Handle<Object> Context::Lookup(Handle<String> name,
           *attributes = READ_ONLY;
           DCHECK(mode == CONST_LEGACY || mode == CONST);
           *binding_flags = BINDING_IS_INITIALIZED;
+          *variable_mode = mode;
           return context;
         }
       }
@@ -371,6 +373,7 @@ Handle<Object> Context::Lookup(Handle<String> name,
         *index = Context::THROWN_OBJECT_INDEX;
         *attributes = NONE;
         *binding_flags = BINDING_IS_INITIALIZED;
+        *variable_mode = VAR;
         return context;
       }
     } else if (context->IsDebugEvaluateContext()) {
@@ -389,7 +392,8 @@ Handle<Object> Context::Lookup(Handle<String> name,
       obj = context->get(WRAPPED_CONTEXT_INDEX);
       if (obj->IsContext()) {
         Handle<Object> result = Context::cast(obj)->Lookup(
-            name, DONT_FOLLOW_CHAINS, index, attributes, binding_flags);
+            name, DONT_FOLLOW_CHAINS, index, attributes, binding_flags,
+            variable_mode);
         if (!result.is_null()) return result;
       }
       // Check whitelist. Names that do not pass whitelist shall only resolve

src/contexts.h

@@ -505,11 +505,10 @@ class Context: public FixedArray {
   // 3) 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,
-                        BindingFlags* binding_flags);
+  Handle<Object> Lookup(Handle<String> name, ContextLookupFlags flags,
+                        int* index, PropertyAttributes* attributes,
+                        BindingFlags* binding_flags,
+                        VariableMode* variable_mode);
 
   // Code generation support.
   static int SlotOffset(int index) {

src/runtime/runtime-scopes.cc

@@ -267,10 +267,12 @@ Object* DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   int index;
   PropertyAttributes attributes;
   BindingFlags binding_flags;
+  VariableMode mode;
 
   // Check for a conflict with a lexically scoped variable
-  context_arg->Lookup(name, LEXICAL_TEST, &index, &attributes, &binding_flags);
-  if (attributes != ABSENT && binding_flags == BINDING_CHECK_INITIALIZED) {
+  context_arg->Lookup(name, LEXICAL_TEST, &index, &attributes, &binding_flags,
+                      &mode);
+  if (attributes != ABSENT && IsLexicalVariableMode(mode)) {
     // ES#sec-evaldeclarationinstantiation 5.a.i.1:
     // If varEnvRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
     // exception.
@@ -281,7 +283,7 @@ Object* DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   }
 
   Handle<Object> holder = context->Lookup(name, DONT_FOLLOW_CHAINS, &index,
-                                          &attributes, &binding_flags);
+                                          &attributes, &binding_flags, &mode);
   DCHECK(!isolate->has_pending_exception());
 
   Handle<JSObject> object;
@@ -771,8 +773,9 @@ RUNTIME_FUNCTION(Runtime_DeleteLookupSlot) {
   int index;
   PropertyAttributes attributes;
   BindingFlags flags;
+  VariableMode mode;
   Handle<Object> holder = isolate->context()->Lookup(
-      name, FOLLOW_CHAINS, &index, &attributes, &flags);
+      name, FOLLOW_CHAINS, &index, &attributes, &flags, &mode);
 
   // If the slot was not found the result is true.
   if (holder.is_null()) {
@@ -806,8 +809,9 @@ MaybeHandle<Object> LoadLookupSlot(Handle<String> name,
   int index;
   PropertyAttributes attributes;
   BindingFlags flags;
+  VariableMode mode;
   Handle<Object> holder = isolate->context()->Lookup(
-      name, FOLLOW_CHAINS, &index, &attributes, &flags);
+      name, FOLLOW_CHAINS, &index, &attributes, &flags, &mode);
   if (isolate->has_pending_exception()) return MaybeHandle<Object>();
 
   if (index != Context::kNotFound) {
@@ -909,8 +913,9 @@ MaybeHandle<Object> StoreLookupSlot(Handle<String> name, Handle<Object> value,
   int index;
   PropertyAttributes attributes;
   BindingFlags flags;
+  VariableMode mode;
   Handle<Object> holder =
-      context->Lookup(name, FOLLOW_CHAINS, &index, &attributes, &flags);
+      context->Lookup(name, FOLLOW_CHAINS, &index, &attributes, &flags, &mode);
   if (holder.is_null()) {
     // In case of JSProxy, an exception might have been thrown.
     if (isolate->has_pending_exception()) return MaybeHandle<Object>();

test/mjsunit/es6/block-eval-var-over-let.js

@@ -61,6 +61,23 @@ assertDoesNotThrow(function() {
   eval('var x');
 });
 
+// The same should work for lexical function declarations:
+// If the const is in its own block scope, with the eval, throws
+assertThrows(function() {
+  {
+    function x() {}
+    eval('var x');
+  }
+}, SyntaxError);
+
+// If the eval is in its own block scope, throws
+assertThrows(function() {
+  {
+    function y() {}
+    { eval('var y'); }
+  }
+}, SyntaxError);
+
 // In global scope
 let caught = false;
 try {