JVM_IR KT-41915 compare Kotlin signatures when adding collection stubs

compiler/fir/fir2ir/tests/org/jetbrains/kotlin/codegen/ir/FirBlackBoxCodegenTestGenerated.java

@@ -5008,6 +5008,16 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT
             runTest("compiler/testData/codegen/box/collections/removeClash.kt");
         }
 
+        @TestMetadata("removeClashJava.kt")
+        public void testRemoveClashJava() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashJava.kt");
+        }
+
+        @TestMetadata("removeClashKotlin.kt")
+        public void testRemoveClashKotlin() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashKotlin.kt");
+        }
+
         @TestMetadata("removeOverriddenInJava.kt")
         public void testRemoveOverriddenInJava() throws Exception {
             runTest("compiler/testData/codegen/box/collections/removeOverriddenInJava.kt");

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

@@ -24,6 +24,9 @@ import org.jetbrains.kotlin.ir.symbols.IrClassSymbol
 import org.jetbrains.kotlin.ir.symbols.IrTypeParameterSymbol
 import org.jetbrains.kotlin.ir.types.*
 import org.jetbrains.kotlin.ir.util.*
+import org.jetbrains.kotlin.types.AbstractTypeChecker
+import org.jetbrains.kotlin.types.AbstractTypeCheckerContext
+import org.jetbrains.kotlin.utils.addToStdlib.cast
 
 internal val collectionStubMethodLowering = makeIrFilePhase(
     ::CollectionStubMethodLowering,
@@ -47,26 +50,27 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
         // stub generation is still needed to avoid invocation error.
         val existingMethodsBySignature = irClass.functions.filterNot {
             it.modality == Modality.ABSTRACT && it.isFakeOverride
-        }.associateBy { it.toSignature() }
+        }.associateBy { it.toJvmSignature() }
 
-        for ((signature, member) in methodStubsToGenerate) {
+        for (member in methodStubsToGenerate) {
+            val signature = member.toJvmSignature()
             val existingMethod = existingMethodsBySignature[signature] // TODO KT-41915
-            if (existingMethod != null) {
+            if (existingMethod != null && areEquivalentSignatures(existingMethod, member)) {
                 // In the case that we find a defined method that matches the stub signature, we add the overridden symbols to that
                 // defined method, so that bridge lowering can still generate correct bridge for that method
                 existingMethod.overriddenSymbols += member.overriddenSymbols
             } else {
-                irClass.declarations.add(member)
+                irClass.declarations.add(member.hackRemoveMethodIfRequired())
             }
         }
     }
 
-    private fun IrSimpleFunction.toSignature(): String = collectionStubComputer.getSignature(this)
+    private fun IrSimpleFunction.toJvmSignature(): String = collectionStubComputer.getJvmSignature(this)
 
     private fun createStubMethod(
         function: IrSimpleFunction, irClass: IrClass, substitutionMap: Map<IrTypeParameterSymbol, IrType>
-    ): Pair<String, IrSimpleFunction> {
-        val newMethod = context.irFactory.buildFun {
+    ): IrSimpleFunction {
+        return context.irFactory.buildFun {
             name = function.name
             returnType = function.returnType.substitute(substitutionMap)
             visibility = function.visibility
@@ -89,21 +93,34 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
                 }
             }
         }
-        val signatureToCheckPresence = newMethod.toSignature()
-        // NB 'remove' method requires some special handling (and that's why we need to track both signature and function):
+    }
+
+    private fun areEquivalentSignatures(fun1: IrSimpleFunction, fun2: IrSimpleFunction): Boolean {
+        if (fun1.typeParameters.size != fun2.typeParameters.size) return false
+        if (fun1.valueParameters.size != fun2.valueParameters.size) return false
+        val typeChecker = IrTypeCheckerContextWithAdditionalAxioms(context.irBuiltIns, fun1.typeParameters, fun2.typeParameters)
+            .cast<AbstractTypeCheckerContext>()
+        return fun1.valueParameters.zip(fun2.valueParameters)
+            .all { (valueParameter1, valueParameter2) ->
+                AbstractTypeChecker.equalTypes(typeChecker, valueParameter1.type, valueParameter2.type)
+            }
+    }
+
+    private fun IrSimpleFunction.hackRemoveMethodIfRequired(): IrSimpleFunction {
+        // NB 'remove' method requires some special handling:
         // - we check that 'remove(T)' is present in the class, where 'T' is a type argument of the collection type
         //   (this matches the signature of the method created above);
         // - if it is absent, we add 'remove(Any?)' member function,
         //   which corresponds to method 'remove' in java.util.Collection and java.util.Map.
-        if (newMethod.name.asString() == "remove") {
+        if (name.asString() == "remove") {
             // Note that replacing value parameter types with 'Any?' handles both MutableCollection and MutableMap case:
             // java.util.Collection<E>#remove has signature 'boolean remove(Object)', and
             // java.util.Map<K, V>#remove has signature 'V remove(Object)'.
-            newMethod.valueParameters = function.valueParameters.map {
-                it.copyWithCustomTypeSubstitution(newMethod) { context.irBuiltIns.anyNType }
+            valueParameters = valueParameters.map {
+                it.copyWithCustomTypeSubstitution(this) { context.irBuiltIns.anyNType }
             }
         }
-        return signatureToCheckPresence to newMethod
+        return this
     }
 
     // Copy value parameter with type substitution
@@ -150,7 +167,7 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
     }
 
     // Compute stubs that should be generated, compare based on signature
-    private fun generateRelevantStubMethods(irClass: IrClass): Map<String, IrSimpleFunction> {
+    private fun generateRelevantStubMethods(irClass: IrClass): List<IrSimpleFunction> {
         val ourStubsForCollectionClasses = collectionStubComputer.stubsForCollectionClasses(irClass)
         val superStubClasses = irClass.superClass?.superClassChain?.map { superClass ->
             collectionStubComputer.stubsForCollectionClasses(superClass).map { it.readOnlyClass }
@@ -169,7 +186,7 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
             mutableOnlyMethods.map { function ->
                 createStubMethod(function, irClass, substitutionMap)
             }
-        }.toMap()
+        }
     }
 
     private fun Collection<IrType>.findMostSpecificTypeForClass(classifier: IrClassSymbol): IrType {
@@ -190,7 +207,7 @@ internal class CollectionStubMethodLowering(val context: JvmBackendContext) : Cl
 }
 
 internal class CollectionStubComputer(val context: JvmBackendContext) {
-    fun getSignature(irFunction: IrSimpleFunction): String = context.methodSignatureMapper.mapAsmMethod(irFunction).toString()
+    fun getJvmSignature(irFunction: IrSimpleFunction): String = context.methodSignatureMapper.mapAsmMethod(irFunction).toString()
 
     inner class StubsForCollectionClass(
         val readOnlyClass: IrClassSymbol,
@@ -207,12 +224,12 @@ internal class CollectionStubComputer(val context: JvmBackendContext) {
         val mutableOnlyMethods: Collection<IrSimpleFunction> by lazy {
             val readOnlyMethodSignatures = readOnlyClass
                 .functions
-                .map { getSignature(it.owner) }
+                .map { getJvmSignature(it.owner) }
                 .filter { it !in specialCaseStubSignaturesForOldBackend }
                 .toHashSet()
             mutableClass.functions
                 .map { it.owner }
-                .filter { getSignature(it) !in readOnlyMethodSignatures }
+                .filter { getJvmSignature(it) !in readOnlyMethodSignatures }
                 .toHashSet()
         }
 

compiler/testData/codegen/box/collections/removeClashJava.kt

@@ -0,0 +1,33 @@
+// TARGET_BACKEND: JVM
+// WITH_RUNTIME
+// FILE: removeClashJava.kt
+class Queue<T>() : Collection<T> {
+    override val size: Int = 1
+    override fun contains(element: T): Boolean = TODO()
+    override fun containsAll(elements: Collection<T>): Boolean = TODO()
+    override fun isEmpty(): Boolean = TODO()
+    override fun iterator(): Iterator<T> = TODO()
+
+    fun remove(v: Any?): Any? = v
+}
+
+fun box(): String {
+    val q = Queue<String>()
+    J.testRemove(q)
+    return q.remove("OK") as String
+}
+
+// FILE: J.java
+import java.util.Collection;
+
+public class J {
+    public static void testRemove(Collection<String> c) {
+        try {
+            c.remove("");
+            throw new AssertionError("c.remove(...) should throw UnsupportedOperationException");
+        } catch (UnsupportedOperationException e) {
+        } catch (Throwable e) {
+            throw new AssertionError("c.remove(...) should throw UnsupportedOperationException");
+        }
+    }
+}

compiler/testData/codegen/box/collections/removeClashKotlin.kt

@@ -0,0 +1,68 @@
+// TARGET_BACKEND: JVM
+// WITH_RUNTIME
+
+var removed: String? = ""
+
+open class RemoveStringNImpl {
+    fun remove(s: String?): Boolean {
+        removed = s
+        return false
+    }
+}
+
+class S1 : Set<String>, RemoveStringNImpl() {
+    override val size: Int get() = TODO()
+    override fun contains(element: String): Boolean = TODO()
+    override fun containsAll(elements: Collection<String>): Boolean = TODO()
+    override fun isEmpty(): Boolean = TODO()
+    override fun iterator(): Iterator<String> = TODO()
+}
+
+class S2 : Set<String> {
+    override val size: Int get() = TODO()
+    override fun contains(element: String): Boolean = TODO()
+    override fun containsAll(elements: Collection<String>): Boolean = TODO()
+    override fun isEmpty(): Boolean = TODO()
+    override fun iterator(): Iterator<String> = TODO()
+
+    fun remove(s: String?): Boolean {
+        removed = s
+        return false
+    }
+}
+
+class S3 : Set<String> {
+    override val size: Int get() = 0
+    override fun contains(element: String): Boolean = false
+    override fun containsAll(elements: Collection<String>): Boolean = false
+    override fun isEmpty(): Boolean = true
+    override fun iterator(): Iterator<String> = emptyList<String>().iterator()
+
+    fun remove(s: String) = s
+}
+
+fun javaSetRemoveShouldThrowUOE(message: String, s: Set<String>) {
+    try {
+        (s as java.util.Set<String>).remove("")
+        throw AssertionError(message)
+    } catch (e: UnsupportedOperationException) {
+    }
+}
+
+fun box(): String {
+    javaSetRemoveShouldThrowUOE("(S1 as java.util.Set).remove", S1())
+
+    javaSetRemoveShouldThrowUOE("(S2 as java.util.Set).remove", S2())
+
+    removed = ""
+    S1().remove("OK")
+    if (removed != "OK") throw AssertionError("S1.remove")
+
+    removed = ""
+    S2().remove("OK")
+    if (removed != "OK") throw AssertionError("S2.remove")
+
+    if (S3().remove("OK") != "OK") throw AssertionError("S3.remove")
+
+    return "OK"
+}

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

@@ -5038,6 +5038,16 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             runTest("compiler/testData/codegen/box/collections/removeClash.kt");
         }
 
+        @TestMetadata("removeClashJava.kt")
+        public void testRemoveClashJava() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashJava.kt");
+        }
+
+        @TestMetadata("removeClashKotlin.kt")
+        public void testRemoveClashKotlin() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashKotlin.kt");
+        }
+
         @TestMetadata("removeOverriddenInJava.kt")
         public void testRemoveOverriddenInJava() throws Exception {
             runTest("compiler/testData/codegen/box/collections/removeOverriddenInJava.kt");

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

@@ -5038,6 +5038,16 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes
             runTest("compiler/testData/codegen/box/collections/removeClash.kt");
         }
 
+        @TestMetadata("removeClashJava.kt")
+        public void testRemoveClashJava() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashJava.kt");
+        }
+
+        @TestMetadata("removeClashKotlin.kt")
+        public void testRemoveClashKotlin() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashKotlin.kt");
+        }
+
         @TestMetadata("removeOverriddenInJava.kt")
         public void testRemoveOverriddenInJava() throws Exception {
             runTest("compiler/testData/codegen/box/collections/removeOverriddenInJava.kt");

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

@@ -5008,6 +5008,16 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes
             runTest("compiler/testData/codegen/box/collections/removeClash.kt");
         }
 
+        @TestMetadata("removeClashJava.kt")
+        public void testRemoveClashJava() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashJava.kt");
+        }
+
+        @TestMetadata("removeClashKotlin.kt")
+        public void testRemoveClashKotlin() throws Exception {
+            runTest("compiler/testData/codegen/box/collections/removeClashKotlin.kt");
+        }
+
         @TestMetadata("removeOverriddenInJava.kt")
         public void testRemoveOverriddenInJava() throws Exception {
             runTest("compiler/testData/codegen/box/collections/removeOverriddenInJava.kt");