From c6ee2ea43df1269e4f23aedabe76ddf75dc49fa9 Mon Sep 17 00:00:00 2001 From: SvyatoslavScherbina Date: Fri, 14 Feb 2020 09:48:32 +0300 Subject: [PATCH] Workaround Swift bug (SR-12201) triggering #3825 (#3852) Avoid placing Objective-C error out parameter right after block parameters. Fix #3825. --- .../konan/objcexport/ObjCExportMapper.kt | 14 ++++- .../tests/framework/values/expectedLazy.h | 19 +++++++ .../tests/framework/values/values.kt | 46 +++++++++++++++- .../tests/framework/values/values.swift | 55 ++++++++++++++++--- 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportMapper.kt b/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportMapper.kt index 2e9a5a02e21..9ac925a0fb1 100644 --- a/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportMapper.kt +++ b/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportMapper.kt @@ -316,12 +316,24 @@ private fun ObjCExportMapper.bridgeMethodImpl(descriptor: FunctionDescriptor): M val returnBridge = bridgeReturnType(descriptor, convertExceptionsToErrors) if (convertExceptionsToErrors) { - valueParameters += MethodBridgeValueParameter.ErrorOutParameter + // Add error out parameter before tail block parameters. The convention allows this. + // Placing it after would trigger https://bugs.swift.org/browse/SR-12201 + // (see also https://github.com/JetBrains/kotlin-native/issues/3825). + val tailBlocksCount = valueParameters.reversed().takeWhile { it.isBlockPointer() }.count() + valueParameters.add(valueParameters.size - tailBlocksCount, MethodBridgeValueParameter.ErrorOutParameter) } return MethodBridge(returnBridge, receiver, valueParameters) } +private fun MethodBridgeValueParameter.isBlockPointer(): Boolean = when (this) { + is MethodBridgeValueParameter.Mapped -> when (this.bridge) { + ReferenceBridge, is ValueTypeBridge -> false + is BlockPointerBridge -> true + } + MethodBridgeValueParameter.ErrorOutParameter -> false +} + internal fun ObjCExportMapper.bridgePropertyType(descriptor: PropertyDescriptor): TypeBridge { assert(isBaseProperty(descriptor)) diff --git a/backend.native/tests/framework/values/expectedLazy.h b/backend.native/tests/framework/values/expectedLazy.h index 7b18c4bb369..998ae61282a 100644 --- a/backend.native/tests/framework/values/expectedLazy.h +++ b/backend.native/tests/framework/values/expectedLazy.h @@ -1106,6 +1106,24 @@ __attribute__((swift_name("TestStringConversion"))) @property id str __attribute__((swift_name("str"))); @end; +__attribute__((swift_name("GH3825"))) +@protocol ValuesGH3825 +@required +- (BOOL)call0AndReturnError:(NSError * _Nullable * _Nullable)error callback:(ValuesBoolean *(^)(void))callback __attribute__((swift_name("call0(callback:)"))); +- (BOOL)call1DoThrow:(BOOL)doThrow error:(NSError * _Nullable * _Nullable)error callback:(void (^)(void))callback __attribute__((swift_name("call1(doThrow:callback:)"))); +- (BOOL)call2Callback:(void (^)(void))callback doThrow:(BOOL)doThrow error:(NSError * _Nullable * _Nullable)error __attribute__((swift_name("call2(callback:doThrow:)"))); +@end; + +__attribute__((objc_subclassing_restricted)) +__attribute__((swift_name("GH3825KotlinImpl"))) +@interface ValuesGH3825KotlinImpl : ValuesBase +- (instancetype)init __attribute__((swift_name("init()"))) __attribute__((objc_designated_initializer)); ++ (instancetype)new __attribute__((availability(swift, unavailable, message="use object initializers instead"))); +- (BOOL)call0AndReturnError:(NSError * _Nullable * _Nullable)error callback:(ValuesBoolean *(^)(void))callback __attribute__((swift_name("call0(callback:)"))); +- (BOOL)call1DoThrow:(BOOL)doThrow error:(NSError * _Nullable * _Nullable)error callback:(void (^)(void))callback __attribute__((swift_name("call1(doThrow:callback:)"))); +- (BOOL)call2Callback:(void (^)(void))callback doThrow:(BOOL)doThrow error:(NSError * _Nullable * _Nullable)error __attribute__((swift_name("call2(callback:doThrow:)"))); +@end; + @interface ValuesEnumeration (ValuesKt) - (ValuesEnumeration *)getAnswer __attribute__((swift_name("getAnswer()"))); @end; @@ -1199,6 +1217,7 @@ __attribute__((swift_name("ValuesKt"))) + (int32_t)testAbstractInterfaceCallX:(id)x __attribute__((swift_name("testAbstractInterfaceCall(x:)"))); + (int32_t)testAbstractInterfaceCall2X:(id)x __attribute__((swift_name("testAbstractInterfaceCall2(x:)"))); + (void)fooA:(ValuesKotlinAtomicReference *)a __attribute__((swift_name("foo(a:)"))); ++ (BOOL)testGH3825Gh3825:(id)gh3825 error:(NSError * _Nullable * _Nullable)error __attribute__((swift_name("testGH3825(gh3825:)"))); @property (class, readonly) double dbl __attribute__((swift_name("dbl"))); @property (class, readonly) float flt __attribute__((swift_name("flt"))); @property (class, readonly) int32_t integer __attribute__((swift_name("integer"))); diff --git a/backend.native/tests/framework/values/values.kt b/backend.native/tests/framework/values/values.kt index e17a08097c3..524a6c443c6 100644 --- a/backend.native/tests/framework/values/values.kt +++ b/backend.native/tests/framework/values/values.kt @@ -10,6 +10,7 @@ package conversions import kotlin.native.concurrent.freeze import kotlin.native.concurrent.isFrozen +import kotlin.native.internal.ObjCErrorException import kotlin.native.ref.WeakReference import kotlin.properties.ReadWriteProperty import kotlin.reflect.KClass @@ -328,7 +329,7 @@ fun testSwiftThrowing(methods: SwiftOverridableMethodsWithThrows) = with(methods } private inline fun assertSwiftThrowing(block: () -> Unit) = - assertFailsWith(block = block) + assertFailsWith(block = block) @Throws(Throwable::class) fun testSwiftNotThrowing(methods: SwiftOverridableMethodsWithThrows) = with(methods) { @@ -356,7 +357,7 @@ abstract class ThrowsWithBridge : ThrowsWithBridgeBase { @Throws(Throwable::class) fun testSwiftThrowing(test: ThrowsWithBridgeBase, flag: Boolean) { - assertFailsWith { + assertFailsWith { if (flag) { test.plusOne(0) } else { @@ -935,4 +936,43 @@ class TestStringConversion { lateinit var str: Any } -fun foo(a: kotlin.native.concurrent.AtomicReference<*>) {} \ No newline at end of file +fun foo(a: kotlin.native.concurrent.AtomicReference<*>) {} + +interface GH3825 { + @Throws(MyException::class) fun call0(callback: () -> Boolean) + @Throws(MyException::class) fun call1(doThrow: Boolean, callback: () -> Unit) + @Throws(MyException::class) fun call2(callback: () -> Unit, doThrow: Boolean) +} + +class GH3825KotlinImpl : GH3825 { + override fun call0(callback: () -> Boolean) { + if (callback()) throw MyException() + } + + override fun call1(doThrow: Boolean, callback: () -> Unit) { + if (doThrow) throw MyException() + callback() + } + + override fun call2(callback: () -> Unit, doThrow: Boolean) { + if (doThrow) throw MyException() + callback() + } +} + +@Throws(Throwable::class) +fun testGH3825(gh3825: GH3825) { + var count = 0 + + assertFailsWith { gh3825.call0({ true }) } + gh3825.call0({ count += 1; false }) + assertEquals(1, count) + + assertFailsWith { gh3825.call1(true, { fail() }) } + gh3825.call1(false, { count += 1 }) + assertEquals(2, count) + + assertFailsWith { gh3825.call2({ fail() }, true) } + gh3825.call2({ count += 1 }, false) + assertEquals(3, count) +} \ No newline at end of file diff --git a/backend.native/tests/framework/values/values.swift b/backend.native/tests/framework/values/values.swift index 6ca77d914b0..8f6d51e110f 100644 --- a/backend.native/tests/framework/values/values.swift +++ b/backend.native/tests/framework/values/values.swift @@ -316,15 +316,15 @@ class SwiftNotThrowingWithBridge : ThrowsWithBridge { } } -func testExceptions() throws { - func testThrowing(_ block: () throws -> Void) throws { - do { - try block() - } catch let error as NSError { - try assertTrue(error.kotlinException is MyException) - } +private func testThrowing(_ block: () throws -> Void) throws { + do { + try block() + } catch let error as NSError { + try assertTrue(error.kotlinException is MyException) } +} +func testExceptions() throws { try testThrowing { try ValuesKt.throwException(error: false) } do { try ValuesKt.throwException(error: true) @@ -1197,6 +1197,46 @@ func testStringConversion() throws { try test2() } +class GH3825SwiftImpl : GH3825 { + class E : Error {} + + func call0(callback: () -> KotlinBoolean) throws { + if callback().boolValue { throw E() } + } + + func call1(doThrow: Bool, callback: () -> Void) throws { + if doThrow { throw E() } + callback() + } + + func call2(callback: () -> Void, doThrow: Bool) throws { + if doThrow { throw E() } + callback() + } +} + +func testGH3825() throws { + try ValuesKt.testGH3825(gh3825: GH3825SwiftImpl()) + + let test = GH3825KotlinImpl() + var count = 0 + + try testThrowing { try test.call0 { true } } + try test.call0 { + count += 1 + return false + } + try assertEquals(actual: count, expected: 1) + + try testThrowing { try test.call1(doThrow: true) { count += 1 } } + try test.call1(doThrow: false) { count += 1 } + try assertEquals(actual: count, expected: 2) + + try testThrowing { try test.call2(callback: { count += 1 }, doThrow: true)} + try test.call2(callback: { count += 1 }, doThrow: false) + try assertEquals(actual: count, expected: 3) +} + // -------- Execution of the test -------- class ValuesTests : TestProvider { @@ -1253,6 +1293,7 @@ class ValuesTests : TestProvider { TestCase(name: "TestGH3503_3", method: withAutorelease(testGH3503_3)), TestCase(name: "TestGH3525", method: withAutorelease(testGH3525)), TestCase(name: "TestStringConversion", method: withAutorelease(testStringConversion)), + TestCase(name: "TestGH3825", method: withAutorelease(testGH3825)), // Stress test, must remain the last one: TestCase(name: "TestGH2931", method: withAutorelease(testGH2931)),