[parser] Change how conflicting param and let in body declarations are detected

Now we just check for each variable declared in the parameter scope whether it
occurs as a lexical variable in the body scope. This way the preparser will
also identify them.

Bug: v8:2728, v8:5064
Change-Id: I9fd96590fa431de0656c85295fd31af9b36f2e32
Reviewed-on: https://chromium-review.googlesource.com/c/1384225
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58375}

src/ast/scopes.cc

@@ -1204,24 +1204,13 @@ Variable* Scope::NewTemporary(const AstRawString* name,
 
 Declaration* Scope::CheckConflictingVarDeclarations() {
   for (Declaration* decl : decls_) {
-    VariableMode mode = decl->proxy()->var()->mode();
-
     // 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, or any declarations within a declaration
+    // are lexical vs nested var.
-    // block scope vs lexical declarations in its surrounding (function) scope.
-    Scope* current = this;
     if (decl->IsVariableDeclaration() &&
         decl->AsVariableDeclaration()->AsNested() != nullptr) {
-      DCHECK_EQ(mode, VariableMode::kVar);
+      DCHECK_EQ(decl->proxy()->var()->mode(), VariableMode::kVar);
-      current = decl->AsVariableDeclaration()->AsNested()->scope();
+      Scope* current = decl->AsVariableDeclaration()->AsNested()->scope();
-    } else if (IsLexicalVariableMode(mode)) {
-      if (!is_block_scope()) continue;
-      DCHECK(is_declaration_scope());
-      DCHECK_EQ(outer_scope()->scope_type(), FUNCTION_SCOPE);
-      current = outer_scope();
-    }
-
       // Iterate through all scopes until and including the declaration scope.
       while (true) {
         // There is a conflict if there exists a non-VAR binding.
@@ -1234,21 +1223,18 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
         current = current->outer_scope();
       }
     }
+  }
   return nullptr;
 }
 
-const AstRawString* Scope::FindLexVariableDeclaredIn(Scope* scope) {
+const AstRawString* Scope::FindVariableDeclaredIn(Scope* scope,
-  DCHECK(is_block_scope());
+                                                  VariableMode mode_limit) {
   const VariableMap& variables = scope->variables_;
   for (ZoneHashMap::Entry* p = variables.Start(); p != nullptr;
        p = variables.Next(p)) {
     const AstRawString* name = static_cast<const AstRawString*>(p->key);
     Variable* var = LookupLocal(name);
-    if (var != nullptr) {
+    if (var != nullptr && var->mode() <= mode_limit) return name;
-      // Conflict; find and return its declaration.
-      DCHECK(IsLexicalVariableMode(var->mode()));
-      return name;
-    }
   }
   return nullptr;
 }

src/ast/scopes.h

@@ -312,11 +312,13 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // scope over a let binding of the same name.
   Declaration* CheckConflictingVarDeclarations();
 
-  // Find lexical variable that has a name that was declared in |scope|. This is
+  // Find variable with (variable->mode() <= |mode_limit|) that was declared in
-  // used to catch patterns like `try{}catch(e){let e;}`, which is an error even
+  // |scope|. This is used to catch patterns like `try{}catch(e){let e;}` and
-  // though the two 'e's are declared in different scopes. Returns the first
+  // function([e]) { let e }, which are errors even though the two 'e's are each
-  // duplicate variable name if there is one, nullptr otherwise.
+  // time declared in different scopes. Returns the first duplicate variable
-  const AstRawString* FindLexVariableDeclaredIn(Scope* scope);
+  // name if there is one, nullptr otherwise.
+  const AstRawString* FindVariableDeclaredIn(Scope* scope,
+                                             VariableMode mode_limit);
 
   // Find the declaration that introduced |name|.
   Declaration* DeclarationFor(const AstRawString* name);

src/globals.h

@@ -1056,10 +1056,12 @@ enum class VariableMode : uint8_t {
                    // variable is global unless it has been shadowed
                    // by an eval-introduced variable
 
-  kDynamicLocal  // requires dynamic lookup, but we know that the
+  kDynamicLocal,  // requires dynamic lookup, but we know that the
                   // variable is local and where it is unless it
                   // has been shadowed by an eval-introduced
                   // variable
+
+  kLastLexicalVariableMode = kConst,
 };
 
 // Printing support
@@ -1104,7 +1106,7 @@ inline bool IsDeclaredVariableMode(VariableMode mode) {
 inline bool IsLexicalVariableMode(VariableMode mode) {
   STATIC_ASSERT(static_cast<uint8_t>(VariableMode::kLet) ==
                 0);  // Implies that mode >= VariableMode::kLet.
-  return mode <= VariableMode::kConst;
+  return mode <= VariableMode::kLastLexicalVariableMode;
 }
 
 enum VariableLocation : uint8_t {

src/parsing/parser-base.h

@@ -3905,6 +3905,11 @@ void ParserBase<Impl>::ParseFunctionBody(
 
     inner_scope->set_end_position(end_position());
     if (inner_scope->FinalizeBlockScope() != nullptr) {
+      const AstRawString* conflict = inner_scope->FindVariableDeclaredIn(
+          function_scope, VariableMode::kLastLexicalVariableMode);
+      if (conflict != nullptr) {
+        impl()->ReportVarRedeclarationIn(conflict, inner_scope);
+      }
       impl()->CheckConflictingVarDeclarations(inner_scope);
       impl()->InsertShadowingVarBindingInitializers(inner_block);
     } else {
@@ -4042,6 +4047,13 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
         // body is preparsed; move relevant parts of parameter handling to
         // simulate consistent parameter handling.
 
+        // Building the parameter initialization block declares the parameters.
+        // TODO(verwaest): Rely on ArrowHeadParsingScope instead.
+        if (!formal_parameters.is_simple) {
+          impl()->BuildParameterInitializationBlock(formal_parameters);
+          if (has_error()) return impl()->FailureExpression();
+        }
+
         // For arrow functions, we don't need to retrieve data about function
         // parameters.
         int dummy_num_parameters = -1;
@@ -5290,11 +5302,11 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseTryStatement() {
                 const AstRawString* name = catch_info.variable->raw_name();
                 if (inner_scope->LookupLocal(name)) conflict = name;
               } else {
-                conflict = inner_scope->FindLexVariableDeclaredIn(scope());
+                conflict = inner_scope->FindVariableDeclaredIn(
+                    scope(), VariableMode::kVar);
               }
               if (conflict != nullptr) {
-                impl()->ReportConflictingDeclarationInCatch(conflict,
+                impl()->ReportVarRedeclarationIn(conflict, inner_scope);
-                                                            inner_scope);
               }
             }
 

src/parsing/parser.cc

@@ -1589,8 +1589,7 @@ Block* Parser::RewriteCatchPattern(CatchInfo* catch_info) {
   return init_block;
 }
 
-void Parser::ReportConflictingDeclarationInCatch(const AstRawString* name,
+void Parser::ReportVarRedeclarationIn(const AstRawString* name, Scope* scope) {
-                                                 Scope* scope) {
   for (Declaration* decl : *scope->declarations()) {
     if (decl->proxy()->raw_name() == name) {
       int position = decl->proxy()->position();
@@ -2869,9 +2868,6 @@ Block* Parser::BuildParameterInitializationBlock(
 
     if (param_block != init_block) {
       param_scope = param_scope->FinalizeBlockScope();
-      if (param_scope != nullptr) {
-        CheckConflictingVarDeclarations(param_scope);
-      }
       init_block->statements()->Add(param_block, zone());
     }
     ++index;

src/parsing/parser.h

@@ -318,8 +318,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   Statement* RewriteSwitchStatement(SwitchStatement* switch_statement,
                                     Scope* scope);
   Block* RewriteCatchPattern(CatchInfo* catch_info);
-  void ReportConflictingDeclarationInCatch(const AstRawString* name,
+  void ReportVarRedeclarationIn(const AstRawString* name, Scope* scope);
-                                           Scope* scope);
   Statement* RewriteTryStatement(Block* try_block, Block* catch_block,
                                  const SourceRange& catch_range,
                                  Block* finally_block,

src/parsing/preparser.cc

@@ -116,13 +116,7 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   function_scope->set_is_being_lazily_parsed(true);
 #endif
 
-  // Start collecting data for a new function which might contain skippable
+  PreParserFormalParameters formals(function_scope);
-  // functions.
-  PreParsedScopeDataBuilder::DataGatheringScope
-      preparsed_scope_data_builder_scope(this);
-  if (!IsArrowFunction(kind)) {
-    preparsed_scope_data_builder_scope.Start(function_scope);
-  }
 
   // In the preparser, we use the function literal ids to count how many
   // FunctionLiterals were encountered. The PreParser doesn't actually persist
@@ -136,11 +130,18 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   DCHECK_NULL(scope_);
   FunctionState function_state(&function_state_, &scope_, function_scope);
 
-  PreParserFormalParameters formals(function_scope);
+  // Start collecting data for a new function which might contain skippable
+  // functions.
+  PreParsedScopeDataBuilder::DataGatheringScope
+      preparsed_scope_data_builder_scope(this);
+
+  if (IsArrowFunction(kind)) {
+    formals.is_simple = function_scope->has_simple_parameters();
+  } else {
+    preparsed_scope_data_builder_scope.Start(function_scope);
 
     // Parse non-arrow function parameters. For arrow functions, the parameters
     // have already been parsed.
-  if (!IsArrowFunction(kind)) {
     DeclarationParsingScope formals_scope(
         this, ExpressionScope::kParameterDeclaration);
     // We return kPreParseSuccess in failure cases too - errors are retrieved
@@ -187,7 +188,11 @@ PreParser::PreParseResult PreParser::PreParseFunction(
 
     SetLanguageMode(function_scope, inner_scope->language_mode());
     inner_scope->set_end_position(scanner()->peek_location().end_pos);
-    inner_scope->FinalizeBlockScope();
+    if (inner_scope->FinalizeBlockScope() != nullptr) {
+      const AstRawString* conflict = inner_scope->FindVariableDeclaredIn(
+          function_scope, VariableMode::kLastLexicalVariableMode);
+      if (conflict != nullptr) ReportVarRedeclarationIn(conflict, inner_scope);
+    }
   }
 
   use_counts_ = nullptr;

src/parsing/preparser.h

@@ -1163,7 +1163,7 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserBlock::Default();
   }
 
-  V8_INLINE void ReportConflictingDeclarationInCatch(const AstRawString* name,
+  V8_INLINE void ReportVarRedeclarationIn(const AstRawString* name,
                                           Scope* scope) {
     ReportUnidentifiableError();
   }

test/message/fail/param-arrow-redeclaration-as-let.js

@@ -0,0 +1,7 @@
+// Copyright 2018 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.
+
+({x}) => {
+  let x
+}

test/message/fail/param-arrow-redeclaration-as-let.out

@@ -0,0 +1,5 @@
+*%(basename)s:6: SyntaxError: Identifier 'x' has already been declared
+  let x
+      ^
+SyntaxError: Identifier 'x' has already been declared
+

test/message/fail/param-async-arrow-redeclaration-as-let.js

@@ -0,0 +1,7 @@
+// Copyright 2018 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.
+
+async ({x}) => {
+  let x
+}

test/message/fail/param-async-arrow-redeclaration-as-let.out

@@ -0,0 +1,5 @@
+*%(basename)s:6: SyntaxError: Identifier 'x' has already been declared
+  let x
+      ^
+SyntaxError: Identifier 'x' has already been declared
+

test/message/fail/param-async-function-redeclaration-as-let.js

@@ -0,0 +1,7 @@
+// Copyright 2018 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.
+
+async function f({x}) {
+  let x
+}

test/message/fail/param-async-function-redeclaration-as-let.out

@@ -0,0 +1,5 @@
+*%(basename)s:6: SyntaxError: Identifier 'x' has already been declared
+  let x
+      ^
+SyntaxError: Identifier 'x' has already been declared
+

test/message/fail/param-function-redeclaration-as-let.js

@@ -0,0 +1,7 @@
+// Copyright 2018 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 f({x}) {
+  let x
+}

test/message/fail/param-function-redeclaration-as-let.out

@@ -0,0 +1,5 @@
+*%(basename)s:6: SyntaxError: Identifier 'x' has already been declared
+  let x
+      ^
+SyntaxError: Identifier 'x' has already been declared
+

test/test262/test262.status

@@ -471,10 +471,6 @@
   'language/statements/try/tco-finally': [SKIP],
   'language/statements/while/tco-body': [SKIP],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5064
-  'language/expressions/arrow-function/dflt-params-duplicates': [FAIL],
-  'language/expressions/async-arrow-function/dflt-params-duplicates': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=5327
   'built-ins/TypedArrayConstructors/internals/Set/key-is-minus-zero': [FAIL],
   'built-ins/TypedArrayConstructors/internals/Set/BigInt/key-is-minus-zero': [FAIL],