Deal with cases when different visibility of overridden members could produce incorrect fake override.

(by Pavel Talanov and Andrey Breslav)

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

@@ -16,6 +16,8 @@
 
 package org.jetbrains.jet.lang.resolve;
 
+import com.google.common.base.Predicate;
+import com.google.common.collect.Collections2;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
@@ -175,83 +177,134 @@ public class OverrideResolver {
     }
     
     public static void generateOverridesInFunctionGroup(
-            @NotNull Name name,
+            @NotNull Name name, //DO NOT DELETE THIS PARAMETER: needed to make sure all descriptors have the same name
             @NotNull Collection<? extends CallableMemberDescriptor> membersFromSupertypes,
             @NotNull Collection<? extends CallableMemberDescriptor> membersFromCurrent,
             @NotNull ClassDescriptor current,
             @NotNull DescriptorSink sink
     ) {
         List<CallableMemberDescriptor> notOverridden = Lists.newArrayList(membersFromSupertypes);
+
         for (CallableMemberDescriptor fromCurrent : membersFromCurrent) {
-
+            extractAndBindOverridesForMember(fromCurrent, notOverridden, current, sink);
-            // Find what this descriptor overrides, bind them and removed from notOverridden
-            for (Iterator<CallableMemberDescriptor> iterator = notOverridden.iterator(); iterator.hasNext(); ) {
-                CallableMemberDescriptor fromSupertype = iterator.next();
-                OverridingUtil.OverrideCompatibilityInfo.Result result =
-                        OverridingUtil.isOverridableBy(fromSupertype, fromCurrent).getResult();
-
-                boolean isVisible = Visibilities.isVisible(fromSupertype, current);
-                switch (result) {
-                    case OVERRIDABLE:
-                        if (isVisible) {
-                            OverridingUtil.bindOverride(fromCurrent, fromSupertype);
-                        }
-                        iterator.remove();
-                        break;
-                    case CONFLICT:
-                        if (isVisible) {
-                            sink.conflict(fromSupertype, fromCurrent);
-                        }
-                        iterator.remove();
-                        break;
-                    case INCOMPATIBLE:
-                        break;
-                }
-            }
         }
 
+        bindFakeOverrides(current, notOverridden, sink);
+    }
+
+    private static void extractAndBindOverridesForMember(
+            @NotNull CallableMemberDescriptor fromCurrent,
+            @NotNull List<CallableMemberDescriptor> notOverridden, @NotNull ClassDescriptor current,
+            @NotNull DescriptorSink sink
+    ) {
+        for (Iterator<CallableMemberDescriptor> iterator = notOverridden.iterator(); iterator.hasNext(); ) {
+            CallableMemberDescriptor fromSupertype = iterator.next();
+            OverridingUtil.OverrideCompatibilityInfo.Result result =
+                    OverridingUtil.isOverridableBy(fromSupertype, fromCurrent).getResult();
+
+            boolean isVisible = Visibilities.isVisible(fromSupertype, current);
+            switch (result) {
+                case OVERRIDABLE:
+                    if (isVisible) {
+                        OverridingUtil.bindOverride(fromCurrent, fromSupertype);
+                    }
+                    iterator.remove();
+                    break;
+                case CONFLICT:
+                    if (isVisible) {
+                        sink.conflict(fromSupertype, fromCurrent);
+                    }
+                    iterator.remove();
+                    break;
+                case INCOMPATIBLE:
+                    break;
+            }
+        }
+    }
+
+    private static void bindFakeOverrides(
+            @NotNull ClassDescriptor current,
+            @NotNull List<CallableMemberDescriptor> notOverridden,
+            @NotNull DescriptorSink sink
+    ) {
         Queue<CallableMemberDescriptor> fromSuperQueue = new LinkedList<CallableMemberDescriptor>(notOverridden);
         while (!fromSuperQueue.isEmpty()) {
-            CallableMemberDescriptor aFromSuper = fromSuperQueue.remove();
+            CallableMemberDescriptor notOverriddenFromSuper = fromSuperQueue.remove();
-
+            Collection<CallableMemberDescriptor> overridables = extractMembersOverridableBy(notOverriddenFromSuper, fromSuperQueue, sink);
-            Collection<CallableMemberDescriptor> overridableByA = Lists.newArrayList();
+            bindFakeOverride(notOverriddenFromSuper, overridables, current, sink);
-            overridableByA.add(aFromSuper);
-            for (Iterator<CallableMemberDescriptor> iterator = fromSuperQueue.iterator(); iterator.hasNext(); ) {
-                CallableMemberDescriptor bFromSuper = iterator.next();
-                OverridingUtil.OverrideCompatibilityInfo.Result result =
-                        OverridingUtil.isOverridableBy(bFromSuper, aFromSuper).getResult();
-                switch (result) {
-                    case OVERRIDABLE:
-                        overridableByA.add(bFromSuper);
-                        iterator.remove();
-                        break;
-                    case CONFLICT:
-                        sink.conflict(aFromSuper, bFromSuper);
-                        iterator.remove();
-                        break;
-                    case INCOMPATIBLE:
-                        break;
-                }
-            }
-
-            boolean isVisible = true;
-            Modality modality = Modality.ABSTRACT;
-            for (CallableMemberDescriptor descriptor : overridableByA) {
-                isVisible &= Visibilities.isVisible(descriptor, current);
-                if (descriptor.getModality().compareTo(modality) < 0) {
-                    modality = descriptor.getModality();
-                }
-            }
-
-            CallableMemberDescriptor fakeOverride =
-                    aFromSuper.copy(current, modality, !isVisible, CallableMemberDescriptor.Kind.FAKE_OVERRIDE, false);
-            for (CallableMemberDescriptor descriptor : overridableByA) {
-                OverridingUtil.bindOverride(fakeOverride, descriptor);
-            }
-            sink.addToScope(fakeOverride);
         }
     }
 
+    private static void bindFakeOverride(
+            @NotNull CallableMemberDescriptor notOverriddenFromSuper,
+            @NotNull Collection<CallableMemberDescriptor> overridables,
+            @NotNull ClassDescriptor current,
+            @NotNull DescriptorSink sink
+    ) {
+        Collection<CallableMemberDescriptor> visibleOverridables = filterVisible(current, overridables);
+        Modality modality = getMinimalModality(visibleOverridables);
+        boolean allInvisible = visibleOverridables.isEmpty();
+        Collection<CallableMemberDescriptor> effectiveOverridden = allInvisible ? overridables : visibleOverridables;
+        CallableMemberDescriptor fakeOverride =
+                notOverriddenFromSuper.copy(current, modality, allInvisible, CallableMemberDescriptor.Kind.FAKE_OVERRIDE, false);
+        for (CallableMemberDescriptor descriptor : effectiveOverridden) {
+            OverridingUtil.bindOverride(fakeOverride, descriptor);
+        }
+        sink.addToScope(fakeOverride);
+    }
+
+    @NotNull
+    private static Modality getMinimalModality(@NotNull Collection<CallableMemberDescriptor> descriptors) {
+        Modality modality = Modality.ABSTRACT;
+        for (CallableMemberDescriptor descriptor : descriptors) {
+            if (descriptor.getModality().compareTo(modality) < 0) {
+                modality = descriptor.getModality();
+            }
+        }
+        return modality;
+    }
+
+    @NotNull
+    private static Collection<CallableMemberDescriptor> filterVisible(
+            @NotNull final ClassDescriptor current,
+            @NotNull Collection<CallableMemberDescriptor> toFilter
+    ) {
+        return Collections2.filter(toFilter, new Predicate<CallableMemberDescriptor>() {
+            @Override
+            public boolean apply(@Nullable CallableMemberDescriptor descriptor) {
+                return Visibilities.isVisible(descriptor, current);
+            }
+        });
+    }
+
+    @NotNull
+    private static Collection<CallableMemberDescriptor> extractMembersOverridableBy(
+            @NotNull CallableMemberDescriptor overrider,
+            @NotNull Queue<CallableMemberDescriptor> extractFrom,
+            @NotNull DescriptorSink sink
+    ) {
+        Collection<CallableMemberDescriptor> overridable = Lists.newArrayList();
+        overridable.add(overrider);
+        for (Iterator<CallableMemberDescriptor> iterator = extractFrom.iterator(); iterator.hasNext(); ) {
+            CallableMemberDescriptor candidate = iterator.next();
+            OverridingUtil.OverrideCompatibilityInfo.Result result =
+                    OverridingUtil.isOverridableBy(candidate, overrider).getResult();
+            switch (result) {
+                case OVERRIDABLE:
+                    overridable.add(candidate);
+                    iterator.remove();
+                    break;
+                case CONFLICT:
+                    sink.conflict(overrider, candidate);
+                    iterator.remove();
+                    break;
+                case INCOMPATIBLE:
+                    break;
+            }
+        }
+        return overridable;
+    }
+
     private static <T extends DeclarationDescriptor> MultiMap<Name, T> groupDescriptorsByName(Collection<T> properties) {
         MultiMap<Name, T> r = new LinkedMultiMap<Name, T>();
         for (T property : properties) {
@@ -336,30 +389,37 @@ public class OverrideResolver {
         }
     }
 
-    private static void collectMissingImplementations(CallableMemberDescriptor descriptor, Set<CallableMemberDescriptor> abstractNoImpl, Set<CallableMemberDescriptor> manyImpl) {
+    private static void collectMissingImplementations(
-        if (!descriptor.getKind().isReal()) {
+            @NotNull CallableMemberDescriptor descriptor,
-            Collection<CallableMemberDescriptor> overriddenDeclarations = OverridingUtil.getOverriddenDeclarations(descriptor);
+            @NotNull Set<CallableMemberDescriptor> abstractNoImpl,
-            if (overriddenDeclarations.size() == 0) {
+            @NotNull Set<CallableMemberDescriptor> manyImpl
-                throw new IllegalStateException("A 'fake override' must override something");
+    ) {
-            }
+        if (descriptor.getKind().isReal()) return;
-            else {
+
-                List<CallableMemberDescriptor> nonAbstractManyImpl = Lists.newArrayList();
+        Collection<CallableMemberDescriptor> overriddenDeclarations = OverridingUtil.getOverriddenDeclarations(descriptor);
-                Set<CallableMemberDescriptor> filteredOverriddenDeclarations = OverridingUtil.filterOverrides(Sets.newHashSet(overriddenDeclarations));
+        if (overriddenDeclarations.size() == 0) {
-                boolean allSuperAbstract = true;
+            throw new IllegalStateException("A 'fake override' must override something");
-                for (CallableMemberDescriptor overridden : filteredOverriddenDeclarations) {
+        }
-                    if (overridden.getModality() != Modality.ABSTRACT) {
+
-                        nonAbstractManyImpl.add(overridden);
+        List<CallableMemberDescriptor> nonAbstractManyImpl = Lists.newArrayList();
-                        allSuperAbstract = false;
+        Set<CallableMemberDescriptor> filteredOverriddenDeclarations = OverridingUtil.filterOverrides(Sets.newHashSet(overriddenDeclarations));
-                    }
+
-                }
+        boolean allSuperAbstract = true;
-                if (nonAbstractManyImpl.size() > 1) {
+        for (CallableMemberDescriptor overridden : filteredOverriddenDeclarations) {
-                    manyImpl.addAll(nonAbstractManyImpl);
+            if (overridden.getModality() != Modality.ABSTRACT) {
-                }
+                if (descriptor.getVisibility() != Visibilities.INVISIBLE_FAKE) {
-                else if (allSuperAbstract) {
+                    nonAbstractManyImpl.add(overridden);
-                    abstractNoImpl.addAll(overriddenDeclarations);
                 }
+                allSuperAbstract = false;
             }
         }
+        if (nonAbstractManyImpl.size() > 1) {
+            manyImpl.addAll(nonAbstractManyImpl);
+        }
+        else if (allSuperAbstract) {
+            abstractNoImpl.addAll(overriddenDeclarations);
+        }
+
     }
 
     public static Multimap<CallableMemberDescriptor, CallableMemberDescriptor> collectSuperMethods(MutableClassDescriptor classDescriptor) {
@@ -395,11 +455,12 @@ public class OverrideResolver {
     private void checkOverrideForMember(@NotNull CallableMemberDescriptor declared) {
         JetNamedDeclaration member = (JetNamedDeclaration) BindingContextUtils.descriptorToDeclaration(trace.getBindingContext(), declared);
         if (member == null) {
-            if (declared.getKind() != CallableMemberDescriptor.Kind.DELEGATION)
+            if (declared.getKind() != CallableMemberDescriptor.Kind.DELEGATION) {
                 throw new IllegalStateException(
                         "decriptor is not resolved to declaration" +
                         " and it is not delegate: " + declared + ", DELEGATED: " +
                         (declared.getKind() == CallableMemberDescriptor.Kind.DELEGATION));
+            }
             return;
         }
 

compiler/testData/diagnostics/tests/override/AllPrivateFromSuperTypes.kt

@@ -0,0 +1,14 @@
+package test
+
+trait A {
+    private val a: String
+      get() = "AAAA!"
+}
+
+open class C {
+    private val a: String = ""
+}
+
+class Subject : C(), A {
+    val c = <!INVISIBLE_MEMBER!>a<!>
+}

compiler/testData/diagnostics/tests/override/AllProtectedFromSupertypes.kt

@@ -0,0 +1,13 @@
+package test
+
+trait A {
+    protected val a: String
+}
+
+open class C {
+    protected val a: String = ""
+}
+
+class Subject : C(), A {
+    val c = a
+}

compiler/testData/diagnostics/tests/override/ProtectedAndPrivateFromSupertypes.kt

@@ -0,0 +1,17 @@
+package test
+
+trait A {
+    protected val a: String
+}
+
+trait B {
+    protected val a: String
+}
+
+open class C {
+    private val a: String = ""
+}
+
+class <!ABSTRACT_MEMBER_NOT_IMPLEMENTED!>Subject<!> : C(), A, B {
+    val c = a
+}

compiler/tests/org/jetbrains/jet/checkers/JetDiagnosticsTestGenerated.java

@@ -15,15 +15,12 @@
  */
 package org.jetbrains.jet.checkers;
 
-import junit.framework.Assert;
 import junit.framework.Test;
 import junit.framework.TestSuite;
-
-import java.io.File;
 import org.jetbrains.jet.JetTestUtils;
 import org.jetbrains.jet.test.TestMetadata;
 
-import org.jetbrains.jet.checkers.AbstractDiagnosticsTestWithEagerResolve;
+import java.io.File;
 
 /** This class is generated by {@link org.jetbrains.jet.checkers.AbstractDiagnosticsTestWithEagerResolve}. DO NOT MODIFY MANUALLY */
 public class JetDiagnosticsTestGenerated extends AbstractDiagnosticsTestWithEagerResolve {
@@ -1646,6 +1643,16 @@ public class JetDiagnosticsTestGenerated extends AbstractDiagnosticsTestWithEage
                 JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), "org.jetbrains.jet.checkers.AbstractDiagnosticsTestWithEagerResolve", new File("compiler/testData/diagnostics/tests/override"), "kt", false);
             }
             
+            @TestMetadata("AllPrivateFromSuperTypes.kt")
+            public void testAllPrivateFromSuperTypes() throws Exception {
+                doTest("compiler/testData/diagnostics/tests/override/AllPrivateFromSuperTypes.kt");
+            }
+            
+            @TestMetadata("AllProtectedFromSupertypes.kt")
+            public void testAllProtectedFromSupertypes() throws Exception {
+                doTest("compiler/testData/diagnostics/tests/override/AllProtectedFromSupertypes.kt");
+            }
+            
             @TestMetadata("ComplexValRedeclaration.kt")
             public void testComplexValRedeclaration() throws Exception {
                 doTest("compiler/testData/diagnostics/tests/override/ComplexValRedeclaration.kt");
@@ -1751,6 +1758,11 @@ public class JetDiagnosticsTestGenerated extends AbstractDiagnosticsTestWithEage
                 doTest("compiler/testData/diagnostics/tests/override/ParameterDefaultValues-DefaultValueFromOnlyOneSupertype.kt");
             }
             
+            @TestMetadata("ProtectedAndPrivateFromSupertypes.kt")
+            public void testProtectedAndPrivateFromSupertypes() throws Exception {
+                doTest("compiler/testData/diagnostics/tests/override/ProtectedAndPrivateFromSupertypes.kt");
+            }
+            
             @TestMetadata("SuspiciousCase1.kt")
             public void testSuspiciousCase1() throws Exception {
                 doTest("compiler/testData/diagnostics/tests/override/SuspiciousCase1.kt");