CYCLIC_INHERITANCE_HIERARCHY reworked

We do not try to recover too gracefully from a cyclic hierarchy any more:
we simply remove all the edges that belong to a cycle instead of intelligently finding one most convenient edge to cut.
This is done in both lazy and eager resolve to keep tests passing.

compiler/frontend/src/org/jetbrains/jet/lang/resolve/TypeHierarchyResolver.java

@@ -33,6 +33,7 @@ import org.jetbrains.jet.lang.resolve.scopes.JetScope;
 import org.jetbrains.jet.lang.resolve.scopes.WritableScope;
 import org.jetbrains.jet.lang.resolve.scopes.WriteThroughScope;
 import org.jetbrains.jet.lang.types.JetType;
+import org.jetbrains.jet.lang.types.TypeConstructor;
 import org.jetbrains.jet.lang.types.lang.KotlinBuiltIns;
 import org.jetbrains.jet.utils.DFS;
 
@@ -247,88 +248,75 @@ public class TypeHierarchyResolver {
 
     private void detectAndDisconnectLoops(@NotNull TopDownAnalysisContext c) {
         // Loop detection and disconnection
-        Set<ClassDescriptor> visited = Sets.newHashSet();
+        List<Runnable> tasks = new ArrayList<Runnable>();
-        Set<ClassDescriptor> beingProcessed = Sets.newHashSet();
+        for (final MutableClassDescriptorLite klass : c.getClassesTopologicalOrder()) {
-        List<ClassDescriptor> currentPath = Lists.newArrayList();
+            for (final JetType supertype : klass.getSupertypes()) {
-        for (MutableClassDescriptorLite klass : c.getClassesTopologicalOrder()) {
+                ClassifierDescriptor supertypeDescriptor = supertype.getConstructor().getDeclarationDescriptor();
-            traverseTypeHierarchy(klass, visited, beingProcessed, currentPath);
+                if (supertypeDescriptor instanceof MutableClassDescriptorLite) {
+                    MutableClassDescriptorLite superclass = (MutableClassDescriptorLite) supertypeDescriptor;
+                    if (isReachable(superclass, klass, new HashSet<ClassDescriptor>())) {
+                        tasks.add(new Runnable() {
+                            @Override
+                            public void run() {
+                                klass.getSupertypes().remove(supertype);
+                            }
+                        });
+                        reportCyclicInheritanceHierarchyError(trace, klass, superclass);
+                    }
+                }
+            }
+        }
+
+        for (Runnable task : tasks) {
+            task.run();
         }
     }
 
-    private void traverseTypeHierarchy(
+    // Temporary. Duplicates logic from LazyClassTypeConstructor.isReachable
-            MutableClassDescriptorLite currentClass,
+    private static boolean isReachable(MutableClassDescriptorLite from, MutableClassDescriptorLite to, Set<ClassDescriptor> visited) {
-            Set<ClassDescriptor> visited,
+        if (!visited.add(from)) return false;
-            Set<ClassDescriptor> beingProcessed,
+        for (JetType supertype : from.getSupertypes()) {
-            List<ClassDescriptor> currentPath
+            TypeConstructor supertypeConstructor = supertype.getConstructor();
+            if (supertypeConstructor.getDeclarationDescriptor() == to) {
+                return true;
+            }
+            ClassifierDescriptor superclass = supertypeConstructor.getDeclarationDescriptor();
+            if (superclass instanceof MutableClassDescriptorLite && isReachable((MutableClassDescriptorLite) superclass, to, visited)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    public static void reportCyclicInheritanceHierarchyError(
+            @NotNull BindingTrace trace,
+            @NotNull ClassDescriptor classDescriptor,
+            @NotNull ClassDescriptor superclass
     ) {
-        if (!visited.add(currentClass)) {
+        PsiElement psiElement = BindingContextUtils.classDescriptorToDeclaration(trace.getBindingContext(), classDescriptor);
-            if (beingProcessed.contains(currentClass)) {
-                markCycleErrors(currentPath, currentClass);
-                assert !currentPath.isEmpty() : "Cycle cannot be found on an empty currentPath";
-                ClassDescriptor subclassOfCurrent = currentPath.get(currentPath.size() - 1);
-                assert subclassOfCurrent instanceof MutableClassDescriptor;
-                // Disconnect the loop
-                for (Iterator<JetType> iterator = ((MutableClassDescriptor) subclassOfCurrent).getSupertypes().iterator();
-                     iterator.hasNext(); ) {
-                    JetType type = iterator.next();
-                    if (type.getConstructor() == currentClass.getTypeConstructor()) {
-                        iterator.remove();
-                        break;
-                    }
-                }
-            }
-            return;
-        }
 
-        beingProcessed.add(currentClass);
+        PsiElement elementToMark = null;
-        currentPath.add(currentClass);
+        if (psiElement instanceof JetClassOrObject) {
-        for (JetType supertype : Lists.newArrayList(currentClass.getSupertypes())) {
+            JetClassOrObject classOrObject = (JetClassOrObject) psiElement;
-            DeclarationDescriptor declarationDescriptor = supertype.getConstructor().getDeclarationDescriptor();
+            for (JetDelegationSpecifier delegationSpecifier : classOrObject.getDelegationSpecifiers()) {
-            if (declarationDescriptor instanceof MutableClassDescriptor) {
+                JetTypeReference typeReference = delegationSpecifier.getTypeReference();
-                MutableClassDescriptor mutableClassDescriptor = (MutableClassDescriptor) declarationDescriptor;
+                if (typeReference == null) continue;
-                traverseTypeHierarchy(mutableClassDescriptor, visited, beingProcessed, currentPath);
+                JetType supertype = trace.get(TYPE, typeReference);
+                if (supertype != null && supertype.getConstructor() == superclass.getTypeConstructor()) {
+                    elementToMark = typeReference;
+                }
             }
         }
-        beingProcessed.remove(currentClass);
+        if (elementToMark == null && psiElement instanceof PsiNameIdentifierOwner) {
-        currentPath.remove(currentPath.size() - 1);
+            PsiNameIdentifierOwner namedElement = (PsiNameIdentifierOwner) psiElement;
-    }
+            PsiElement nameIdentifier = namedElement.getNameIdentifier();
-
+            if (nameIdentifier != null) {
-    private void markCycleErrors(List<ClassDescriptor> currentPath, @NotNull ClassDescriptor current) {
+                elementToMark = nameIdentifier;
-        int size = currentPath.size();
-        for (int i = size - 1; i >= 0; i--) {
-            ClassDescriptor classDescriptor = currentPath.get(i);
-
-            ClassDescriptor superclass = (i < size - 1) ? currentPath.get(i + 1) : current;
-            PsiElement psiElement = BindingContextUtils.classDescriptorToDeclaration(trace.getBindingContext(), classDescriptor);
-
-            PsiElement elementToMark = null;
-            if (psiElement instanceof JetClassOrObject) {
-                JetClassOrObject classOrObject = (JetClassOrObject) psiElement;
-                for (JetDelegationSpecifier delegationSpecifier : classOrObject.getDelegationSpecifiers()) {
-                    JetTypeReference typeReference = delegationSpecifier.getTypeReference();
-                    if (typeReference == null) continue;
-                    JetType supertype = trace.get(TYPE, typeReference);
-                    if (supertype != null && supertype.getConstructor() == superclass.getTypeConstructor()) {
-                        elementToMark = typeReference;
-                    }
-                }
-            }
-            if (elementToMark == null && psiElement instanceof PsiNameIdentifierOwner) {
-                PsiNameIdentifierOwner namedElement = (PsiNameIdentifierOwner) psiElement;
-                PsiElement nameIdentifier = namedElement.getNameIdentifier();
-                if (nameIdentifier != null) {
-                    elementToMark = nameIdentifier;
-                }
-            }
-            if (elementToMark != null) {
-                trace.report(CYCLIC_INHERITANCE_HIERARCHY.on(elementToMark));
-            }
-
-            if (classDescriptor == current) {
-                // Beginning of cycle is found
-                break;
             }
         }
+        if (elementToMark != null) {
+            trace.report(CYCLIC_INHERITANCE_HIERARCHY.on(elementToMark));
+        }
     }
 
     private void checkTypesInClassHeaders(@NotNull TopDownAnalysisContext c) {

compiler/frontend/src/org/jetbrains/jet/lang/resolve/lazy/descriptors/LazyClassDescriptor.java

@@ -24,6 +24,7 @@ import kotlin.Function0;
 import kotlin.Function1;
 import kotlin.Unit;
 import kotlin.KotlinPackage;
+import org.jetbrains.annotations.Mutable;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.jet.lang.descriptors.*;
@@ -33,6 +34,7 @@ import org.jetbrains.jet.lang.psi.*;
 import org.jetbrains.jet.lang.resolve.AnnotationResolver;
 import org.jetbrains.jet.lang.resolve.BindingContext;
 import org.jetbrains.jet.lang.resolve.DescriptorUtils;
+import org.jetbrains.jet.lang.resolve.TypeHierarchyResolver;
 import org.jetbrains.jet.lang.resolve.lazy.ForceResolveUtil;
 import org.jetbrains.jet.lang.resolve.lazy.LazyEntity;
 import org.jetbrains.jet.lang.resolve.lazy.ResolveSession;
@@ -383,44 +385,65 @@ public class LazyClassDescriptor extends ClassDescriptorBase implements LazyEnti
         getVisibility();
     }
 
+    private static class Supertypes {
+        @Mutable
+        public final Collection<JetType> trueSupertypes;
+        @Mutable
+        public final Collection<JetType> cyclicSupertypes;
+
+        private Supertypes(@Mutable @NotNull Collection<JetType> trueSupertypes) {
+            this(trueSupertypes, new ArrayList<JetType>(0));
+        }
+
+        private Supertypes(@Mutable @NotNull Collection<JetType> trueSupertypes, @Mutable @NotNull Collection<JetType> cyclicSupertypes) {
+            this.trueSupertypes = trueSupertypes;
+            this.cyclicSupertypes = cyclicSupertypes;
+        }
+
+        @NotNull
+        public Collection<JetType> getAllSupertypes() {
+            return KotlinPackage.plus(trueSupertypes, cyclicSupertypes);
+        }
+    }
+
     private class LazyClassTypeConstructor implements LazyEntity, TypeConstructor {
-        private final NotNullLazyValue<Collection<JetType>> supertypes = resolveSession.getStorageManager().createLazyValueWithPostCompute(
+        private final NotNullLazyValue<Supertypes> supertypes = resolveSession.getStorageManager().createLazyValueWithPostCompute(
-                new Function0<Collection<JetType>>() {
+                new Function0<Supertypes>() {
                     @Override
-                    public Collection<JetType> invoke() {
+                    public Supertypes invoke() {
                         if (KotlinBuiltIns.isSpecialClassWithNoSupertypes(LazyClassDescriptor.this)) {
-                            return Collections.emptyList();
+                            return new Supertypes(Collections.<JetType>emptyList());
                         }
 
                         JetClassLikeInfo info = declarationProvider.getOwnerInfo();
                         if (info instanceof SyntheticClassObjectInfo) {
                             LazyClassDescriptor descriptor = ((SyntheticClassObjectInfo) info).getClassDescriptor();
                             if (descriptor.getKind().isSingleton()) {
-                                return Collections.singleton(descriptor.getDefaultType());
+                                return new Supertypes(Collections.singleton(descriptor.getDefaultType()));
                             }
                         }
 
                         JetClassOrObject classOrObject = info.getCorrespondingClassOrObject();
                         if (classOrObject == null) {
-                            return Collections.singleton(KotlinBuiltIns.getInstance().getAnyType());
+                            return new Supertypes(Collections.singleton(KotlinBuiltIns.getInstance().getAnyType()));
                         }
 
                         List<JetType> allSupertypes = resolveSession.getDescriptorResolver()
                                 .resolveSupertypes(getScopeForClassHeaderResolution(), LazyClassDescriptor.this, classOrObject,
                                                    resolveSession.getTrace());
 
-                        return Lists.newArrayList(Collections2.filter(allSupertypes, VALID_SUPERTYPE));
+                        return new Supertypes(Lists.newArrayList(Collections2.filter(allSupertypes, VALID_SUPERTYPE)));
                     }
                 },
-                new Function1<Boolean, Collection<JetType>>() {
+                new Function1<Boolean, Supertypes>() {
                     @Override
-                    public Collection<JetType> invoke(Boolean firstTime) {
+                    public Supertypes invoke(Boolean firstTime) {
-                        return Collections.emptyList();
+                        return new Supertypes(Collections.<JetType>emptyList());
                     }
                 },
-                new Function1<Collection<JetType>, Unit>() {
+                new Function1<Supertypes, Unit>() {
                     @Override
-                    public Unit invoke(@NotNull Collection<JetType> supertypes) {
+                    public Unit invoke(@NotNull Supertypes supertypes) {
                         findAndDisconnectLoopsInTypeHierarchy(supertypes);
                         return Unit.VALUE;
                     }
@@ -460,21 +483,32 @@ public class LazyClassDescriptor extends ClassDescriptorBase implements LazyEnti
         @NotNull
         @Override
         public Collection<JetType> getSupertypes() {
-            return supertypes.invoke();
+            return supertypes.invoke().trueSupertypes;
         }
 
-        private void findAndDisconnectLoopsInTypeHierarchy(Collection<JetType> supertypes) {
+        private void findAndDisconnectLoopsInTypeHierarchy(Supertypes supertypes) {
-            for (Iterator<JetType> iterator = supertypes.iterator(); iterator.hasNext(); ) {
+            for (Iterator<JetType> iterator = supertypes.trueSupertypes.iterator(); iterator.hasNext(); ) {
                 JetType supertype = iterator.next();
                 if (isReachable(supertype.getConstructor(), this, new HashSet<TypeConstructor>())) {
                     iterator.remove();
+                    supertypes.cyclicSupertypes.add(supertype);
+
+                    ClassifierDescriptor supertypeDescriptor = supertype.getConstructor().getDeclarationDescriptor();
+                    if (supertypeDescriptor instanceof ClassDescriptor) {
+                        ClassDescriptor superclass = (ClassDescriptor) supertypeDescriptor;
+                        TypeHierarchyResolver.reportCyclicInheritanceHierarchyError(resolveSession.getTrace(), LazyClassDescriptor.this,
+                                                                                    superclass);
+                    }
                 }
             }
         }
 
         private boolean isReachable(TypeConstructor from, TypeConstructor to, Set<TypeConstructor> visited) {
             if (!visited.add(from)) return false;
-            for (JetType supertype : from.getSupertypes()) {
+            Collection<JetType> supertypes = from instanceof LazyClassTypeConstructor
+                                             ? ((LazyClassTypeConstructor) from).supertypes.invoke().getAllSupertypes()
+                                             : from.getSupertypes();
+            for (JetType supertype : supertypes) {
                 TypeConstructor supertypeConstructor = supertype.getConstructor();
                 if (supertypeConstructor == to) {
                     return true;

compiler/testData/diagnostics/tests/CyclicHierarchy.kt

@@ -2,10 +2,10 @@ trait A {
     fun foo() {}
 }
 trait B : A, <!CYCLIC_INHERITANCE_HIERARCHY!>E<!> {}
-trait C : B {}
+trait C : <!CYCLIC_INHERITANCE_HIERARCHY!>B<!> {}
 trait D : <!CYCLIC_INHERITANCE_HIERARCHY!>B<!> {}
 trait E : <!CYCLIC_INHERITANCE_HIERARCHY!>F<!> {}
-trait F : <!CYCLIC_INHERITANCE_HIERARCHY!>D<!>, C {}
+trait F : <!CYCLIC_INHERITANCE_HIERARCHY!>D<!>, <!CYCLIC_INHERITANCE_HIERARCHY!>C<!> {}
 trait G : F {}
 trait H : F {}
 
@@ -21,10 +21,10 @@ val h : H? = null
 fun test() {
     a?.foo()
     b?.foo()
-    c?.foo()
+    c?.<!UNRESOLVED_REFERENCE!>foo<!>()
-    d?.foo()
+    d?.<!UNRESOLVED_REFERENCE!>foo<!>()
     e?.<!UNRESOLVED_REFERENCE!>foo<!>()
-    f?.foo()
+    f?.<!UNRESOLVED_REFERENCE!>foo<!>()
-    g?.foo()
+    g?.<!UNRESOLVED_REFERENCE!>foo<!>()
-    h?.foo()
+    h?.<!UNRESOLVED_REFERENCE!>foo<!>()
 }

compiler/testData/diagnostics/tests/cast/IsRecursionSustainable.kt

@@ -3,4 +3,4 @@ open class RecB<T>: <!CYCLIC_INHERITANCE_HIERARCHY!>RecA<T><!>()
 open class SelfR<T>: <!CYCLIC_INHERITANCE_HIERARCHY!>SelfR<T><!>()
 
 fun test(f: SelfR<String>) = f is <!CANNOT_CHECK_FOR_ERASED!>RecA<String><!>
-fun test(f: RecB<String>) = f is RecA<String>
+fun test(f: RecB<String>) = f is <!CANNOT_CHECK_FOR_ERASED!>RecA<String><!>

compiler/testData/lazyResolve/recursiveComparator/CyclicHierarchy.kt

@@ -0,0 +1,10 @@
+package test
+
+trait A
+trait B : A, E
+trait C : B
+trait D : B
+trait E : F
+trait F : D, C
+trait G : F
+trait H : F

compiler/testData/lazyResolve/recursiveComparator/CyclicHierarchy.txt

@@ -0,0 +1,25 @@
+package test
+
+internal trait A {
+}
+
+internal trait B : test.A {
+}
+
+internal trait C {
+}
+
+internal trait D {
+}
+
+internal trait E {
+}
+
+internal trait F {
+}
+
+internal trait G : test.F {
+}
+
+internal trait H : test.F {
+}

compiler/tests/org/jetbrains/jet/lang/resolve/lazy/LazyResolveRecursiveComparingTestGenerated.java

@@ -2515,6 +2515,11 @@ public class LazyResolveRecursiveComparingTestGenerated extends AbstractLazyReso
             doTestCheckingPrimaryConstructors("compiler/testData/lazyResolve/recursiveComparator/classObjectHeader.kt");
         }
         
+        @TestMetadata("CyclicHierarchy.kt")
+        public void testCyclicHierarchy() throws Exception {
+            doTestCheckingPrimaryConstructors("compiler/testData/lazyResolve/recursiveComparator/CyclicHierarchy.kt");
+        }
+        
         @TestMetadata("enum.kt")
         public void testEnum() throws Exception {
             doTestCheckingPrimaryConstructors("compiler/testData/lazyResolve/recursiveComparator/enum.kt");

idea/testData/checker/CyclicHierarchy.kt

@@ -2,10 +2,10 @@ trait A {
     fun foo() {}
 }
 trait B : A, <error>E</error> {}
-trait C : B {}
+trait C : <error>B</error> {}
 trait D : <error>B</error> {}
 trait E : <error>F</error> {}
-trait F : <error>D</error>, C {}
+trait F : <error>D</error>, <error>C</error> {}
 trait G : F {}
 trait H : F {}
 
@@ -21,10 +21,10 @@ val h : H? = null
 fun test() {
     a?.foo()
     b?.foo()
-    c?.foo()
+    c?.<error>foo</error>()
-    d?.foo()
+    d?.<error>foo</error>()
     e?.<error>foo</error>()
-    f?.foo()
+    f?.<error>foo</error>()
-    g?.foo()
+    g?.<error>foo</error>()
-    h?.foo()
+    h?.<error>foo</error>()
 }