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

replace TyppeLiteral anonymous-object syntax with KType #21

Open
Groostav opened this issue Oct 21, 2020 · 3 comments
Open

replace TyppeLiteral anonymous-object syntax with KType #21

Groostav opened this issue Oct 21, 2020 · 3 comments

Comments

@Groostav
Copy link

Groostav commented Oct 21, 2020

the extensive use of this function:

inline fun <reified T> typeLiteral() = object : TypeLiteral<T>() {}

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 Groostav changed the title replace anonymous-object syntax with KType replace TyppeLiteral anonymous-object syntax with KType Oct 21, 2020
@johnlcox
Copy link
Member

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.

@Groostav
Copy link
Author

Groostav commented Oct 23, 2020

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants