-
Notifications
You must be signed in to change notification settings - Fork 566
Kpgalligan/20190315/generics #2850
Kpgalligan/20190315/generics #2850
Conversation
...iler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/CustomTypeMapper.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
Updated with feedback. Much appreciated. Please let me know if you have more. |
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
open class GenBase<T:Any>(val t:T) | ||
class GenEx<TT:Any, T:Any>(val myT:T, baseT:TT):GenBase<TT>(baseT) |
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.
Would the test pass if TT
was named T
instead?
I.e. class GenEx<T:Any, S:Any>(val myT:S, baseT:T):GenBase<T>(baseT)
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 updated test with your example and it's OK.
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.
Sure.
I mean that ge.t
here is AnyObject
, but it turns out that Swift allows to access .num
on it.
Please turn this line into
let geT: SomeData = ge.t
try assertEquals(actual: geT.num, expected: 11)
and the test will fail to compile.
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.
Btw, renaming T
in GenEx
to TT
makes the resulting header invalid.
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.
This is related to how the supertype is declared. I wasn't outputting the type arguments, which compiles but creates issues. I'm not sure the best way to get the supertype. I can get the super class, but without the type args. During compile, classes are mostly represented by LazyClassDescriptor
, which has computeSupertypes()
, which returns Collection<KotlinType>
, and I can get what I need from there, but that's obviously not a good idea (it's not public). Before I go digging around more, is there an obvious way to get the equivalent with a ClassDescriptor
?
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.
Try classDescriptor.typeConstructor.supertypes
.
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.
Thanks! I didn't see this reply till just now. Spent more time yesterday trying to navigate the parsing code. Assuming I can get this to work, should have an update today.
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
nameSet.contains(kotlinType.typeParameterName()) | ||
override fun getName(typeParameterDescriptor: TypeParameterDescriptor?): String? = | ||
if(typeParameterDescriptor != null && types.contains(typeParameterDescriptor)){ | ||
typeParameterDescriptor.name.asString() |
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.
Somewhat clash-prone.
Kotlin:
class Clashing<id> {
fun foo(x: Any) {}
}
Swift:
Clashing<NSString>().foo(x: NSObject())
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.
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.
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.
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.
- Some "built-in" types emitted (like
id
) - Objective-C types imported to Kotlin (e.g.
NSObject
) when exported to Objective-C header - Kotlin class names (not that serious, because Kotlin class names are prefixed)
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.
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.
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.
This is ok for now.
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.
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.
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.
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!
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 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.
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.
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.
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 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.
|
||
if (!typeProjection.type.isTypeParameter() && mapper.shouldBeExposed(typeParamClass)) { | ||
if (typeParamClass.isInterface) | ||
referenceProtocol(typeParamClass) |
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.
What is this required for? How is it different from what mapReferenceTypeIgnoringNullability
does?
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 had a very special case of self-referencing type that didn't have a forward declaration and was producing a failing header, but am unable to repro. It was something like:
class SelfRef : GenBasic<SelfRef>()
but that's not causing trouble. Likely fixed by another change. Will remove. The code was forcing the forward declaration for that type, but that should happen anyway if needed.
@@ -316,8 +378,8 @@ internal class ObjCExportTranslatorImpl( | |||
} | |||
|
|||
private fun StubBuilder.translatePlainMembers(methods: List<FunctionDescriptor>, properties: List<PropertyDescriptor>) { | |||
methods.forEach { +buildMethod(it, it) } | |||
properties.forEach { +buildProperty(it, it) } | |||
methods.forEach { +buildMethod(it, it, genericExportScope(it.containingDeclaration)) } |
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 more consistent to receive ObjCExportScope
as an argument here.
//needed. If upper bounds are added back to type parameters, this may need to be expanded. | ||
fun selfReferencingClassType(descriptor: ClassDescriptor): Boolean { | ||
val parentType = computeSuperClassType(descriptor) | ||
return parentType != null && parentType.arguments.any { descriptor == it.type.getErasedTypeClass() } |
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 is not the only case.
Instead of trying to hack it here, please try disabling this optimization:
Line 1018 in 3a0be1c
if (descriptor !in generatedClasses) classForwardDeclarations += objCName |
(i.e. always add forward declaration).
I find it a better option. I would enable it back after some refactoring if required.
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
...ackend.native/src/org/jetbrains/kotlin/backend/konan/objcexport/ObjCExportHeaderGenerator.kt
Outdated
Show resolved
Hide resolved
Extensions could in theory also have generics defined, but I hadn't put much thought into that and how they're translated currently. Those now explicitly ignore generics and can potentially be refactored. The forward declaration now writes all classes to avoid the self-reference issue, but only if generics flag enabled. In a code base of any reasonable size that forward declaration will get long, but I'm not sure that'll have much impact. My experience with j2objc says you can give the compiler some pretty huge Objective-C without much trouble, but refactoring that later would be good. |
It's ok. I'm not sure if proper full support is possible here, e.g.
doesn't seem to be representable in Objective-C.
I don't think it will have any impact. |
} | ||
|
||
private fun classNameSet(element: TypeParameterDescriptor): MutableSet<String> { | ||
return typeParameterNameClassOverrides.getOrPut(element.containingDeclaration as ClassDescriptor) { |
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.
Doesn't seem correct for (yet unsupported) outer class type parameters.
class Foo<T> {
inner class Bar<T>
}
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.
Generics are definitely not supporting outer generics on nested or inner types at this point. Although the resulting syntax isn't great, it's possible to cast your way around it. Can be added, though. Will defer to you. Will add if you think this would be valuable.
|
||
func testGenericInheritance() throws { | ||
let ge = GenEx<SomeData, SomeOtherData>(myT:SomeOtherData(str:"Hello"), baseT:SomeData(num: 11)) | ||
try assertTrue(ge.t is SomeData, "base property not SomeData") |
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.
AFAIK, this checks only the dynamic type, not the static one.
So this assert doesn't ensure that ge.t
is statically known to be SomeData
and can be assigned to a a SomeData
-typed variable.
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.
Yeah, that check doesn't really do what I want. I've replaced how that works (in next push) for all type checks. Rather than check type, if the type isn't correct Swift needs a conversion, so the fact that it compiles and runs should imply that the types are as expected.
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.
What do you mean?
How does the test now ensure that ge.t
is declared as SomeData
?
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.
That line is old. The last push was referencing the value instead of "is SomeData", which should fail to compile if not SomeData
.
try assertEquals(actual: ge.t.num, expected: 11)
However, I have some more testing to figure out as the properties and methods of the base class being written in the child class means disabling the superclass generics didn't cause that test to fail.
__attribute__((objc_subclassing_restricted))
__attribute__((swift_name("GenEx")))
@interface MainGenEx<TT, T> : MainGenBase
- (instancetype)initWithMyT:(T)myT baseT:(TT)baseT __attribute__((swift_name("init(myT:baseT:)"))) __attribute__((objc_designated_initializer));
- (instancetype)initWithT:(id)t __attribute__((swift_name("init(t:)"))) __attribute__((objc_designated_initializer)) __attribute__((unavailable));
- (TT)goBase __attribute__((swift_name("goBase()")));
@property (readonly) T myT __attribute__((swift_name("myT")));
@property (readonly) TT t __attribute__((swift_name("t")));
@end;
Property 't' is still SomeData even though we don't tell MainGenBase what T
is because t
is also written to MainGenEx
. If we specify T
as Any
, Swift will report true with is SomeData
but requires a cast to SomeData
.
In this example, GenExAny is defined as:
class GenExAny<TT:Any, T:Any>(val myT:T, baseT:TT):GenBase<Any>(baseT)
That forces the cast to SomeData
. If GenEx was also writing t
as Any/id
, it should fail to compile.
However, I went in and disabled the generic superclass write to see what would fail and while one thing did fail, it was less than I expected. Will be diving into that more and update with better testing.
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.
ge.t.num
doesn't fail because Swift seems to allow accessing any method or property on id
.
That's why the proper way to test the type is to store the value into explicitly typed variable:
let geT: SomeData = ge.t
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.
Tests updated to specific types and assigned as above. I wasn't understanding why the parent class property was being written out, but I see that now. It filters by string.
__attribute__((swift_name("GenEx")))
@interface MainGenEx<TT, T> : MainGenBase
- (instancetype)initWithMyT:(T)myT baseT:(TT)baseT __attribute__((swift_name("init(myT:baseT:)"))) __attribute__((objc_designated_initializer));
- (instancetype)initWithT:(id)t __attribute__((swift_name("init(t:)"))) __attribute__((objc_designated_initializer)) __attribute__((unavailable));
- (TT)goBase __attribute__((swift_name("goBase()")));
@property (readonly) T myT __attribute__((swift_name("myT")));
@property (readonly) TT t __attribute__((swift_name("t")));
@end;
I'm not sure why redefining the property exists in current code, so I'm a little reluctant to try to be smarter about filtering for generics. The possible parameter name differences make redefining more likely, but it should still work as expected.
I added an example that won't redefine the property to the test just to make sure that works.
open class GenBase<T:Any>(val t:T)
class GenEx<TT:Any, T:Any>(val myT:T, baseT:TT):GenBase<TT>(baseT)
class GenEx2<T:Any, S:Any>(val myT:S, baseT:T):GenBase<T>(baseT)
Can write a smarter filter, but want to be clear on the cases it exists for.
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 filters by string.
This machinery has to be revised later. Let's keep current simple filter for now.
@@ -181,6 +181,9 @@ class K2NativeCompilerArguments : CommonCompilerArguments() { | |||
@Argument(value = "-Xcoverage-file", valueDescription = "<path>", description = "Save coverage information to the given file") | |||
var coverageFile: String? = null | |||
|
|||
@Argument(value = "-Xobjc-generics", description = "Write objc header with generics support") |
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.
Could description be more like "Enable experimental generics support for framework header"
?
To be consistent with other options and clearly stating experimental status.
@@ -599,7 +660,14 @@ internal class ObjCExportTranslatorImpl( | |||
return if (classDescriptor.isInterface) { | |||
ObjCProtocolType(referenceProtocol(classDescriptor).objCName) | |||
} else { | |||
ObjCClassType(referenceClass(classDescriptor).objCName) | |||
val typeArgs = if (objcGenerics) { | |||
kotlinType.arguments.map { typeProjection -> |
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.
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.
I believe we can merge this PR after getting serious issues fixed, ignoring remaining ones for now, to make this feature included into 1.3.40. Issues to be fixed before merge: cc @olonho |
There is an issue that comes up with generics, Swift, and inner classes. The header defines the Swift class name with a '.' between the outer and inner classes, which can cause a crash. For example, from the tests, this is OK let innerClass = GenOuter.GenInner<SomeData, SomeOtherData>(GenOuter<SomeOtherData>(a: SomeOtherData(str: "ggg")), c: SomeData(num: 66), aInner: SomeOtherData(str: "ttt")) However, when you go to use that in Swift, it can actually crash the Swift compiler with a seg fault. Uncomment these lines in the test to see this. I've done some local header testing, and manually removing the '.' from the Swift class name allows the Swift to compile and run. Turning off generics and keeping the '.' will also compile and run (if you remove the generics from the Swift code as well, obviously). In summary, if '.' is a valid character in a class name, this looks like a Swift compiler bug. I think we have the following options:
|
Generally '.' is a valid character in class name. See Import as member (SE-0044). So this crash is likely a bug in Swift compiler. #import <Foundation/Foundation.h>
@interface A : NSObject
@end;
NS_SWIFT_NAME(A.B)
@interface AB<T> : NSObject
@end;
void takeAB(AB<id>* ab);
let ab = A.B<AnyObject>()
takeAB(ab) swiftc 1.swift -import-objc-header 1.h Also it's not clear how importing as member should work if outer class has generics, because Swift itself seems to require specifying type arguments even when accessing static members (like nested classes): class A<T> {
class B<S> {}
}
let ab = A<Any>.B<Any>() So let's avoid using '.' in class names if |
// Swift doesn't support neither nested nor outer protocols. | ||
false | ||
} | ||
generateClassOrProtocolSwiftName(descriptor) |
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 original implementation was supposed to use mangled outer class name, but the new one mangles the concatenated name instead, which is not desirable.
Updated Swift generics naming issue fix. For a class like the following class GenOuterDeep2() {
inner class Before()
inner class GenShallowOuterInner() {
inner class GenShallowInner<T>() {
inner class GenDeepInner()
}
}
inner class After()
} The Swift names will be as follows GenOuterDeep2()
GenOuterDeep2.Before(rec)
GenOuterDeep2.After(rec)
GenOuterDeep2GenShallowOuterInner(rec)
//etc Dropping the '.' only happens if a deeper class has a defined generic, but otherwise naming remains as it was. |
One more coming. Still an issue with nested. |
Available under `-Xobjc-generics` flag.
This is a draft post to start the discussion around generics output in Objective-C headers. There are a number of limitations involved, but this is also a much discussed feature, so we've made an attempt to implement them and see if the development community finds this useful. A longer explanation of the design and compromise decisions can be found here: https://www.kgalligan.com/kotlin-native-interop-generics/