Clean up scope-chain recursion handling in AbstractScope.

In particular, remove the boolean "recurse" parameter in favor of separate methods isDeclared() and isDeclaredInOwnScope().  There are also two less-common methods, isDeclaredWithSameCfgRoot() and isDeclared(String, Predicate<Node>), which are used in a few places.  All callsites have been migrated to the appropriate variant.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=191140890

src/com/google/javascript/jscomp/AbstractScope.java

@@ -33,21 +33,27 @@
  * points back to its parent scope. A Scope contains information about variables defined in that
  * scope.
  *
- * <p>ES6 introduces new scoping rules, which adds some complexity to this class. In particular,
- * scopes fall into four mutually exclusive categories based on their root node: block, function,
- * module, or global. Function, module, and global scopes are collectively referred to as "non-block
- * scopes". We also define a scope as a "hoist scope" if it is a non-block scope *or* it is the
- * outermost block scope within a function (i.e. a "function block scope"). Hoist scopes are
- * important because "var" declarations are hoisted to the closest hoist scope, as opposed to ES6
- * "let" and "const" which are not hoisted, but instead added directly to whatever scope they're
- * declared in.
+ * <p>ES 2015 introduces new scoping rules, which adds some complexity to this class. In particular,
+ * scopes fall into two mutually exclusive categories: <i>block</i> and <i>container</i>.  Block
+ * scopes are all scopes whose roots are blocks, as well as any control structures whose optional
+ * blocks are omitted.  These scopes did not exist at all prior to ES 2015.  Container scopes
+ * comprise function scopes, global scopes, and module scopes, and (aside from modules, which
+ * didn't exist in ES5) corresponds to the ES5 scope rules.  This corresponds roughly to one
+ * container scope per CFG root (but not exactly, due to SCRIPT-level CFGs).
  *
- * <p>Finally, a caution about function scopes and function block scopes: the language does not
- * permit any shadowing to occur between them (with the exception of bleeding function names), so in
- * many situations these scopes are treated as a single scope. Under block scoping, only function
- * parameters (and optionally, bleeding function names) are declared in the function scope. It is
- * kept as a separate scope so that default parameter initializers may be evaluated in a separate
- * scope from the function body.
+ * <p>All container scopes, plus the outermost block scope within a function (i.e. the <i>function
+ * block scope</i>) are considered <i>hoist scopes</i>.  Hoist scopes are relevant because "var"
+ * declarations are hoisted to the closest hoist scope, as opposed to "let" and "const" which always
+ * apply to the specific scope in which they occur.
+ *
+ * <p>Note that every function actually has two distinct hoist scopes: a container scope on the
+ * FUNCTION node, and a block-scope on the top-level BLOCK in the function (the "function block").
+ * Local variables are declared on the function block, while parameters and optionally the function
+ * name (if it bleeds, i.e. from a named function expression) are declared on the container scope.
+ * This is required so that default parameter initializers can refer to names from outside the
+ * function that could possibly be shadowed in the function block.  But these scopes are not fully
+ * independent of one another, since the language does not allow a top-level local variable to
+ * shadow a parameter name - so in some situations these scopes must be treated as a single scope.
  *
  * @see NodeTraversal
  */
@@ -145,6 +151,9 @@ final void clearVarsInternal() {
 
   @Override
   public final V getSlot(String name) {
+    // TODO(sdh): This behavior is inconsistent with getOwnSlot, hasSlot, and hasOwnSlot
+    // when it comes to implicit vars.  The other three methods all exclude implicits,
+    // but this one returns them.  It would be good to clean this up one way or the other.
     return getVar(name);
   }
 
@@ -202,35 +211,21 @@ private final V getImplicitVar(ImplicitVar var, boolean allowDeclaredVars) {
     return null;
   }
 
-  /**
-   * Use only when in a function block scope and want to tell if a name is either at the top of the
-   * function block scope or the function parameter scope.  This obviously only applies to ES6 block
-   * scopes.
-   */
-  public final boolean isDeclaredInFunctionBlockOrParameter(String name) {
-    // In ES6, we create a separate "function parameter scope" above the function block scope to
-    // handle default parameters. Since nothing in the function block scope is allowed to shadow
-    // the variables in the function scope, we treat the two scopes as one in this method.
-    checkState(isFunctionBlockScope());
-    return isDeclared(name, false) || (getParent().isDeclared(name, false));
+  /** Returns true if a variable is declared in this scope, with no recursion. */
+  public final boolean hasOwnSlot(String name) {
+    return vars.containsKey(name);
   }
 
-  /**
-   * Returns true if a variable is declared.
-   */
-  public final boolean isDeclared(String name, boolean recurse) {
+  /** Returns true if a variable is declared in this or any parent scope. */
+  public final boolean hasSlot(String name) {
     S scope = thisScope();
-    while (true) {
-      if (scope.getOwnSlot(name) != null) {
+    while (scope != null) {
+      if (scope.hasOwnSlot(name)) {
         return true;
       }
-
-      if (scope.getParent() != null && recurse) {
-        scope = scope.getParent();
-        continue;
-      }
-      return false;
+      scope = scope.getParent();
     }
+    return false;
   }
 
   /**
@@ -333,11 +328,11 @@ final boolean isHoistScope() {
   /**
    * If a var were declared in this scope, return the scope it would be hoisted to.
    *
-   * For function scopes, we return back the scope itself, since even though there is no way
-   * to declare a var inside function parameters, it would make even less sense to say that
-   * such declarations would be "hoisted" somewhere else.
+   * <p>For function scopes, we return back the scope itself, since even though there is no way to
+   * declare a var inside function parameters, it would make even less sense to say that such
+   * declarations would be "hoisted" somewhere else.
    */
-  public final S getClosestHoistScope() {
+  final S getClosestHoistScope() {
     S current = thisScope();
     while (current != null) {
       if (current.isHoistScope()) {
@@ -349,10 +344,10 @@ public final S getClosestHoistScope() {
   }
 
   /**
-   * Returns the closest non-block scope. This is equivalent to what the current scope would have
-   * been for non-block-scope creators, and is thus useful for migrating code to use block scopes.
+   * Returns the closest container scope. This is equivalent to what the current scope would have
+   * been for non-ES6 scope creators, and is thus useful for migrating code to use block scopes.
    */
-  public final S getClosestNonBlockScope() {
+  public final S getClosestContainerScope() {
     S scope = getClosestHoistScope();
     if (scope.isBlockScope()) {
       scope = scope.getParent();
@@ -385,6 +380,15 @@ void checkRootScope() {
         NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode);
   }
 
+  /**
+   * Whether {@code this} and the {@code other} scope have the same container scope (i.e. are in
+   * the same function, or else both in the global hoist scope). This is equivalent to asking
+   * whether the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world.
+   */
+  boolean hasSameContainerScope(S other) {
+    return getClosestContainerScope() == other.getClosestContainerScope();
+  }
+
   /**
    * The three implicit var types, which are defined implicitly (at least) in
    * every vanilla function scope without actually being declared.

src/com/google/javascript/jscomp/CoalesceVariableNames.java

@@ -227,7 +227,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
 
       pseudoName = Joiner.on("_").join(allMergedNames);
 
-      while (t.getScope().isDeclared(pseudoName, true)) {
+      while (t.getScope().hasSlot(pseudoName)) {
         pseudoName += "$";
       }
 

src/com/google/javascript/jscomp/Es6RenameReferences.java

@@ -64,7 +64,7 @@ private void renameReference(NodeTraversal t, Node n) {
         n.setString(newName);
         t.reportCodeChange();
         return;
-      } else if (current.isDeclared(oldName, false)) {
+      } else if (current.hasOwnSlot(oldName)) {
         return;
       } else {
         current = current.getParent();

src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java

@@ -96,10 +96,10 @@ && inLoop(n)) {
     Scope hoistScope = scope.getClosestHoistScope();
     if (scope != hoistScope) {
       String newName = oldName;
-      if (hoistScope.isDeclared(oldName, true) || undeclaredNames.contains(oldName)) {
+      if (hoistScope.hasSlot(oldName) || undeclaredNames.contains(oldName)) {
         do {
           newName = oldName + "$" + compiler.getUniqueNameIdSupplier().get();
-        } while (hoistScope.isDeclared(newName, true));
+        } while (hoistScope.hasSlot(newName));
         nameNode.setString(newName);
         compiler.reportChangeToEnclosingScope(nameNode);
         Node scopeRoot = scope.getRootNode();
@@ -218,7 +218,7 @@ private void rewriteDeclsToVars() {
   private class CollectUndeclaredNames extends AbstractPostOrderCallback {
     @Override
     public void visit(NodeTraversal t, Node n, Node parent) {
-      if (n.isName() && !t.getScope().isDeclared(n.getString(), true)) {
+      if (n.isName() && !t.getScope().hasSlot(n.getString())) {
         undeclaredNames.add(n.getString());
       }
     }

src/com/google/javascript/jscomp/Es6TypedToEs6Converter.java

@@ -509,7 +509,7 @@ private void maybeVisitColonType(NodeTraversal t, Node n, Node jsDocNode) {
 
   private void visitTypeAlias(NodeTraversal t, Node n, Node parent) {
     String alias = n.getString();
-    if (t.getScope().isDeclared(alias, true)) {
+    if (t.getScope().hasSlot(alias)) {
       compiler.report(
           JSError.make(n, TYPE_ALIAS_ALREADY_DECLARED, alias));
     }

src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java

@@ -633,7 +633,7 @@ public boolean apply(Node input) {
             public boolean apply(Node input) {
               if (input.isName()) {
                 String name = input.getString();
-                if (!name.isEmpty() && !usageScope.isDeclared(name, true)) {
+                if (!name.isEmpty() && !usageScope.hasSlot(name)) {
                   return true; // unsafe to inline.
                 }
               }

src/com/google/javascript/jscomp/LiveVariablesAnalysis.java

@@ -368,11 +368,11 @@ private void addToSetIfLocal(Node node, BitSet set) {
     // ES6 separates the scope but if the variable is declared in the param it should be local
     // to the function body.
     if (localScope.isFunctionBlockScope()) {
-      local = localScope.isDeclaredInFunctionBlockOrParameter(name);
+      local = isDeclaredInFunctionBlockOrParameter(localScope, name);
     } else if (localScope == jsScope && jsScopeChild != null) {
-      local = jsScopeChild.isDeclaredInFunctionBlockOrParameter(name);
+      local = isDeclaredInFunctionBlockOrParameter(jsScopeChild, name);
     } else {
-      local = localScope.isDeclared(name, false);
+      local = localScope.hasOwnSlot(name);
     }
 
     if (!local) {
@@ -384,6 +384,14 @@ private void addToSetIfLocal(Node node, BitSet set) {
     }
   }
 
+  private static boolean isDeclaredInFunctionBlockOrParameter(Scope scope, String name) {
+    // In ES6, we create a separate container scope above the function block scope to handle
+    // default parameters. Since nothing in the function block scope is allowed to shadow
+    // the variables in the function scope, we treat the two scopes as one in this method.
+    checkState(scope.isFunctionBlockScope());
+    return scope.hasOwnSlot(name) || scope.getParent().hasOwnSlot(name);
+  }
+
   /**
    * Give up computing liveness of formal parameter by putting all the parameter names in the
    * escaped set.
@@ -402,12 +410,12 @@ void markAllParametersEscaped() {
   private boolean isArgumentsName(Node n) {
     boolean childDeclared;
     if (jsScopeChild != null) {
-      childDeclared = jsScopeChild.isDeclared(ARGUMENT_ARRAY_ALIAS, false);
+      childDeclared = jsScopeChild.hasOwnSlot(ARGUMENT_ARRAY_ALIAS);
     } else {
       childDeclared = true;
     }
     return n.isName()
         && n.getString().equals(ARGUMENT_ARRAY_ALIAS)
-        && (!jsScope.isDeclared(ARGUMENT_ARRAY_ALIAS, false) || !childDeclared);
+        && (!jsScope.hasOwnSlot(ARGUMENT_ARRAY_ALIAS) || !childDeclared);
   }
 }

src/com/google/javascript/jscomp/NodeTraversal.java

@@ -967,7 +967,7 @@ public Node getClosestHoistScopeRoot() {
     return scopes.peek().getClosestHoistScope().getRootNode();
   }
 
-  public Node getClosestNonBlockScopeRoot() {
+  public Node getClosestContainerScopeRoot() {
     int roots = scopeRoots.size();
     for (int i = roots; i > 0; i--) {
       Node rootNode = scopeRoots.get(i - 1);
@@ -976,7 +976,7 @@ public Node getClosestNonBlockScopeRoot() {
       }
     }
 
-    return scopes.peek().getClosestNonBlockScope().getRootNode();
+    return scopes.peek().getClosestContainerScope().getRootNode();
   }
 
   public AbstractScope<?, ?> getClosestHoistScope() {

src/com/google/javascript/jscomp/RemoveUnusedCode.java

@@ -235,7 +235,7 @@ private void traverseAndRemoveUnusedReferences(Node root) {
     // Create scope from parent of root node, which also has externs as a child, so we'll
     // have extern definitions in scope.
     Scope scope = scopeCreator.createScope(root.getParent(), null);
-    if (!scope.isDeclared(NodeUtil.JSC_PROPERTY_NAME_FN, /* recurse */ true)) {
+    if (!scope.hasSlot(NodeUtil.JSC_PROPERTY_NAME_FN)) {
       // TODO(b/70730762): Passes that add references to this should ensure it is declared.
       // NOTE: null input makes this an extern var.
       scope.declare(

src/com/google/javascript/jscomp/ShadowVariables.java

@@ -247,7 +247,7 @@ private Assignment findBestShadow(Scope curScope, Var var) {
       for (Assignment assignment : varsByFrequency) {
         if (assignment.isLocal) {
           if (!scopeUpRefMap.containsEntry(curScope.getRootNode(), assignment.oldName)) {
-            if (curScope.isDeclared(assignment.oldName, true)) {
+            if (curScope.hasSlot(assignment.oldName)) {
               // Don't shadow if the scopes are the same eg.:
               // function f() { var a = 1; { var a = 2; } } // Unsafe
               Var toShadow = curScope.getVar(assignment.oldName);

src/com/google/javascript/jscomp/SymbolTable.java

@@ -1212,7 +1212,7 @@ private SymbolScope createScopeFrom(StaticScope otherScope) {
     Node otherScopeRoot = otherScope.getRootNode();
 
     // NOTE: Kythe is not set up to handle block scopes yet, so only create
-    // SymbolScopes for CFG root nodes, giving a pre-ES6 view of the world.
+    // SymbolScopes for container scope roots, giving a pre-ES6 view of the world.
     while (!NodeUtil.isValidCfgRoot(otherScopeRoot)) {
       otherScopeRoot = otherScopeRoot.getParent();
     }

src/com/google/javascript/jscomp/SyntacticScopeCreator.java

@@ -186,7 +186,7 @@ private void declareVar(Node n) {
 
     CompilerInput input = compiler.getInput(inputId);
     String name = n.getString();
-    if (!scope.isDeclared(name, false) && !(scope.isLocal() && name.equals(Var.ARGUMENTS))) {
+    if (!scope.hasOwnSlot(name) && (!scope.isLocal() || !name.equals(Var.ARGUMENTS))) {
       if (isTyped) {
         ((TypedScope) scope).declare(name, n, null, input);
       } else {

src/com/google/javascript/jscomp/TransformAMDToCJSModule.java

@@ -181,11 +181,10 @@ private void handleRequire(NodeTraversal t, Node defineNode, Node script,
 
       String aliasName = aliasNode != null ? aliasNode.getString() : null;
       Scope globalScope = t.getScope();
-      if (aliasName != null &&
-          globalScope.isDeclared(aliasName, true)) {
+      if (aliasName != null && globalScope.hasSlot(aliasName)) {
         while (true) {
           String renamed = aliasName + VAR_RENAME_SUFFIX + renameIndex;
-          if (!globalScope.isDeclared(renamed, true)) {
+          if (!globalScope.hasSlot(renamed)) {
             NodeTraversal.traverseEs6(compiler, callback,
                 new RenameCallback(aliasName, renamed));
             aliasName = renamed;

src/com/google/javascript/jscomp/TypeCheck.java

@@ -483,17 +483,15 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
       case FUNCTION:
         // normal type checking
         final TypedScope outerScope = t.getTypedScope();
-        final String functionPrivateName = n.getFirstChild().getString();
-        if (functionPrivateName != null
-            && functionPrivateName.length() > 0
-            && outerScope.isDeclared(functionPrivateName, false)
+        final TypedVar var = outerScope.getVar(n.getFirstChild().getString());
+        if (var != null
+            && var.getScope().hasSameContainerScope(outerScope)
             // Ideally, we would want to check whether the type in the scope
             // differs from the type being defined, but then the extern
             // redeclarations of built-in types generates spurious warnings.
-            && !(outerScope.getVar(functionPrivateName).getType() instanceof FunctionType)
-            && !TypeValidator.hasDuplicateDeclarationSuppression(
-                compiler, outerScope.getVar(functionPrivateName).getNameNode())) {
-          report(t, n, FUNCTION_MASKS_VARIABLE, functionPrivateName);
+            && !(var.getType() instanceof FunctionType)
+            && !TypeValidator.hasDuplicateDeclarationSuppression(compiler, var.getNameNode())) {
+          report(t, n, FUNCTION_MASKS_VARIABLE, var.getName());
         }
 
         // TODO(user): Only traverse the function's body. The function's