Make default nullability in the type system work identically for glob…

…al and local types

This is mostly important for moving module rewriting post-checks, as module types are local.

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

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

@@ -61,8 +61,8 @@
 import com.google.javascript.jscomp.CodingConvention.ObjectLiteralCast;
 import com.google.javascript.jscomp.CodingConvention.SubclassRelationship;
 import com.google.javascript.jscomp.FunctionTypeBuilder.AstFunctionContents;
+import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
 import com.google.javascript.jscomp.NodeTraversal.AbstractScopedCallback;
-import com.google.javascript.jscomp.NodeTraversal.AbstractShallowStatementCallback;
 import com.google.javascript.jscomp.ProcessClosureProvidesAndRequires.ProvidedName;
 import com.google.javascript.jscomp.modules.Export;
 import com.google.javascript.jscomp.modules.Module;
@@ -413,9 +413,6 @@ private TypedScope createScopeInternal(Node root, TypedScope typedParent) {
       externsRoot.setJSType(globalThis);
       jsRoot.setJSType(globalThis);
 
-      // Run a first-order analysis over the syntax tree.
-      new FirstOrderFunctionAnalyzer().process(root.getFirstChild(), root.getLastChild());
-
       // Find all the classes in the global scope.
       newScope = createInitialScope(root);
     } else {
@@ -615,7 +612,7 @@ void patchGlobalScope(TypedScope globalScope, Node scriptRoot) {
     assignedVarNames.removeIf(var -> inScript.test(var.getScopeRoot()));
     functionsWithNonEmptyReturns.removeIf(inScript);
 
-    new FirstOrderFunctionAnalyzer().process(null, scriptRoot);
+    NodeTraversal.traverse(compiler, scriptRoot, new FirstOrderFunctionAnalyzer());
 
     // TODO(bashir): Variable declaration is not the only side effect of last
     // global scope generation but here we only wipe that part off.
@@ -657,10 +654,19 @@ void patchGlobalScope(TypedScope globalScope, Node scriptRoot) {
   TypedScope createInitialScope(Node root) {
     checkArgument(root.isRoot(), root);
 
-    NodeTraversal.traverse(
-        compiler,
-        root,
-        new IdentifyGlobalEnumsAndTypedefsAsNonNullable(typeRegistry, codingConvention));
+    // Gather global information used in typed scope creation. Use a memoized scope creator because
+    // scope-building takes a nontrivial amount of time.
+    MemoizedScopeCreator scopeCreator =
+        new MemoizedScopeCreator(new Es6SyntacticScopeCreator(compiler));
+
+    new NodeTraversal(compiler, new FirstOrderFunctionAnalyzer(), scopeCreator)
+        .traverseRoots(root.getFirstChild(), root.getLastChild());
+
+    new NodeTraversal(
+            compiler,
+            new IdentifyEnumsAndTypedefsAsNonNullable(typeRegistry, codingConvention),
+            scopeCreator)
+        .traverse(root);
 
     TypedScope s = TypedScope.createGlobalScope(root);
     declareNativeFunctionType(s, ARRAY_FUNCTION_TYPE);
@@ -733,17 +739,12 @@ void resolveTypes() {
     typeRegistry.resolveTypes();
   }
 
-  /**
-   * Adds all globally-defined enums and typedefs to the registry's list of non-nullable types.
-   *
-   * <p>TODO(b/123710194): We should also make locally-defined enums and typedefs non-nullable.
-   */
-  private static class IdentifyGlobalEnumsAndTypedefsAsNonNullable
-      extends AbstractShallowStatementCallback {
+  /** Adds all enums and typedefs to the registry's list of non-nullable types. */
+  private static class IdentifyEnumsAndTypedefsAsNonNullable extends AbstractPostOrderCallback {
     private final JSTypeRegistry registry;
     private final CodingConvention codingConvention;
 
-    IdentifyGlobalEnumsAndTypedefsAsNonNullable(
+    IdentifyEnumsAndTypedefsAsNonNullable(
         JSTypeRegistry registry, CodingConvention codingConvention) {
       this.registry = registry;
       this.codingConvention = codingConvention;
@@ -755,29 +756,22 @@ public void visit(NodeTraversal t, Node node, Node parent) {
         case LET:
         case CONST:
         case VAR:
-          // Note that this class expects to be invoked on the root node and does not traverse into
-          // functions.
-          Scope scope = t.getScope();
-          if (!(scope.isGlobal() || scope.getClosestHoistScope().isGlobal())) {
-            return;
-          }
-          if (node.isVar() || scope.isGlobal()) {
             for (Node child = node.getFirstChild(); child != null; child = child.getNext()) {
-              // TODO(b/116853368): make this work for destructuring aliases as well.
-              identifyEnumOrTypedefDeclaration(
-                  child, child.getFirstChild(), NodeUtil.getBestJSDocInfo(child));
+            // TODO(b/116853368): make this work for destructuring aliases as well.
+            identifyEnumOrTypedefDeclaration(
+                t, child, child.getFirstChild(), NodeUtil.getBestJSDocInfo(child));
             }
-          }
+
           break;
         case EXPR_RESULT:
           Node firstChild = node.getFirstChild();
           if (firstChild.isAssign()) {
             Node assign = firstChild;
             identifyEnumOrTypedefDeclaration(
-                assign.getFirstChild(), assign.getSecondChild(), assign.getJSDocInfo());
-          } else {
+                t, assign.getFirstChild(), assign.getSecondChild(), assign.getJSDocInfo());
+          } else if (firstChild.isGetProp()) {
             identifyEnumOrTypedefDeclaration(
-                firstChild, /* rvalue= */ null, firstChild.getJSDocInfo());
+                t, firstChild, /* rvalue= */ null, firstChild.getJSDocInfo());
           }
           break;
         default:
@@ -786,19 +780,19 @@ public void visit(NodeTraversal t, Node node, Node parent) {
     }
 
     private void identifyEnumOrTypedefDeclaration(
-        Node nameNode, @Nullable Node rvalue, JSDocInfo info) {
+        NodeTraversal t, Node nameNode, @Nullable Node rvalue, JSDocInfo info) {
       if (!nameNode.isQualifiedName()) {
         return;
       }
       if (info != null && info.hasEnumParameterType()) {
-        registry.identifyNonNullableName(nameNode.getQualifiedName());
+        registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName());
       } else if (info != null && info.hasTypedefType()) {
-        registry.identifyNonNullableName(nameNode.getQualifiedName());
+        registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName());
       } else if (rvalue != null
           && rvalue.isQualifiedName()
-          && registry.isNonNullableName(rvalue.getQualifiedName())
+          && registry.isNonNullableName(t.getScope(), rvalue.getQualifiedName())
           && NodeUtil.isConstantDeclaration(codingConvention, info, nameNode)) {
-        registry.identifyNonNullableName(nameNode.getQualifiedName());
+        registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName());
       }
     }
   }
@@ -2335,7 +2329,7 @@ private void declareAliasTypeIfRvalueIsAliasable(
           // Propagate typedef type to typedef aliases.
           actualLvalueNode.setTypedefTypeProp(typedefType);
           if (lValueName != null) {
-            typeRegistry.identifyNonNullableName(lValueName);
+            typeRegistry.identifyNonNullableName(aliasDeclarationScope, lValueName);
             typeRegistry.declareType(aliasDeclarationScope, lValueName, typedefType);
           }
           return;
@@ -2363,10 +2357,7 @@ private void declareAliasTypeIfRvalueIsAliasable(
         // Look for cases where the rValue is an Enum namespace
         typeRegistry.declareType(
             aliasDeclarationScope, lValueName, rValueType.toMaybeEnumType().getElementsType());
-        if (isLValueRootedInGlobalScope(actualLvalueNode)) {
-          // TODO(b/123710194): Also make local aliases non-nullable
-          typeRegistry.identifyNonNullableName(lValueName);
-        }
+        typeRegistry.identifyNonNullableName(aliasDeclarationScope, lValueName);
       }
     }
 
@@ -3433,15 +3424,6 @@ private JSType determineGetterType(FunctionType methodType) {
    * <p>The syntactic scopes created in this traversal are also stored for later use.
    */
   private class FirstOrderFunctionAnalyzer extends AbstractScopedCallback {
-
-    void process(Node externs, Node root) {
-      if (externs == null) {
-        NodeTraversal.traverse(compiler, root, this);
-      } else {
-        NodeTraversal.traverseRoots(compiler, this, externs, root);
-      }
-    }
-
     @Override
     public void enterScope(NodeTraversal t) {
       Scope scope = t.getScope();

src/com/google/javascript/rhino/jstype/JSTypeRegistry.java

@@ -78,7 +78,6 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -158,20 +157,22 @@ public class JSTypeRegistry implements Serializable {
   // when serializing and deserializing compiler state for multistage builds.
   private final Node nameTableGlobalRoot = new Node(Token.ROOT);
 
-  // NOTE(nicksantos): This is a terrible terrible hack. When type expressions
-  // are evaluated, we need to be able to decide whether that type name
-  // resolves to a nullable type or a non-nullable type. Object types are
-  // nullable, but enum types are not.
+  // NOTE(nicksantos): This is a terrible terrible hack. When type expressions are evaluated, we
+  // need to be able to decide whether that type name resolves to a nullable type or a non-nullable
+  // type. Object types are nullable, but enum types and typedefs are not.
   //
-  // Notice that it's not good enough to just declare enum types sooner.
+  // Notice that it's not good enough to just declare enum types and typedefs sooner.
   // For example, if we have
-  // /** @enum {MyObject} */ var MyEnum = ...;
-  // we won't be to declare "MyEnum" without evaluating the expression
-  // {MyObject}, and following those dependencies starts to lead us into
-  // undecidable territory. Instead, we "pre-declare" enum types and typedefs,
-  // so that the expression resolver can decide whether a given name is
+  //   /** @enum {MyObject} */ var MyEnum = ...;
+  // we won't be to declare "MyEnum" without evaluating the expression {MyObject}, and following
+  // those dependencies starts to lead us into undecidable territory. Instead, we "pre-declare" enum
+  // types and typedefs, so that the expression resolver can decide whether a given name is
   // nullable or not.
-  private final Set<String> nonNullableTypeNames = new LinkedHashSet<>();
+  // Also, note that this solution is buggy and the type resolution still gets default nullability
+  // wrong in some cases. See b/116853368. Probably NamedType should be responsible for
+  // applying nullability to forward references instead of the type expression evaluation.
+  private final Multimap<Node, String> nonNullableTypeNames =
+      MultimapBuilder.hashKeys().hashSetValues().build();
 
   // Types that have been "forward-declared."
   // If these types are not declared anywhere in the binary, we shouldn't
@@ -490,7 +491,7 @@ private void initializeBuiltInTypes() {
 
     // Thenable is an @typedef
     JSType thenableType = createRecordType(ImmutableMap.of("then", unknownType));
-    identifyNonNullableName("Thenable");
+    identifyNonNullableName(null, "Thenable");
     registerNativeType(JSTypeNative.THENABLE_TYPE, thenableType);
 
     // Create built-in Promise type, whose constructor takes one parameter.
@@ -1900,20 +1901,18 @@ public NamedType createNamedType(
     return new NamedType(scope, this, reference, sourceName, lineno, charno);
   }
 
-  /**
-   * Identifies the name of a typedef or enum before we actually declare it.
-   */
-  public void identifyNonNullableName(String name) {
+  /** Identifies the name of a typedef or enum before we actually declare it. */
+  public void identifyNonNullableName(StaticScope scope, String name) {
     checkNotNull(name);
-    nonNullableTypeNames.add(name);
+    StaticScope lookupScope = getLookupScope(scope, name);
+    nonNullableTypeNames.put(getRootNodeForScope(lookupScope), name);
   }
 
-  /**
-   * Identifies the name of a typedef or enum before we actually declare it.
-   */
-  public boolean isNonNullableName(String name) {
+  /** Identifies the name of a typedef or enum before we actually declare it. */
+  public boolean isNonNullableName(StaticScope scope, String name) {
     checkNotNull(name);
-    return nonNullableTypeNames.contains(name);
+    scope = getLookupScope(scope, name);
+    return nonNullableTypeNames.containsEntry(getRootNodeForScope(scope), name);
   }
 
   public JSType evaluateTypeExpression(JSTypeExpression expr, StaticTypedScope scope) {
@@ -2019,7 +2018,7 @@ private JSType createFromTypeNodesInternal(
                 n.getLineno(),
                 n.getCharno(),
                 recordUnresolvedTypes);
-        if (namedType instanceof ObjectType && !nonNullableTypeNames.contains(n.getString())) {
+        if (namedType instanceof ObjectType && !isNonNullableName(scope, n.getString())) {
           Node typeList = n.getFirstChild();
           boolean isForwardDeclared = namedType instanceof NamedType;
           if ((!namedType.isUnknownType() || isForwardDeclared) && typeList != null) {

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

@@ -1027,7 +1027,6 @@ public void testGlobalEnumWithConst() {
 
   @Test
   public void testLocalEnumWithLet() {
-    // TODO(bradfordcsmith): Local enum types should be non-nullable just like the global ones.
     testTypes(
         lines(
             "{",
@@ -1039,16 +1038,11 @@ public void testLocalEnumWithLet() {
             "   * @return {number}",
             "   */",
             "  function f(x) {return x}",
-            "}"),
-        lines(
-            "inconsistent return type",
-            "found   : (E<number>|null)",
-            "required: number"));
+            "}"));
   }
 
   @Test
   public void testLocalEnumWithConst() {
-    // TODO(bradfordcsmith): Local enum types should be non-nullable just like the global ones.
     testTypes(
         lines(
             "{",
@@ -1060,11 +1054,7 @@ public void testLocalEnumWithConst() {
             "   * @return {number}",
             "   */",
             "  function f(x) {return x}",
-            "}"),
-        lines(
-            "inconsistent return type",
-            "found   : (E<number>|null)",
-            "required: number"));
+            "}"));
   }
 
   @Test
@@ -6157,18 +6147,25 @@ public void testLocalEnumDoesNotInfluenceGlobalDefaultNullablity() {
 
   @Test
   public void testGlobalEnumDoesNotInfluenceLocalDefaultNullablity() {
-    // TODO(b/123710194): the local Foo should be nullable and this should not warn
     testTypes(
         lines(
             "/** @enum {number} */ const Foo = {A: 1};",
             "function f() {",
             "  class Foo {};",
             "  /** @type {Foo} */ let x = null;",
-            "}"),
+            "}"));
+  }
+
+  @Test
+  public void testLocalEnumAliasDoesNotInfluenceGlobalDefaultNullablity() {
+    testTypes(
         lines(
-            "initializing variable", //
-            "found   : null",
-            "required: Foo"));
+            "class Foo {};",
+            "/** @enum {number} */ const Bar = {A: 1};",
+            "function f() {",
+            "  const Foo = Bar;",
+            "}",
+            "/** @type {Foo} */ let x = null;"));
   }
 
   @Test

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

@@ -77,9 +77,9 @@ protected CompilerOptions getDefaultOptions() {
 
   @Test
   public void testInitialTypingScope() {
-    TypedScope s = new TypedScopeCreator(compiler,
-        CodingConventions.getDefault()).createInitialScope(
-            new Node(Token.ROOT));
+    TypedScope s =
+        new TypedScopeCreator(compiler, CodingConventions.getDefault())
+            .createInitialScope(new Node(Token.ROOT, new Node(Token.ROOT), new Node(Token.ROOT)));
 
     assertTypeEquals(getNativeArrayConstructorType(), s.getVar("Array").getType());
     assertTypeEquals(getNativeBooleanObjectConstructorType(), s.getVar("Boolean").getType());

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

@@ -4825,6 +4825,43 @@ public void testGoogRequire_exportObjectLiteralWithLiteralValue() {
     assertNode(xNode).hasJSTypeThat().toStringIsEqualTo("{x: number}");
   }
 
+  @Test
+  public void testModuleTypedef_isNonnullableWithinModule() {
+    testSame(
+        lines(
+            "goog.module('a.b'); var /** strType */ earlyRef; ",
+            "/** @typedef {string} */ let strType; ",
+            "var /** strType */ normalRef; MODULE: mod;"));
+
+    TypedScope moduleScope = getLabeledStatement("MODULE").enclosingScope;
+    assertThat(moduleScope.getSlot("earlyRef")).hasJSTypeThat().isString();
+    assertThat(moduleScope.getSlot("normalRef")).hasJSTypeThat().isString();
+  }
+
+  @Test
+  public void testModuleTypedef_isNonnullableWhenRequired() {
+
+    testSame(
+        srcs(
+            lines(
+                "goog.module('root');",
+                "/** @typedef {string} */ let strType; ",
+                "exports.strType = strType;"),
+            lines(
+                "goog.module('a.b'); const {strType} = goog.require('root');",
+                "/** @param {strType} earlyRef */ function f(earlyRef) { EARLY_REF: earlyRef; }",
+                "var /** strType */ normalRef; NORMAL_REF: normalRef;")));
+
+    assertNode(getLabeledStatement("EARLY_REF").statementNode.getOnlyChild())
+        .hasJSTypeThat()
+        // TODO(b/116853368): This should just be 'string', but the function declaration is
+        // hoisted so it hits bugs in nullability of forward references to types.
+        .toStringIsEqualTo("(null|string)");
+    assertNode(getLabeledStatement("NORMAL_REF").statementNode.getOnlyChild())
+        .hasJSTypeThat()
+        .isString();
+  }
+
   @Test
   public void testGoogRequire_namedExportImportedAsNamespace() {
     testSame(