Rollback "Changes the collection used to track subtypes..."

Also fix the underlying issue in a simpler way.

Some digging reveals that the only source of duplication is PrototypeObjectType re-adding extended superclasses (as opposed to extended interfaces), which is only necessary to pick up some corner cases of externs whose superclasses were not added the first time around.

Instead of a complicated new set implementation, we simply keep track of whether the subclass was added to its superclass the first time around, and skip adding it again if so.

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

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

@@ -42,20 +42,16 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.javascript.rhino.jstype.JSTypeNative.OBJECT_TYPE;
 import static com.google.javascript.rhino.jstype.JSTypeNative.U2U_CONSTRUCTOR_TYPE;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.javascript.rhino.CyclicSerializableLinkedHashSet;
 import com.google.javascript.rhino.ErrorReporter;
 import com.google.javascript.rhino.Node;
 import com.google.javascript.rhino.Token;
-import com.google.javascript.rhino.jstype.JSType.EqCache;
-import com.google.javascript.rhino.jstype.JSType.SubtypingMode;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -67,7 +63,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import javax.annotation.Nullable;
 
 /**
  * This derived type provides extended information about a function, including its return type and
@@ -149,13 +144,18 @@ enum ConstructorAmbiguity {
   private ImmutableList<ObjectType> extendedInterfaces = ImmutableList.of();
 
   /**
-   * For the instance type of this ctor, the ctor types of all known subtypes of that instance type.
-   *
-   * <p>This field is only applicable to ctor functions.
+   * The types which are subtypes of this function. It is lazily initialized and only relevant for
+   * constructors. In all other cases it is {@code null}.
+   */
+  private List<FunctionType> subTypes = null;
+
+  /**
+   * Whether this constructor was added to its superclass constructor's subtypes list, to avoid a
+   * limited amount of duplication that can happen from unresolved supertypes. This only tracks
+   * classes extending classes (no interfaces), since there is no way to duplicate interfaces via
+   * methods accessible outside this class.
    */
-  @Nullable
-  // @MonotonicNonNull
-  private CyclicSerializableLinkedHashSet<FunctionType> knownSubtypeCtors;
+  private boolean wasAddedToExtendedConstructorSubtypes = false;
 
   /** Creates an instance for a function that might be a constructor. */
   FunctionType(
@@ -507,13 +507,14 @@ private boolean setPrototypeNoCheck(ObjectType prototype, Node propertyNode) {
     if (isConstructor() || isInterface()) {
       FunctionType superClass = getSuperClassConstructor();
       if (superClass != null) {
-        superClass.addSubTypeIfNotPresent(this);
+        superClass.addSubType(this);
+        wasAddedToExtendedConstructorSubtypes = true;
       }
 
       if (isInterface()) {
         for (ObjectType interfaceType : getExtendedInterfaces()) {
           if (interfaceType.getConstructor() != null) {
-            interfaceType.getConstructor().addSubTypeIfNotPresent(this);
+            interfaceType.getConstructor().addSubType(this);
           }
         }
       }
@@ -1132,15 +1133,28 @@ public final void setSource(Node source) {
     this.source = source;
   }
 
-  /** Adds a type to the set of known subtype ctors for this type. */
-  final void addSubTypeIfNotPresent(FunctionType subtype) {
-    checkState(isConstructor() || isInterface());
-
-    if (knownSubtypeCtors == null) {
-      knownSubtypeCtors = new CyclicSerializableLinkedHashSet<>();
+  /** Adds a type to the list of subtypes for this type. */
+  private void addSubType(FunctionType subType) {
+    if (subTypes == null) {
+      subTypes = new ArrayList<>();
     }
+    subTypes.add(subType);
+  }
 
-    knownSubtypeCtors.add(subtype);
+  /**
+   * Restricted package-accessible version of {@link #addSubType}, which ensures subtypes are not
+   * duplicated. Generally subtypes are added internally and are guaranteed not to be duplicated,
+   * but this has the possibility of missing unresolved supertypes (typically from externs). To
+   * handle that case, {@link PrototypeObjectType} also adds subclasses after resolution. This
+   * method only adds a subclass to the list if it didn't already add itself to its superclass in
+   * the earlier pass. Ideally, "subclass" here would only refer to classes, but there's an edge
+   * case where interfaces have the {@code Object} constructor added as its "superclass".
+   */
+  final void addSubClassAfterResolution(FunctionType subClass) {
+    checkArgument(this == subClass.getSuperClassConstructor());
+    if (!subClass.wasAddedToExtendedConstructorSubtypes) {
+      addSubType(subClass);
+    }
   }
 
   // NOTE(sdh): One might assume that immediately after calling this, hasCachedValues() should
@@ -1150,8 +1164,8 @@ final void addSubTypeIfNotPresent(FunctionType subtype) {
   public final void clearCachedValues() {
     super.clearCachedValues();
 
-    if (knownSubtypeCtors != null) {
-      for (FunctionType subType : knownSubtypeCtors) {
+    if (subTypes != null) {
+      for (FunctionType subType : subTypes) {
         subType.clearCachedValues();
       }
     }
@@ -1169,7 +1183,7 @@ public final void clearCachedValues() {
 
   public final Iterable<FunctionType> getDirectSubTypes() {
     return Iterables.concat(
-        knownSubtypeCtors != null ? knownSubtypeCtors : ImmutableList.of(),
+        subTypes != null ? subTypes : ImmutableList.<FunctionType>of(),
         this.registry.getDirectImplementors(this));
   }
 
@@ -1215,8 +1229,11 @@ JSType resolveInternal(ErrorReporter reporter) {
       extendedInterfaces = resolvedExtended;
     }
 
-    if (knownSubtypeCtors != null) {
-      resolveKnownSubtypeCtors(reporter);
+    if (subTypes != null) {
+      for (int i = 0; i < subTypes.size(); i++) {
+        FunctionType subType = subTypes.get(i);
+        subTypes.set(i, JSType.toMaybeFunctionType(subType.resolve(reporter)));
+      }
     }
 
     return super.resolveInternal(reporter);
@@ -1247,30 +1264,6 @@ private ImmutableList<ObjectType> resolveTypeListHelper(
     return changed ? resolvedList.build() : null;
   }
 
-  private void resolveKnownSubtypeCtors(ErrorReporter reporter) {
-    // We want to resolve all of the known subtypes-ctors so we can store those resolved types
-    // instead.
-    ImmutableList<FunctionType> setCopy;
-    do {
-      setCopy =
-          // However, resolving a type may cause more subtype-ctors to be registered. To avoid a
-          // `ConcurrentModificationException` we operate on a copy of the original set. We leave
-          // the original set in place in case resolution would re-add a previously known type.
-          ImmutableList.copyOf(knownSubtypeCtors).stream()
-              .map((t) -> JSType.toMaybeFunctionType(t.resolve(reporter)))
-              .collect(toImmutableList());
-
-      // We do this iteratively until resolving adds no more subtypes.
-    } while (setCopy.size() != knownSubtypeCtors.size());
-
-    // Additionally, resolving a type may change its `hashCode`, so we have to rebuild the set of
-    // subtype-ctors.
-    knownSubtypeCtors = null; // Reset
-    for (FunctionType subtypeCtor : setCopy) {
-      addSubTypeIfNotPresent(subtypeCtor);
-    }
-  }
-
   @Override
   public final String toDebugHashCodeString() {
     if (this == registry.getNativeType(JSTypeNative.FUNCTION_INSTANCE_TYPE)) {

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

@@ -493,7 +493,7 @@ JSType resolveInternal(ErrorReporter reporter) {
         if (superCtor != null) {
           // If the super ctor of this prototype object was not known before resolution, then the
           // subTypes would not have been set. Update them.
-          superCtor.addSubTypeIfNotPresent(ctor);
+          superCtor.addSubClassAfterResolution(ctor);
         }
       }
     }