JVM_IR more precise superclass stub filtering

Filter out abstract members of superclasses when generating collection
stubs.

KT-43207

compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/CollectionStubMethodLowering.kt

@@ -66,7 +66,7 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
         for (stub in methodStubsToGenerate) {
             val stubNameAndArity = stub.nameAndArity
             val relevantMembers = nonAbstractMethodsByNameAndArity[stubNameAndArity].orEmpty()
-            val existingOverrides = relevantMembers.filter { isOverriddenBy(stub, it) }
+            val existingOverrides = relevantMembers.filter { isEffectivelyOverriddenBy(stub, it) }
 
             if (existingOverrides.isNotEmpty()) {
                 // In the case that we find a defined method that matches the stub signature,
@@ -78,7 +78,8 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
             }
 
             // Generated stub might still override some abstract member(s), which affects resulting method signature.
-            val overriddenAbstractMethods = abstractMethodsByNameAndArity[stubNameAndArity].orEmpty().filter { isOverriddenBy(it, stub) }
+            val overriddenAbstractMethods = abstractMethodsByNameAndArity[stubNameAndArity].orEmpty()
+                .filter { isEffectivelyOverriddenBy(it, stub) }
             stub.overriddenSymbols += overriddenAbstractMethods.map { it.symbol }
 
             // Some stub members require special handling.
@@ -121,8 +122,6 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
                 else ->
                     irClass.declarations.add(stub)
             }
-
-
         }
     }
 
@@ -196,7 +195,7 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
                 }
         }
 
-    private fun isOverriddenBy(superFun: IrSimpleFunction, overridingFun: IrSimpleFunction): Boolean {
+    private fun isEffectivelyOverriddenBy(superFun: IrSimpleFunction, overridingFun: IrSimpleFunction): Boolean {
         // Function 'f0' is overridden by function 'f1' if all of the following conditions are met,
         // assuming type parameter Ti of 'f1' is "equal" to type parameter Si of 'f0':
         //  - names are same;
@@ -301,7 +300,17 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
 
     // Compute stubs that should be generated, compare based on signature
     private fun generateRelevantStubMethods(irClass: IrClass): List<IrSimpleFunction> {
-        fun createStubFuns(stubs: StubsForCollectionClass): List<IrSimpleFunction> {
+        val classStubFuns = collectionStubComputer.stubsForCollectionClasses(irClass)
+            .flatMap { createStubFuns(irClass, it) }
+
+        val superClassStubSignatures = computeStubsForSuperClasses(irClass)
+            .flatMap { createStubFuns(irClass, it) }
+            .mapTo(HashSet()) { it.toJvmSignature() }
+
+        return classStubFuns.filter { it.toJvmSignature() !in superClassStubSignatures }
+    }
+
+    private fun createStubFuns(irClass: IrClass, stubs: StubsForCollectionClass): List<IrSimpleFunction> {
         val (readOnlyClass, mutableClass, candidatesForStubs) = stubs
         val substitutionMap = computeSubstitutionMap(readOnlyClass.owner, mutableClass.owner, irClass)
         return candidatesForStubs.map { function ->
@@ -309,24 +318,49 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
         }
     }
 
-        val classStubs = collectionStubComputer.stubsForCollectionClasses(irClass)
+    private fun computeStubsForSuperClasses(irClass: IrClass): List<StubsForCollectionClass> {
-
+        val immediateSuperClass = irClass.superClass ?: return emptyList()
-        val superClassesStubs = irClass.superClass?.run {
+        return immediateSuperClass.superClassChain
-            superClassChain.flatMap { superClass ->
+            .flatMap { superClass -> computeStubsForSuperClass(superClass) }
-                collectionStubComputer.stubsForCollectionClasses(superClass)
+            .toList()
-            }.toList()
-        } ?: emptyList()
-
-        val relevantStubs =
-            classStubs.filter { (readOnlyClass) ->
-                classStubs.none { readOnlyClass != it.readOnlyClass && it.readOnlyClass.isSubtypeOfClass(readOnlyClass) } &&
-                        superClassesStubs.none { it.readOnlyClass == readOnlyClass }
     }
 
-        val classStubFuns = relevantStubs.flatMap { createStubFuns(it) }
+    private class FilteredStubsForCollectionClass(
-        val superClassStubSignatures = superClassesStubs.flatMap { createStubFuns(it) }.mapTo(HashSet()) { it.toJvmSignature() }
+        override val readOnlyClass: IrClassSymbol,
+        override val mutableClass: IrClassSymbol,
+        override val candidatesForStubs: Collection<IrSimpleFunction>
+    ) : StubsForCollectionClass
 
-        return classStubFuns.filter { it.toJvmSignature() !in superClassStubSignatures }
+    private fun computeStubsForSuperClass(superClass: IrClass): List<StubsForCollectionClass> {
+        val superClassStubs = collectionStubComputer.stubsForCollectionClasses(superClass)
+
+        if (superClass.modality != Modality.ABSTRACT) return superClassStubs
+
+        // An abstract superclass might contain an abstract function declaration that effectively overrides a stub to be generated,
+        // and thus have no non-abstract stub.
+        // This calculation happens for each abstract class multiple times. TODO memoize.
+
+        val abstractFunsByNameAndArity = superClass.functions
+            .filter { !it.isFakeOverride && it.modality == Modality.ABSTRACT }
+            .groupBy { it.nameAndArity }
+
+        if (abstractFunsByNameAndArity.isEmpty()) return superClassStubs
+
+        return superClassStubs.map {
+            val (readOnlyClass, mutableClass, candidatesForStubs) = it
+            // NB here we should build a stub in substitution context of the given superclass.
+            // Resulting stub can be different from a stub created in substitution context of the "current" class
+            // in case of (partially) specialized generic superclass.
+            val substitutionMap = computeSubstitutionMap(readOnlyClass.owner, mutableClass.owner, superClass)
+            val filteredCandidates = candidatesForStubs.filter { candidateFun ->
+                val stubMethod = createStubMethod(candidateFun, superClass, substitutionMap)
+                val stubNameAndArity = stubMethod.nameAndArity
+                abstractFunsByNameAndArity[stubNameAndArity].orEmpty().none { abstractFun ->
+                    isEffectivelyOverriddenBy(stubMethod, abstractFun)
+                }
+            }
+            FilteredStubsForCollectionClass(readOnlyClass, mutableClass, filteredCandidates)
+        }
     }
 
     private fun Collection<IrType>.findMostSpecificTypeForClass(classifier: IrClassSymbol): IrType {
@@ -346,11 +380,23 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
         get() = generateSequence(this) { it.superClass }
 }
 
-
+internal interface StubsForCollectionClass {
-internal class StubsForCollectionClass(
+    val readOnlyClass: IrClassSymbol
-    val readOnlyClass: IrClassSymbol,
     val mutableClass: IrClassSymbol
-) {
+    val candidatesForStubs: Collection<IrSimpleFunction>
+}
+
+internal operator fun StubsForCollectionClass.component1() = readOnlyClass
+internal operator fun StubsForCollectionClass.component2() = mutableClass
+internal operator fun StubsForCollectionClass.component3() = candidatesForStubs
+
+internal class CollectionStubComputer(val context: JvmBackendContext) {
+
+    private class LazyStubsForCollectionClass(
+        override val readOnlyClass: IrClassSymbol,
+        override val mutableClass: IrClassSymbol
+    ) : StubsForCollectionClass {
+        override val candidatesForStubs: Collection<IrSimpleFunction> by lazy {
             // Old back-end generates stubs for 'class A : C', where
             //  'C' is some "read-only collection" interface from kotlin.collections,
             //  'MC' is a corresponding "mutable collection" interface from kotlin.collections,
@@ -380,7 +426,7 @@ internal class StubsForCollectionClass(
             // See https://youtrack.jetbrains.com/issue/KT-36724.
             // In the ideal world, it should be enough to check that
             // the given member function 'f' from 'MC' doesn't override anything from 'C'.
-    val candidatesForStubs: Collection<IrSimpleFunction> by lazy {
+
             mutableClass.functions
                 .map { it.owner }
                 .filter { memberFun ->
@@ -391,40 +437,38 @@ internal class StubsForCollectionClass(
                 }
                 .toHashSet()
         }
-
-    operator fun component1() = readOnlyClass
-    operator fun component2() = mutableClass
-    operator fun component3() = candidatesForStubs
     }
 
-
-internal class CollectionStubComputer(val context: JvmBackendContext) {
-
     private val preComputedStubs: Collection<StubsForCollectionClass> by lazy {
         with(context.ir.symbols) {
             listOf(
-                collection to mutableCollection,
+                LazyStubsForCollectionClass(collection, mutableCollection),
-                set to mutableSet,
+                LazyStubsForCollectionClass(set, mutableSet),
-                list to mutableList,
+                LazyStubsForCollectionClass(list, mutableList),
-                map to mutableMap,
+                LazyStubsForCollectionClass(map, mutableMap),
-                mapEntry to mutableMapEntry,
+                LazyStubsForCollectionClass(mapEntry, mutableMapEntry),
-                iterable to mutableIterable,
+                LazyStubsForCollectionClass(iterable, mutableIterable),
-                iterator to mutableIterator,
+                LazyStubsForCollectionClass(iterator, mutableIterator),
-                listIterator to mutableListIterator
+                LazyStubsForCollectionClass(listIterator, mutableListIterator)
-            ).map { (readOnlyClass, mutableClass) ->
+            )
-                StubsForCollectionClass(readOnlyClass, mutableClass)
-            }
         }
     }
 
-    private val stubsCache = mutableMapOf<IrClass, Collection<StubsForCollectionClass>>()
+    private val stubsCache = mutableMapOf<IrClass, List<StubsForCollectionClass>>()
 
-    fun stubsForCollectionClasses(irClass: IrClass): Collection<StubsForCollectionClass> =
+    fun stubsForCollectionClasses(irClass: IrClass): List<StubsForCollectionClass> =
         stubsCache.getOrPut(irClass) {
-            if (irClass.isFromJava()) emptySet()
+            computeStubsForCollectionClasses(irClass)
-            else preComputedStubs.filter { (readOnlyClass, mutableClass) ->
+        }
+
+    private fun computeStubsForCollectionClasses(irClass: IrClass): List<StubsForCollectionClass> {
+        if (irClass.isFromJava()) return emptyList()
+        val stubs = preComputedStubs.filter { (readOnlyClass, mutableClass) ->
             !irClass.symbol.isSubtypeOfClass(mutableClass) &&
                     irClass.superTypes.any { it.isSubtypeOfClass(readOnlyClass) }
         }
+        return stubs.filter { (readOnlyClass) ->
+            stubs.none { readOnlyClass != it.readOnlyClass && it.readOnlyClass.isSubtypeOfClass(readOnlyClass) }
+        }
     }
 }

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractCollections.kt

@@ -0,0 +1,14 @@
+// WITH_RUNTIME
+
+abstract class AIterD : AbstractIterator<Double>()
+
+abstract class ACollD : AbstractCollection<Double>()
+
+abstract class AMCollD : AbstractMutableCollection<Double>()
+
+abstract class ASetD : AbstractSet<Double>()
+
+abstract class AMSetD : AbstractMutableSet<Double>()
+
+abstract class AListD : AbstractList<Double>()
+

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractCollections.txt

@@ -0,0 +1,54 @@
+@kotlin.Metadata
+public abstract class ACollD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public method iterator(): java.util.Iterator
+}
+
+@kotlin.Metadata
+public abstract class AIterD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+}
+
+@kotlin.Metadata
+public abstract class AListD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public bridge method indexOf(p0: double): int
+    public bridge final method indexOf(p0: java.lang.Object): int
+    public bridge method lastIndexOf(p0: double): int
+    public bridge final method lastIndexOf(p0: java.lang.Object): int
+}
+
+@kotlin.Metadata
+public abstract class AMCollD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public bridge method remove(p0: double): boolean
+    public bridge final method remove(p0: java.lang.Object): boolean
+}
+
+@kotlin.Metadata
+public abstract class AMSetD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public bridge method remove(p0: double): boolean
+    public bridge final method remove(p0: java.lang.Object): boolean
+}
+
+@kotlin.Metadata
+public abstract class ASetD {
+    // source: 'inheritingFromAbstractCollections.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+}

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMap.kt

@@ -0,0 +1,5 @@
+// WITH_RUNTIME
+
+abstract class AMapSD : AbstractMap<String, Double>()
+
+abstract class AMMapSD : AbstractMutableMap<String, Double>()

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMap.txt

@@ -0,0 +1,25 @@
+@kotlin.Metadata
+public abstract class AMMapSD {
+    // source: 'inheritingFromAbstractMap.kt'
+    public method <init>(): void
+    public bridge final method containsKey(p0: java.lang.Object): boolean
+    public bridge method containsKey(p0: java.lang.String): boolean
+    public bridge method containsValue(p0: double): boolean
+    public bridge final method containsValue(p0: java.lang.Object): boolean
+    public bridge final method get(p0: java.lang.Object): java.lang.Object
+    public bridge method get(p0: java.lang.String): java.lang.Double
+    public bridge final method remove(p0: java.lang.Object): java.lang.Object
+    public bridge method remove(p0: java.lang.String): java.lang.Double
+}
+
+@kotlin.Metadata
+public abstract class AMapSD {
+    // source: 'inheritingFromAbstractMap.kt'
+    public method <init>(): void
+    public bridge final method containsKey(p0: java.lang.Object): boolean
+    public bridge method containsKey(p0: java.lang.String): boolean
+    public bridge method containsValue(p0: double): boolean
+    public bridge final method containsValue(p0: java.lang.Object): boolean
+    public bridge final method get(p0: java.lang.Object): java.lang.Object
+    public bridge method get(p0: java.lang.String): java.lang.Double
+}

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMap_ir.txt

@@ -0,0 +1,25 @@
+@kotlin.Metadata
+public abstract class AMMapSD {
+    // source: 'inheritingFromAbstractMap.kt'
+    public method <init>(): void
+    public bridge final method containsKey(p0: java.lang.Object): boolean
+    public bridge method containsKey(p0: java.lang.String): boolean
+    public bridge method containsValue(p0: double): boolean
+    public bridge final method containsValue(p0: java.lang.Object): boolean
+    public bridge final method get(p0: java.lang.Object): java.lang.Double
+    public bridge method get(p0: java.lang.String): java.lang.Double
+    public bridge final method remove(p0: java.lang.Object): java.lang.Double
+    public bridge method remove(p0: java.lang.String): java.lang.Double
+}
+
+@kotlin.Metadata
+public abstract class AMapSD {
+    // source: 'inheritingFromAbstractMap.kt'
+    public method <init>(): void
+    public bridge final method containsKey(p0: java.lang.Object): boolean
+    public bridge method containsKey(p0: java.lang.String): boolean
+    public bridge method containsValue(p0: double): boolean
+    public bridge final method containsValue(p0: java.lang.Object): boolean
+    public bridge final method get(p0: java.lang.Object): java.lang.Double
+    public bridge method get(p0: java.lang.String): java.lang.Double
+}

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMutableList.kt

@@ -0,0 +1,3 @@
+// WITH_RUNTIME
+
+abstract class AMListD : AbstractMutableList<Double>()

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMutableList.txt

@@ -0,0 +1,15 @@
+@kotlin.Metadata
+public abstract class AMListD {
+    // source: 'inheritingFromAbstractMutableList.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public bridge method indexOf(p0: double): int
+    public bridge final method indexOf(p0: java.lang.Object): int
+    public bridge method lastIndexOf(p0: double): int
+    public bridge final method lastIndexOf(p0: java.lang.Object): int
+    public bridge method remove(p0: double): boolean
+    public bridge final method remove(p0: int): double
+    public bridge final method remove(p0: java.lang.Object): boolean
+    public synthetic bridge method removeAt(p0: int): java.lang.Object
+}

compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMutableList_ir.txt

@@ -0,0 +1,16 @@
+@kotlin.Metadata
+public abstract class AMListD {
+    // source: 'inheritingFromAbstractMutableList.kt'
+    public method <init>(): void
+    public bridge method contains(p0: double): boolean
+    public bridge final method contains(p0: java.lang.Object): boolean
+    public bridge method indexOf(p0: double): int
+    public bridge final method indexOf(p0: java.lang.Object): int
+    public bridge method lastIndexOf(p0: double): int
+    public bridge final method lastIndexOf(p0: java.lang.Object): int
+    public bridge method remove(p0: double): boolean
+    public bridge final method remove(p0: int): double
+    public bridge final method remove(p0: java.lang.Object): boolean
+    public abstract method removeAt(p0: int): java.lang.Double
+    public synthetic bridge method removeAt(p0: int): java.lang.Object
+}

compiler/tests/org/jetbrains/kotlin/codegen/BytecodeListingTestGenerated.java

@@ -339,6 +339,21 @@ public class BytecodeListingTestGenerated extends AbstractBytecodeListingTest {
             runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/extendingAbstractCollection.kt");
         }
 
+        @TestMetadata("inheritingFromAbstractCollections.kt")
+        public void testInheritingFromAbstractCollections() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractCollections.kt");
+        }
+
+        @TestMetadata("inheritingFromAbstractMap.kt")
+        public void testInheritingFromAbstractMap() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMap.kt");
+        }
+
+        @TestMetadata("inheritingFromAbstractMutableList.kt")
+        public void testInheritingFromAbstractMutableList() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMutableList.kt");
+        }
+
         @TestMetadata("mapOfPrimitivesFullJdk.kt")
         public void testMapOfPrimitivesFullJdk() throws Exception {
             runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/mapOfPrimitivesFullJdk.kt");

compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBytecodeListingTestGenerated.java

@@ -339,6 +339,21 @@ public class IrBytecodeListingTestGenerated extends AbstractIrBytecodeListingTes
             runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/extendingAbstractCollection.kt");
         }
 
+        @TestMetadata("inheritingFromAbstractCollections.kt")
+        public void testInheritingFromAbstractCollections() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractCollections.kt");
+        }
+
+        @TestMetadata("inheritingFromAbstractMap.kt")
+        public void testInheritingFromAbstractMap() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMap.kt");
+        }
+
+        @TestMetadata("inheritingFromAbstractMutableList.kt")
+        public void testInheritingFromAbstractMutableList() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/inheritingFromAbstractMutableList.kt");
+        }
+
         @TestMetadata("mapOfPrimitivesFullJdk.kt")
         public void testMapOfPrimitivesFullJdk() throws Exception {
             runTest("compiler/testData/codegen/bytecodeListing/collectionStubs/mapOfPrimitivesFullJdk.kt");