Skip to content
This repository has been archived by the owner on Aug 10, 2021. It is now read-only.

Kpgalligan/20190315/generics #2850

Merged
merged 26 commits into from
May 14, 2019
Merged

Kpgalligan/20190315/generics #2850

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a89b4e2
Generics support
kpgalligan Mar 18, 2019
3027193
Update for tests
kpgalligan Mar 19, 2019
f798121
More generics
kpgalligan Mar 29, 2019
92fbe6c
Updated generic types
kpgalligan Apr 2, 2019
3f79a03
Boolean flag to toggle generics, disable upper bounds
kpgalligan Apr 2, 2019
3363c82
External config for generics
kpgalligan Apr 2, 2019
fa5ddee
Java system property to enable/disable generics
kpgalligan Apr 3, 2019
e71c10c
Test updates for flag
kpgalligan Apr 4, 2019
29e0e40
Merge branch 'master' into kpgalligan/20190315/generics
kpgalligan Apr 4, 2019
c030659
Update readme
kpgalligan Apr 5, 2019
9c2b095
Update with changes from feedback
kpgalligan Apr 6, 2019
41342ac
Refactor names and support inner and nested classes
kpgalligan Apr 8, 2019
366d648
Clash cleanup, supertype generics
kpgalligan Apr 18, 2019
56c52bb
Delay outputting generic parameters until all class names are known
kpgalligan Apr 22, 2019
1cf5783
Name clashing tests
kpgalligan Apr 22, 2019
f2aed6a
Refactor namer
kpgalligan Apr 22, 2019
68ae5e4
Refactored clashing logic for class and protocol names
kpgalligan Apr 23, 2019
7b2bc6d
Merge remote-tracking branch 'upstream/master' into mastermerge
kpgalligan Apr 23, 2019
3a0be1c
Remove class name mangling
kpgalligan Apr 24, 2019
ea11c37
Simplified forward declaration
kpgalligan Apr 25, 2019
3f38a28
Test updates
kpgalligan Apr 26, 2019
6a4111e
Updated tests to verify types, especially on inherited types
kpgalligan Apr 29, 2019
897d5c0
Handle inner class generics properly, update experimental message
kpgalligan May 7, 2019
21458b2
Remove dot in Swift class name when generics involved
kpgalligan May 7, 2019
d3d8744
Refactored Swift name issue with generics
kpgalligan May 13, 2019
8d3b3fe
Prevent dot in Swift name when containing classes have generics
kpgalligan May 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions ...r/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/CustomTypeMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import org.jetbrains.kotlin.types.TypeUtils

internal interface CustomTypeMapper {
val mappedClassId: ClassId
fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, typeParameterProvider: TypeParameterProvider): ObjCNonNullReferenceType
fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, objCExportScope: ObjCExportScope): ObjCNonNullReferenceType
}

internal object CustomTypeMappers {
Expand Down Expand Up @@ -81,7 +81,7 @@ internal object CustomTypeMappers {
objCClassName: String
) : this(mappedClassId, { objCClassName })

override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, typeParameterProvider: TypeParameterProvider): ObjCNonNullReferenceType =
override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, objCExportScope: ObjCExportScope): ObjCNonNullReferenceType =
ObjCClassType(translator.getObjCClassName())
}

Expand All @@ -97,14 +97,14 @@ internal object CustomTypeMappers {

override val mappedClassId = ClassId.topLevel(mappedClassFqName)

override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, typeParameterProvider: TypeParameterProvider): ObjCNonNullReferenceType {
override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, objCExportScope: ObjCExportScope): ObjCNonNullReferenceType {
val typeArguments = mappedSuperType.arguments.map {
val argument = it.type
if (TypeUtils.isNullableType(argument)) {
// Kotlin `null` keys and values are represented as `NSNull` singleton.
ObjCIdType
} else {
translator.mapReferenceTypeIgnoringNullability(argument, typeParameterProvider)
translator.mapReferenceTypeIgnoringNullability(argument, objCExportScope)
}
}

Expand All @@ -115,16 +115,16 @@ internal object CustomTypeMappers {
private class Function(parameterCount: Int) : CustomTypeMapper {
override val mappedClassId: ClassId = KotlinBuiltIns.getFunctionClassId(parameterCount)

override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, typeParameterProvider: TypeParameterProvider): ObjCNonNullReferenceType {
override fun mapType(mappedSuperType: KotlinType, translator: ObjCExportTranslatorImpl, objCExportScope: ObjCExportScope): ObjCNonNullReferenceType {
val functionType = mappedSuperType

val returnType = functionType.getReturnTypeFromFunctionType()
val parameterTypes = listOfNotNull(functionType.getReceiverTypeFromFunctionType()) +
functionType.getValueParameterTypesFromFunctionType().map { it.type }

return ObjCBlockPointerType(
translator.mapReferenceType(returnType, typeParameterProvider),
parameterTypes.map { translator.mapReferenceType(it, typeParameterProvider) }
translator.mapReferenceType(returnType, objCExportScope),
parameterTypes.map { translator.mapReferenceType(it, objCExportScope) }
)
}
}
Expand Down
73 changes: 31 additions & 42 deletions ...end.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ internal class ObjCExportTranslatorImpl(
val mapper: ObjCExportMapper,
val namer: ObjCExportNamer,
val warningCollector: ObjCExportWarningCollector,
val objcGenerics:Boolean
val objcGenerics: Boolean
) : ObjCExportTranslator {

private val kotlinAnyName = namer.kotlinAnyName
Expand Down Expand Up @@ -186,7 +186,7 @@ internal class ObjCExportTranslatorImpl(
)
}
ClassKind.ENUM_CLASS -> {
val type = mapType(descriptor.defaultType, ReferenceBridge, NoneTypeParameterProvider)
val type = mapType(descriptor.defaultType, ReferenceBridge, ObjCNoneExportScope)

descriptor.enumEntries.forEach {
val entryName = namer.getEnumEntrySelector(it)
Expand Down Expand Up @@ -548,7 +548,7 @@ internal class ObjCExportTranslatorImpl(
method.allOverriddenDescriptors.forEach { exportThrown(it) }
}

private fun genericTypeParameterFactory(method: FunctionDescriptor) = if(objcGenerics){ClassParameterProvider(method)}else{NoneTypeParameterProvider}
private fun genericTypeParameterFactory(method: FunctionDescriptor) = if(objcGenerics){ObjCClassExportScope(method)}else{ObjCNoneExportScope}

private fun mapReturnType(returnBridge: MethodBridge.ReturnValue, method: FunctionDescriptor): ObjCType = when (returnBridge) {
MethodBridge.ReturnValue.Void -> ObjCVoidType
Expand All @@ -566,16 +566,16 @@ internal class ObjCExportTranslatorImpl(
MethodBridge.ReturnValue.Instance.FactoryResult -> ObjCInstanceType
}

internal fun mapReferenceType(kotlinType: KotlinType, typeParameterProvider: TypeParameterProvider): ObjCReferenceType =
mapReferenceTypeIgnoringNullability(kotlinType, typeParameterProvider).let {
internal fun mapReferenceType(kotlinType: KotlinType, objCExportScope: ObjCExportScope): ObjCReferenceType =
mapReferenceTypeIgnoringNullability(kotlinType, objCExportScope).let {
if (kotlinType.binaryRepresentationIsNullable()) {
ObjCNullableReferenceType(it)
} else {
it
}
}

internal fun mapReferenceTypeIgnoringNullability(kotlinType: KotlinType, typeParameterProvider: TypeParameterProvider): ObjCNonNullReferenceType {
internal fun mapReferenceTypeIgnoringNullability(kotlinType: KotlinType, objCExportScope: ObjCExportScope): ObjCNonNullReferenceType {
class TypeMappingMatch(val type: KotlinType, val descriptor: ClassDescriptor, val mapper: CustomTypeMapper)

val typeMappingMatches = (listOf(kotlinType) + kotlinType.supertypes()).mapNotNull { type ->
Expand Down Expand Up @@ -606,11 +606,13 @@ internal class ObjCExportTranslatorImpl(
}

mostSpecificMatches.firstOrNull()?.let {
return it.mapper.mapType(it.type, this, typeParameterProvider)
return it.mapper.mapType(it.type, this, objCExportScope)
}

if(objcGenerics && kotlinType.isTypeParameter() && typeParameterProvider.typeAvailable(kotlinType)){
return ObjCGenericType(kotlinType.typeParameterName())
if(objcGenerics && kotlinType.isTypeParameter()){
val typeParameterName = objCExportScope.getName(TypeUtils.getTypeParameterDescriptorOrNull(kotlinType))
if(typeParameterName != null)
return ObjCGenericType(typeParameterName)
}

val classDescriptor = kotlinType.getErasedTypeClass()
Expand All @@ -631,7 +633,7 @@ internal class ObjCExportTranslatorImpl(
} else {
val typeArgs = if (objcGenerics) {
kotlinType.arguments.map { typeProjection ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, this doesn't match ObjCClassExportScope:

class Outer<T> {
    inner class Inner<S>
}

fun foo(obj: Outer<String>.Inner<Any>) {}

->

@interface HelloOuterInner<S> : KotlinBase

but

+ (void)fooObj:(HelloOuterInner<id, NSString *> *)obj __attribute__((swift_name("foo(obj:)")));

Supporting inner classes properly in ObjCClassExportScope would fix this.

mapReferenceTypeIgnoringNullability(typeProjection.type, typeParameterProvider)
mapReferenceTypeIgnoringNullability(typeProjection.type, objCExportScope)
}
} else {
emptyList()
Expand Down Expand Up @@ -668,8 +670,8 @@ internal class ObjCExportTranslatorImpl(
return ObjCIdType
}

private fun mapType(kotlinType: KotlinType, typeBridge: TypeBridge, typeParameterProvider: TypeParameterProvider): ObjCType = when (typeBridge) {
ReferenceBridge -> mapReferenceType(kotlinType, typeParameterProvider)
private fun mapType(kotlinType: KotlinType, typeBridge: TypeBridge, objCExportScope: ObjCExportScope): ObjCType = when (typeBridge) {
ReferenceBridge -> mapReferenceType(kotlinType, objCExportScope)
is ValueTypeBridge -> {
when (typeBridge.objCValueType) {
ObjCValueType.BOOL -> ObjCPrimitiveType("BOOL")
Expand Down Expand Up @@ -1019,49 +1021,36 @@ private fun ObjCExportNamer.ClassOrProtocolName.toNameAttributes(): List<String>
private fun swiftNameAttribute(swiftName: String) = "swift_name(\"$swiftName\")"
private fun objcRuntimeNameAttribute(name: String) = "objc_runtime_name(\"$name\")"

interface TypeParameterProvider{
fun typeAvailable(kotlinType: KotlinType):Boolean
interface ObjCExportScope{
fun getName(typeParameterDescriptor: TypeParameterDescriptor?): String?
}

internal class ClassParameterProvider(container:DeclarationDescriptor): TypeParameterProvider {
constructor(method: FunctionDescriptor) : this(method.containingDeclaration)
private val nameSet:Set<String>
internal class ObjCClassExportScope constructor(method: FunctionDescriptor): ObjCExportScope {
private val types:List<TypeParameterDescriptor>
init {
nameSet = mutableSetOf()
if(container is ClassDescriptor && !container.isInterface) {
container.declaredTypeParameters.forEach {
nameSet.add(it.name.asString())
}
val container = method.containingDeclaration
types = if(container is ClassDescriptor && !container.isInterface) {
container.declaredTypeParameters
} else {
emptyList()
}
}

override fun typeAvailable(kotlinType: KotlinType): Boolean =
nameSet.contains(kotlinType.typeParameterName())
override fun getName(typeParameterDescriptor: TypeParameterDescriptor?): String? =
if(typeParameterDescriptor != null && types.contains(typeParameterDescriptor)){
typeParameterDescriptor.name.asString()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat clash-prone.
Kotlin:

class Clashing<id> {
    fun foo(x: Any) {}
}

Swift:

Clashing<NSString>().foo(x: NSObject())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, interesting. Xcode seems to be OK with id, which is a surprise, but it should be renamed. I want to think about that a bit to see if there are other problematic names.

Copy link
Collaborator

@SvyatoslavScherbina SvyatoslavScherbina Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xcode seems to be OK with id, which is a surprise

id is just a type name. It is not forbidden to reuse such names for anything else.
The problem here is that we are trying to refer id in foo, i.e. from the scope where it is overridden by type parameter.

if there are other problematic names.

Sure!
Potentially type parameter name can override name of any type. E.g.

  1. Some "built-in" types emitted (like id)
  2. Objective-C types imported to Kotlin (e.g. NSObject) when exported to Objective-C header
  3. Kotlin class names (not that serious, because Kotlin class names are prefixed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through a couple iterations on 2. I was hoping to delay writing names until the end as it looks like stubs delay rendering, but they can evaluate earlier, so that didn't work. Then I landed on the far easier method of checking type param names and objc class/protocol names, and appending underscores to either. So, in the following:

class GenClashNames<ValuesGenericsClashnameClass, ValuesGenericsClashnameProtocol, ValuesGenericsValues_genericsKt>() {
    fun foo(): Any = ClashnameClass("nnn")
}

data class ClashnameClass(val str: String)
interface ClashnameProtocol {
    val str: String
}
@interface ValuesGenericsGenClashNames<ValuesGenericsClashnameClass,

The type param name is above, the class name written later is:

@interface ValuesGenericsClashnameClass_ : KotlinBase

This is rare, obviously, and swift names don't change. Let me know what you think. The alternatives seemed like a lot of work for little gain.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to clarify, "Wanted a more complete implementation". I'm just cleaning up tests and had planned to push last night, but wanted better test coverage and also want a full review that with the experimental flag off that nothing is changed, but the implementation is "complete" assuming I don't find anything with tests and review.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you.

the generator code evaluates properties, methods, and parent classes first

Yes, this is the detail I missed.

Summary, the type param may be mangled because is clashes with a class name, but not the other way around.

That's great!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the generator code evaluates properties, methods, and parent classes first

Btw, what if type parameter is used in properties or methods?

class Foo<ValuesGenericsBar> {
    fun foo(p: ValuesGenericsBar) {}
    fun bar(p: Bar) {}
}

class Bar

->

__attribute__((objc_subclassing_restricted))
__attribute__((swift_name("Foo")))
@interface ValuesGenericsFoo<ValuesGenericsBar> : KotlinBase
// ...
- (void)fooP:(ValuesGenericsBar _Nullable)p __attribute__((swift_name("foo(p:)")));
- (void)barP:(ValuesGenericsBar *)p __attribute__((swift_name("bar(p:)")));
@end;

I'm still not sure if it makes much sense to fix this now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm not happy that I missed that. My testing got caught up in the outer class definition. Anyway, the RenderedStub will render the name as buildAsDeclaredOrInheritedMethods creates a Set, which hash/equals results in rendering the underlying stub. To avoid this, we'd either need run a pass through the classes properties and methods before calling collectMethodsOrProperties (or some similar first pass, perhaps higher up), or mangle the class name.

It is obviously unlikely that names would clash, and if we're expecting significant refactoring before generics would be standard, if ever, then fixing later is OK, as long as it's addressed at some point.

Let me know if you want me to deal with it now. I'd be inclined to pass through class members in StubBuilder.translateClassMembers to flush out name issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it ok to stop at this point.
We have some plans to fully rework clash handling in generated header, and I believe that we'll handle then these clashes between type parameters and classes.

} else {
null
}
}

internal object NoneTypeParameterProvider: TypeParameterProvider{
override fun typeAvailable(kotlinType: KotlinType): Boolean = false
internal object ObjCNoneExportScope: ObjCExportScope{
override fun getName(typeParameterDescriptor: TypeParameterDescriptor?): String? = null
}


private fun Variance.objcDeclaration():String = when(this){
Variance.OUT_VARIANCE -> "__covariant "
Variance.IN_VARIANCE -> "__contravariant "
else -> ""
}

//To anybody reviewing this code, I don't know my way
//around the Kotlin parsed type system too well. What
//I'm looking for here is generic type name minus the '?'
//I'm sure there's a much simpler way to get that
internal fun KotlinType.typeParameterName(): String {
return if (this.isMarkedNullable) {
val ts = this.toString()
if (ts.endsWith("?"))
ts.substring(0, ts.length - 1)
else
ts
} else {
this.toString()
}
}
16 changes: 16 additions & 0 deletions backend.native/tests/framework/values_generics/values.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,21 @@ func testGenericInheritance() throws {
try assertEquals(actual: geBase.t.num, expected: 11)
}

func testGenericInnerClass() throws {

let nestedClass = GenOuter.GenNested<SomeData>(b: SomeData(num: 543))
try assertEquals(actual: nestedClass.b.num, expected: 543)

let innerClass = GenOuter.GenInner<SomeData>(GenOuter<SomeOtherData>(a: SomeOtherData(str: "ggg")) as! GenOuter<AnyObject>, c: SomeData(num: 66), aInner: SomeOtherData(str: "ttt") as AnyObject)
try assertEquals(actual: innerClass.c.num, expected: 66)

let nestedClassSame = GenOuterSame.GenNestedSame<SomeData>(a: SomeData(num: 545))
try assertEquals(actual: nestedClassSame.a.num, expected: 545)

let innerClassSame = GenOuterSame.GenInnerSame<SomeOtherData>(GenOuterSame<SomeData>(a: SomeData(num: 44)) as! GenOuterSame<AnyObject>, a: SomeOtherData(str: "rrr"))
try assertEquals(actual: innerClassSame.a.str, expected: "rrr")
}

// -------- Execution of the test --------

class ValuesGenericsTests : TestProvider {
Expand All @@ -170,6 +185,7 @@ class ValuesGenericsTests : TestProvider {
TestCase(name: "TestGenericUseSiteVariance", method: withAutorelease(testGenericUseSiteVariance)),
TestCase(name: "TestGenericInheritance", method: withAutorelease(testGenericInheritance)),
TestCase(name: "TestGenericInterface", method: withAutorelease(testGenericInterface)),
TestCase(name: "TestGenericInnerClass", method: withAutorelease(testGenericInnerClass)),
]
}
}
11 changes: 10 additions & 1 deletion backend.native/tests/framework/values_generics/values_generics.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ fun variContraType():GenVarIn<SomeData>{
}

open class GenBase<T:Any>(val t:T)
class GenEx<TT:Any, T:Any>(val myT:T, baseT:TT):GenBase<TT>(baseT)
class GenEx<T:Any, S:Any>(val myT:S, baseT:T):GenBase<T>(baseT)

class GenNullability<T:Any>(val arg: T, val nArg:T?){
fun asNullable():T? = arg
Expand All @@ -88,3 +88,12 @@ fun starGeneric(arg: GenNonNull<*>):Any{
return arg.arg
}

class GenOuter<A:Any>(val a:A){
class GenNested<B:Any>(val b:B)
inner class GenInner<C:Any>(val c:C, val aInner:A)
}

class GenOuterSame<A:Any>(val a:A){
class GenNestedSame<A:Any>(val a:A)
inner class GenInnerSame<A:Any>(val a:A)
}