Make a separate LexicalScope for catch parameters

Summary:
`catch` parameters are subject to similar scoping rules as actual
function parameters.

The previously functioning redeclaration logic has to be updated to
account for the fact that the catch parameters are in a separate scope
from the body of the catch. Instead of checking `topLevel`, we have to
check `let`s in the top-level of the catch body itself against the Catch
parameters. To do this, add a new `Decl::Kind` for ES6-style `Catch`,
which lets us easily detect conflicts against the catch parameters.

However, there's one complication due to scoped function promotion. We
can promote scoped functions past ES5 catches, which wasn't being
handled properly before because the ES5 catch binding was reusing the
promoted `Var`'s `Decl`. What we actually want is to make a new `Decl`
for the `ES5Catch`, and then _avoid_ reusing the catch's `Decl` when
rebinding the scoped binding for the promoted function, instead using
the `Var`. To facilitate this, we have to also store the promoted `Decl`
for every promoted function and use that instead. In practice, this
shouldn't increase memory usage much given how rarely we have to promote
functions.

Reviewed By: tmikov

Differential Revision: D59021855

fbshipit-source-id: 33e59f35679f121da16f6bb527ffcdfe7197f686

include/hermes/Sema/SemContext.h

@@ -59,6 +59,9 @@ class Decl {
     Const,
     Class,
     Import,
+    /// A catch variable bound with let-like rules (non-ES5).
+    /// ES14.0 B.3.4 handles ES5-style catch bindings differently.
+    Catch,
     /// Function declaration visible only in its lexical scope.
     ScopedFunction,
     /// A single catch variable declared like this "catch (e)", see
@@ -105,7 +108,7 @@ class Decl {
     return kind >= Kind::Var;
   }
   static bool isKindVarLikeOrScopedFunction(Kind kind) {
-    return kind >= Kind::ScopedFunction;
+    return isKindVarLike(kind) || kind == Kind::ScopedFunction;
   }
   /// \return true if this kind of declaration is lexically scoped (and cannot
   /// be re-declared).

lib/IRGen/ESTreeIRGen-func.cpp

@@ -762,6 +762,7 @@ void ESTreeIRGen::emitScopeDeclarations(sema::LexicalScope *scope) {
       case sema::Decl::Kind::Let:
       case sema::Decl::Kind::Const:
       case sema::Decl::Kind::Class:
+      case sema::Decl::Kind::Catch:
         assert(
             ((curFunction()->debugAllowRecompileCounter != 0) ||
              (decl->customData == nullptr)) &&

lib/Sema/DeclCollector.cpp

@@ -172,9 +172,9 @@ void DeclCollector::visit(ESTree::CatchClauseNode *node) {
     addToCur(node);
     visitESTreeNode(*this, node->_param, node);
   }
-  // Declarations in the catch body are associated with CatchClauseNode,
-  // not BlockStatementNode.
-  visitESTreeChildren(*this, node->_body);
+  // CatchClauseNode is supposed to have separate scopes for the body and the
+  // parameters.
+  visitESTreeNode(*this, node->_body, node);
   closeScope(node);
 }
 void DeclCollector::visit(ESTree::SwitchStatementNode *node) {

lib/Sema/FlowChecker.cpp

@@ -879,9 +879,7 @@ void FlowChecker::visit(ESTree::CatchClauseNode *node) {
   ScopeRAII scope(*this);
   if (!resolveScopeTypesAndAnnotate(node, node->getScope()))
     return;
-  visitESTreeNode(*this, node->_param, node);
-  // Process body's declarations, skip visiting it, visit its children.
-  visitESTreeChildren(*this, node->_body);
+  visitESTreeChildren(*this, node);
 }
 
 void FlowChecker::visitFunctionLike(

lib/Sema/ScopedFunctionPromoter.cpp

@@ -63,6 +63,9 @@ class ScopedFunctionPromoter {
   void visit(WithStatementNode *node) {
     visitScope(node);
   }
+  void visit(CatchClauseNode *node) {
+    visitScope(node);
+  }
 
   /// Needed by RecursiveVisitorDispatch. Optionally can protect against too
   /// deep nesting.
@@ -196,8 +199,10 @@ void ScopedFunctionPromoter::processDeclarations(Node *scope) {
     idents.clear();
     Decl::Kind declKind = extractDeclaredIdents(node, idents);
 
-    // We are only interested in let-like declarations.
-    if (!Decl::isKindLetLike(declKind))
+    // We are only interested in let-like declarations, but not ES5Catch.
+    // ES5Catch doesn't conflict with Var declarations.
+    // See ES14.0 B.3.4.
+    if (!Decl::isKindLetLike(declKind) || declKind == Decl::Kind::ES5Catch)
       continue;
 
     // Remember only idents matching the set.
@@ -257,6 +262,16 @@ Decl::Kind ScopedFunctionPromoter::extractDeclaredIdents(
     return Decl::Kind::Class;
   }
 
+  if (auto *catchClause = llvh::dyn_cast<CatchClauseNode>(node)) {
+    resolver_.extractDeclaredIdentsFromID(catchClause->_param, idents);
+    if (auto *id =
+            llvh::dyn_cast_or_null<IdentifierNode>(catchClause->_param)) {
+      return Decl::Kind::ES5Catch;
+    } else {
+      return Decl::Kind::Catch;
+    }
+  }
+
   {
     auto *id = llvh::cast<ImportDeclarationNode>(node);
     for (Node &spec : id->_specifiers) {

lib/Sema/SemContext.cpp

@@ -482,6 +482,7 @@ void SemContextDumper::printDecl(llvh::raw_ostream &os, const Decl *d) {
     CASE(Let)
     CASE(Const)
     CASE(Class)
+    CASE(Catch)
     CASE(Import)
     CASE(ES5Catch)
     CASE(FunctionExprName)

lib/Sema/SemanticResolver.cpp

@@ -440,8 +440,7 @@ void SemanticResolver::visit(
   // Some nodes with attached BlockStatement have already dealt with the scope.
   if (llvh::isa<FunctionDeclarationNode>(parent) ||
       llvh::isa<FunctionExpressionNode>(parent) ||
-      llvh::isa<ArrowFunctionExpressionNode>(parent) ||
-      llvh::isa<CatchClauseNode>(parent)) {
+      llvh::isa<ArrowFunctionExpressionNode>(parent)) {
     return visitESTreeChildren(*this, node);
   }
 
@@ -747,16 +746,10 @@ void SemanticResolver::visit(ESTree::TryStatementNode *tryStatement) {
 
 void SemanticResolver::visit(ESTree::CatchClauseNode *node) {
   ScopeRAII scope{*this, node};
-  // Process body's declarations, skip visiting the block, visit its children.
+  // Process catch clause's declarations (not the ones in the body).
   processCollectedDeclarations(node);
-  // Visit the catch param, in case there is destructuring.
-  visitESTreeNode(*this, node->_param, node);
-  // The scope declarations are associated with the CatchClauseNode in the
-  // DeclCollector.
-  visitESTreeChildren(*this, node->_body);
-  assert(
-      !functionContext()->decls->getScopeDeclsForNode(node->_body) &&
-      "CatchClause body block shouldn't have any decls associated");
+  // Visit the body, which will make a new scope.
+  visitESTreeChildren(*this, node);
 }
 
 void SemanticResolver::visit(RegExpLiteralNode *regexp) {
@@ -1563,7 +1556,8 @@ void SemanticResolver::processPromotedFuncDecls(
   for (FunctionDeclarationNode *funcDecl : promotedFuncDecls) {
     auto *ident = llvh::cast<IdentifierNode>(funcDecl->_id);
     validateAndDeclareIdentifier(kind, ident);
-    functionContext()->promotedFuncDecls.insert(ident->_name);
+    functionContext()->promotedFuncDecls.try_emplace(
+        ident->_name, semCtx_.getDeclarationDecl(ident));
   }
 }
 
@@ -1635,7 +1629,7 @@ Decl::Kind SemanticResolver::extractIdentsFromDecl(
       // https://www.ecma-international.org/ecma-262/10.0/index.html#sec-variablestatements-in-catch-blocks
       return Decl::Kind::ES5Catch;
     } else {
-      return Decl::Kind::Let;
+      return Decl::Kind::Catch;
     }
   }
 
@@ -1753,6 +1747,7 @@ void SemanticResolver::validateAndDeclareIdentifier(
     const bool sameScope = prevName.decl->scope == curScope_;
     const bool topLevel =
         curScope_->parentFunction->getFunctionBodyScope() == curScope_;
+    const bool prevInPrevScope = prevName.decl->scope == curScope_->parentScope;
 
     // Check whether the redeclaration is invalid.
     // Note that since "var" declarations have been hoisted to the function
@@ -1771,6 +1766,19 @@ void SemanticResolver::validateAndDeclareIdentifier(
     //   FormalParameters also occurs in the LexicallyDeclaredNames of
     //   FunctionBody.
     //
+    // Catch (non-ES5) clause variables must not conflict with the lexically
+    // scoped names or var-declared names in their block:
+    // * It is a Syntax Error if BoundNames of CatchParameter contains any
+    //   duplicate elements.
+    // * It is a Syntax Error if any element of the BoundNames of CatchParameter
+    //   also occurs in the LexicallyDeclaredNames of Block.
+    //   NOTE: It's possible that a function in the body of the catch has been
+    //   promoted to a Var at function scope, so it has to be accounted for.
+    // * It is a Syntax Error if any element of the BoundNames of CatchParameter
+    //   also occurs in the VarDeclaredNames of Block unless CatchParameter is
+    //   CatchParameter : BindingIdentifier.
+    //   visit(VariableDeclarationNode *) will handle this final case.
+    //
     // Case by case explanations for our representation:
     //
     // ES5Catch, var
@@ -1813,7 +1821,13 @@ void SemanticResolver::validateAndDeclareIdentifier(
            prevKind == Decl::Kind::ScopedFunction &&
            kind == Decl::Kind::ScopedFunction)) ||
         (prevKind == Decl::Kind::Parameter && Decl::isKindLetLike(kind) &&
-         topLevel)) {
+         topLevel) ||
+        // LexicallyDeclaredNames of CatchBlock are only in the block scope
+        // itself, so check prevInPrevScope (it's like checking topLevel for
+        // parameters).
+        // This is an error regardless of if it's an ES5 or ES6 catch.
+        ((prevKind == Decl::Kind::Catch || prevKind == Decl::Kind::ES5Catch) &&
+         Decl::isKindLetLike(kind) && prevInPrevScope)) {
       sm_.error(
           ident->getSourceRange(),
           llvh::Twine("Identifier '") + ident->_name->str() +
@@ -1832,15 +1846,32 @@ void SemanticResolver::validateAndDeclareIdentifier(
     // Var, ScopedFunc -> if non-param non-strict or same scope, then use prev,
     //                    else declare new
     else if (
-        Decl::isKindVarLike(prevKind) &&
-        Decl::isKindVarLikeOrScopedFunction(kind)) {
+        Decl::isKindVarLike(prevKind) && kind == Decl::Kind::ScopedFunction) {
+      decl = nullptr;
       if (sameScope) {
         decl = prevName.decl;
-      } else if (functionContext()->promotedFuncDecls.count(ident->_name)) {
+      } else {
+        auto it = functionContext()->promotedFuncDecls.find(ident->_name);
+        if (it != functionContext()->promotedFuncDecls.end()) {
+          // We've already promoted this function, so add a new binding
+          // and point it to the original Decl.
+          reuseDeclForNewBinding = true;
+          decl = it->second;
+        }
+      }
+    }
+    // ES5Catch, ScopedFunc ->
+    //   if promoted, use promoted function, else declare new
+    //   ES5Catch doesn't prevent promotion, so we have to check it specially.
+    else if (
+        prevKind == Decl::Kind::ES5Catch &&
+        kind == Decl::Kind::ScopedFunction) {
+      auto it = functionContext()->promotedFuncDecls.find(ident->_name);
+      if (it != functionContext()->promotedFuncDecls.end()) {
         // We've already promoted this function, so add a new binding
         // and point it to the original Decl.
         reuseDeclForNewBinding = true;
-        decl = prevName.decl;
+        decl = it->second;
       } else {
         decl = nullptr;
       }

lib/Sema/SemanticResolver.h

@@ -492,9 +492,10 @@ class FunctionContext {
   /// All declarations in the function.
   std::unique_ptr<DeclCollector> decls;
 
-  /// The set of names that have been promoted to function scope by
-  /// promoteScopedFunctionDecls in this function.
-  llvh::DenseSet<UniqueString *> promotedFuncDecls{};
+  /// The map of names that have been promoted to function scope by
+  /// promoteScopedFunctionDecls in this function, mapped to their Var
+  /// declaration in function scope.
+  llvh::DenseMap<UniqueString *, Decl *> promotedFuncDecls{};
 
   /// The depth of the function's scope in the binding table.
   /// Populated when ScopeRAII is created within the function.

test/Sema/catch-block-destr.js

@@ -16,9 +16,10 @@ try {} catch ([a, b]) { let x; }
 // CHECK-NEXT:    Scope %s.1
 // CHECK-NEXT:        Scope %s.2
 // CHECK-NEXT:        Scope %s.3
-// CHECK-NEXT:            Decl %d.1 'a' Let
-// CHECK-NEXT:            Decl %d.2 'b' Let
-// CHECK-NEXT:            Decl %d.3 'x' Let
+// CHECK-NEXT:            Decl %d.1 'a' Catch
+// CHECK-NEXT:            Decl %d.2 'b' Catch
+// CHECK-NEXT:            Scope %s.4
+// CHECK-NEXT:                Decl %d.3 'x' Let
 
 // CHECK:Program Scope %s.1
 // CHECK-NEXT:    TryStatement
@@ -27,7 +28,7 @@ try {} catch ([a, b]) { let x; }
 // CHECK-NEXT:            ArrayPattern
 // CHECK-NEXT:                Id 'a' [D:E:%d.1 'a']
 // CHECK-NEXT:                Id 'b' [D:E:%d.2 'b']
-// CHECK-NEXT:            BlockStatement
+// CHECK-NEXT:            BlockStatement Scope %s.4
 // CHECK-NEXT:                VariableDeclaration
 // CHECK-NEXT:                    VariableDeclarator
 // CHECK-NEXT:                        Id 'x' [D:E:%d.3 'x']

test/Sema/catch-block.js

@@ -17,14 +17,15 @@ try {} catch (e) { let x; }
 // CHECK-NEXT:        Scope %s.2
 // CHECK-NEXT:        Scope %s.3
 // CHECK-NEXT:            Decl %d.1 'e' ES5Catch
-// CHECK-NEXT:            Decl %d.2 'x' Let
+// CHECK-NEXT:            Scope %s.4
+// CHECK-NEXT:                Decl %d.2 'x' Let
 
 // CHECK:Program Scope %s.1
 // CHECK-NEXT:    TryStatement
 // CHECK-NEXT:        BlockStatement Scope %s.2
 // CHECK-NEXT:        CatchClause Scope %s.3
 // CHECK-NEXT:            Id 'e' [D:E:%d.1 'e']
-// CHECK-NEXT:            BlockStatement
+// CHECK-NEXT:            BlockStatement Scope %s.4
 // CHECK-NEXT:                VariableDeclaration
 // CHECK-NEXT:                    VariableDeclarator
 // CHECK-NEXT:                        Id 'x' [D:E:%d.2 'x']

test/Sema/flow/catch-block.js

@@ -25,7 +25,8 @@ try {} catch (e) { let x: number = 1; }
 // CHECK-NEXT:            Scope %s.3
 // CHECK-NEXT:            Scope %s.4
 // CHECK-NEXT:                Decl %d.3 'e' ES5Catch : any
-// CHECK-NEXT:                Decl %d.4 'x' Let : number
+// CHECK-NEXT:                Scope %s.5
+// CHECK-NEXT:                    Decl %d.4 'x' Let : number
 
 // CHECK:Program Scope %s.1
 // CHECK-NEXT:    ExpressionStatement
@@ -37,7 +38,7 @@ try {} catch (e) { let x: number = 1; }
 // CHECK-NEXT:                        BlockStatement Scope %s.3
 // CHECK-NEXT:                        CatchClause Scope %s.4
 // CHECK-NEXT:                            Id 'e' [D:E:%d.3 'e']
-// CHECK-NEXT:                            BlockStatement
+// CHECK-NEXT:                            BlockStatement Scope %s.5
 // CHECK-NEXT:                                VariableDeclaration
 // CHECK-NEXT:                                    VariableDeclarator
 // CHECK-NEXT:                                        NumericLiteral : number

test/Sema/flow/implicit-return.js

@@ -128,24 +128,26 @@ function main(x) {
 // CHECK-NEXT:                    Decl %d.18 'arguments' Var Arguments
 // CHECK-NEXT:                    Scope %s.13
 // CHECK-NEXT:                    Scope %s.14
+// CHECK-NEXT:                        Scope %s.15
 // CHECK-NEXT:            Func strict
-// CHECK-NEXT:                Scope %s.15
+// CHECK-NEXT:                Scope %s.16
 // CHECK-NEXT:                    Decl %d.19 'arguments' Var Arguments
-// CHECK-NEXT:                    Scope %s.16
 // CHECK-NEXT:                    Scope %s.17
+// CHECK-NEXT:                    Scope %s.18
 // CHECK-NEXT:            Func strict
-// CHECK-NEXT:                Scope %s.18
+// CHECK-NEXT:                Scope %s.19
 // CHECK-NEXT:                    Decl %d.20 'arguments' Var Arguments
-// CHECK-NEXT:                    Scope %s.19
-// CHECK-NEXT:                        Scope %s.20
+// CHECK-NEXT:                    Scope %s.20
 // CHECK-NEXT:                        Scope %s.21
-// CHECK-NEXT:                    Scope %s.22
+// CHECK-NEXT:                        Scope %s.22
+// CHECK-NEXT:                    Scope %s.23
+// CHECK-NEXT:                        Scope %s.24
 // CHECK-NEXT:            Func strict
-// CHECK-NEXT:                Scope %s.23
+// CHECK-NEXT:                Scope %s.25
 // CHECK-NEXT:                    Decl %d.21 'arguments' Var Arguments
-// CHECK-NEXT:                    Scope %s.24
-// CHECK-NEXT:                        Scope %s.25
-// CHECK-NEXT:                        Scope %s.26
+// CHECK-NEXT:                    Scope %s.26
+// CHECK-NEXT:                        Scope %s.27
+// CHECK-NEXT:                        Scope %s.28
 
 // CHECK:Program Scope %s.1
 // CHECK-NEXT:    ExpressionStatement
@@ -201,42 +203,42 @@ function main(x) {
 // CHECK-NEXT:                                            ReturnStatement
 // CHECK-NEXT:                                                NumericLiteral : number
 // CHECK-NEXT:                                        CatchClause Scope %s.14
-// CHECK-NEXT:                                            BlockStatement
+// CHECK-NEXT:                                            BlockStatement Scope %s.15
 // CHECK-NEXT:                                                ReturnStatement
 // CHECK-NEXT:                                                    NumericLiteral : number
 // CHECK-NEXT:                            FunctionDeclaration : %function.4
 // CHECK-NEXT:                                Id 'f6' [D:E:%d.10 'f6']
 // CHECK-NEXT:                                BlockStatement
 // CHECK-NEXT:                                    TryStatement
-// CHECK-NEXT:                                        BlockStatement Scope %s.16
+// CHECK-NEXT:                                        BlockStatement Scope %s.17
 // CHECK-NEXT:                                            ReturnStatement
 // CHECK-NEXT:                                                NumericLiteral : number
-// CHECK-NEXT:                                        BlockStatement Scope %s.17
+// CHECK-NEXT:                                        BlockStatement Scope %s.18
 // CHECK-NEXT:                            FunctionDeclaration : %function.4
 // CHECK-NEXT:                                Id 'f7' [D:E:%d.11 'f7']
 // CHECK-NEXT:                                BlockStatement
 // CHECK-NEXT:                                    TryStatement
-// CHECK-NEXT:                                        BlockStatement Scope %s.19
+// CHECK-NEXT:                                        BlockStatement Scope %s.20
 // CHECK-NEXT:                                            TryStatement
-// CHECK-NEXT:                                                BlockStatement Scope %s.20
+// CHECK-NEXT:                                                BlockStatement Scope %s.21
 // CHECK-NEXT:                                                    ReturnStatement
 // CHECK-NEXT:                                                        NumericLiteral : number
-// CHECK-NEXT:                                                BlockStatement Scope %s.21
-// CHECK-NEXT:                                        CatchClause Scope %s.22
-// CHECK-NEXT:                                            BlockStatement
+// CHECK-NEXT:                                                BlockStatement Scope %s.22
+// CHECK-NEXT:                                        CatchClause Scope %s.23
+// CHECK-NEXT:                                            BlockStatement Scope %s.24
 // CHECK-NEXT:                                                ReturnStatement
 // CHECK-NEXT:                                                    NumericLiteral : number
 // CHECK-NEXT:                            FunctionDeclaration : %function.4
 // CHECK-NEXT:                                Id 'f8' [D:E:%d.12 'f8']
 // CHECK-NEXT:                                BlockStatement
 // CHECK-NEXT:                                    LabeledStatement
 // CHECK-NEXT:                                        Id 'label'
-// CHECK-NEXT:                                        BlockStatement Scope %s.24
+// CHECK-NEXT:                                        BlockStatement Scope %s.26
 // CHECK-NEXT:                                            TryStatement
-// CHECK-NEXT:                                                BlockStatement Scope %s.25
+// CHECK-NEXT:                                                BlockStatement Scope %s.27
 // CHECK-NEXT:                                                    BreakStatement
 // CHECK-NEXT:                                                        Id 'label'
-// CHECK-NEXT:                                                BlockStatement Scope %s.26
+// CHECK-NEXT:                                                BlockStatement Scope %s.28
 // CHECK-NEXT:                                                    ReturnStatement
 // CHECK-NEXT:                                                        NumericLiteral : number
 // CHECK-NEXT:            ObjectExpression : %object.5