Better fix for type-checking regression. Construct error strings lazi…

…ly in DisambiguateProperties instead of capping the number of strings constructed. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=144231894
google · Jan 12, 2017 · b5615b1 · b5615b1
1 parent 63cdab9
commit b5615b1
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 22 deletions.
diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java
@@ -356,12 +356,12 @@ public void process(Node externs, Node root) {
     // For each pair (A, B), here we mark both A and B as types whose properties
     // cannot be renamed.
     for (TypeMismatch mis : compiler.getTypeMismatches()) {
-      recordInvalidatingType(mis.typeA, mis.src);
-      recordInvalidatingType(mis.typeB, mis.src);
+      recordInvalidatingType(mis.typeA, mis);
+      recordInvalidatingType(mis.typeB, mis);
     }
     for (TypeMismatch mis : compiler.getImplicitInterfaceUses()) {
-      recordInvalidatingType(mis.typeA, mis.src);
-      recordInvalidatingType(mis.typeB, mis.src);
+      recordInvalidatingType(mis.typeA, mis);
+      recordInvalidatingType(mis.typeB, mis);
     }
     // Gather names of properties in externs; these properties can't be renamed.
     NodeTraversal.traverseEs6(compiler, externs, new FindExternProperties());
@@ -372,13 +372,19 @@ public void process(Node externs, Node root) {
     renameProperties();
   }
 
-  private void recordInvalidationError(JSType t, JSError error) {
+  private void recordInvalidationError(JSType t, TypeMismatch mis) {
     if (!t.isObject()) {
       return;
     }
     if (invalidationMap != null) {
       Collection<JSError> errors = this.invalidationMap.get(t);
       if (errors.size() < MAX_INVALIDATION_WARNINGS_PER_PROPERTY) {
+        JSError error = mis.src;
+        if (error.getType().equals(TypeValidator.TYPE_MISMATCH_WARNING)
+            && error.description.isEmpty()) {
+          String msg = "Implicit use of type " + mis.typeA + " as " + mis.typeB;
+          error = JSError.make(error.node, TypeValidator.TYPE_MISMATCH_WARNING, msg);
+        }
         errors.add(error);
       }
     }
@@ -387,22 +393,22 @@ private void recordInvalidationError(JSType t, JSError error) {
   /**
    * Invalidates the given type, so that no properties on it will be renamed.
    */
-  private void recordInvalidatingType(JSType type, JSError error) {
+  private void recordInvalidatingType(JSType type, TypeMismatch mis) {
     type = type.restrictByNotNullOrUndefined();
     if (type.isUnionType()) {
       for (JSType alt : type.toMaybeUnionType().getAlternatesWithoutStructuralTyping()) {
-        recordInvalidatingType(alt, error);
+        recordInvalidatingType(alt, mis);
       }
     } else if (type.isEnumElementType()) {
       recordInvalidatingType(
-          type.toMaybeEnumElementType().getPrimitiveType(), error);
+          type.toMaybeEnumElementType().getPrimitiveType(), mis);
     } else {
       addInvalidatingType(type);
-      recordInvalidationError(type, error);
+      recordInvalidationError(type, mis);
       ObjectType objType = ObjectType.cast(type);
       if (objType != null && objType.getImplicitPrototype() != null) {
         addInvalidatingType(objType.getImplicitPrototype());
-        recordInvalidationError(objType.getImplicitPrototype(), error);
+        recordInvalidationError(objType.getImplicitPrototype(), mis);
       }
       if (objType != null
           && objType.isConstructor() && objType.isFunctionType()) {

diff --git a/src/com/google/javascript/jscomp/TypeValidator.java b/src/com/google/javascript/jscomp/TypeValidator.java
@@ -792,8 +792,6 @@ private JSType removeNullUndefinedAndTemplates(JSType t) {
     return result;
   }
 
-  private int numImplicitUses = 0;
-
   private void recordImplicitInterfaceUses(Node src, JSType sourceType, JSType targetType) {
     sourceType = removeNullUndefinedAndTemplates(sourceType);
     targetType = removeNullUndefinedAndTemplates(targetType);
@@ -804,16 +802,9 @@ private void recordImplicitInterfaceUses(Node src, JSType sourceType, JSType tar
     if (strictMismatch || mismatch) {
       // We don't report a type error, but we still need to construct a JSError,
       // for people who enable the invalidation diagnostics in DisambiguateProperties.
-      // Constructing these error strings can take a large amount of time, so we stop after the
-      // first few. Picked 10 arbitrarily.
-      String msg;
-      if (numImplicitUses < 10) {
-        msg = "Implicit use of type " + sourceType + " as " + targetType;
-      } else {
-        msg = "";
-      }
-      numImplicitUses++;
-      JSError err = JSError.make(src, TYPE_MISMATCH_WARNING, msg);
+      // Use the empty string as the error string. Creating an actual error message can be slow
+      // for large types; we create an error string lazily in DisambiguateProperties.
+      JSError err = JSError.make(src, TYPE_MISMATCH_WARNING, "");
       implicitInterfaceUses.add(new TypeMismatch(sourceType, targetType, err));
     }
   }