kohesive / injekt

Dependency Injection for Kotlin
MIT License
235 stars 20 forks source link

Generics support #13

Closed yoavst closed 9 years ago

yoavst commented 9 years ago

I have a simple parser interface

public interface Parser<K> {
    public fun parse(document: Document): K
}

so I register singletons based on this interface:

addSingleton(javaClass<Parser<CourseDetails>>(), CourseDetailsParser)
addSingleton(javaClass<Parser<Array<CourseTitle>>>(), CoursesParser)
addSingleton(javaClass<Parser<MessageData>>(), MessageParser)
addSingleton(javaClass<Parser<Array<MessageTitle>>>(), MessagesParser)
addSingleton(javaClass<Parser<RequestData>>(), RequestParser)
addSingleton(javaClass<Parser<Array<RequestTitle>>>(), RequestsParser)
addSingleton(javaClass<Parser<CalendarWeek>>(), WeeklyCalendarParser)

However when I try to get Injekt.get(javaClass<Parser<Array<CourseTitle>>>()) it return me wrong one (CourseDetailsParser instead of CoursesParser)

apatrida commented 9 years ago

Good point, easy change. 1 level can be done with reified generics, more levels of generics require a TypeReference style class, which I have handy from upcoming data binder project.

So we will be able to quickly support: generics 1 level reified without any other effort, generics multi-level if you use a TypeReference but you would need it at the inject site too, so a little ugly, and Array element types should be ok, that is a different trick in itself, but I recently handled that one in another project so I think it will apply here.

I'll take a peek and try to have a release out tomorrow.

apatrida commented 9 years ago

Also note, you can avoid using javaClass and either can use this form:

addSingleton<MyClass>(instance)
apatrida commented 9 years ago

A temporary work around for you, assuming you aren't at some limit on number of classes in Android.

public class CoursesParser : Parser<Array<CourseTitle> >{ ... }

then the CoursesParser type can be used instead of the generic type.

apatrida commented 9 years ago

It looks like M12 reified generics gives me the whole parameterized type tree fine, more than 1 level. So going with that and making the change.

yoavst commented 9 years ago

addSingleton<MyClass>(instance) is ugly for long classes, at least for me,. And I'm trying to keep it generic.

If It looks like M12 reified generics gives me the whole parameterized type tree fine, more than 1 level, what is the problem?

apatrida commented 9 years ago

You can also infer the type from the instance if it is the actual type and not a descendant.

yoavst commented 9 years ago

I know I can. However, I want to be able to test only the class that uses the parser, rather then testing the class + parser. So I'm using an interface to allow me do that.

apatrida commented 9 years ago

Ok, great. Almost done with the fix.

apatrida commented 9 years ago

Note that in Kotlin M13 that javaClass<T>() is deprecated and it'll be dropped from Inject.

Using T::class isn't sufficient either because it erases types.

So for Injekt: use the infered type, the generics in the method signature, or after this 1.4 release use typeRef<T>() instead of javaClass<T>().

yoavst commented 9 years ago

Is M13 released yet? I saw there are a lot of versions on Kotlin's github, but not a maven version.

apatrida commented 9 years ago

No, M13 is not released. But it has a huge change in reflection so when it comes, Injekt and some other libraries will have to update to match. I keep another branch updated for M13 so trying to make sure changes to Injekt for M12 line up with that future.

yoavst commented 9 years ago

Will Injekt require kotlin-reflections? Also, when will you push the update?

apatrida commented 9 years ago

no it doesn't. It only uses it in tests.

And upcoming data binder won't either, but if you add it, you'll get extra happy functionality like being able to deal with nullable values, default values, and other Kotlin specific things that other data binders cannot see (Jackson, Gson, etc)

yoavst commented 9 years ago

So when will you push the update to M12?

Also, data binder seem to be too much for DI, for my case at least.

apatrida commented 9 years ago

Data binder is separate project, an uses Injekt.

M12 in a few minutes. Just updated README and about to do the build.

apatrida commented 9 years ago

release 1.4.0 sent to Maven Central, should be available in 10-15 minutes.

yoavst commented 9 years ago

I've asked in gitter, but maybe it is better to ask here. I have like 20 api calls, so I have those 2 methods:

    private inline fun parser<reified K>(): Parser<K> = Injekt.get(fullType<Parser<K>>())
    private inline fun arrayParser<reified K>(): Parser<Array<K>> = Injekt.get(fullType<Parser<Array<K>>>())

However, it fails with the exception of:

09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ java.lang.AssertionError: illegal type variable reference
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at libcore.reflect.TypeVariableImpl.resolve(TypeVariableImpl.java:110)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at libcore.reflect.TypeVariableImpl.getGenericDeclaration(TypeVariableImpl.java:124)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at libcore.reflect.TypeVariableImpl.hashCode(TypeVariableImpl.java:47)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at java.util.Objects.hashCode(Objects.java:90)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at libcore.reflect.GenericArrayTypeImpl.hashCode(GenericArrayTypeImpl.java:49)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at java.util.Arrays.hashCode(Arrays.java:1261)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at libcore.reflect.ParameterizedTypeImpl.hashCode(ParameterizedTypeImpl.java:95)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:746)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at uy.kohesive.injekt.registry.default.DefaultRegistrar.getByKey(DefaultRegistrar.kt:21)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at uy.kohesive.injekt.registry.default.DefaultRegistrar.getInstance(DefaultRegistrar.kt:99)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at uy.kohesive.injekt.api.InjektScope.getInstance(Scope.kt)
09-04 07:59:23.654    5004-5165/com.yoavst.openu W/System.err﹕ at com.yoavst.openu.controller.ApiControllerImpl$getRequests$1.invoke(ApiControllerImpl.kt:136)
apatrida commented 9 years ago

Hi @yoavst ... It is because the type isn't really coming through the layers of reified generics. When TypeVariableImpl ends up being passed to me as a Type it means something bad happened along the chain of the type information.

fullType<Parser<K>>() cannot work because it will receive that literally through to the function, I will receive type Parser and it will have generics of type TypeVariableImpl name K which means that it has no idea what is in that place.

Kotlin reification works basically at one level of calling and the inline-able code is already decided before you then inline again with a wrapper function. So my inlining sees Parser<K> and that byte code is decided, then you inline again hoping that K will be replaced at the caller of parser() method with whatever they have available for the compiler to see. But it doesn't do that.

There are limitations with Reified generics, and you hit one. You could file a YouTrack issue asking if it could be resolved, but that would mean the inline function would change its code for each user of the function, and I don't think it was intended to do that.

You could make some pre-packaged type references and keep those around to use. Maybe make some constants that are things like:

val INT_PARSER = fullType<Parser<Int>>()

then use those during injection. Injekt.get(INT_PARSER)

Or if your injection site already has the type, you don't need anything. (for others watching, they should know they can also do this)

val myVar: Parser<Int> = Injekt.get()
val myMemberProperty: Parser<Int> by Delegates.injectValue()

so if you are using for local variables you might not be specifying the type and you are doing that in the get() call instead.

The change I should make to Injekt is to see a TypeVariableImpl come through and throw an IllegalStateException that things are messed up in the reified generic.

yoavst commented 9 years ago

Very disappointing. At least it is better than Java.

Also, thanks for the information, it is a very useful stuff that you can't find in other places :+1:

apatrida commented 9 years ago

I can't even make a test case that works like yours, get a recursive error in the compiler, I reported it and then I can look at it more. Was trying to add the better error message.

apatrida commented 9 years ago

And be disappointed, but in the Java VM, it is where the issue lies. It is very hard to circumvent how it works, and the compiler can only do so many tricks safely.