-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[KT-48068] Add opt-in support for NSEnum for Kotlin Native iOS via an annotation #5539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
/test-private |
|
Failed to process command due to an unexpected exception. |
native/objcexport-header-generator/src/org/jetbrains/kotlin/backend/konan/objcexport/stubs.kt
Outdated
Show resolved
Hide resolved
native/objcexport-header-generator/src/org/jetbrains/kotlin/backend/konan/objcexport/stubs.kt
Outdated
Show resolved
Hide resolved
native/native.tests/testData/framework/objcexport/nativeEnum.kt
Outdated
Show resolved
Hide resolved
kotlin-native/runtime/src/main/kotlin/kotlin/native/Annotations.kt
Outdated
Show resolved
Hide resolved
...-generator/impl/k1/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportTranslator.kt
Show resolved
Hide resolved
...-generator/impl/k1/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportTranslator.kt
Outdated
Show resolved
Hide resolved
...-generator/impl/k1/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportTranslator.kt
Outdated
Show resolved
Hide resolved
...er/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportCodeSpec.kt
Outdated
Show resolved
Hide resolved
...bjcexport-header-generator/src/org/jetbrains/kotlin/backend/konan/objcexport/StubRenderer.kt
Outdated
Show resolved
Hide resolved
|
The K2 (Analysis API, aka AA) implementation is missing, which makes the new test fail in this mode. ./gradlew :native:objcexport-header-generator:testAnalysisApi --tests "org.jetbrains.kotlin.backend.konan.tests.ObjCExportHeaderGeneratorTest"to reproduce. Related code: Line 54 in f90cbf3
|
|
Another thing to fix: adding a declaration to stdlib requires updating the ABI dump. |
I have added this, but AA emits the a comment about the annotation whereas K1 does not... Any suggestions how to resolve this? Current state is the AA output |
| methodBridge: MethodBridge, | ||
| selectorName: String | ||
| ): ObjCToKotlinMethodAdapter { | ||
| val imp = generateObjCImp(symbol.owner, symbol.owner.getLowered<IrSimpleFunction>(), methodBridge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both symbol.owner usages here should be getLowered. The first two arguments of generateObjCImp are supposed to be consistent – either the same function or two functions, one overriding another.
| import org.jetbrains.kotlin.backend.konan.lower.getLoweredConstructorFunction | ||
| import org.jetbrains.kotlin.backend.konan.lower.getObjectClassInstanceFunction | ||
| import org.jetbrains.kotlin.backend.konan.objcexport.* | ||
| import org.jetbrains.kotlin.backend.konan.objcexport.ObjCMethodSpec.BaseMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import?
| adapters += createConstructorAdapter(it.baseMethod) | ||
| } | ||
| is ObjCGetterForNSEnumType -> { | ||
| adapters += createNSEnumAdapter(it.symbol, it.bridge, it. selector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant space in it. selector.
| if (descriptor.kind == ClassKind.ENUM_CLASS) { | ||
| if (namer.getNSEnumFunctionTypeName(descriptor) != null) { | ||
| val superClass = descriptor.getSuperClassNotAny()!! // ordinal is declared in KotlinEnum | ||
| val ordinalDescriptor = superClass.contributedMethods.first { it.name.asString() == "<get-ordinal>" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit less magical way to do that would be:
superClass.contributedMethods.first { it.propertyIfAccessor.name.asString() == "ordinal" }or
superClass.unsubstitutedMemberScope.getContributedVariables(Name.identifier("ordinal"), NoLookupLocation.FROM_BACKEND).single().getter| is ObjCInitMethodForKotlinConstructor -> "$objcClass.${baseMethod.selector},${baseMethod.symbol.signature}" | ||
| is ObjCKotlinThrowableAsErrorMethod -> null | ||
| is ObjCMethodForKotlinMethod -> "$objcClass.${baseMethod.selector},${baseMethod.symbol.signature}" | ||
| is ObjCGetterForNSEnumType -> "$objcClass.$selector,${symbol.signature}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, that's a bit tricky.
IIRC, the dump generated here is used for some additional DCE: if the Objective-C/Swift code doesn't use certain generated method, using this dump we can find the original Kotlin declaration that doesn't need to be exposed then.
The problem with this implementation is therefore the following: if the generated nsEnum property of a class is not used, the compiler will be instructed to hide Enum.ordinal, which is not exactly intended.
See these changes for more details:
| val objCInterface: ObjCInterface, | ||
| ) | ||
|
|
||
| fun TranslatedClass(objCInterface: ObjCInterface) = TranslatedClass(emptyList(), objCInterface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate function is not really necessary – we can simply add a default argument to the TranslatedClass constructor:
class TranslatedClass(
val auxiliaryDeclarations: List<ObjCExportStub> = emptyList(),
val objCInterface: ObjCInterface,
)| val type = mapType(descriptor.defaultType, ReferenceBridge, ObjCRootExportScope) | ||
|
|
||
| namer.getNSEnumFunctionTypeName(descriptor)?.let { nsEnumTypeName -> | ||
| // Map the enum entries in declaration order, preserving the ordinal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ordering here is tricky.
I mean, I couldn't find anything in the KDocs that would mention that it preserves the declaration order. Neither for K1, nor for AA.
Moreover, not many things rely on the declaration order here, and certain parts of the compiler perform alphabetical sorting.
In other words, relying on a specific order here might be fragile. Let's address it the following way: in tests (native/objcexport-header-generator/testData/headers as well as native/native.tests/testData/framework/objcexport), make enum entries have a non-alphabetical order.
So that, if someone starts sorting them alphabetically elsewhere in the compiler, the test will catch that.
| superProtocols = superProtocols, | ||
| members = members, | ||
| attributes = attributes, | ||
| comment = objCCommentOrNull(mustBeDocumentedAttributeList(descriptor.annotations)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general convention in Kotlin is to put the closing ) on a new line whenever the opening ( is on a different line.
|
|
||
| abstract class ObjCTopLevel : ObjCExportStub | ||
|
|
||
| class ObjCNSEnum( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be reasonable to make it also have an explicit Swift name using the swift_name attribute, just like it is done for the rest of the declaration classes.
| listOf("readonly"), | ||
| declarationAttributes = emptyList(), | ||
| comment = null) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the enum already has a property named nsEnum, we'll get a name conflict here.
I don't propose fixing this (as it would be unreasonably complicated), so let's meaningfully postpone it:
please create a new issue (https://youtrack.jetbrains.com/newIssue?project=KT) for this problem.
And also add a // TODO here mentioning that issue, so that the next person reading this code won't need to think about that.
Basically implementing the feature suggested here: https://youtrack.jetbrains.com/issue/KT-48068