Workaround Swift bug (SR-12201) triggering #3825 (#3852)

Avoid placing Objective-C error out parameter right after block parameters.
Fix #3825.

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))
 

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 <ValuesGH3825>
+- (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<ValuesIAbstractInterface>)x __attribute__((swift_name("testAbstractInterfaceCall(x:)")));
 + (int32_t)testAbstractInterfaceCall2X:(id<ValuesIAbstractInterface2>)x __attribute__((swift_name("testAbstractInterfaceCall2(x:)")));
 + (void)fooA:(ValuesKotlinAtomicReference<id> *)a __attribute__((swift_name("foo(a:)")));
++ (BOOL)testGH3825Gh3825:(id<ValuesGH3825>)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")));

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<kotlin.native.internal.ObjCErrorException>(block = block)
+        assertFailsWith<ObjCErrorException>(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<kotlin.native.internal.ObjCErrorException> {
+    assertFailsWith<ObjCErrorException> {
         if (flag) {
             test.plusOne(0)
         } else {
@@ -935,4 +936,43 @@ class TestStringConversion {
     lateinit var str: Any
 }
 
-fun foo(a: kotlin.native.concurrent.AtomicReference<*>) {}
+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<ObjCErrorException> { gh3825.call0({ true }) }
+    gh3825.call0({ count += 1; false })
+    assertEquals(1, count)
+
+    assertFailsWith<ObjCErrorException> { gh3825.call1(true, { fail() }) }
+    gh3825.call1(false, { count += 1 })
+    assertEquals(2, count)
+
+    assertFailsWith<ObjCErrorException> { gh3825.call2({ fail() }, true) }
+    gh3825.call2({ count += 1 }, false)
+    assertEquals(3, count)
+}

backend.native/tests/framework/values/values.swift

@@ -316,15 +316,15 @@ class SwiftNotThrowingWithBridge : ThrowsWithBridge {
     }
 }
 
-func testExceptions() throws {
+private func testThrowing(_ block: () throws -> Void) throws {
-    func testThrowing(_ block: () throws -> Void) throws {
+    do {
-        do {
+        try block()
-            try block()
+    } catch let error as NSError {
-        } catch let error as NSError {
+        try assertTrue(error.kotlinException is MyException)
-            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)),