Do all parsing for try/catch destructuring inside the appropriate scopes

ajklein · Commit bot · commit 7166503f6e90 · 2016-06-30T06:52:13.000Z
Previously, any expressions inside destructuring patterns in a catch would be parsed in the surrounding scope, instead of in the catch's scope. This change fixes that by entering not only the catch scope, but also the block scope inside it. R=neis@chromium.org BUG=v8:5106, v8:5112 Review-Url: https://codereview.chromium.org/2110193002 Cr-Commit-Position: refs/heads/master@{#37415}
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
@@ -2999,40 +2999,40 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
     catch_scope = NewScope(scope_, CATCH_SCOPE);
     catch_scope->set_start_position(scanner()->location().beg_pos);
 
-    ExpressionClassifier pattern_classifier(this);
-    Expression* pattern = ParsePrimaryExpression(&pattern_classifier, CHECK_OK);
-    ValidateBindingPattern(&pattern_classifier, CHECK_OK);
-
-    const AstRawString* name = ast_value_factory()->dot_catch_string();
-    bool is_simple = pattern->IsVariableProxy();
-    if (is_simple) {
-      auto proxy = pattern->AsVariableProxy();
-      scope_->RemoveUnresolved(proxy);
-      name = proxy->raw_name();
-    }
-
-    catch_variable = catch_scope->DeclareLocal(name, VAR, kCreatedInitialized,
-                                               Variable::NORMAL);
-
-    Expect(Token::RPAREN, CHECK_OK);
-
     {
       CollectExpressionsInTailPositionToListScope
           collect_tail_call_expressions_scope(
               function_state_, &tail_call_expressions_in_catch_block);
       BlockState block_state(&scope_, catch_scope);
 
-      // TODO(adamk): Make a version of ParseBlock that takes a scope and
-      // a block.
       catch_block =
           factory()->NewBlock(nullptr, 16, false, RelocInfo::kNoPosition);
-      Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
 
+      // Create a block scope to hold any lexical declarations created
+      // as part of destructuring the catch parameter.
+      Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
       block_scope->set_start_position(scanner()->location().beg_pos);
       {
         BlockState block_state(&scope_, block_scope);
         Target target(&this->target_stack_, catch_block);
 
+        ExpressionClassifier pattern_classifier(this);
+        Expression* pattern =
+            ParsePrimaryExpression(&pattern_classifier, CHECK_OK);
+        ValidateBindingPattern(&pattern_classifier, CHECK_OK);
+
+        const AstRawString* name = ast_value_factory()->dot_catch_string();
+        bool is_simple = pattern->IsVariableProxy();
+        if (is_simple) {
+          auto proxy = pattern->AsVariableProxy();
+          scope_->RemoveUnresolved(proxy);
+          name = proxy->raw_name();
+        }
+        catch_variable = catch_scope->DeclareLocal(
+            name, VAR, kCreatedInitialized, Variable::NORMAL);
+
+        Expect(Token::RPAREN, CHECK_OK);
+
         if (!is_simple) {
           DeclarationDescriptor descriptor;
           descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
@@ -3054,6 +3054,8 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
           catch_block->statements()->Add(init_block, 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);
diff --git a/test/mjsunit/regress/regress-5106.js b/test/mjsunit/regress/regress-5106.js
@@ -0,0 +1,29 @@
+// Copyright 2016 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* g1() {
+  try {
+    throw {};
+  } catch ({a = class extends (yield) {}}) {
+  }
+}
+g1().next();  // crashes without fix
+
+function* g2() {
+  let x = function(){};
+  try {
+    throw {};
+  } catch ({b = class extends x {}}) {
+  }
+}
+g2().next();  // crashes without fix
+
+function* g3() {
+  let x = 42;
+  try {
+    throw {};
+  } catch ({c = (function() { return x })()}) {
+  }
+}
+g3().next();  // throws a ReferenceError without fix
diff --git a/test/test262/test262.status b/test/test262/test262.status
@@ -322,7 +322,6 @@
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=5112
   'language/statements/try/scope-catch-block-lex-open': [FAIL],
-  'language/statements/try/scope-catch-param-lex-open': [FAIL],
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=5012
   'intl402/Intl/getCanonicalLocales/*': [FAIL],
