Finish block scope support in TypedScopeCreator.

Now that all the groundwork has been laid, this change flips the scope creator to actually operate in block-scope mode, and updates the last few little bits that are required to support that.

Note that the type checker still does not understand let/const at all, but at least it now has distinct scopes it can hang the different variables on.

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

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

@@ -65,7 +65,7 @@ abstract class AbstractScope<S extends AbstractScope<S, V>, V extends AbstractVa
   private final Node rootNode;
 
   AbstractScope(Node rootNode) {
-    this.rootNode = rootNode;
+    this.rootNode = checkNotNull(rootNode);
   }
 
   /** The depth of the scope. The global scope has depth 0. */
@@ -143,6 +143,7 @@ final void undeclareInteral(V var) {
   }
 
   final void declareInternal(String name, V var) {
+    checkState(hasOwnSlot(name) || canDeclare(name), "Illegal shadow: %s", var.getNode());
     vars.put(name, var);
   }
 
@@ -229,6 +230,34 @@ public final boolean hasSlot(String name) {
     return false;
   }
 
+  /**
+   * Returns true if the name can be declared on this scope without causing illegal shadowing.
+   * Specifically, this is aware of the connection between function container scopes and function
+   * block scopes and returns false for redeclaring parameters on the block scope.
+   */
+  final boolean canDeclare(String name) {
+    return !hasOwnSlot(name)
+        && (!isFunctionBlockScope()
+            || !getParent().hasOwnSlot(name)
+            || isBleedingFunctionName(name));
+  }
+
+  /**
+   * Returns true if the given name is a bleeding function name in this scope. Local variables in
+   * the function block are not allowed to shadow parameters, but they are allowed to shadow a
+   * bleeding function name.
+   */
+  private boolean isBleedingFunctionName(String name) {
+    V var = getVar(name);
+    if (var == null) {
+      return false;
+    }
+    Node n = var.getScopeRoot();
+    return n.isFunction()
+        && NodeUtil.isBleedingFunctionName(n.getFirstChild())
+        && name.equals(n.getFirstChild().getString());
+  }
+
   /**
    * Return an iterable over all of the variables declared in this scope
    * (except the special 'arguments' variable).

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

@@ -939,6 +939,16 @@ private void popScope(boolean quietly) {
     return scope;
   }
 
+  /**
+   * Instantiate some, but not necessarily all, scopes from stored roots.
+   *
+   * <p>NodeTraversal instantiates scopes lazily when getScope() or similar is called, by iterating
+   * over a stored list of not-yet-instantiated scopeRoots.  When a not-yet-instantiated parent
+   * scope is requested, it doesn't make sense to instantiate <i>all</i> pending scopes.  Instead,
+   * we count the number that are needed to ensure the requested parent is instantiated and call
+   * this function to instantiate only as many scopes as are needed, shifting their roots off the
+   * queue, and returning the deepest scope actually created.
+   */
   private AbstractScope<?, ?> instantiateScopes(int count) {
     checkArgument(count <= scopeRoots.size());
     AbstractScope<?, ?> scope = scopes.peek();
@@ -967,16 +977,13 @@ public Node getClosestHoistScopeRoot() {
     return scopes.peek().getClosestHoistScope().getRootNode();
   }
 
-  public Node getClosestContainerScopeRoot() {
-    int roots = scopeRoots.size();
-    for (int i = roots; i > 0; i--) {
-      Node rootNode = scopeRoots.get(i - 1);
-      if (!NodeUtil.createsBlockScope(rootNode)) {
-        return rootNode;
+  public AbstractScope<?, ?> getClosestContainerScope() {
+    for (int i = scopeRoots.size(); i > 0; i--) {
+      if (!NodeUtil.createsBlockScope(scopeRoots.get(i - 1))) {
+        return instantiateScopes(i);
       }
     }
-
-    return scopes.peek().getClosestContainerScope().getRootNode();
+    return scopes.peek().getClosestContainerScope();
   }
 
   public AbstractScope<?, ?> getClosestHoistScope() {

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

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

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

@@ -200,8 +200,7 @@ FlowScope flowThrough(Node n, FlowScope input) {
       return input;
     }
 
-    // TODO(sdh): Change to NodeUtil.getEnclosingScopeRoot(n) once we have block scopes.
-    Node root = NodeUtil.getEnclosingNode(n, TypeInference::createsContainerScope);
+    Node root = NodeUtil.getEnclosingScopeRoot(n);
     FlowScope output = input.createChildFlowScope(scopeCreator.createScope(root));
     inferDeclarativelyUnboundVarsWithoutTypes(output);
     output = traverse(n, output);

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

@@ -163,15 +163,23 @@ private JSType getImplicitVarType(ImplicitVar var) {
     return getTypeOfThis();
   }
 
+  /**
+   * Returns the variables in this scope that have been declared with 'var' (or 'let') and not
+   * declared with a known type. These variables can safely be set to undefined (rather than
+   * unknown) at the start of type inference, and will be reset to the correct type when analyzing
+   * the first assignment to them. Parameters and externs are excluded because they are not
+   * initialized in the function body.
+   */
   public Iterable<TypedVar> getDeclarativelyUnboundVarsWithoutTypes() {
-    return Iterables.filter(
-        getVarIterable(),
-        var ->
-            // declaratively unbound vars without types
-            var.getParentNode() != null
-                && var.getType() == null
-                && var.getParentNode().isVar()
-                && !var.isExtern());
+    return Iterables.filter(getVarIterable(), this::isDeclarativelyUnboundVarWithoutType);
+  }
+
+  private boolean isDeclarativelyUnboundVarWithoutType(TypedVar var) {
+    return var.getParentNode() != null
+        && var.getType() == null
+        // TODO(sdh): should we include LET here as well?
+        && var.getParentNode().isVar()
+        && !var.isExtern();
   }
 
   static interface TypeResolver {

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

@@ -257,19 +257,12 @@ void removeScopesForScript(String scriptName) {
     memoized.keySet().removeIf(n -> scriptName.equals(NodeUtil.getSourceName(n)));
   }
 
-  /** Create a scope, looking up in the map for the parent scope. */
+  /** Create a scope if it doesn't already exist, looking up in the map for the parent scope. */
   TypedScope createScope(Node n) {
-    // NOTE(sdh): Ideally we could just look up the node in the map,
-    // but sometimes TypedScopeCreator does not create the scope in
-    // the first place (particularly for empty blocks).  When these
-    // cases show up in the CFG, we need to do some extra legwork to
-    // ensure the scope exists.
-    // TODO(sdh): Use NodeUtil.getEnclosingScopeRoot(n.getParent()) once we're block-scoped.
     TypedScope s = memoized.get(n);
     return s != null
         ? s
-        : createScope(
-            n, createScope(NodeUtil.getEnclosingNode(n.getParent(), NodeUtil::isValidCfgRoot)));
+        : createScope(n, createScope(NodeUtil.getEnclosingScopeRoot(n.getParent())));
   }
 
   /**
@@ -575,18 +568,24 @@ public void resolveTypes() {
     @Override
     public final boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
       inputId = t.getInputId();
-      if (n.isFunction() || n.isScript()) {
+      if (n.isFunction() || n.isScript() || (n == currentScope.getRootNode() && inputId != null)) {
         checkNotNull(inputId);
         sourceName = NodeUtil.getSourceName(n);
       }
 
-      // We do want to traverse the name of a named function, but we don't
-      // want to traverse the arguments or body.
+      // The choice of whether to descend depends on the type of scope.  Function scopes
+      // should traverse the outer FUNCTION node, and the NAME and PARAM_LIST nodes, but
+      // should not descend into the body.  Non-function nodes need to at least look at
+      // every child node (parent == currentScope.root), but should not descend *into*
+      // any nodes that create scopes.  The only exception to this is that we do want
+      // to inspect the first (NAME) child of a named function.
       boolean descend =
-          parent == null
-              || !parent.isFunction()
-              || n == parent.getFirstChild()
-              || parent == currentScope.getRootNode();
+          currentScope.isFunctionScope()
+              ? !n.isNormalBlock()
+              : parent == null
+                  || !NodeUtil.createsScope(parent)
+                  || (parent.isFunction() && n == parent.getFirstChild())
+                  || parent == currentScope.getRootNode();
 
       if (descend) {
         // Handle hoisted functions on pre-order traversal, so that they
@@ -602,6 +601,24 @@ public final boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
         }
       }
 
+      // Create any child block scopes "pre-order" as we see them. This is required because hoisted
+      // or qualified names defined in earlier blocks might be referred to later outside the block.
+      // This isn't a big deal in most cases since a NamedType will be created and resolved later,
+      // but if a NamedType is used for a superclass, we lose a lot of valuable checking. Recursing
+      // into child blocks immediately prevents this from being a problem.
+      if (NodeUtil.createsBlockScope(n) && n != currentScope.getRootNode()) {
+        // Only create immediately-nested scopes.  We sometimes encounter nodes that create
+        // grandchild scopes, such as a BLOCK inside a FOR - in that case, don't create the
+        // scope here - it will be created while traversing its parent scope.
+        Node ancestor = parent;
+        while (ancestor != null && !NodeUtil.createsScope(ancestor)) {
+          ancestor = ancestor.getParent();
+        }
+        if (ancestor != null && ancestor == currentScope.getRootNode()) {
+          createScope(n, currentScope);
+        }
+      }
+
       return descend;
     }
 
@@ -1265,9 +1282,8 @@ void defineSlot(Node n, Node parent, String variableName,
       // who declare "global" names in an anonymous namespace.
       TypedScope ownerScope = null;
       if (n.isGetProp()) {
-        ownerScope = getLValueRootScope(n);
+        ownerScope = scopeToDeclareIn;
       } else if (variableName.contains(".")) {
-        // TODO(sdh): can we pull this from 'n' instead of munging the string?
         TypedVar rootVar =
             currentScope.getVar(variableName.substring(0, variableName.indexOf('.')));
         if (rootVar != null) {
@@ -1305,7 +1321,7 @@ void defineSlot(Node n, Node parent, String variableName,
 
       // declared in closest scope?
       CompilerInput input = compiler.getInput(inputId);
-      if (scopeToDeclareIn.hasOwnSlot(variableName)) {
+      if (!scopeToDeclareIn.canDeclare(variableName)) {
         TypedVar oldVar = scopeToDeclareIn.getVar(variableName);
         newVar = validator.expectUndeclaredVariable(
             sourceName, input, n, parent, oldVar, variableName, type);
@@ -2249,10 +2265,10 @@ void process(Node externs, Node root) {
         return;
       }
 
-      Node containerScopeRoot = t.getClosestContainerScopeRoot();
+      Scope containerScope = (Scope) t.getClosestContainerScope();
 
       if (n.isReturn() && n.getFirstChild() != null) {
-        functionsWithNonEmptyReturns.add(containerScopeRoot);
+        functionsWithNonEmptyReturns.add(containerScope.getRootNode());
       }
 
       // Be careful of bleeding functions, which create variables
@@ -2261,13 +2277,17 @@ void process(Node externs, Node root) {
         String name = n.getString();
         Scope scope = t.getScope();
         Var var = scope.getVar(name);
+        // TODO(sdh): consider checking hasSameHoistScope instead of container scope here and
+        // below. This will detect function(a) { a.foo = bar } as an escaped qualified name,
+        // which seems like the right thing to do (but could possibly break things?)
+        // Doing so will allow removing the warning on TypeCheckTest#testIssue1024b.
         if (var != null) {
-          Scope ownerScope = var.getScope().getClosestContainerScope();
+          Scope ownerScope = var.getScope();
           if (ownerScope.isLocal()) {
-            Node ownerScopeRoot = ownerScope.getRootNode();
-            assignedVarNames.add(ScopedName.of(name, ownerScopeRoot));
-            if (containerScopeRoot != ownerScopeRoot) {
-              escapedVarNames.add(ScopedName.of(name, ownerScopeRoot));
+            ScopedName scopedName = ScopedName.of(name, ownerScope.getRootNode());
+            assignedVarNames.add(scopedName);
+            if (!containerScope.hasSameContainerScope(ownerScope)) {
+              escapedVarNames.add(scopedName);
             }
           }
         }
@@ -2276,10 +2296,9 @@ void process(Node externs, Node root) {
         Scope scope = t.getScope();
         Var var = scope.getVar(name);
         if (var != null) {
-          Scope ownerScope = var.getScope().getClosestContainerScope();
-          Node ownerScopeRoot = ownerScope.getRootNode();
-          if (ownerScope.isLocal() && containerScopeRoot != ownerScopeRoot) {
-            escapedVarNames.add(ScopedName.of(n.getQualifiedName(), ownerScopeRoot));
+          Scope ownerScope = var.getScope();
+          if (ownerScope.isLocal() && !containerScope.hasSameContainerScope(ownerScope)) {
+            escapedVarNames.add(ScopedName.of(n.getQualifiedName(), ownerScope.getRootNode()));
           }
         }
       }
@@ -2288,6 +2307,6 @@ void process(Node externs, Node root) {
 
   @Override
   public boolean hasBlockScope() {
-    return false;
+    return true;
   }
 }

test/com/google/javascript/jscomp/TypeCheckTest.java

@@ -3146,6 +3146,28 @@ public void testDuplicateLocalVarDecl() {
             "required: number"));
   }
 
+  public void testDuplicateLocalVarDeclSuppressed() {
+    // We can't just leave this to VarCheck since otherwise if that warning is suppressed, we'll end
+    // up redeclaring it as undefined in the function block, which can cause spurious errors.
+    testTypes(
+        lines(
+            "/** @suppress {duplicate} */",
+            "function f(x) {",
+            "  var x = x;",
+            "  x.y = true;",
+            "}"));
+  }
+
+  public void testShadowBleedingFunctionName() {
+    // This is allowed and creates a new binding inside the function shadowing the bled name.
+    testTypes(
+        lines(
+            "var f = function x() {",
+            "  var x;",
+            "  var /** undefined */ y = x;",
+            "};"));
+  }
+
   public void testDuplicateInstanceMethod1() {
     // If there's no jsdoc on the methods, then we treat them like
     // any other inferred properties.
@@ -19463,7 +19485,7 @@ public void testSuperclassDefinedInBlockOnNamespace() {
             "required: string"));
   }
 
-    public void testCovariantIThenable1() {
+  public void testCovariantIThenable1() {
     testTypes(
         lines(
             "/** @type {!IThenable<string|number>} */ var x;",

test/com/google/javascript/jscomp/TypeInferenceTest.java

@@ -37,6 +37,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec;
 import com.google.javascript.jscomp.DataFlowAnalysis.BranchedFlowState;
+import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
 import com.google.javascript.jscomp.type.FlowScope;
 import com.google.javascript.jscomp.type.ReverseAbstractInterpreter;
 import com.google.javascript.rhino.Node;
@@ -116,8 +117,13 @@ private void parseAndRunTypeInference(String js) {
     Node n = root.getFirstFirstChild();
     // Create the scope with the assumptions.
     TypedScopeCreator scopeCreator = new TypedScopeCreator(compiler);
-    TypedScope assumedScope = scopeCreator.createScope(
-        n, scopeCreator.createScope(root, null));
+    new NodeTraversal(compiler, new AbstractPostOrderCallback() {
+      @Override
+      public void visit(NodeTraversal t, Node n, Node parent) {
+        t.getTypedScope();
+      }
+    }, scopeCreator).traverse(root);
+    TypedScope assumedScope = scopeCreator.createScope(n);
     for (Map.Entry<String,JSType> entry : assumptions.entrySet()) {
       assumedScope.declare(entry.getKey(), null, entry.getValue(), null, false);
     }
@@ -134,7 +140,10 @@ private void parseAndRunTypeInference(String js) {
     // Get the scope of the implicit return.
     BranchedFlowState<FlowScope> rtnState =
         cfg.getImplicitReturn().getAnnotation();
-    returnScope = rtnState.getIn();
+    // Reset the flow scope's syntactic scope to the function block, rather than the function node
+    // itself.  This allows pulling out local vars from the function by name to verify their types.
+    returnScope =
+        rtnState.getIn().createChildFlowScope(scopeCreator.createScope(n.getLastChild()));
   }
 
   private JSType getType(String name) {