kotlin-guice icon indicating copy to clipboard operation
kotlin-guice copied to clipboard

replace TyppeLiteral anonymous-object syntax with KType

Open Groostav opened this issue 4 years ago • 3 comments

the extensive use of this function:

https://github.com/misfitlabsdev/kotlin-guice/blob/7d2a0523b4449da0b2d80e1217a052a3d881753d/kotlin-guice/src/main/kotlin/dev/misfitlabs/kotlinguice4/TypeLiteral.kt#L29

means you are going to end up with a lot of class files pretaining to anonymous overloads. The TypeLiteral hack was added by google as a clever way to avoid type-erasure.

But in a library such as this, I believe you should use java.lang.reflect.Type and kotlin.reflect.KType, which each do not suffer from type-erasure and will not require any additional *.class files.

If you're unfamiliar with java.lang.reflect.Type, its effectively wrappers on fully-qualified class strings such as "java.util.ArrayList<? extends java.lang.String>" and they can be used to preserve generic info, making them the obvious choice for guice-binding logic, they're just difficult for users to handle correctly.

I suggest using something like this:

inline fun <reified T> kotlinTypeKey(
        annotation: Annotation? = null,
        annotationType: KClass<out Annotation>? = null
): Key<T> {
    if(annotation != null) require(annotationType == null)
    if(annotationType != null) require(annotation == null)

    val ktype = typeOf<T>()
    val baseType = ktype.javaType

    val result = when {
        annotation != null -> Key.get(baseType, annotation)
        annotationType != null -> Key.get(baseType, annotationType.java)
        else -> Key.get(baseType)
    }

    @Suppress("UNCHECKED_CAST")
    return result as Key<T>
}

Once completed, there should be a (measurable) reduction in class files and possibly a measurable increase in performance, particularly for slow hard-drive computers using *.class file distributions (rather than jar distributions).

an implementation note:

while this is technically 1.3 compatible, on 1.3.61 the javaType extension in kotlin-jvm will throw. Unfortunately this cant be solved via a simple if-statement because if you expose

public inline fun <reified T> someBindWrapper(...) {
  if(version == "1.4") {
    //do lightweight j.l.r.Type & KType binding
    doBinding(typeOf<T>())
  }
  else {
    //use heavyweight anonymous typeLiteral overload
    doBinding(object: TypeLiteral<T>(){}) //<-- generates a class file
  }
}

then, of course, because the if statement cannot be done at compile time, the compiler will generate the class files that we're trying to avoid in the first place! While the native community might solve this with something like #IFDEF KOTLIN_1_4, for better and for worse we have no such device

I'm really not sure what an elegent solution here is:

  • You could of course create a separate release for 1.4+ and 1.0-1.3, but that's cloogy and opens the door for version hell.
  • You could simply wait to introduce such a patch until 1.4 becomes main-stream, and then add a hard-dependency on kotlin 1.4.
  • You could shadow (read: copy and paste into a new package) the javaType extension function from 1.4. Its not huge and its not too dependent on kotlin 1.4, but it is a reasonably sophisticated string parser and it is responsible for mapping things like kotlin.String to java.lang.String, and kotlin.collections.List<A> to java.util.List<? extends A>.

Anyways, if you're interested, I'd be happy to give an implemenation a try and issue a pull request!

Groostav avatar Oct 21 '20 21:10 Groostav

This sounds like an interesting idea. Currently typeOf() is still marked with @ExperimentalStdlibApi even on version 1.4.10 so that concerns me.

The experimental issue aside, I'm curious how this would change the binding DSL. I played around using your kotlinTypeKey example and one side-effect I encountered using a Key instead of TypeLiteral is the annotations always need to be defined up front so the DSL method annotatedWith would be completely removed in Kotlin usage.

johnlcox avatar Oct 22 '20 14:10 johnlcox

Currently typeOf() is still marked with @ExperimentalStdlibApi even on version 1.4.10

Thats a very good point, I'm sorry I missed it. I wonder how other DSL's using reified T to handle generic types get the type parameter info out of the T, if not through typeOf --i presume not everybody is using the object: T {} strategy.

I'm curious how this would change the binding DSL.

I think you can provide a workaround.

when using @JvmName, you cant overload based on the number of type arguments, so what im doing is having two separate methods

  • inline fun <reified T> bind(): ...
  • inline fun <reified T, reified A: Annotation> bindAnnotated()

if you wanted to keep strict compatibility, I think it would be possible to box-up a KType instance into a Key, and then unbox it to a TypeLiteral on a called to annotatedWith, to re-wrap it into a new Key. If you're thinking its a pretty long route to a binding, but it'l get you there.

Groostav avatar Oct 23 '20 19:10 Groostav

I am still hoping to get back around to this, im just swamped

Groostav avatar Nov 13 '20 20:11 Groostav