Fix infinite recursion in recursive-typedef subtype check

Recursive typedefs are not supposed to be possible, but there's a way to create them using forward-referenced named types. If such a recursive type is checked against a separate-but-equivalent @record or @typedef type, the type checker currently blows up with infinite recursion. This can be prevented by taking advantage of the ImplCache in PrototypeObjectType#isSubtype.

Doing this correctly requires carefully threading the needle through the various special cases and overrides of isSubtype, particularly due to a separate structural-subtype check that sometimes runs in addition to the version in PrototypeObjectType - if it reports (and caches) a different result then we end up with problems in union coalescing (either under- or over-collapsing), missing property detection, or various other places.
This CL attempts to consolidate the two structural checks into one by adding some extra dimensions to the check: (1) whether the supertype is an inline record literal or not (to determines which properties to match) and (2) whether to require explicitly voidable supertype props to be present in the subtype.

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

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

@@ -42,7 +42,6 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.base.Predicate;
-import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -746,8 +745,7 @@ boolean checkEquivalenceHelper(
     // 2. if we are comparing to anonymous record types (e.g. `{a: 3}`), always use structural types
     // Ideally we would also use structural typing to compare anything declared @record, but
     // that is harder to do with our current representation of types.
-    if ((eqCache.isStructuralTyping() && this.isStructuralType() && that.isStructuralType())
-        || (this.isRecordType() && that.isRecordType())) {
+    if (eqCache.shouldMatchStructurally(this, that)) {
       return toMaybeObjectType()
           .checkStructuralEquivalenceHelper(that.toMaybeObjectType(), eqMethod, eqCache);
     }
@@ -1611,24 +1609,27 @@ static boolean isSubtypeHelper(JSType thisType, JSType thatType,
 
     // If the super type is a structural type, then we can't safely remove a templatized type
     // (since it might affect the types of the properties)
-    if (implicitImplCache.isStructuralTyping()
-        && thisType.isObject() && thatType.isStructuralType()) {
-      return thisType.toMaybeObjectType().isStructuralSubtype(
-          thatType.toMaybeObjectType(), implicitImplCache, subtypingMode);
+    if (implicitImplCache.shouldMatchStructurally(thisType, thatType)) {
+      return thisType
+          .toMaybeObjectType()
+          .isStructuralSubtype(thatType.toMaybeObjectType(), implicitImplCache, subtypingMode);
     }
 
     // Templatized types. For nominal types, the above check guarantees TemplateTypeMap
     // equivalence; check if the base type is a subtype.
     if (thisType.isTemplatizedType()) {
-      return thisType.toMaybeTemplatizedType().getReferencedType().isSubtype(
-          thatType, implicitImplCache, subtypingMode);
+      return thisType
+          .toMaybeTemplatizedType()
+          .getReferencedType()
+          .isSubtype(thatType, implicitImplCache, subtypingMode);
     }
 
     // proxy types
     if (thatType instanceof ProxyObjectType) {
       return thisType.isSubtype(
           ((ProxyObjectType) thatType).getReferencedTypeInternal(),
-          implicitImplCache, subtypingMode);
+          implicitImplCache,
+          subtypingMode);
     }
     return false;
   }
@@ -1866,6 +1867,10 @@ static enum MatchStatus {
     boolean subtypeValue() {
       return this.isSubtype;
     }
+
+    static MatchStatus valueOf(boolean match) {
+      return match ? MATCH : NOT_MATCH;
+    }
   }
 
   /**
@@ -1943,6 +1948,12 @@ private HashMap<Key, MatchStatus> getMatchCache() {
       return matchCache;
     }
 
+    boolean shouldMatchStructurally(JSType left, JSType right) {
+      return isStructuralTyping()
+          ? left.isStructuralType() && right.isStructuralType()
+          : left.isRecordType() && right.isRecordType();
+    }
+
     private static final class Key {
       private final Object left;
       private final Object right;
@@ -1977,8 +1988,7 @@ public boolean equals(Object other) {
             | ((this.left == that.right) & (this.right == that.left));
       }
 
-      @SuppressWarnings("ReferenceEquality")
-      private Key(Object left, Object right) {
+      Key(Object left, Object right) {
         this.left = left;
         this.right = right;
 
@@ -1996,7 +2006,8 @@ private Key(Object left, Object right) {
    * cache used by check sub-type logic
    */
   static class ImplCache extends MatchCache {
-    private HashBasedTable<JSType, JSType, MatchStatus> matchCache;
+    private HashMap<Key, MatchStatus> matchCache;
+    private EqCache eqCache;
 
     static ImplCache create() {
       return new ImplCache(true);
@@ -2008,23 +2019,66 @@ static ImplCache createWithoutStructuralTyping() {
 
     private ImplCache(boolean isStructuralTyping) {
       super(isStructuralTyping);
-      this.matchCache = null;
     }
 
-    void updateCache(JSType subType, JSType superType, MatchStatus isMatch) {
-      this.matchCache.put(subType, superType, isMatch);
+    boolean updateCache(JSType subType, JSType superType, MatchStatus isMatch) {
+      matchCache.put(new Key(subType, superType), isMatch);
+      return isMatch.subtypeValue();
     }
 
     MatchStatus checkCache(JSType subType, JSType superType) {
-      if (this.matchCache == null) {
-        this.matchCache = HashBasedTable.create();
+      if (matchCache == null) {
+        this.matchCache = new HashMap<>();
       }
       // check the cache
-      if (this.matchCache.contains(subType, superType)) {
-        return this.matchCache.get(subType, superType);
-      } else {
-        this.updateCache(subType, superType, MatchStatus.PROCESSING);
-        return null;
+      return matchCache.putIfAbsent(new Key(subType, superType), MatchStatus.PROCESSING);
+    }
+
+    boolean shouldMatchStructurally(JSType subType, JSType superType) {
+      return isStructuralTyping() && subType.isObject() && superType.isStructuralType();
+    }
+
+    private boolean equal(JSType left, JSType right) {
+      if (eqCache == null) {
+        this.eqCache = new EqCache(isStructuralTyping());
+      }
+      return left.checkEquivalenceHelper(right, EquivalenceMethod.IDENTITY, eqCache);
+    }
+
+    private final class Key {
+      final JSType left;
+      final JSType right;
+      final int hashCode; // Cache this calculation because it is made often.
+
+      @Override
+      public int hashCode() {
+        return hashCode;
+      }
+
+      @Override
+      @SuppressWarnings({"ReferenceEquality", "EqualsBrokenForNull", "EqualsUnsafeCast"})
+      public boolean equals(Object other) {
+        // Calling this with `null` or not a `Key` should never happen, so it's fine to crash.
+        Key that = (Key) other;
+        if (this.left == that.left && this.right == that.right) {
+          return true;
+        }
+
+        // The vast majority of cases will have already returned by now, since equality isn't even
+        // checked unless the hash code matches, and in most cases there's only one instance of any
+        // equivalent JSType floating around. The remainder only occurs for cyclic (or otherwise
+        // complicated) data structures where equivalent types are being synthesized by recursive
+        // application of type parameters, or (even more rarely) for hash collisions. Identity is
+        // insufficient in the case of recursive parameterized types because new equivalent copies
+        // of the type are generated on-the-fly to compute the types of various properties.
+        return equal(this.left, that.left) && equal(this.right, that.right);
+      }
+
+      Key(JSType left, JSType right) {
+        this.left = left;
+        this.right = right;
+        // NOTE: order matters here, since we're expressing an asymmetric relationship.
+        this.hashCode = 31 * left.hashCode() + right.hashCode();
       }
     }
   }

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

@@ -40,6 +40,7 @@
 package com.google.javascript.rhino.jstype;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.javascript.rhino.jstype.ObjectType.PropertyOptionality.VOIDABLE_PROPS_ARE_OPTIONAL;
 import static com.google.javascript.rhino.jstype.TernaryValue.FALSE;
 import static com.google.javascript.rhino.jstype.TernaryValue.UNKNOWN;
 
@@ -601,31 +602,52 @@ final boolean checkStructuralEquivalenceHelper(
     return true;
   }
 
-  private static boolean isStructuralSubtypeHelper(
-      ObjectType typeA, ObjectType typeB,
-      ImplCache implicitImplCache, SubtypingMode subtypingMode) {
+  protected static boolean isStructuralSubtypeHelper(
+      ObjectType typeA,
+      ObjectType typeB,
+      ImplCache implicitImplCache,
+      SubtypingMode subtypingMode,
+      PropertyOptionality optionality) {
 
     // typeA is a subtype of record type typeB iff:
     // 1) typeA has all the non-optional properties declared in typeB.
     // 2) And for each property of typeB, its type must be
     //    a super type of the corresponding property of typeA.
-    for (String property : typeB.getPropertyNames()) {
+
+    Iterable<String> props =
+        // NOTE: Inline record literal types always have Object as a supertype. In these cases, we
+        // really only care about the properties explicitly declared in the record literal, and not
+        // about any properties inherited from Object.prototype. On the other hand, @record types
+        // allow inheritance and we need to match against inherited properties as well.
+        typeB.isRecordType() ? typeB.getOwnPropertyNames() : typeB.getPropertyNames();
+
+    for (String property : props) {
       JSType propB = typeB.getPropertyType(property);
-      if (!typeA.hasProperty(property)) {
-        // Currently, any type that explicitly includes undefined (eg, `?|undefined`) is optional.
-        if (propB.isExplicitlyVoidable()) {
-          continue;
+      if (typeA.hasProperty(property)) {
+        JSType propA = typeA.getPropertyType(property);
+        if (!propA.isSubtype(propB, implicitImplCache, subtypingMode)) {
+          return false;
         }
-        return false;
-      }
-      JSType propA = typeA.getPropertyType(property);
-      if (!propA.isSubtype(propB, implicitImplCache, subtypingMode)) {
+      } else if (!optionality.isOptional(propB)) {
+        // Currently, any type that explicitly includes undefined (eg, `?|undefined`) is optional.
         return false;
       }
     }
     return true;
   }
 
+  /** How to treat explicitly voidable properties for structural subtype checking. */
+  protected enum PropertyOptionality {
+    /** Explicitly voidable properties are treated as optional. */
+    VOIDABLE_PROPS_ARE_OPTIONAL,
+    /** All properties are always required, even if explicitly voidable. */
+    ALL_PROPS_ARE_REQUIRED;
+
+    boolean isOptional(JSType propType) {
+      return this == VOIDABLE_PROPS_ARE_OPTIONAL && propType.isExplicitlyVoidable();
+    }
+  };
+
   /**
    * Determine if {@code this} is a an implicit subtype of {@code superType}.
    */
@@ -642,11 +664,10 @@ final boolean isStructuralSubtype(ObjectType superType,
       return cachedResult.subtypeValue();
     }
 
-    boolean result = isStructuralSubtypeHelper(
-        this, superType, implicitImplCache, subtypingMode);
-    implicitImplCache.updateCache(
-        this, superType, result ? MatchStatus.MATCH : MatchStatus.NOT_MATCH);
-    return result;
+    boolean result =
+        isStructuralSubtypeHelper(
+            this, superType, implicitImplCache, subtypingMode, VOIDABLE_PROPS_ARE_OPTIONAL);
+    return implicitImplCache.updateCache(this, superType, MatchStatus.valueOf(result));
   }
 
   /**

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

@@ -40,6 +40,7 @@
 package com.google.javascript.rhino.jstype;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.javascript.rhino.jstype.ObjectType.PropertyOptionality.ALL_PROPS_ARE_REQUIRED;
 
 import com.google.common.collect.ImmutableList;
 import com.google.javascript.rhino.ErrorReporter;
@@ -429,23 +430,21 @@ protected boolean isSubtype(JSType that,
   }
 
   /** Determines if typeA is a subtype of typeB */
-  private static boolean isSubtype(ObjectType typeA, RecordType typeB,
-      ImplCache implicitImplCache, SubtypingMode subtypingMode) {
-    // typeA is a subtype of record type typeB iff:
-    // 1) typeA has all the properties declared in typeB.
-    // 2) And for each property of typeB, its type must be
-    //    a super type of the corresponding property of typeA.
-    for (String property : typeB.getOwnPropertyNames()) {
-      if (!typeA.hasProperty(property)) {
-        return false;
-      }
-      JSType propA = typeA.getPropertyType(property);
-      JSType propB = typeB.getPropertyType(property);
-      if (!propA.isSubtype(propB, implicitImplCache, subtypingMode)) {
-        return false;
-      }
+  private static boolean isSubtype(
+      ObjectType typeA,
+      RecordType typeB,
+      ImplCache implicitImplCache,
+      SubtypingMode subtypingMode) {
+
+    MatchStatus cached = implicitImplCache.checkCache(typeA, typeB);
+    if (cached != null) {
+      return cached.subtypeValue();
     }
-    return true;
+
+    boolean result =
+        isStructuralSubtypeHelper(
+            typeA, typeB, implicitImplCache, subtypingMode, ALL_PROPS_ARE_REQUIRED);
+    return implicitImplCache.updateCache(typeA, typeB, MatchStatus.valueOf(result));
   }
 
   /** Whether this is a built-in object. */

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

@@ -229,7 +229,7 @@ public boolean checkEquivalenceHelper(TemplateTypeMap that,
       boolean result =
           checkEquivalenceHelper(eqMethod, this, that, eqCache, subtypingMode)
               && checkEquivalenceHelper(eqMethod, that, this, eqCache, subtypingMode);
-      eqCache.updateCache(this, that, result ? MatchStatus.MATCH : MatchStatus.NOT_MATCH);
+      eqCache.updateCache(this, that, MatchStatus.valueOf(result));
       return result;
     } else {
       return status.subtypeValue();