Skip to content
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

Kpgalligan/20190315/generics #2850

Merged

Conversation

Projects
None yet
4 participants
@kpgalligan
Copy link
Contributor

commented Apr 5, 2019

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/

@kpgalligan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Updated with feedback. Much appreciated. Please let me know if you have more.

@kpgalligan kpgalligan marked this pull request as ready for review Apr 6, 2019

}

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

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 8, 2019

Contributor

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)

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 8, 2019

Author Contributor

I updated test with your example and it's OK.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

Btw, renaming T in GenEx to TT makes the resulting header invalid.

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 16, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 17, 2019

Contributor

Try classDescriptor.typeConstructor.supertypes.

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 23, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

Somewhat clash-prone.
Kotlin:

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

Swift:

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

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 9, 2019

Contributor

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)

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 23, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 24, 2019

Contributor

This is ok for now.

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 24, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 24, 2019

Contributor

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!

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 25, 2019

Contributor

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.

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 25, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 25, 2019

Contributor

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)

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 24, 2019

Contributor

What is this required for? How is it different from what mapReferenceTypeIgnoringNullability does?

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 24, 2019

Author Contributor

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)) }

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 24, 2019

Contributor

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() }

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 24, 2019

Contributor

It is not the only case.
Instead of trying to hack it here, please try disabling this optimization:


(i.e. always add forward declaration).
I find it a better option. I would enable it back after some refactoring if required.

@kpgalligan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

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.

@SvyatoslavScherbina

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Extensions could in theory also have generics defined, but I hadn't put much thought into that and how they're translated currently.

It's ok. I'm not sure if proper full support is possible here, e.g.

class Foo<T : Bound>
fun <T : MoreSpecificBound> Foo<T>.bar()

doesn't seem to be representable in Objective-C.

In a code base of any reasonable size that forward declaration will get long, but I'm not sure that'll have much impact.

I don't think it will have any impact.
Having forward declarations in one line shouldn't affect the compiler. And the size is limited by the amount of classes.

}

private fun classNameSet(element: TypeParameterDescriptor): MutableSet<String> {
return typeParameterNameClassOverrides.getOrPut(element.containingDeclaration as ClassDescriptor) {

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 25, 2019

Contributor

Doesn't seem correct for (yet unsupported) outer class type parameters.

class Foo<T> {
    inner class Bar<T>
}

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 26, 2019

Author Contributor

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")

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 25, 2019

Contributor

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.

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 26, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 26, 2019

Contributor

What do you mean?

How does the test now ensure that ge.t is declared as SomeData?

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 26, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina Apr 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@kpgalligan

kpgalligan Apr 29, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina May 6, 2019

Contributor

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")

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina May 6, 2019

Contributor

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 ->

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina May 6, 2019

Contributor

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.

@SvyatoslavScherbina

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

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:
#2850 (comment)
#2850 (comment)

cc @olonho

@kpgalligan

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

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:

  1. Leave it as is, although this is a frustrating bug if you happen to hit it
  2. Not include the '.' in class names
  3. Not include the '.' in class names, but only if generics are enabled (to avoid breaking current code)
@SvyatoslavScherbina

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Generally '.' is a valid character in class name. See Import as member (SE-0044).
The document doesn't state that this feature works for classes too, however in fact it does, and this is used by system frameworks.

So this crash is likely a bug in Swift compiler.
Reproducer:
1.h:

#import <Foundation/Foundation.h>
@interface A : NSObject
@end;

NS_SWIFT_NAME(A.B)
@interface AB<T> : NSObject
@end;

void takeAB(AB<id>* ab);

1.swift:

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 -Xobjc-generics is enabled and either inner or outer class has generics.

// Swift doesn't support neither nested nor outer protocols.
false
}
generateClassOrProtocolSwiftName(descriptor)

This comment has been minimized.

Copy link
@SvyatoslavScherbina

SvyatoslavScherbina May 13, 2019

Contributor

The original implementation was supposed to use mangled outer class name, but the new one mangles the concatenated name instead, which is not desirable.

@kpgalligan

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

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.

@kpgalligan

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

One more coming. Still an issue with nested.

@SvyatoslavScherbina SvyatoslavScherbina merged commit e553684 into JetBrains:master May 14, 2019

LepilkinaElena added a commit that referenced this pull request May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.