Treat early references to aliases of non-nullable names as non-nullable

This still doesn't handle destructuring aliases. The drawback of this CL is that IdentifyGlobalEnumsAndTypedefsAsNonnullable duplicates work done in regular typed scope creation. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=239500615
google · Mar 21, 2019 · 7eae1e6 · 7eae1e6
1 parent 431ca7d
commit 7eae1e6
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 23 deletions.
diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java
@@ -419,14 +419,16 @@ void patchGlobalScope(TypedScope globalScope, Node scriptRoot) {
   }
 
   /**
-   * Create the outermost scope. This scope contains native binding such as
+   * Create the outermost scope. This scope contains native binding such as {@code Object}, {@code
-   * {@code Object}, {@code Date}, etc.
+   * Date}, etc.
    */
   @VisibleForTesting
   TypedScope createInitialScope(Node root) {
 
     NodeTraversal.traverse(
-        compiler, root, new IdentifyGlobalEnumsAndTypedefsAsNonNullable(typeRegistry));
+        compiler,
+        root,
+        new IdentifyGlobalEnumsAndTypedefsAsNonNullable(typeRegistry, codingConvention));
 
     TypedScope s = TypedScope.createGlobalScope(root);
     declareNativeFunctionType(s, ARRAY_FUNCTION_TYPE);
@@ -489,14 +491,17 @@ void resolveTypes() {
   /**
    * Adds all globally-defined enums and typedefs to the registry's list of non-nullable types.
    *
-   * TODO(bradfordcsmith): We should also make locally-defined enums and typedefs non-nullable.
+   * <p>TODO(b/123710194): We should also make locally-defined enums and typedefs non-nullable.
    */
   private static class IdentifyGlobalEnumsAndTypedefsAsNonNullable
       extends AbstractShallowStatementCallback {
     private final JSTypeRegistry registry;
+    private final CodingConvention codingConvention;
 
-    IdentifyGlobalEnumsAndTypedefsAsNonNullable(JSTypeRegistry registry) {
+    IdentifyGlobalEnumsAndTypedefsAsNonNullable(
+        JSTypeRegistry registry, CodingConvention codingConvention) {
       this.registry = registry;
+      this.codingConvention = codingConvention;
     }
 
     @Override
@@ -510,35 +515,43 @@ public void visit(NodeTraversal t, Node node, Node parent) {
           Scope scope = t.getScope();
           checkState(scope.isGlobal() || scope.getClosestHoistScope().isGlobal());
           if (node.isVar() || scope.isGlobal()) {
-            for (Node child = node.getFirstChild();
+            for (Node child = node.getFirstChild(); child != null; child = child.getNext()) {
-                 child != null; child = child.getNext()) {
+              // TODO(b/116853368): make this work for destructuring aliases as well.
-              identifyNameNode(child, NodeUtil.getBestJSDocInfo(child));
+              identifyEnumOrTypedefDeclaration(
+                  child, child.getFirstChild(), NodeUtil.getBestJSDocInfo(child));
             }
           }
           break;
         case EXPR_RESULT:
           Node firstChild = node.getFirstChild();
           if (firstChild.isAssign()) {
-            identifyNameNode(
+            Node assign = firstChild;
-                firstChild.getFirstChild(), firstChild.getJSDocInfo());
+            identifyEnumOrTypedefDeclaration(
+                assign.getFirstChild(), assign.getSecondChild(), assign.getJSDocInfo());
           } else {
-            identifyNameNode(
+            identifyEnumOrTypedefDeclaration(
-                firstChild, firstChild.getJSDocInfo());
+                firstChild, /* rvalue= */ null, firstChild.getJSDocInfo());
           }
           break;
         default:
           break;
       }
     }
 
-    private void identifyNameNode(
+    private void identifyEnumOrTypedefDeclaration(
-        Node nameNode, JSDocInfo info) {
+        Node nameNode, @Nullable Node rvalue, JSDocInfo info) {
-      if (info != null && nameNode.isQualifiedName()) {
+      if (!nameNode.isQualifiedName()) {
-        if (info.hasEnumParameterType()) {
+        return;
-          registry.identifyNonNullableName(nameNode.getQualifiedName());
+      }
-        } else if (info.hasTypedefType()) {
+      if (info != null && info.hasEnumParameterType()) {
-          registry.identifyNonNullableName(nameNode.getQualifiedName());
+        registry.identifyNonNullableName(nameNode.getQualifiedName());
-        }
+      } else if (info != null && info.hasTypedefType()) {
+        registry.identifyNonNullableName(nameNode.getQualifiedName());
+      } else if (rvalue != null
+          && rvalue.isQualifiedName()
+          && registry.isNonNullableName(rvalue.getQualifiedName())
+          && NodeUtil.isConstantDeclaration(codingConvention, info, nameNode)) {
+        registry.identifyNonNullableName(nameNode.getQualifiedName());
       }
     }
   }

diff --git a/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java b/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java
@@ -1943,6 +1943,14 @@ public void identifyNonNullableName(String name) {
     nonNullableTypeNames.add(name);
   }
 
+  /**
+   * Identifies the name of a typedef or enum before we actually declare it.
+   */
+  public boolean isNonNullableName(String name) {
+    checkNotNull(name);
+    return nonNullableTypeNames.contains(name);
+  }
+
   public JSType evaluateTypeExpression(JSTypeExpression expr, StaticTypedScope scope) {
     return createTypeFromCommentNode(expr.getRoot(), expr.getSourceName(), scope);
   }

diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
@@ -5726,14 +5726,32 @@ public void testTypeNameAliasOnAliasedClassSideNamespace() {
   }
 
   @Test
-  public void testForwardDeclaredGlobalAliasOfEnumIsNonNullable() {
+  public void testForwardDeclaredGlobalAliasOfEnumIsNonNullable_constDeclaration() {
-    // TODO(b/116853368): this should be non-nullable and warn
     testTypes(
         lines(
             "/** @enum {string} */",
             "const Colors = {RED: 'red', YELLOW: 'yellow'};",
             "const /** ColorsAlias */ c = null",
-            "const ColorsAlias = Colors;"));
+            "const ColorsAlias = Colors;"),
+        lines(
+            "initializing variable", //
+            "found   : null",
+            "required: Colors<string>"));
+  }
+
+  @Test
+  public void testForwardDeclaredGlobalAliasOfEnumIsNonNullable_constJSDoc() {
+    testTypes(
+        lines(
+            "/** @enum {string} */",
+            "const Colors = {RED: 'red', YELLOW: 'yellow'};",
+            "const /** ns.ColorsAlias */ c = null",
+            "const ns = {};",
+            "/** @const */ ns.ColorsAlias = Colors;"),
+        lines(
+            "initializing variable", //
+            "found   : null",
+            "required: Colors<string>"));
   }
 
   @Test