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

Kotlin+Dagger best practices/documentation/pain points #900

Open
ronshapiro opened this Issue Oct 15, 2017 · 34 comments

Comments

Projects
None yet
@ronshapiro

ronshapiro commented Oct 15, 2017

Opening this as a tracking bug for all kotlin related documentation that we should be add/best practices that we should call out to make using Dagger w/ Kotlin easier.

One example: How to achieve the effect of static @Provides in Kotlin.

Feel free to comment new ideas, but don't make "me too" or "i agree with XYZ" comments.

@google google deleted a comment from bejibx Oct 16, 2017

@hzsweers

This comment has been minimized.

hzsweers commented Oct 16, 2017

If you have injected properties (as "fields"), qualifiers must have field: designation.

Good

@Inject @field:ForApi lateinit var gson: Gson

Bad

@Inject @ForApi lateinit var gson: Gson // @ForApi is ignored!

Forgetting this can lead to subtle bugs if an unqualified instance of that type also exists on the graph at that scope, as that's the one that will end up being injected. This would make a great lint as well

@hzsweers

This comment has been minimized.

hzsweers commented Oct 16, 2017

Static provides can be achieved via @JvmStatic. There are two scenarios I see this come up:

top-level objects

@Module
object DataModule {
  @JvmStatic @Provides fun provideDiskCache() = DiskCache()
}

If you have an existing class module, things get a bit weirder

@Module
abstract class DataModule {
  @Binds abstract fun provideCache(diskCache: DiskCache): Cache

  @Module
  companion object {
    @JvmStatic @Provides fun provideDiskCache() = DiskCache()
  }
}

The way this works is as follows:

  • the companion object must also be annotated as @Module
  • under the hood, the kotlin compiler will duplicate those static provides methods into the DataModule class. Dagger will see those and treat them like regular static fields. Dagger will also see them in the companion object, but that "module" will get code gen from dagger but be marked as "unused". The IDE will mark this as such, as the provideDiskCache method will be marked as unused. You can tell IntelliJ to ignore this for annotations annotated with @Provides via quickfix

I sent Ron a braindump once of how dagger could better leverage this, i.e. no requirement for JvmStatic and just call through to the generated Companion class, but I think that's out of the scope of this issue :). I've been meaning to write up a more concrete proposal for how that would work.

@hzsweers

This comment has been minimized.

hzsweers commented Oct 16, 2017

Another gotcha is inline method bodies. Dagger relies heavily on return types to connect the dots. In kotlin, specifying the return type is optional sometimes when you can inline the method body. This can lead to confusing behavior if you're counting on implicit types, especially since the IDE will often try to coerce you into quickfixing them away

That is, you could write in one of four ways

// Option 1
@Provides fun provideDiskCache() = DiskCache()

// Option 2
@Provides fun provideDiskCache(): DiskCache = DiskCache()

// Option 3
@Provides fun provideDiskCache(): DiskCache {
  return DiskCache()
}

// Option 4
@Provides fun provideDiskCache(): Cache {
  return DiskCache()
}

// Option 5
@Provides fun provideDiskCache(): Cache = DiskCache()

The first function is valid, but DiskCache is what's on the DI graph there because that's the inferred return type. The first three functions are functionally identical.

The fourth function is also valid, but now Cache is what's on the graph and DiskCache is just an implementation detail. The fifth function is functionally identical to the fourth.

The IDE will try to suggest inlining the fourth one. You can do so, but be mindful of potentially changing return types if you also drop the explicit return type.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Oct 16, 2017

@hzsweers

This comment has been minimized.

hzsweers commented Oct 16, 2017

Good point, tweaked the wording to make it clear that it's only if you drop the return type and added more examples

@tasomaniac

This comment has been minimized.

tasomaniac commented Oct 16, 2017

@JvmSuppressWildcards is incredibly useful when you are injecting classes with generics. Can be handy when using multimap injection.

@jhowens89

This comment has been minimized.

jhowens89 commented Oct 17, 2017

What a great thread! I was fighting this fight this last weekend. Here is the syntax for injection that's both qualified and nullable:

@field:[Inject ChildProvider] @JvmField var coordinatorProvider: CoordinatorProvider? = null

@heinrichreimer

This comment has been minimized.

heinrichreimer commented Oct 26, 2017

@tasomaniac #807 has also cost me quite some time to debug.
Doesn't look nice but at least it works:

@Inject lateinit var foo: Set<@JvmSuppressWildcards Foo>

(taken from https://stackoverflow.com/a/43149382/2037482)

@charlesdurham

This comment has been minimized.

charlesdurham commented Nov 27, 2017

As an add-on from @heinrichreimer, the same thing is required when injecting functions that have parameters:

... @Inject constructor(val functionalThing: @JvmSuppressWildcards(true) (String) -> String)
@InvisibleGit

This comment has been minimized.

InvisibleGit commented Dec 2, 2017

With Kotlin 1.2 we can use array literals in annotations, ditching arrayOf() call in them

@Component(modules = [LibraryModule::class, ServicesModule::class])

@guelo

This comment has been minimized.

guelo commented Jan 23, 2018

As far as I can tell Dagger is unable to inject Kotlin functions. This fails

@Module
class Module() {
  @Provides fun adder(): (Int, Int) -> Int = {x, y -> x + y}
}

class SomeClass {
  @Inject lateinit var adder:  (Int, Int) -> Int

  fun init() {
    component.inject(this)
    println("${adder(1, 2)} = 3")
  }
}

error: kotlin.jvm.functions.Function2<? super java.lang.Integer,? super java.lang.Integer,java.lang.Integer> cannot be provided without an @Provides- or @Produces-annotated method.

Theoretically you could provide a kotlin.jvm.functions.Function2 but I failed to make it work. Even with this

@Provides
fun adder(): kotlin.jvm.functions.Function2<Integer, Integer, Integer> {
  return object : kotlin.jvm.functions.Function2<Integer, Integer, Integer> {
    override fun invoke(p1: Integer, p2: Integer): Integer {
      return Integer(p1.toInt() + p2.toInt())
    }
  }
}

it still says a Function2 cannot be provided.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Jan 23, 2018

@gildor

This comment has been minimized.

gildor commented Feb 8, 2018

I understand that this issue about "best practices", but on Reddit AMA @ronshapiro mentioned that this issue the place to collect kotlin-specific features.
There is a couple possible kotlin-specific features that require work with Kotlin Metadata, but would be very useful for Kotlin projects and make dagger usage even more pleasant

  1. Support typealias as an alternative for @Named and custom @Qualifier annotations. It's completely compiler feature, but you can get typealias name from @metadata annotation, so can use it to detect different qualifiers of the same class
  2. Support nullability as an alternative for Optional value. But I see some drawbacks: you cannot distinguish between non-initilized nullable and inilized one, but it's still can be a usefult and efficient replacement for Optional dependencies.
  3. Native support of lambda injection that doesn't require to use @JvmSuppressWildcards. This proposal can be extended to other generics, like for Lists and so on, but it not always required behavior, but in the case of lambdas I don't see any good reason do not consider any lambda (kotlin.jvm.functions.* interface) as generic without a wildcard. It will make lambda injection much less wordy. Also, works pretty well with typealias as qualifier.
@ronshapiro

This comment has been minimized.

ronshapiro commented May 11, 2018

It would probably be good to make sure that no binding method used default parameters. It wouldn't make sense, and it definitely would be misleading.

@tobeylin

This comment has been minimized.

tobeylin commented Jul 12, 2018

The method injection works well on Kotlin setter.

@set: Inject
lateinit var booleanProvider: Provider<Boolean>

If you need the qualifier, must write the custom setter.

var booleanProvider: Provider<Boolean>? = null
        @Inject set(@MyQualifier value){
            field = value
}
@sophiataskova

This comment has been minimized.

sophiataskova commented Aug 1, 2018

@hzsweers re: your comment on static provides methods in Kotlin, does the Kotlin implementation offer the same performance wins as "true" Java static methods? Does it provide any performance wins? What's your main reason for making your modules this way?

Static provides can be achieved via @JvmStatic. There are two scenarios I see this come up:

top-level objects

@module
object DataModule {
@JvmStatic @provides fun provideDiskCache() = DiskCache()
}
If you have an existing class module, things get a bit weirder

@module
abstract class DataModule {
@BINDS abstract fun provideCache(diskCache: DiskCache): Cache

@module
companion object {
@JvmStatic @provides fun provideDiskCache() = DiskCache()
}
}
The way this works is as follows:

the companion object must also be annotated as @module
under the hood, the kotlin compiler will duplicate those static provides methods into the DataModule class. Dagger will see those and treat them like regular static fields. Dagger will also see them in the companion object, but that "module" will get code gen from dagger but be marked as "unused". The IDE will mark this as such, as the provideDiskCache method will be marked as unused. You can tell IntelliJ to ignore this for annotations annotated with @provides via quickfix
I sent Ron a braindump once of how dagger could better leverage this, i.e. no requirement for JvmStatic and just call through to the generated Companion class, but I think that's out of the scope of this issue :). I've been meaning to write up a more concrete proposal for how that would work.

@hzsweers

This comment has been minimized.

hzsweers commented Aug 1, 2018

static provides allow you to make your modules truly stateless and allows you to simplify plumbing (no instance wiring required). The biggest win there is basically architecture. The "performance" wins of invokestatic always seemed kind of a reach. 25% of something measured in nanoseconds on a non-hotpath seems a nice-to-have rather than a strong motivating factor.

@ronshapiro

This comment has been minimized.

ronshapiro commented Aug 1, 2018

The invokestatic might not be the true benefit - not needing the module instance allows all static provides to get horizontally merged, eliminating the class loading of all of the module classes.

@sophiataskova

This comment has been minimized.

sophiataskova commented Aug 1, 2018

@ronshapiro same question -- does that same behavior occur with Kotlin's "static" implementations? In the case of top-level Kotlin object, an instance of that class still gets created; in the case of companion object, the "static" methods are only accessible through an instance of the companion object. (I am learning both Kotlin and Dagger right now so please correct me if anything I said was false)

@arekolek

This comment has been minimized.

arekolek commented Aug 2, 2018

@sophiataskova See the discussion I had with Jeff Bowman on this topic, maybe you'll find it useful.

@tasomaniac

This comment has been minimized.

tasomaniac commented Aug 2, 2018

My 2 cents:

If somebody put a gun to your head to do 100% Kotlin, sure this is a solution. But I really prefer Java Modules to this. It is totally fine for me to 1 or 2 Java abstract classes in my feature. No inner classes, single flat file of provisions. Much simpler 🎉

@sophiataskova

This comment has been minimized.

sophiataskova commented Aug 2, 2018

Should I take it that Kotlin does not offer any of the advantages of static @Provides that you get in Java?

Ron says that not needing the module instance allows all static provides to get horizontally merged, eliminating the class loading of all of the module classes but both Kotlin implementations offered here seem to still involve class loading for each module. Again, I kinda hope I'm wrong, and can make my app faster without converting to Java.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Aug 2, 2018

First of all, this speed benefit will be extraordinarily minor. If you are looking to make your app faster then use a profiler. You will find 100 easier optimization targets.

Beyond that, don't use companion object for modules. Use object. In that case the instance will be unused and its initialization code will be removed by R8 and the methods will be truly static and can also be inlined just like Java.

@tasomaniac

This comment has been minimized.

tasomaniac commented Aug 2, 2018

It is unfortunately not possible to have abstract methods in object. You would need 2 modules. I try not to mix abstract and static provisions anyways. But if you need to, object is not a solution. This JvmStatic trick allows you to have them in 1 module.

@ronshapiro

This comment has been minimized.

ronshapiro commented Aug 29, 2018

Recording a thought from Droidcon: someone mentioned that @Inject on constructors is awkward in Kotlin because the constructor is often implicit via the properties list. Using the constructor keyword is not idiomatic.

Could we support an annotation on the class, detect that it's a kotlin class, and treat the sole constructor as having @Inject? Or could we have an annotation in modules that adds a binding to a type's sole constructor as if it had @Inject (which may also make adding bindings for code that you don't own easier).

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Aug 29, 2018

@warabei14

This comment has been minimized.

warabei14 commented Aug 30, 2018

@ronshapiro
It would be nice to have at least second one. But both sounds nice. It is very common case to have the only one constructor in kotlin code.
We don't use bindings because at least it looks awkward (imposes requirement to have @inject in a constructor we need to declare first only for that purpose).

class Rectangle @Inject constructor(
        @Named("length") private val length: Int,
        private val width: Int
    )

vs

class Rectangle (
        @Named("length") private val length: Int,
        private val width: Int
    )

And furthermore there are too much work to change @Provides to @Binds, when you need to find all the classes and change them for using inject. It would be much better if all I need to do is to get done the work only by changing modules mostly (if I understand correctly).
In general we very rarely use @Named or any other qualifiers, so the samples could look even clearer.

@tasomaniac

This comment has been minimized.

tasomaniac commented Aug 30, 2018

It is easier to have only 1 constructor in Kotlin since they have default value support. Dagger does not understand that and that's why often times I find myself having secondary constructor with Inject annotation to provide defaults.

If Dagger is going to get a Kotlin specific features, I think understanding the default values provided would be my favorite.

@cgruber

This comment has been minimized.

cgruber commented Aug 31, 2018

@ronshapiro

This comment has been minimized.

ronshapiro commented Aug 31, 2018

@hzsweers

This comment has been minimized.

hzsweers commented Aug 31, 2018

You could have a separate artifact of kotlin-specific annotations that, when used, require Kotlin @Metadata annotations to be present too. In the case of @Inject on a class, kotlin metadata annotations can tell you what the primary constructor is of a given class, and you'd then just want to wire that up in Dagger as a fake @Inject constructor. That'd allow you to have something like @KotlinInject that doesn't tread on the existing JSR330 annotations. That would also allow you to be a bit more intentional about Kotlin support in dagger as well, like expectations of a primary constructor or expectation of metadata annotations.

@JakeWharton

This comment has been minimized.

Member

JakeWharton commented Sep 1, 2018

@cgruber

This comment has been minimized.

cgruber commented Sep 3, 2018

@ronshapiro ronshapiro changed the title from Kotlin+Dagger best practices/documentation to Kotlin+Dagger best practices/documentation/pain points Sep 13, 2018

@tasomaniac

This comment has been minimized.

tasomaniac commented Nov 12, 2018

Here is how you would add Dagger's annotation processor options with Kotlin's kapt. It looks pretty nice actually.

kapt {
  arguments {
    arg('dagger.formatGeneratedSource', 'disabled')
  }
}

You can use ☝️ for all the options mentioned here: https://google.github.io/dagger/compiler-options.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment