From fbfe56e0cce44686c75cd265c81933e25a380165 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Wed, 16 Sep 2020 16:55:19 +0300 Subject: [PATCH] JVM_IR KT-41915 compare Kotlin signatures when adding collection stubs --- .../ir/FirBlackBoxCodegenTestGenerated.java | 10 +++ .../jvm/lower/CollectionStubMethodLowering.kt | 53 ++++++++++----- .../box/collections/removeClashJava.kt | 33 +++++++++ .../box/collections/removeClashKotlin.kt | 68 +++++++++++++++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 10 +++ .../LightAnalysisModeTestGenerated.java | 10 +++ .../ir/IrBlackBoxCodegenTestGenerated.java | 10 +++ 7 files changed, 176 insertions(+), 18 deletions(-) create mode 100644 compiler/testData/codegen/box/collections/removeClashJava.kt create mode 100644 compiler/testData/codegen/box/collections/removeClashKotlin.kt diff --git a/compiler/fir/fir2ir/tests/org/jetbrains/kotlin/codegen/ir/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests/org/jetbrains/kotlin/codegen/ir/FirBlackBoxCodegenTestGenerated.java index 74b56670686..003bfa55e10 100644 --- a/compiler/fir/fir2ir/tests/org/jetbrains/kotlin/codegen/ir/FirBlackBoxCodegenTestGenerated.java +++ b/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"); diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/CollectionStubMethodLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/CollectionStubMethodLowering.kt index 000ebfa9000..c4adc073c44 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/CollectionStubMethodLowering.kt +++ b/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 - ): Pair { - 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() + 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#remove has signature 'boolean remove(Object)', and // java.util.Map#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 { + private fun generateRelevantStubMethods(irClass: IrClass): List { 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.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 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() } diff --git a/compiler/testData/codegen/box/collections/removeClashJava.kt b/compiler/testData/codegen/box/collections/removeClashJava.kt new file mode 100644 index 00000000000..47beee3958b --- /dev/null +++ b/compiler/testData/codegen/box/collections/removeClashJava.kt @@ -0,0 +1,33 @@ +// TARGET_BACKEND: JVM +// WITH_RUNTIME +// FILE: removeClashJava.kt +class Queue() : Collection { + override val size: Int = 1 + override fun contains(element: T): Boolean = TODO() + override fun containsAll(elements: Collection): Boolean = TODO() + override fun isEmpty(): Boolean = TODO() + override fun iterator(): Iterator = TODO() + + fun remove(v: Any?): Any? = v +} + +fun box(): String { + val q = Queue() + J.testRemove(q) + return q.remove("OK") as String +} + +// FILE: J.java +import java.util.Collection; + +public class J { + public static void testRemove(Collection 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"); + } + } +} \ No newline at end of file diff --git a/compiler/testData/codegen/box/collections/removeClashKotlin.kt b/compiler/testData/codegen/box/collections/removeClashKotlin.kt new file mode 100644 index 00000000000..da2c7e9cafd --- /dev/null +++ b/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, RemoveStringNImpl() { + override val size: Int get() = TODO() + override fun contains(element: String): Boolean = TODO() + override fun containsAll(elements: Collection): Boolean = TODO() + override fun isEmpty(): Boolean = TODO() + override fun iterator(): Iterator = TODO() +} + +class S2 : Set { + override val size: Int get() = TODO() + override fun contains(element: String): Boolean = TODO() + override fun containsAll(elements: Collection): Boolean = TODO() + override fun isEmpty(): Boolean = TODO() + override fun iterator(): Iterator = TODO() + + fun remove(s: String?): Boolean { + removed = s + return false + } +} + +class S3 : Set { + override val size: Int get() = 0 + override fun contains(element: String): Boolean = false + override fun containsAll(elements: Collection): Boolean = false + override fun isEmpty(): Boolean = true + override fun iterator(): Iterator = emptyList().iterator() + + fun remove(s: String) = s +} + +fun javaSetRemoveShouldThrowUOE(message: String, s: Set) { + try { + (s as java.util.Set).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" +} diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index fa1264c1e95..48f8593d1a0 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/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"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 86c2beacb78..6e56bb5a607 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/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"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index 40e7e755f4a..d9725b94556 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/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");