Fix bug in DisambiguateProperties involving generic interfaces and in…

…terface inheritance. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=127994567
google · Jul 21, 2016 · 2cdc020 · 2cdc020
1 parent 338167e
commit 2cdc020
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 13 deletions.
diff --git a/src/com/google/javascript/rhino/jstype/PropertyMap.java b/src/com/google/javascript/rhino/jstype/PropertyMap.java
@@ -44,9 +44,11 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
-import java.util.HashSet;
-
+import com.google.common.collect.Sets;
 import java.io.Serializable;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.Map;
 import java.util.Set;
 
@@ -163,16 +165,29 @@ Set<String> getOwnPropertyNames() {
   }
 
   void collectPropertyNames(Set<String> props) {
+    Set<PropertyMap> identitySet = Sets.newIdentityHashSet();
+    collectPropertyNamesHelper(props, identitySet);
+  }
+
+  // The interface inheritance chain can have cycles.
+  // Use cache to avoid stack overflow.
+  private void collectPropertyNamesHelper(
+      Set<String> props, Set<PropertyMap> cache) {
+    if (cache.contains(this)) {
+      return;
+    }
+    cache.add(this);
     props.addAll(properties.keySet());
     PropertyMap primaryParent = getPrimaryParent();
     if (primaryParent != null) {
-      primaryParent.collectPropertyNames(props);
+      primaryParent.collectPropertyNamesHelper(props, cache);
     }
     for (PropertyMap p : getSecondaryParents()) {
       if (p != null) {
-        p.collectPropertyNames(props);
+        p.collectPropertyNamesHelper(props, cache);
       }
     }
+    cache.remove(this);
   }
 
   boolean removeProperty(String name) {

diff --git a/src/com/google/javascript/rhino/jstype/ProxyObjectType.java b/src/com/google/javascript/rhino/jstype/ProxyObjectType.java
@@ -44,7 +44,6 @@
 import com.google.javascript.rhino.ErrorReporter;
 import com.google.javascript.rhino.JSDocInfo;
 import com.google.javascript.rhino.Node;
-
 import java.util.Collections;
 
 /**
@@ -259,6 +258,13 @@ public Iterable<ObjectType> getCtorImplementedInterfaces() {
         referencedObjType.getCtorImplementedInterfaces();
   }
 
+  @Override
+  public Iterable<ObjectType> getCtorExtendedInterfaces() {
+    return this.referencedObjType == null
+        ? Collections.<ObjectType>emptyList()
+            : this.referencedObjType.getCtorExtendedInterfaces();
+  }
+
   @Override
   public int hashCode() {
     return referencedType.hashCode();

diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
@@ -23,7 +23,6 @@
 import com.google.javascript.rhino.jstype.JSTypeRegistry;
 import com.google.javascript.rhino.testing.BaseJSTypeTestCase;
 import com.google.javascript.rhino.testing.TestErrorReporter;
-
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
@@ -1830,6 +1829,70 @@ public void testStructuralInterfacesInExterns() {
     testSets(externs, js, js, "{}");
   }
 
+  public void testPropInParentInterface1() {
+    String js = LINE_JOINER.join(
+        "/** @interface */",
+        "function MyIterable() {}",
+        "MyIterable.prototype.iterator = function() {};",
+        "/**",
+        " * @interface",
+        " * @extends {MyIterable}",
+        " * @template T",
+        " */",
+        "function MyCollection() {}",
+        "/**",
+        " * @constructor",
+        " * @implements {MyCollection<?>}",
+        " */",
+        "function MyAbstractCollection() {}",
+        "/** @override */",
+        "MyAbstractCollection.prototype.iterator = function() {};");
+
+    testSets(js, "{iterator=[[MyAbstractCollection.prototype, MyIterable.prototype]]}");
+  }
+
+  public void testPropInParentInterface2() {
+    String js = LINE_JOINER.join(
+        "/** @interface */",
+        "function MyIterable() {}",
+        "MyIterable.prototype.iterator = function() {};",
+        "/**",
+        " * @interface",
+        " * @extends {MyIterable}",
+        " */",
+        "function MyCollection() {}",
+        "/**",
+        " * @constructor",
+        " * @implements {MyCollection<?>}",
+        " */",
+        "function MyAbstractCollection() {}",
+        "/** @override */",
+        "MyAbstractCollection.prototype.iterator = function() {};");
+
+    testSets(js, "{iterator=[[MyAbstractCollection.prototype, MyIterable.prototype]]}");
+  }
+
+  public void testPropInParentInterface3() {
+    String js = LINE_JOINER.join(
+        "/** @interface */",
+        "function MyIterable() {}",
+        "MyIterable.prototype.iterator = function() {};",
+        "/**",
+        " * @interface",
+        " * @extends {MyIterable}",
+        " */",
+        "function MyCollection() {}",
+        "/**",
+        " * @constructor",
+        " * @implements {MyCollection}",
+        " */",
+        "function MyAbstractCollection() {}",
+        "/** @override */",
+        "MyAbstractCollection.prototype.iterator = function() {};");
+
+    testSets(js, js, "{iterator=[[MyAbstractCollection.prototype, MyIterable.prototype]]}");
+  }
+
   public void testErrorOnProtectedProperty() {
     testError("function addSingletonGetter(foo) { foo.foobar = 'a'; };",
          DisambiguateProperties.Warnings.INVALIDATION);

diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java
@@ -31,7 +31,6 @@
 import com.google.javascript.rhino.jstype.JSType;
 import com.google.javascript.rhino.jstype.JSTypeNative;
 import com.google.javascript.rhino.jstype.ObjectType;
-
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
@@ -10146,17 +10145,13 @@ public void testInterfaceExtendsLoop() {
   }
 
   public void testInterfaceExtendsLoop2() {
-    testClosureTypesMultipleWarnings(
+    testClosureTypes(
         suppressMissingProperty("foo") +
             "/** @record \n * @extends {F} */var G = function() {};" +
             "/** @record \n * @extends {G} */var F = function() {};" +
             "/** @constructor \n * @implements {F} */var H = function() {};" +
         "alert((new H).foo);",
-        ImmutableList.of(
-            "extends loop involving F, "
-            + "loop: F -> G -> F",
-            "extends loop involving G, "
-            + "loop: G -> F -> G"));
+        "Parse error. Cycle detected in inheritance chain of type F");
   }
 
   public void testConversionFromInterfaceToRecursiveConstructor() {