Skip to content

Commit 45d6c85

Browse files
SvyatoslavScherbinaSpace Team
authored andcommitted
[K/N] Don't generate generic safe casts for Objective-C types
With `genericSafeCasts=true` enabled by default, the Kotlin/Native compiler now generates additional type checks to make sure that generic type erasure doesn't allow objects of unexpected types to slip through. As a justified side effect, this also checks types of objects passed from Objective-C libraries. Objective-C is tricky, though. For example, it is common to use "duck typing" in Objective-C: if an Obj-C function has an Obj-C class or protocol as the return type, the return value doesn't need to actually be of that type: it is enough to respond to the same method calls. Such mismatches happen even in Apple's own frameworks, which caused KT-85508. So, it is justified to not generate such type checks for Objective-C types. This commit does exactly that (and also removes handling of special cases of Objective-C types there, as they are now covered with the general Obj-C type exclusion). It is also safe enough: calls to Objective-C methods mostly use dynamic dispatch, which will fail with a readable error if a method is not implemented. As a side effect, this commit also fixes KT-85399: in that case, due to a bug in cinterop (KT-56860), Kotlin translated the imported Objective-C type wrong. So, the value was of the correct type, but Kotlin didn't know it. Improving handling of Obj-C types in generic type checks is tracked in KT-85681. ^KT-85358 Fixed ^KT-85399 Fixed ^KT-85508 Fixed ^RCA: the regression reached the release because it occurs in interaction with tricky Objective-C code that wasn't present in our tests but is found in the wild. Basically, sometimes the declared Obj-C type or its Kotlin translation doesn't match the actual object type when getting values from Obj-C, and it was unexpected. This commit adds regression tests and effectively disables `genericSafeCasts` for Obj-C types to make sure these and similar problems won't occur further. The problem was triggered by enabling `genericSafeCasts` in 21b204a. (cherry picked from commit dd46827)
1 parent 9261a6f commit 45d6c85

4 files changed

Lines changed: 44 additions & 21 deletions

File tree

kotlin-native/backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/lower/Autoboxing.kt

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ import org.jetbrains.kotlin.ir.declarations.*
2323
import org.jetbrains.kotlin.ir.expressions.*
2424
import org.jetbrains.kotlin.ir.expressions.impl.IrConstantPrimitiveImpl
2525
import org.jetbrains.kotlin.ir.irAttribute
26-
import org.jetbrains.kotlin.ir.objcinterop.isObjCForwardDeclaration
27-
import org.jetbrains.kotlin.ir.objcinterop.isObjCMetaClass
2826
import org.jetbrains.kotlin.ir.symbols.*
2927
import org.jetbrains.kotlin.ir.symbols.impl.IrFieldSymbolImpl
3028
import org.jetbrains.kotlin.ir.symbols.impl.IrPropertySymbolImpl
@@ -182,8 +180,6 @@ private class AutoboxingTransformer(val context: Context) : AbstractValueUsageTr
182180
private fun IrClass.canBeAssignedTo(expectedClass: IrClass) =
183181
this.isNothing() || expectedClass == anyClass /* A workaround for plugins emitting classes with empty superTypes */
184182
|| (expectedClass.isCompanion && expectedClass.parentAsClass.isObjCClass()) // TODO: a workaround for CMP-9000.
185-
// TODO: roll back once MapLibre is fixed (KT-85358).
186-
|| (expectedClass.isObjCClass() && expectedClass.name.asString() == "MLNScaleBar")
187183
|| this.symbol.isSubtypeOfClass(expectedClass.symbol)
188184

189185
private fun IrExpression.adaptIfNecessary(actualType: IrType, expectedType: IrType, skipTypeCheck: Boolean = false): IrExpression {
@@ -205,8 +201,34 @@ private class AutoboxingTransformer(val context: Context) : AbstractValueUsageTr
205201
// conservatively insert type check for them (due to unsafe casts).
206202
&& actualClass?.canBeAssignedTo(erasedExpectedType.getClass()!!) != true
207203
&& actualType.getInlinedClassNative() == null
208-
&& !erasedExpectedClass.isObjCForwardDeclaration()
209-
&& !erasedExpectedClass.isObjCMetaClass() // See KT-65260 for details.
204+
/*
205+
Objective-C is tricky.
206+
207+
First of all, it is common to use "duck typing" in Objective-C:
208+
e.g., if an Obj-C method has an Obj-C class or protocol as the return type, the return value doesn't need to
209+
actually be of that type: it is enough to respond to the same method calls.
210+
Such mismatches happen even in Apple's own frameworks, e.g. in KT-85508.
211+
212+
Next, there are forward declarations, and a cast to one shouldn't be generated as we don't have enough information
213+
for that.
214+
215+
Another kind of problem is when the target type can't be found by the linker, like in KT-84678.
216+
217+
Finally, sometimes cinterop translates Objective-C types wrong, e.g. in KT-56860.
218+
So, even if the Objective-C code returns a real instance of the declared type, Kotlin can get the latter wrong.
219+
220+
In other words, in many cases, casts to Obj-C types shouldn't be generated here.
221+
On the other hand, we won't lose much if we don't generate casts to Obj-C types here at all:
222+
most usages of Obj-C objects in Kotlin go through dynamic Objective-C method dispatch. So, having the wrong type
223+
won't lead to heap corruption. Instead, a method call can fail with "unrecognized selector sent to instance", which
224+
is close enough to a `TypeCastException`.
225+
There are also `objc_direct` methods that use direct dispatch (and thus could lead to heap corruption if the type is
226+
wrong), but they are not widely used.
227+
228+
All in all, it is safe and easier to not generate casts to Obj-C types here.
229+
Improving this is tracked in KT-85681.
230+
*/
231+
&& !erasedExpectedClass.isObjCClass()
210232
-> {
211233
this.checkedCast(actualType, erasedExpectedType)
212234
}

native/native.tests/testData/codegen/cinterop/kt57640_with_safe_casts.kt

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,12 @@ private fun testAssignmentBaseToDerived() {
7575

7676
val derivedFoo = derived as Foo3Protocol
7777
val derivedBar = derived as Bar3Protocol
78-
assertFailsWith<ClassCastException> { val delegate14_Foo: Foo3Protocol? = derivedFoo.delegate }
79-
assertFailsWith<ClassCastException> { val delegate15_Bar: Bar3Protocol? = derivedBar.delegate }
80-
assertFailsWith<ClassCastException> { val delegate16_Bar: Bar3Protocol? = derived.delegate() }
78+
// TODO KT-85681: we might want to make it throw a `ClassCastException`.
79+
val delegate14_Foo: Foo3Protocol? = derivedFoo.delegate
80+
// TODO KT-85681
81+
val delegate15_Bar: Bar3Protocol? = derivedBar.delegate
82+
// TODO KT-85681
83+
val delegate16_Bar: Bar3Protocol? = derived.delegate()
8184
}
8285

8386
private fun testAssignmentBaseToDerivedWithPropertyOverride() {
@@ -87,9 +90,14 @@ private fun testAssignmentBaseToDerivedWithPropertyOverride() {
8790

8891
val derived_Foo = derived as Foo3Protocol
8992
val derivedBar = derived as Bar3Protocol
90-
assertFailsWith<ClassCastException> { val delegate19_DerivedWithPropertyOverride: DerivedWithPropertyOverride3? = derived.delegate() }
91-
assertFailsWith<ClassCastException> { val delegate14_Foo: Foo3Protocol? = derived_Foo.delegate }
92-
assertFailsWith<ClassCastException> { val delegate18_Foo: Foo3Protocol? = derived_Foo.delegate() } // static type is FooProtocol?, not DerivedWithPropertyOverride?, this deserves a diagnostic test
93-
assertFailsWith<ClassCastException> { val delegate15_Bar: Bar3Protocol? = derivedBar.delegate }
94-
assertFailsWith<ClassCastException> { val delegate19_Bar: Bar3Protocol? = derivedBar.delegate() } // static type is BarProtocol?, not DerivedWithPropertyOverride?, this deserves a diagnostic test
93+
// TODO KT-85681
94+
val delegate19_DerivedWithPropertyOverride: DerivedWithPropertyOverride3? = derived.delegate()
95+
// TODO KT-85681
96+
val delegate14_Foo: Foo3Protocol? = derived_Foo.delegate
97+
// TODO KT-85681
98+
val delegate18_Foo: Foo3Protocol? = derived_Foo.delegate() // static type is FooProtocol?, not DerivedWithPropertyOverride?, this deserves a diagnostic test
99+
// TODO KT-85681
100+
val delegate15_Bar: Bar3Protocol? = derivedBar.delegate
101+
// TODO KT-85681
102+
val delegate19_Bar: Bar3Protocol? = derivedBar.delegate() // static type is BarProtocol?, not DerivedWithPropertyOverride?, this deserves a diagnostic test
95103
}

native/native.tests/testData/codegen/cinterop/objc/kt85399.kt

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
// TARGET_BACKEND: NATIVE
22
// DISABLE_NATIVE: isAppleTarget=false
33

4-
// KT-85399
5-
// IGNORE_NATIVE: optimizationMode=DEBUG
6-
// IGNORE_NATIVE: optimizationMode=NO
7-
84
// MODULE: cinterop
95
// FILE: lib.def
106
language = Objective-C

native/native.tests/testData/codegen/cinterop/objc/kt85508.kt

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
// TARGET_BACKEND: NATIVE
22
// WITH_PLATFORM_LIBS
33
// DISABLE_NATIVE: isAppleTarget=false
4-
5-
// KT-85508
6-
// DISABLE_NATIVE: isAppleTarget=true
74
@file:OptIn(kotlinx.cinterop.ExperimentalForeignApi::class)
85

96
import platform.Network.NW_PARAMETERS_DISABLE_PROTOCOL

0 commit comments

Comments
 (0)