[parser] Detect var/let conflicts in the preparser

Also insert NestedVariableDeclarations in the preparser if they occur. This
should be uncommon enough to not hurt preparser performance. This will also
allow us to stop checking for conflicts on already preparsed code. Since the
preparser itself will mainly run off the main thread, this can allow us to free
some main-thread time.

Bug: v8:7829, v8:8706
Change-Id: I03f2690eb7b22e941995d6f2697e64211ddbeffb
Reviewed-on: https://chromium-review.googlesource.com/c/1430069Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59044}

src/ast/scopes.cc

@@ -1139,6 +1139,16 @@ Variable* Scope::NewTemporary(const AstRawString* name,
 }
 
 Declaration* Scope::CheckConflictingVarDeclarations() {
+  bool is_sloppy_eval = is_eval_scope() && is_sloppy(language_mode());
+  // In case of a regular function/script, check all scopes except for the scope
+  // in which the var ends up being declared.
+  Scope* end = this;
+  // In the case of eval, check all scopes up to and including the next
+  // declaration scope.
+  if (is_sloppy_eval) {
+    while (end->is_eval_scope()) end = end->outer_scope_->GetDeclarationScope();
+    end = end->outer_scope_;
+  }
   for (Declaration* decl : decls_) {
     // Lexical vs lexical conflicts within the same scope have already been
     // captured in Parser::Declare. The only conflicts we still need to check
@@ -1147,7 +1157,7 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
     if (decl->IsVariableDeclaration() &&
         decl->AsVariableDeclaration()->AsNested() != nullptr) {
       current = decl->AsVariableDeclaration()->AsNested()->scope();
-    } else if (is_eval_scope() && is_sloppy(language_mode())) {
+    } else if (is_sloppy_eval) {
       if (IsLexicalVariableMode(decl->var()->mode())) continue;
       current = outer_scope_;
     }
@@ -1155,19 +1165,15 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
     DCHECK(decl->var()->mode() == VariableMode::kVar ||
            decl->var()->mode() == VariableMode::kDynamic);
     // Iterate through all scopes until and including the declaration scope.
-    while (true) {
+    do {
       // 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();
-    }
+    } while (current != end);
   }
   return nullptr;
 }

src/parsing/parser-base.h

@@ -1078,6 +1078,31 @@ class ParserBase {
                          FunctionLiteral::FunctionType function_type,
                          FunctionBodyType body_type);
 
+  // Check if the scope has conflicting var/let declarations from different
+  // scopes. This covers for example
+  //
+  // function f() { { { var x; } let x; } }
+  // function g() { { var x; let x; } }
+  //
+  // The var declarations are hoisted to the function scope, but originate from
+  // a scope where the name has also been let bound or the var declaration is
+  // hoisted over such a scope.
+  void CheckConflictingVarDeclarations(Scope* scope) {
+    if (has_error()) return;
+    Declaration* decl = scope->CheckConflictingVarDeclarations();
+    if (decl != nullptr) {
+      // In ES6, conflicting variable bindings are early errors.
+      const AstRawString* name = decl->var()->raw_name();
+      int position = decl->position();
+      Scanner::Location location =
+          position == kNoSourcePosition
+              ? Scanner::Location::invalid()
+              : Scanner::Location(position, position + 1);
+      impl()->ReportMessageAt(location, MessageTemplate::kVarRedeclaration,
+                              name);
+    }
+  }
+
   // TODO(nikolaos, marja): The first argument should not really be passed
   // by value. The method is expected to add the parsed statements to the
   // list. This works because in the case of the parser, StatementListT is
@@ -3800,6 +3825,8 @@ void ParserBase<Impl>::ParseFunctionBody(
 
   bool allow_duplicate_parameters = false;
 
+  CheckConflictingVarDeclarations(inner_scope);
+
   if (V8_LIKELY(parameters.is_simple)) {
     DCHECK_EQ(inner_scope, function_scope);
     if (is_sloppy(function_scope->language_mode())) {
@@ -3828,7 +3855,6 @@ void ParserBase<Impl>::ParseFunctionBody(
       if (conflict != nullptr) {
         impl()->ReportVarRedeclarationIn(conflict, inner_scope);
       }
-      impl()->CheckConflictingVarDeclarations(inner_scope);
       impl()->InsertShadowingVarBindingInitializers(inner_block);
     }
   }

src/parsing/parser.cc

@@ -2411,7 +2411,6 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   if (is_strict(language_mode)) {
     CheckStrictOctalLiteral(scope->start_position(), scope->end_position());
   }
-  CheckConflictingVarDeclarations(scope);
 
   FunctionLiteral::ParameterFlag duplicate_parameters =
       has_duplicate_parameters ? FunctionLiteral::kHasDuplicateParameters
@@ -2937,21 +2936,6 @@ Expression* Parser::RewriteClassLiteral(Scope* block_scope,
   return class_literal;
 }
 
-void Parser::CheckConflictingVarDeclarations(Scope* scope) {
-  if (has_error()) return;
-  Declaration* decl = scope->CheckConflictingVarDeclarations();
-  if (decl != nullptr) {
-    // In ES6, conflicting variable bindings are early errors.
-    const AstRawString* name = decl->var()->raw_name();
-    int position = decl->position();
-    Scanner::Location location =
-        position == kNoSourcePosition
-            ? Scanner::Location::invalid()
-            : Scanner::Location(position, position + 1);
-    ReportMessageAt(location, MessageTemplate::kVarRedeclaration, name);
-  }
-}
-
 bool Parser::IsPropertyWithPrivateFieldKey(Expression* expression) {
   if (!expression->IsProperty()) return false;
   Property* property = expression->AsProperty();

src/parsing/parser.h

@@ -401,17 +401,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return object_literal;
   }
 
-  // Check if the scope has conflicting var/let declarations from different
-  // scopes. This covers for example
-  //
-  // function f() { { { var x; } let x; } }
-  // function g() { { var x; let x; } }
-  //
-  // The var declarations are hoisted to the function scope, but originate from
-  // a scope where the name has also been let bound or the var declaration is
-  // hoisted over such a scope.
-  void CheckConflictingVarDeclarations(Scope* scope);
-
   bool IsPropertyWithPrivateFieldKey(Expression* property);
 
   // Insert initializer statements for var-bindings shadowing parameter bindings

src/parsing/preparser.cc

@@ -89,6 +89,7 @@ PreParser::PreParseResult PreParser::PreParseProgram() {
   int start_position = peek_position();
   PreParserScopedStatementList body(pointer_buffer());
   ParseStatementList(&body, Token::EOS);
+  CheckConflictingVarDeclarations(scope);
   original_scope_ = nullptr;
   if (stack_overflow()) return kPreParseStackOverflow;
   if (is_strict(language_mode())) {
@@ -173,6 +174,7 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   }
 
   bool allow_duplicate_parameters = false;
+  CheckConflictingVarDeclarations(inner_scope);
 
   if (formals.is_simple) {
     if (is_sloppy(function_scope->language_mode())) {

src/parsing/preparser.h

@@ -1047,8 +1047,6 @@ class PreParser : public ParserBase<PreParser> {
       const PreParserExpression& expression) {
     return expression.IsPropertyWithPrivateFieldKey();
   }
-  V8_INLINE void CheckConflictingVarDeclarations(Scope* scope) {}
-
   V8_INLINE void SetLanguageMode(Scope* scope, LanguageMode mode) {
     scope->SetLanguageMode(mode);
   }
@@ -1092,14 +1090,27 @@ class PreParser : public ParserBase<PreParser> {
   void DeclareVariable(VariableProxy* proxy, VariableKind kind,
                        VariableMode mode, InitializationFlag init, Scope* scope,
                        bool* was_added, int position) {
-    DeclareVariableName(proxy->raw_name(), mode, scope, was_added, kind);
+    DeclareVariableName(proxy->raw_name(), mode, scope, was_added, position,
+                        kind);
   }
 
   void DeclareVariableName(const AstRawString* name, VariableMode mode,
                            Scope* scope, bool* was_added,
+                           int position = kNoSourcePosition,
                            VariableKind kind = NORMAL_VARIABLE) {
-    if (scope->DeclareVariableName(name, mode, was_added, kind) == nullptr) {
+    Variable* var = scope->DeclareVariableName(name, mode, was_added, kind);
+    if (var == nullptr) {
       ReportUnidentifiableError();
+      return;
+    }
+    if (var->scope() != scope) {
+      DCHECK_NE(kNoSourcePosition, position);
+      DCHECK_EQ(VariableMode::kVar, mode);
+      Declaration* nested_declaration =
+          factory()->ast_node_factory()->NewNestedVariableDeclaration(scope,
+                                                                      position);
+      nested_declaration->set_var(var);
+      var->scope()->declarations()->Add(nested_declaration);
     }
   }
 

test/test262/test262.status

@@ -374,9 +374,6 @@
   'language/literals/regexp/u-unicode-esc-non-hex': [FAIL_PHASE_ONLY],
   'language/literals/regexp/unicode-escape-nls-err': [FAIL_PHASE_ONLY],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=7829
-  'language/block-scope/syntax/redeclaration/function-declaration-attempt-to-redeclare-with-var-declaration-nested-in-function': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=4628
   'language/eval-code/direct/non-definable-function-with-function': [FAIL],
   'language/eval-code/direct/non-definable-function-with-variable': [FAIL],
@@ -575,15 +572,6 @@
   'language/expressions/await/for-await-of-interleaved': ['--harmony-await-optimization'],
   'language/expressions/await/async-await-interleaved': ['--harmony-await-optimization'],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=8706
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-async-function': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-async-generator': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-class': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-const': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-function': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-generator': [FAIL],
-  'language/block-scope/syntax/redeclaration/fn-scope-var-name-redeclaration-attempt-with-let' : [FAIL],
-
   # https://github.com/tc39/test262/issues/2033
   'language/expressions/class/elements/private-derived-cls-direct-eval-err-contains-supercall': [SKIP],
   'language/expressions/class/elements/private-derived-cls-direct-eval-err-contains-supercall-1': [SKIP],