Reland of Add errors for declarations which conflict with catch param…

…eters. (patchset #1 id:1 of https://codereview.chromium.org/2112223002/ )

Reason for revert:
Correcting issue.

Original issue's description:
> Revert of Add errors for declarations which conflict with catch parameters. (patchset #6 id:100001 of https://codereview.chromium.org/2109733003/ )
>
> Reason for revert:
> Fuzzer claims `try {  \"\" ; } catch(x) { let x1 = [1,,], x = x; }` causes a crash.
>
> Original issue's description:
> > Add errors for declarations which conflict with catch parameters.
> >
> > Catch parameters are largely treated as lexical declarations in the
> > block which contains their body for the purposes of early syntax errors,
> > with some exceptions outlined in B.3.5. This patch introduces most of
> > those errors, except those from `eval('for (var e of ...);')` inside of
> > a catch with a simple parameter named 'e'.
> >
> > Note that annex B.3.5 allows var declarations to conflict with simple
> > catch parameters, except when the variable declaration is the init of a
> > for-of statement.
> >
> > BUG=v8:5112,v8:4231
> >
> > Committed: https://crrev.com/2907c726b2bb5cf20b2bec639ca9e6a521585406
> > Cr-Commit-Position: refs/heads/master@{#37462}
>
> TBR=littledan@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:5112,v8:4231
>
> Committed: https://crrev.com/8834d5ecb559001c87c42322969471da60574a8c
> Cr-Commit-Position: refs/heads/master@{#37464}

R=littledan@chromium.org
BUG=v8:5112,v8:4231

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

src/ast/scopes.cc

@@ -141,7 +141,7 @@ Scope::Scope(Zone* zone, Scope* inner_scope,
       zone_(zone) {
   SetDefaults(CATCH_SCOPE, NULL, Handle<ScopeInfo>::null());
   AddInnerScope(inner_scope);
-  ++num_var_or_const_;
+  ++num_var_;
   num_heap_slots_ = Context::MIN_CONTEXT_SLOTS;
   Variable* variable = variables_.Declare(this,
                                           catch_variable_name,
@@ -185,7 +185,7 @@ void Scope::SetDefaults(ScopeType scope_type, Scope* outer_scope,
   force_eager_compilation_ = false;
   force_context_allocation_ = (outer_scope != NULL && !is_function_scope())
       ? outer_scope->has_forced_context_allocation() : false;
-  num_var_or_const_ = 0;
+  num_var_ = 0;
   num_stack_slots_ = 0;
   num_heap_slots_ = 0;
   num_global_slots_ = 0;
@@ -339,8 +339,7 @@ Scope* Scope::FinalizeBlockScope() {
   DCHECK(temps_.is_empty());
   DCHECK(params_.is_empty());
 
-  if (num_var_or_const() > 0 ||
-      (is_declaration_scope() && calls_sloppy_eval())) {
+  if (num_var() > 0 || (is_declaration_scope() && calls_sloppy_eval())) {
     return this;
   }
 
@@ -512,7 +511,7 @@ Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
   // introduced during variable allocation, and TEMPORARY variables are
   // allocated via NewTemporary().
   DCHECK(IsDeclaredVariableMode(mode));
-  ++num_var_or_const_;
+  ++num_var_;
   return variables_.Declare(this, name, mode, kind, init_flag,
                             maybe_assigned_flag);
 }
@@ -611,6 +610,25 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
   return NULL;
 }
 
+Declaration* Scope::CheckLexDeclarationsConflictingWith(
+    const ZoneList<const AstRawString*>& names) {
+  DCHECK(is_block_scope());
+  for (int i = 0; i < names.length(); ++i) {
+    Variable* var = LookupLocal(names.at(i));
+    if (var != nullptr) {
+      // Conflict; find and return its declaration.
+      DCHECK(IsLexicalVariableMode(var->mode()));
+      const AstRawString* name = names.at(i);
+      for (int j = 0; j < decls_.length(); ++j) {
+        if (decls_[j]->proxy()->raw_name() == name) {
+          return decls_[j];
+        }
+      }
+      DCHECK(false);
+    }
+  }
+  return nullptr;
+}
 
 class VarAndOrder {
  public:

src/ast/scopes.h

@@ -236,6 +236,14 @@ class Scope: public ZoneObject {
   // scope over a let binding of the same name.
   Declaration* CheckConflictingVarDeclarations();
 
+  // Check if the scope has a conflicting lexical declaration that has a name in
+  // the given list. This is used to catch patterns like
+  // `try{}catch(e){let e;}`,
+  // which is an error even though the two 'e's are declared in different
+  // scopes.
+  Declaration* CheckLexDeclarationsConflictingWith(
+      const ZoneList<const AstRawString*>& names);
+
   // ---------------------------------------------------------------------------
   // Scope-specific info.
 
@@ -491,6 +499,12 @@ class Scope: public ZoneObject {
   // The ModuleDescriptor for this scope; only for module scopes.
   ModuleDescriptor* module() const { return module_descriptor_; }
 
+  const AstRawString* catch_variable_name() const {
+    DCHECK(is_catch_scope());
+    DCHECK(num_var() == 1);
+    return static_cast<AstRawString*>(variables_.Start()->key);
+  }
+
   // ---------------------------------------------------------------------------
   // Variable allocation.
 
@@ -501,8 +515,8 @@ class Scope: public ZoneObject {
                                     ZoneList<Variable*>* context_locals,
                                     ZoneList<Variable*>* context_globals);
 
-  // Current number of var or const locals.
-  int num_var_or_const() { return num_var_or_const_; }
+  // Current number of var locals.
+  int num_var() const { return num_var_; }
 
   // Result of variable allocation.
   int num_stack_slots() const { return num_stack_slots_; }
@@ -673,7 +687,7 @@ class Scope: public ZoneObject {
   bool is_declaration_scope_;
 
   // Computed as variables are declared.
-  int num_var_or_const_;
+  int num_var_;
 
   // Computed via AllocateVariables; function, block and catch scopes only.
   int num_stack_slots_;

src/parsing/parser.cc

@@ -1889,11 +1889,10 @@ Statement* Parser::ParseStatementAsUnlabelled(
 
 VariableProxy* Parser::NewUnresolved(const AstRawString* name,
                                      VariableMode mode) {
-  // If we are inside a function, a declaration of a var/const variable is a
+  // If we are inside a function, a declaration of a 'var' variable is a
   // truly local variable, and the scope of the variable is always the function
   // scope.
-  // Let/const variables in harmony mode are always added to the immediately
-  // enclosing scope.
+  // Let/const variables are always added to the immediately enclosing scope.
   Scope* scope =
       IsLexicalVariableMode(mode) ? scope_ : scope_->DeclarationScope();
   return scope->NewUnresolved(factory(), name, Variable::NORMAL,
@@ -2311,11 +2310,11 @@ Block* Parser::ParseVariableStatement(VariableDeclarationContext var_context,
   // VariableStatement ::
   //   VariableDeclarations ';'
 
-  // The scope of a var/const declared variable anywhere inside a function
+  // The scope of a var declared variable anywhere inside a function
   // is the entire function (ECMA-262, 3rd, 10.1.3, and 12.2). Thus we can
-  // transform a source-level var/const declaration into a (Function)
-  // Scope declaration, and rewrite the source-level initialization into an
-  // assignment statement. We use a block to collect multiple assignments.
+  // transform a source-level var declaration into a (Function) Scope
+  // declaration, and rewrite the source-level initialization into an assignment
+  // statement. We use a block to collect multiple assignments.
   //
   // We mark the block as initializer block because we don't want the
   // rewriter to add a '.result' assignment to such a block (to get compliant
@@ -3013,6 +3012,8 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
 
         Expect(Token::RPAREN, CHECK_OK);
 
+        ZoneList<const AstRawString*> bound_names(1, zone());
+
         if (!is_simple) {
           DeclarationDescriptor descriptor;
           descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
@@ -3033,20 +3034,34 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
           Block* init_block =
               factory()->NewBlock(nullptr, 8, true, kNoSourcePosition);
           PatternRewriter::DeclareAndInitializeVariables(
-              init_block, &descriptor, &decl, nullptr, CHECK_OK);
+              init_block, &descriptor, &decl, &bound_names, CHECK_OK);
           catch_block->statements()->Add(init_block, zone());
+        } else {
+          bound_names.Add(name, zone());
         }
 
-        // TODO(adamk): This should call ParseBlock in order to properly
-        // add an additional block scope for the catch body.
-        Expect(Token::LBRACE, CHECK_OK);
-        while (peek() != Token::RBRACE) {
-          Statement* stat = ParseStatementListItem(CHECK_OK);
-          if (stat && !stat->IsEmpty()) {
-            catch_block->statements()->Add(stat, zone());
+        Block* inner_block = ParseBlock(nullptr, CHECK_OK);
+        catch_block->statements()->Add(inner_block, zone());
+
+        // Check for `catch(e) { let e; }` and similar errors.
+        Scope* inner_block_scope = inner_block->scope();
+        if (inner_block_scope != nullptr) {
+          Declaration* decl =
+              inner_block_scope->CheckLexDeclarationsConflictingWith(
+                  bound_names);
+          if (decl != nullptr) {
+            const AstRawString* name = decl->proxy()->raw_name();
+            int position = decl->proxy()->position();
+            Scanner::Location location =
+                position == kNoSourcePosition
+                    ? Scanner::Location::invalid()
+                    : Scanner::Location(position, position + 1);
+            ParserTraits::ReportMessageAt(
+                location, MessageTemplate::kVarRedeclaration, name);
+            *ok = false;
+            return nullptr;
           }
         }
-        Consume(Token::RBRACE);
       }
       block_scope->set_end_position(scanner()->location().end_pos);
       block_scope = block_scope->FinalizeBlockScope();
@@ -3622,7 +3637,8 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
                                      bool* ok) {
   int stmt_pos = peek_position();
   Statement* init = NULL;
-  ZoneList<const AstRawString*> lexical_bindings(1, zone());
+  ZoneList<const AstRawString*> bound_names(1, zone());
+  bool bound_names_are_lexical = false;
 
   // Create an in-between scope for let-bound iteration variables.
   Scope* for_scope = NewScope(scope_, BLOCK_SCOPE);
@@ -3673,10 +3689,12 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
         }
 
         Block* init_block = nullptr;
+        bound_names_are_lexical =
+            IsLexicalVariableMode(parsing_result.descriptor.mode);
 
-        // special case for legacy for (var/const x =.... in)
-        if (!IsLexicalVariableMode(parsing_result.descriptor.mode) &&
-            decl.pattern->IsVariableProxy() && decl.initializer != nullptr) {
+        // special case for legacy for (var ... = ... in ...)
+        if (!bound_names_are_lexical && decl.pattern->IsVariableProxy() &&
+            decl.initializer != nullptr) {
           DCHECK(!allow_harmony_for_in());
           ++use_counts_[v8::Isolate::kForInInitializer];
           const AstRawString* name =
@@ -3750,11 +3768,44 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
             descriptor.initialization_pos = kNoSourcePosition;
             decl.initializer = factory()->NewVariableProxy(temp);
 
+            bool is_for_var_of =
+                mode == ForEachStatement::ITERATE &&
+                parsing_result.descriptor.mode == VariableMode::VAR;
+
             PatternRewriter::DeclareAndInitializeVariables(
                 each_initialization_block, &descriptor, &decl,
-                IsLexicalVariableMode(descriptor.mode) ? &lexical_bindings
-                                                       : nullptr,
+                bound_names_are_lexical || is_for_var_of ? &bound_names
+                                                         : nullptr,
                 CHECK_OK);
+
+            // Annex B.3.5 prohibits the form
+            // `try {} catch(e) { for (var e of {}); }`
+            // So if we are parsing a statement like `for (var ... of ...)`
+            // we need to walk up the scope chain and look for catch scopes
+            // which have a simple binding, then compare their binding against
+            // all of the names declared in the init of the for-of we're
+            // parsing.
+            if (is_for_var_of) {
+              Scope* catch_scope = scope_;
+              while (catch_scope != nullptr &&
+                     !catch_scope->is_declaration_scope()) {
+                if (catch_scope->is_catch_scope()) {
+                  auto name = catch_scope->catch_variable_name();
+                  if (name !=
+                      ast_value_factory()
+                          ->dot_catch_string()) {  // i.e. is a simple binding
+                    if (bound_names.Contains(name)) {
+                      ParserTraits::ReportMessageAt(
+                          parsing_result.bindings_loc,
+                          MessageTemplate::kVarRedeclaration, name);
+                      *ok = false;
+                      return nullptr;
+                    }
+                  }
+                }
+                catch_scope = catch_scope->outer_scope();
+              }
+            }
           }
 
           body_block->statements()->Add(each_initialization_block, zone());
@@ -3769,18 +3820,17 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
         body_block->set_scope(body_scope);
 
         // Create a TDZ for any lexically-bound names.
-        if (IsLexicalVariableMode(parsing_result.descriptor.mode)) {
+        if (bound_names_are_lexical) {
           DCHECK_NULL(init_block);
 
           init_block =
               factory()->NewBlock(nullptr, 1, false, kNoSourcePosition);
 
-          for (int i = 0; i < lexical_bindings.length(); ++i) {
+          for (int i = 0; i < bound_names.length(); ++i) {
             // TODO(adamk): This needs to be some sort of special
             // INTERNAL variable that's invisible to the debugger
             // but visible to everything else.
-            VariableProxy* tdz_proxy =
-                NewUnresolved(lexical_bindings[i], LET);
+            VariableProxy* tdz_proxy = NewUnresolved(bound_names[i], LET);
             Declaration* tdz_decl = factory()->NewVariableDeclaration(
                 tdz_proxy, LET, scope_, kNoSourcePosition);
             Variable* tdz_var = Declare(
@@ -3801,11 +3851,10 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
           return final_loop;
         }
       } else {
+        bound_names_are_lexical =
+            IsLexicalVariableMode(parsing_result.descriptor.mode);
         init = parsing_result.BuildInitializationBlock(
-            IsLexicalVariableMode(parsing_result.descriptor.mode)
-                ? &lexical_bindings
-                : nullptr,
-            CHECK_OK);
+            bound_names_are_lexical ? &bound_names : nullptr, CHECK_OK);
       }
     } else {
       int lhs_beg_pos = peek_position();
@@ -3880,7 +3929,7 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
   // If there are let bindings, then condition and the next statement of the
   // for loop must be parsed in a new scope.
   Scope* inner_scope = scope_;
-  if (lexical_bindings.length() > 0) {
+  if (bound_names_are_lexical && bound_names.length() > 0) {
     inner_scope = NewScope(for_scope, BLOCK_SCOPE);
     inner_scope->set_start_position(scanner()->location().beg_pos);
   }
@@ -3902,11 +3951,11 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
   }
 
   Statement* result = NULL;
-  if (lexical_bindings.length() > 0) {
+  if (bound_names_are_lexical && bound_names.length() > 0) {
     BlockState block_state(&scope_, for_scope);
     result = DesugarLexicalBindingsInForStatement(
-        inner_scope, parsing_result.descriptor.mode, &lexical_bindings, loop,
-        init, cond, next, body, CHECK_OK);
+        inner_scope, parsing_result.descriptor.mode, &bound_names, loop, init,
+        cond, next, body, CHECK_OK);
     for_scope->set_end_position(scanner()->location().end_pos);
   } else {
     for_scope->set_end_position(scanner()->location().end_pos);

src/parsing/pattern-rewriter.cc

@@ -164,7 +164,7 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
   Scope* declaration_scope = IsLexicalVariableMode(descriptor_->mode)
                                  ? descriptor_->scope
                                  : descriptor_->scope->DeclarationScope();
-  if (declaration_scope->num_var_or_const() > kMaxNumFunctionLocals) {
+  if (declaration_scope->num_var() > kMaxNumFunctionLocals) {
     parser_->ReportMessage(MessageTemplate::kTooManyVariables);
     *ok_ = false;
     return;

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

@@ -170,8 +170,5 @@ for (var v = 0; v < varbinds.length; ++v) {
   TestNoConflict('(function (x) {' + varbinds[v] + '})();');
 }
 
-// Test conflicting catch/function bindings.
-TestNoConflict('try {} catch(x) {' + funbind + '}');
-
 // Test conflicting parameter/function bindings.
 TestNoConflict('(function (x) {' + funbind + '})();');

test/mjsunit/es6/block-conflicts.js

@@ -170,8 +170,5 @@ for (var v = 0; v < varbinds.length; ++v) {
   TestNoConflict('(function (x) {' + varbinds[v] + '})();');
 }
 
-// Test conflicting catch/function bindings.
-TestNoConflict('try {} catch(x) {' + funbind + '}');
-
 // Test conflicting parameter/function bindings.
 TestNoConflict('(function (x) {' + funbind + '})();');