[parser] Check conflicting var declarations from eval at compile-time

Change-Id: I9195c7ffdc4b841f14701662527c97c9698bd472
Reviewed-on: https://chromium-review.googlesource.com/c/1411888
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58859}

src/ast/scopes.cc

@@ -576,29 +576,6 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
   }
 }
 
-void DeclarationScope::AttachOuterScopeInfo(ParseInfo* info, Isolate* isolate) {
-  DCHECK(scope_info_.is_null());
-  Handle<ScopeInfo> outer_scope_info;
-  if (info->maybe_outer_scope_info().ToHandle(&outer_scope_info)) {
-    // If we have a scope info we will potentially need to lookup variable names
-    // on the scope info as internalized strings, so make sure ast_value_factory
-    // is internalized.
-    info->ast_value_factory()->Internalize(isolate);
-    if (outer_scope()) {
-      DeclarationScope* script_scope = new (info->zone())
-          DeclarationScope(info->zone(), info->ast_value_factory());
-      info->set_script_scope(script_scope);
-      ReplaceOuterScope(Scope::DeserializeScopeChain(
-          isolate, info->zone(), *outer_scope_info, script_scope,
-          info->ast_value_factory(),
-          Scope::DeserializationMode::kIncludingVariables));
-    } else {
-      DCHECK_EQ(outer_scope_info->scope_type(), SCRIPT_SCOPE);
-      SetScriptScopeInfo(outer_scope_info);
-    }
-  }
-}
-
 bool DeclarationScope::Analyze(ParseInfo* info) {
   RuntimeCallTimerScope runtimeTimer(
       info->runtime_call_stats(),
@@ -1159,22 +1136,30 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
     // Lexical vs lexical conflicts within the same scope have already been
     // captured in Parser::Declare. The only conflicts we still need to check
     // are lexical vs nested var.
+    Scope* current = nullptr;
     if (decl->IsVariableDeclaration() &&
         decl->AsVariableDeclaration()->AsNested() != nullptr) {
-      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) {
-        // There is a conflict if there exists a non-VAR binding.
-        Variable* other_var =
-            current->variables_.Lookup(decl->var()->raw_name());
-        if (other_var != nullptr && IsLexicalVariableMode(other_var->mode())) {
-          return decl;
-        }
-        if (current->is_declaration_scope()) break;
-        current = current->outer_scope();
+      current = decl->AsVariableDeclaration()->AsNested()->scope();
+    } else if (is_eval_scope() && is_sloppy(language_mode())) {
+      if (IsLexicalVariableMode(decl->var()->mode())) continue;
+      current = outer_scope_;
+    }
+    if (current == nullptr) continue;
+    DCHECK(decl->var()->mode() == VariableMode::kVar ||
+           decl->var()->mode() == VariableMode::kDynamic);
+    // Iterate through all scopes until and including the declaration scope.
+    while (true) {
+      // There is a conflict if there exists a non-VAR binding.
+      Variable* other_var =
+          current->LookupInScopeOrScopeInfo(decl->var()->raw_name());
+      if (other_var != nullptr && IsLexicalVariableMode(other_var->mode())) {
+        return decl;
+      }
+      if (current->is_declaration_scope() &&
+          !(current->is_eval_scope() && is_sloppy(current->language_mode()))) {
+        break;
       }
+      current = current->outer_scope();
     }
   }
   return nullptr;

src/ast/scopes.h

@@ -934,10 +934,6 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
     return sloppy_block_function_map_;
   }
 
-  // Replaces the outer scope with the outer_scope_info in |info| if there is
-  // one.
-  void AttachOuterScopeInfo(ParseInfo* info, Isolate* isolate);
-
   // Compute top scope and allocate variables. For lazy compilation the top
   // scope only contains the single lazily compiled function, so this
   // doesn't re-allocate variables repeatedly.

src/contexts.cc

@@ -205,8 +205,7 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
     // 1. Check global objects, subjects of with, and extension objects.
     DCHECK_IMPLIES(context->IsEvalContext(),
                    context->extension()->IsTheHole(isolate));
-    if ((context->IsNativeContext() ||
-         (context->IsWithContext() && ((flags & SKIP_WITH_CONTEXT) == 0)) ||
+    if ((context->IsNativeContext() || context->IsWithContext() ||
          context->IsFunctionContext() || context->IsBlockContext()) &&
         !context->extension_receiver().is_null()) {
       Handle<JSReceiver> object(context->extension_receiver(), isolate);
@@ -310,8 +309,7 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
       // Check the slot corresponding to the intermediate context holding
       // only the function name variable. It's conceptually (and spec-wise)
       // in an outer scope of the function's declaration scope.
-      if (follow_context_chain && (flags & STOP_AT_DECLARATION_SCOPE) == 0 &&
-          context->IsFunctionContext()) {
+      if (follow_context_chain && context->IsFunctionContext()) {
         int function_index = scope_info->FunctionContextSlotIndex(*name);
         if (function_index >= 0) {
           if (FLAG_trace_contexts) {
@@ -382,20 +380,16 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
     }
 
     // 3. Prepare to continue with the previous (next outermost) context.
-    if (context->IsNativeContext() ||
-        ((flags & STOP_AT_DECLARATION_SCOPE) != 0 &&
-         context->is_declaration_context())) {
-      follow_context_chain = false;
-    } else {
-      do {
-        context = Handle<Context>(context->previous(), isolate);
-        // If we come across a whitelist context, and the name is not
-        // whitelisted, then only consider with, script, module or native
-        // contexts.
-      } while (failed_whitelist && !context->IsScriptContext() &&
-               !context->IsNativeContext() && !context->IsWithContext() &&
-               !context->IsModuleContext());
-    }
+    if (context->IsNativeContext()) break;
+
+    do {
+      context = Handle<Context>(context->previous(), isolate);
+      // If we come across a whitelist context, and the name is not
+      // whitelisted, then only consider with, script, module or native
+      // contexts.
+    } while (failed_whitelist && !context->IsScriptContext() &&
+             !context->IsNativeContext() && !context->IsWithContext() &&
+             !context->IsModuleContext());
   } while (follow_context_chain);
 
   if (FLAG_trace_contexts) {

src/contexts.h

@@ -22,8 +22,6 @@ class RegExpMatchInfo;
 enum ContextLookupFlags {
   FOLLOW_CONTEXT_CHAIN = 1 << 0,
   FOLLOW_PROTOTYPE_CHAIN = 1 << 1,
-  STOP_AT_DECLARATION_SCOPE = 1 << 2,
-  SKIP_WITH_CONTEXT = 1 << 3,
 
   DONT_FOLLOW_CHAINS = 0,
   FOLLOW_CHAINS = FOLLOW_CONTEXT_CHAIN | FOLLOW_PROTOTYPE_CHAIN,

src/parsing/parser.cc

@@ -442,14 +442,15 @@ void Parser::InitializeEmptyScopeChain(ParseInfo* info) {
 
 void Parser::DeserializeScopeChain(
     Isolate* isolate, ParseInfo* info,
-    MaybeHandle<ScopeInfo> maybe_outer_scope_info) {
+    MaybeHandle<ScopeInfo> maybe_outer_scope_info,
+    Scope::DeserializationMode mode) {
   InitializeEmptyScopeChain(info);
   Handle<ScopeInfo> outer_scope_info;
   if (maybe_outer_scope_info.ToHandle(&outer_scope_info)) {
     DCHECK(ThreadId::Current().Equals(isolate->thread_id()));
     original_scope_ = Scope::DeserializeScopeChain(
         isolate, zone(), *outer_scope_info, info->script_scope(),
-        ast_value_factory(), Scope::DeserializationMode::kScopesOnly);
+        ast_value_factory(), mode);
   }
 }
 
@@ -492,7 +493,8 @@ FunctionLiteral* Parser::ParseProgram(Isolate* isolate, ParseInfo* info) {
   if (V8_UNLIKELY(FLAG_log_function_events)) timer.Start();
 
   // Initialize parser state.
-  DeserializeScopeChain(isolate, info, info->maybe_outer_scope_info());
+  DeserializeScopeChain(isolate, info, info->maybe_outer_scope_info(),
+                        Scope::DeserializationMode::kIncludingVariables);
 
   scanner_.Initialize();
   if (FLAG_harmony_hashbang && !info->is_eval()) {
@@ -596,6 +598,12 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
       // unchanged if the property already exists.
       InsertSloppyBlockFunctionVarBindings(scope);
     }
+    // Internalize the ast strings in the case of eval so we can check for
+    // conflicting var declarations with outer scope-info-backed scopes.
+    if (info->is_eval()) {
+      DCHECK(parsing_on_main_thread_);
+      info->ast_value_factory()->Internalize(isolate);
+    }
     CheckConflictingVarDeclarations(scope);
 
     if (info->parse_restriction() == ONLY_SINGLE_FUNCTION_LITERAL) {
@@ -677,7 +685,8 @@ FunctionLiteral* Parser::ParseFunction(Isolate* isolate, ParseInfo* info,
   base::ElapsedTimer timer;
   if (V8_UNLIKELY(FLAG_log_function_events)) timer.Start();
 
-  DeserializeScopeChain(isolate, info, info->maybe_outer_scope_info());
+  DeserializeScopeChain(isolate, info, info->maybe_outer_scope_info(),
+                        Scope::DeserializationMode::kIncludingVariables);
   DCHECK_EQ(factory()->zone(), info->zone());
 
   // Initialize parser state.

src/parsing/parser.h

@@ -194,7 +194,9 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   // their corresponding scope infos. Therefore, looking up variables in the
   // deserialized scopes is not possible.
   void DeserializeScopeChain(Isolate* isolate, ParseInfo* info,
-                             MaybeHandle<ScopeInfo> maybe_outer_scope_info);
+                             MaybeHandle<ScopeInfo> maybe_outer_scope_info,
+                             Scope::DeserializationMode mode =
+                                 Scope::DeserializationMode::kScopesOnly);
 
   // Move statistics to Isolate
   void UpdateStatistics(Isolate* isolate, Handle<Script> script);

src/parsing/parsing.cc

@@ -42,7 +42,6 @@ bool ParseProgram(ParseInfo* info, Isolate* isolate) {
     info->pending_error_handler()->ReportErrors(isolate, info->script(),
                                                 info->ast_value_factory());
   } else {
-    result->scope()->AttachOuterScopeInfo(info, isolate);
     info->set_language_mode(info->literal()->language_mode());
     if (info->is_eval()) {
       info->set_allow_eval_cache(parser.allow_eval_cache());
@@ -80,7 +79,7 @@ bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
     info->pending_error_handler()->ReportErrors(isolate, info->script(),
                                                 info->ast_value_factory());
   } else {
-    result->scope()->AttachOuterScopeInfo(info, isolate);
+    info->ast_value_factory()->Internalize(isolate);
     if (info->is_eval()) {
       info->set_allow_eval_cache(parser.allow_eval_cache());
     }

src/runtime/runtime-scopes.cc

@@ -224,8 +224,7 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   // context, or a declaration block scope. Since this is called from eval, the
   // context passed is the context of the caller, which may be some nested
   // context and not the declaration context.
-  Handle<Context> context_arg(isolate->context(), isolate);
-  Handle<Context> context(context_arg->declaration_context(), isolate);
+  Handle<Context> context(isolate->context()->declaration_context(), isolate);
 
   DCHECK(context->IsFunctionContext() || context->IsNativeContext() ||
          context->IsScriptContext() || context->IsEvalContext() ||
@@ -241,23 +240,6 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   InitializationFlag init_flag;
   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::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
-    // exception.
-    // ES#sec-evaldeclarationinstantiation 5.d.ii.2.a.i:
-    // Throw a SyntaxError exception.
-    return ThrowRedeclarationError(isolate, name,
-                                   RedeclarationType::kSyntaxError);
-  }
-
   Handle<Object> holder =
       Context::Lookup(context, name, DONT_FOLLOW_CHAINS, &index, &attributes,
                       &init_flag, &mode);
@@ -273,9 +255,9 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
                          value, NONE, is_var, is_function,
                          RedeclarationType::kTypeError);
   }
-  if (context_arg->extension()->IsJSGlobalObject()) {
-    Handle<JSGlobalObject> global(
-        JSGlobalObject::cast(context_arg->extension()), isolate);
+  if (context->extension()->IsJSGlobalObject()) {
+    Handle<JSGlobalObject> global(JSGlobalObject::cast(context->extension()),
+                                  isolate);
     return DeclareGlobal(isolate, global, name, value, NONE, is_var,
                          is_function, RedeclarationType::kTypeError);
   } else if (context->IsScriptContext()) {
@@ -303,7 +285,7 @@ Object DeclareEvalHelper(Isolate* isolate, Handle<String> name,
 
   } else if (context->has_extension()) {
     object = handle(context->extension_object(), isolate);
-    DCHECK(object->IsJSContextExtensionObject() || object->IsJSGlobalObject());
+    DCHECK(object->IsJSContextExtensionObject());
   } else {
     // Sloppy varblock and function contexts might not have an extension object
     // yet. Sloppy eval will never have an extension object, as vars are hoisted

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

@@ -141,15 +141,15 @@ try {
 }
 assertTrue(caught);
 
-caught = false
-try {
-  (function() {
-    {
-      let x = 1;
-      eval('{ function x() {} }');
-    }
-  })();
-} catch (e) {
-  caught = true;
-}
-assertFalse(caught);
+// See ES#sec-web-compat-evaldeclarationinstantiation. Sloppy block functions
+// inside of blocks in eval behave similar to regular sloppy block function
+// hoisting: the var declaration on the function level is only created if
+// it would not cause a syntax error. A masking let would cause a conflicting
+// var declaration syntax error, and hence the var isn't introduced.
+(function() {
+  {
+    let x = 1;
+    eval('{ function x() {} }');
+    assertEquals(1, x);
+  }
+})();