Create a stricter version of the BanUnresolvedType conformance config.

This version warns for any AST node that resolves to a forwardDeclared type instead of only GETPROP nodes. When using OTI, allow using this stricter conformance config as the way to avoid forward declared types instead of dropping all forward declares. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=172761296
google · Oct 19, 2017 · 2baef98 · 2baef98
1 parent f59c589
commit 2baef98
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 11 deletions.
diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java
@@ -1527,9 +1527,10 @@ public JSTypeRegistry getTypeRegistry() {
 
   @Override
   void forwardDeclareType(String typeName) {
-    if (options.allowUnfulfilledForwardDeclarations()) {
-      forwardDeclaredTypes.add(typeName);
+    if (options.dropForwardDeclarations()) {
+      return;
     }
+    forwardDeclaredTypes.add(typeName);
   }
 
   @Override

diff --git a/src/com/google/javascript/jscomp/CompilerOptions.java b/src/com/google/javascript/jscomp/CompilerOptions.java
@@ -195,8 +195,8 @@ public boolean allowIjsInputs() {
     return incrementalCheckMode != IncrementalCheckMode.OFF;
   }
 
-  public boolean allowUnfulfilledForwardDeclarations() {
-    return incrementalCheckMode == IncrementalCheckMode.OFF;
+  public boolean dropForwardDeclarations() {
+    return incrementalCheckMode == IncrementalCheckMode.CHECK_IJS && useNewTypeInference;
   }
 
   private Config.JsDocParsing parseJsDocDocumentation = Config.JsDocParsing.TYPES_ONLY;

diff --git a/src/com/google/javascript/jscomp/ConformanceRules.java b/src/com/google/javascript/jscomp/ConformanceRules.java
@@ -1365,7 +1365,7 @@ protected ConformanceResult checkConformance(NodeTraversal t, Node n) {
       return ConformanceResult.CONFORMANCE;
     }
 
-    private boolean conforms(TypeI type) {
+    private static boolean conforms(TypeI type) {
       if (type.isUnionType()) {
         // unwrap union types which might contain unresolved type name
         // references for example {Foo|undefined}
@@ -1381,6 +1381,23 @@ private boolean conforms(TypeI type) {
     }
   }
 
+  /** Ban any use of unresolved forward-declared types */
+  public static final class StrictBanUnresolvedType extends AbstractTypeRestrictionRule {
+    public StrictBanUnresolvedType(AbstractCompiler compiler, Requirement requirement)
+        throws InvalidRequirementSpec {
+      super(compiler, requirement);
+    }
+
+    @Override
+    protected ConformanceResult checkConformance(NodeTraversal t, Node n) {
+      TypeI type = n.getTypeI();
+      if (type != null && !BanUnresolvedType.conforms(type) && !isTypeImmediatelyTightened(n)) {
+        return ConformanceResult.VIOLATION;
+      }
+      return ConformanceResult.CONFORMANCE;
+    }
+  }
+
 
   /**
    * Banned global var declarations.

diff --git a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java
@@ -57,11 +57,12 @@ class ConvertToTypedInterface implements CompilerPass {
 
   private static final ImmutableSet<String> CALLS_TO_PRESERVE =
       ImmutableSet.of(
-          "goog.provide",
           "goog.define",
-          "goog.require",
+          "goog.forwardDeclare",
           "goog.module",
-          "goog.module.declareLegacyNamespace");
+          "goog.module.declareLegacyNamespace",
+          "goog.provide",
+          "goog.require");
 
   private final AbstractCompiler compiler;
 

diff --git a/test/com/google/javascript/jscomp/CheckConformanceTest.java b/test/com/google/javascript/jscomp/CheckConformanceTest.java
@@ -1530,6 +1530,40 @@ public void testCustomBanUnresolvedType() {
         "}"));
   }
 
+  public void testCustomStrictBanUnresolvedType() {
+    configuration =
+        "requirement: {\n"
+        + "  type: CUSTOM\n"
+        + "  java_class: 'com.google.javascript.jscomp.ConformanceRules$StrictBanUnresolvedType'\n"
+        + "  error_message: 'StrictBanUnresolvedType Message'\n"
+        + "}";
+
+    // NTI doesn't model unresolved types separately from unknown, so this check always results
+    // in conformance.
+    // TODO(b/67899666): Implement similar functionality in NTI for preventing uses of forward
+    // declared types by implementing support for resolving forward declarations to a new
+    // "unusable type" instead of to unknown.
+    this.mode = TypeInferenceMode.OTI_ONLY;
+    testWarning(
+        "goog.forwardDeclare('Foo'); /** @param {Foo} a */ var f = function(a) {}",
+        CheckConformance.CONFORMANCE_VIOLATION,
+        "Violation: StrictBanUnresolvedType Message");
+
+    testWarning(
+        new String[] {
+          "goog.forwardDeclare('Foo'); /** @param {!Foo} a */ var f;",
+           "f(5);",
+        },
+        TypeValidator.TYPE_MISMATCH_WARNING);
+
+    testWarning(
+        new String[] {
+          "goog.forwardDeclare('Foo'); /** @return {!Foo} */ var f;",
+          "f();",
+        },
+        CheckConformance.CONFORMANCE_VIOLATION);
+  }
+
   public void testMergeRequirements() {
     Compiler compiler = createCompiler();
     ConformanceConfig.Builder builder = ConformanceConfig.newBuilder();

diff --git a/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java b/test/com/google/javascript/jscomp/ConvertToTypedInterfaceTest.java
@@ -954,9 +954,8 @@ public void testDontRemoveGoogModuleContents() {
   }
 
   public void testDontPreserveUnknownTypeDeclarations() {
-    test(
-        "goog.forwardDeclare('MyType'); /** @type {MyType} */ var x;",
-        "/** @type {MyType} */ var x;");
+    testSame(
+        "goog.forwardDeclare('MyType'); /** @type {MyType} */ var x;");
 
     test(
         "goog.addDependency('zzz.js', ['MyType'], []); /** @type {MyType} */ var x;",