ObjCName annotation #4815

rickclephas · 2022-05-08T19:52:00Z

Relates to KT-42297, KT-50767, KT-48076, KT-48072, KT-43087 and KT-40796.

Sample

@ObjCName(swiftName = "MySwiftArray")
class MyKotlinArray {
    @ObjCName(swiftName = "index")
    fun indexOf(@ObjCName(swiftName = "of") element: String): Int = TODO()
}

// Current usage without the ObjCName annotations
let array = MyKotlinArray()
let index = array.indexOf(element: "element")

// Usage with the ObjCName annotations
let array = MySwiftArray()
let index = array.index(of: "element")

Logic

The ObjCName accepts an ObjC name and a Swift name, both are optional.
The name of the Swift/ObjC declaration is determined as follows:

The Swift name from the ObjCName annotation (Swift only)
The ObjC name from the ObjCName annotation
The Kotlin name from the source file

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

SvyatoslavScherbina · 2022-05-17T14:38:39Z

KT-48948

It is not related: the issue is about producing C API for Kotlin code, the implemented annotation doesn't affect it, IIUC.

rickclephas · 2022-05-17T19:06:17Z

It is not related: the issue is about producing C API for Kotlin code, the implemented annotation doesn't affect it, IIUC.

Yeah you are correct this doesn't affect the C API just Objective-C.

libraries/stdlib/src/kotlin/annotations/NativeAnnotations.kt

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

kotlin-native/backend.native/tests/objcexport/objCName.swift

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

SvyatoslavScherbina · 2022-05-17T18:45:13Z

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

@@ -902,6 +922,45 @@ private fun ObjCExportMapper.canHaveSameName(first: PropertyDescriptor, second:
    return bridgePropertyType(first) == bridgePropertyType(second)
 }

+private fun DeclarationDescriptor.getObjCName(forSwift: Boolean): String {


If the compiler has to mangle an @ObjCName-annotated declaration to prevent a name clash, should it report something in this case?

Ideally yes. But IMO it should report a warning for all mangled names (even if the annotation isn't present).

Yes, I agree. So, in case of a clash, it probably makes sense to emit

an error, if the mangling is applied to something with @ObjCName

a warning, otherwise

What do you think?

Generally, current implementation of naming in ObjCExport makes this not simple, so here I'm mostly trying to bring us at the same page regarding the concept, not insisting on implementing this as a part of this PR.

Yeah I like that! Specifying conflicting @ObjCName annotations is definitely an error.
But that would indeed be a big addition for this PR, so probably best to add that separately.

...ve/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/DefaultErrorMessagesNative.kt

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt

madsager

Sorry for jumping in late in this review. At Google we are interested in controlling Objective-C and Swift names as well. We were going to look into this until I noticed that you are already doing this Rick. Thank you! A couple of questions came to mind while reading the change, so added them as review comments. :)

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

...ve/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/DefaultErrorMessagesNative.kt

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt

SvyatoslavScherbina · 2022-06-06T11:17:59Z

Btw, does the annotation work for enum entries?
Relevant motivational example: https://youtrack.jetbrains.com/issue/KT-40796/Enums-in-multiplatform-code-with-PascalCase-named-entries-are-fully-lower-cased-in-swift

rickclephas · 2022-06-06T11:45:00Z

Btw, does the annotation work for enum entries?

Turns out it can be applied to enum entries, but at the moment it doesn't have any effect.
I guess we should support that as well. Although KT-40796 should probably have a fix itself, right?
Besides that it doesn't make sense to have an exact enum entry name (which is currently allowed as well).

SvyatoslavScherbina · 2022-06-07T08:58:31Z

Although KT-40796 should probably have a fix itself, right?

It should, but this would be a breaking change, which makes fixing it properly harder. Having @ObjCName for that sounds like a nice trade off to me.

kotlin-native/backend.native/tests/objcexport/objCNameB.kt

libraries/stdlib/src/kotlin/annotations/NativeAnnotations.kt

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

SvyatoslavScherbina · 2022-06-08T12:26:55Z

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

+}
+
+private fun CallableDescriptor.getObjCName(): ObjCName =
+        overriddenDescriptors.firstOrNull()?.getObjCName() ?: (this as DeclarationDescriptor).getObjCName()


Can we do this without casts? (also below).
Motivation: Although these are cast to supertypes, the compiler doesn't check that. Now, imagine we are going to rewrite this code to a different representation for declarations. If we forget to change a target type in a particular cast, and we don't have tests covering this case, the compiler won't help us to detect this either, and this bug will remain unnoticed.

Let me propose alternatives:

Implement and use a special generic upcast function that doesn't do as, but simply returns the argument as its supertype.

Use different names for "conflicting" getObjCName functions.

I actually like that the functions have the same name (and naming is hard), so I went with the upcast function.
Not sure if this is a common use case, but I guess upcast could even be part of stdlib.

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

SvyatoslavScherbina · 2022-06-08T12:40:02Z

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

+        }
+
+        fun ValueArgument.getBooleanValue(): Boolean =
+                (getArgumentExpression() as? KtConstantExpression)?.node?.text?.toBoolean() ?: false


This code is used in IDE.
toBoolean will probably throw an exception if the string is not true or false.
This is not fine:

What if it is not a boolean literal, but a reference to a const val? Supporting this case would be complicated, but at least IDE shouldn't throw an exception in this case.

A user might have simply written incorrect code. For example, @ObjCName("abc", exact = "def"). IDE should highlight this as error, but not throw an exception.

Yeah I wasn't sure if this was actually the correct way to get the boolean.
We could use toBooleanOrNull to solve the second issue, but the first one seems harder to fix.

The following compiles fine, but ignores name and exact:

private const val name = "ObjCNameObjCObject" private const val exact = true @ObjCName(name, "ObjCNameSwiftObject", exact) object ObjCNameKotlinObject

Do you have any pointers for this?

As far as I can tell, now that the @ObjCName annotation can change the name, we can no longer compute the name based on the PSI structure, but need to perform resolution to get the name.

I hacked together the following: google@d0232bf

Which just removes the eager computation of the name in the lazy objective-c export code and replaces it with lazy computation based on descriptors which means that resolution has to happen to get the name. I didn't have the time to clean it up, but wanted to share in case it is useful.

@SvyatoslavScherbina does that seem like the right way to go? I don't really see how the lazy objective-c exporter can get the right name unless we go through resolution for it?

I hacked together the following: google@d0232bf

Interesting. Where are the tests for the lazy exporter located?

IIUC testObjCExport is using the lazy exporter and the output (expectedLazy.h) contains the correct names.

but the first one seems harder to fix.

Exactly, that's why I proposed not to support it in IDE for now :)

I hacked together the following: google@d0232bf
@SvyatoslavScherbina does that seem like the right way to go?

Hard to say, I'm not actually an expert in IDE. IIUC, this would mean that indexing would trigger resolution for every class, right? This might be a huge performance hit.

I don't really see how the lazy objective-c exporter can get the right name unless we go through resolution for it?

I see a couple of options. For example, we could prohibit using anything but boolean literals for exact values, or make it a separate annotation (e.g. @ExactObjCName or @ObjCName.Exact).

Interesting. Where are the tests for the lazy exporter located?

expectedLazy.h is the only test I'm aware of.

Alright I think I get the issues now (correct me if I am wrong).

using const val references as values for the annotation isn't currently working and would require resolution.
I don't think there is a real use case for this (since the names and exact value are tightly coupled to a declaration). Meaning we could prohibit it, only allowing literal string and boolean values, correct?

exceptions are thrown in case the arguments aren't in the expected format.
In this case we should make sure to silently ignore such values.
The compiler will fail with an appropriate error message anyway.

expectedLazy.h is the only test I'm aware of.

That one is passing, so we shouldn't have any issues if we prohibit the usage of reference values.

Alright I think I get the issues now (correct me if I am wrong).

using const val references as values for the annotation isn't currently working and would require resolution.
I don't think there is a real use case for this (since the names and exact value are tightly coupled to a declaration). Meaning we could prohibit it, only allowing literal string and boolean values, correct?

Yes. I agree that it is probably fine to limit to string constants and boolean constants for now. If we would like to expand on it in the future, that should be relatively easy as well. In what I hacked up we do resolution always to get the name. However, we can see the @ObjCName in the PSI, so we could do resolution when getting the name only when that annotation is present. Then at least the cost would only be incurred for the declarations where it is necessary. I would be happy to do that if we end up seeing a need, but I would support just limiting to constants for now.

exceptions are thrown in case the arguments aren't in the expected format.
In this case we should make sure to silently ignore such values.
The compiler will fail with an appropriate error message anyway.

That is my understanding as well. If the code cannot be compiled, the editor does a best effort. :)

expectedLazy.h is the only test I'm aware of.

That one is passing, so we shouldn't have any issues if we prohibit the usage of reference values.

toBoolean wouldn't actually throw an exception, but instead return false. Updated to toBooleanStrictOrNull to make the intended behaviour more clear.

The frontend checker will now prohibit non literal string and boolean values, making sure we don't need resolution in the lazy exporter.

I have also added a test case to verify that there aren't any exceptions in case the user writes invalid code.

Alright I think I get the issues now (correct me if I am wrong).

Totally correct, thank you!

rickclephas · 2022-06-12T12:41:39Z

Enum entries are now correctly supported and the frontend checker will prohibit exact on enum entries.

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt

kotlin-native/backend.native/tests/objcexport/objCNameB.kt

...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt

SvyatoslavScherbina · 2022-06-20T16:43:59Z

Status: IIUC, there are no more unresolved comments. We are waiting for the review of the annotation declaration from the Kotlin Libraries team.
I might look through the entire PR again later, just in case I've missed a tricky problem before.

libraries/stdlib/src/kotlin/annotations/NativeAnnotations.kt

...s/tools/binary-compatibility-validator/reference-public-api/kotlin-stdlib-runtime-merged.txt

kotlin-native/backend.native/tests/objcexport/objCName.swift

SvyatoslavScherbina

Status: everything seems fine, the CI is green, we are good to go!
GitHub reports some merge conflicts (?), @rickclephas could you please rebase the branch to master?

Also, please squash the commits and update the message(s). Otherwise, I will squash everything to a single commit and write some commit message myself.

SvyatoslavScherbina · 2022-07-22T14:01:54Z

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt

@@ -465,12 +491,12 @@ internal class ObjCExportNamerImpl(
            parameters.forEachIndexed { index, (bridge, it) ->
                val name = when (bridge) {
                    is MethodBridgeValueParameter.Mapped -> when {
-                        it is ReceiverParameterDescriptor -> ""
+                        it is ReceiverParameterDescriptor -> it.getObjCName().asIdentifier(false) { "" }
                        method is PropertySetterDescriptor -> when (parameters.size) {


(Possible future improvement:)
In certain cases Kotlin property accessors are translated to methods. In this case, @ObjCName can be applied to the setter parameter but likely has no effect.

rickclephas · 2022-07-22T15:39:24Z

@SvyatoslavScherbina rebased and squashed 👍🏻

SvyatoslavScherbina · 2022-07-23T16:59:05Z

Merged.
@rickclephas great job! Thank you for the contribution 👍

rickclephas mentioned this pull request May 9, 2022

HiddenFromObjC and ShouldRefineInSwift annotations #4818

Merged

abelkov added the Native label May 11, 2022

rickclephas force-pushed the feature/objc-name branch 2 times, most recently from 64ceb74 to ffa9465 Compare May 14, 2022 16:20

rickclephas commented May 14, 2022

View reviewed changes

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt Outdated Show resolved Hide resolved

rickclephas marked this pull request as ready for review May 14, 2022 16:22

SvyatoslavScherbina self-requested a review May 17, 2022 14:34

SvyatoslavScherbina reviewed May 17, 2022

View reviewed changes

rickclephas force-pushed the feature/objc-name branch from ffa9465 to 1620108 Compare May 27, 2022 10:25

rickclephas requested a review from SvyatoslavScherbina May 27, 2022 19:49

SvyatoslavScherbina reviewed May 30, 2022

View reviewed changes

madsager reviewed May 31, 2022

View reviewed changes

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt Outdated Show resolved Hide resolved

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt Show resolved Hide resolved

rickclephas force-pushed the feature/objc-name branch 2 times, most recently from 4f11a7e to 5a3eb1a Compare June 4, 2022 17:33

rickclephas commented Jun 4, 2022

View reviewed changes

...piler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportNamer.kt Outdated Show resolved Hide resolved

rickclephas requested a review from SvyatoslavScherbina June 4, 2022 20:06

SvyatoslavScherbina reviewed Jun 6, 2022

View reviewed changes

SvyatoslavScherbina reviewed Jun 8, 2022

View reviewed changes

madsager reviewed Jun 13, 2022

View reviewed changes

native/frontend/src/org/jetbrains/kotlin/resolve/konan/diagnostics/NativeObjCNameChecker.kt Outdated Show resolved Hide resolved

SvyatoslavScherbina reviewed Jun 14, 2022

View reviewed changes

kotlin-native/backend.native/tests/objcexport/objCNameB.kt Show resolved Hide resolved

...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt Show resolved Hide resolved

rickclephas force-pushed the feature/objc-name branch from f6be798 to 215ec19 Compare June 25, 2022 13:40

ilya-g reviewed Jun 28, 2022

View reviewed changes

libraries/stdlib/src/kotlin/annotations/NativeAnnotations.kt Outdated Show resolved Hide resolved

rickclephas force-pushed the feature/objc-name branch from 803968f to 8287ce1 Compare July 20, 2022 10:26

ilya-g reviewed Jul 20, 2022

View reviewed changes

...s/tools/binary-compatibility-validator/reference-public-api/kotlin-stdlib-runtime-merged.txt Outdated Show resolved Hide resolved

SvyatoslavScherbina reviewed Jul 21, 2022

View reviewed changes

kotlin-native/backend.native/tests/objcexport/objCName.swift Outdated Show resolved Hide resolved

SvyatoslavScherbina approved these changes Jul 22, 2022

View reviewed changes

rickclephas force-pushed the feature/objc-name branch from 4256c4e to 2f9ca92 Compare July 22, 2022 15:36

[K/N] Add ObjCName annotation

2cecd9b

rickclephas force-pushed the feature/objc-name branch from 2f9ca92 to 2cecd9b Compare July 23, 2022 09:22

SvyatoslavScherbina merged commit 5a5e6ad into JetBrains:master Jul 23, 2022

rickclephas deleted the feature/objc-name branch July 24, 2022 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjCName annotation #4815

ObjCName annotation #4815

rickclephas commented May 8, 2022 •

edited

Loading

SvyatoslavScherbina commented May 17, 2022

rickclephas commented May 17, 2022

SvyatoslavScherbina May 17, 2022

rickclephas May 18, 2022

SvyatoslavScherbina May 19, 2022

rickclephas May 19, 2022

madsager left a comment

SvyatoslavScherbina commented Jun 6, 2022

rickclephas commented Jun 6, 2022

SvyatoslavScherbina commented Jun 7, 2022

SvyatoslavScherbina Jun 8, 2022

rickclephas Jun 12, 2022

SvyatoslavScherbina Jun 8, 2022

rickclephas Jun 12, 2022

madsager Jun 13, 2022

rickclephas Jun 13, 2022

rickclephas Jun 13, 2022

SvyatoslavScherbina Jun 14, 2022

rickclephas Jun 14, 2022

madsager Jun 15, 2022

rickclephas Jun 18, 2022

SvyatoslavScherbina Jun 20, 2022

rickclephas commented Jun 12, 2022

SvyatoslavScherbina commented Jun 20, 2022

SvyatoslavScherbina left a comment

SvyatoslavScherbina Jul 22, 2022

rickclephas commented Jul 22, 2022

SvyatoslavScherbina commented Jul 23, 2022

ObjCName annotation #4815

ObjCName annotation #4815

Conversation

rickclephas commented May 8, 2022 • edited Loading

Sample

Logic

SvyatoslavScherbina commented May 17, 2022

rickclephas commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsager left a comment

Choose a reason for hiding this comment

SvyatoslavScherbina commented Jun 6, 2022

rickclephas commented Jun 6, 2022

SvyatoslavScherbina commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickclephas commented Jun 12, 2022

SvyatoslavScherbina commented Jun 20, 2022

SvyatoslavScherbina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickclephas commented Jul 22, 2022

SvyatoslavScherbina commented Jul 23, 2022

rickclephas commented May 8, 2022 •

edited

Loading