Proposal: make assisted arguments explicit

[saket]

kotlin-inject's way of injecting dependencies with assisted arguments is really clever but I've been thinking it's very implicit and hidden for readers. Thoughts on making it explicit in some manner? Using dagger-like @Assisted annotation is one option, but I'm not sure if you'd want that. It'll also remove the restriction of keeping assisted arguments at the end of a constructor, which is another hidden restriction.

[evant]

So one thought I had around this is to recommend typealiases when you are doing assisted injection.

typealias FooFactory = (Baz) -> Foo
@Inject class Foo(injectedArg: Bar, assistedArg: Baz)
...
@Inject class UsesFoo(fooFactory: FooFactory)

Since the typealias is in the same file, you can see which args are assisted as they are the ones explicitly declared in the typealias. It also helps with refactoring if you ever need to change the signature.

It'll also remove the restriction of keeping assisted arguments at the end of a constructor, which is another hidden restriction.

I will say, I ended up quite liking this restriction, it makes the argument ordering consistent. Curious if you had any examples where this ordering wouldn't be ideal?

[saket]

I will say, I ended up quite liking this restriction, it makes the argument ordering consistent. Curious if you had any examples where this ordering wouldn't be ideal?

I'd say this is a really subjective opinion. I know that the preference of ordering varies by person. For example, some people like keeping annotated values towards the end for visual hierarchy.

[PaulWoitaschek]

We always use named arguments for almost everything so that changing the position doesn't break existing code. When developers add a dependency to a class, they usually add it at the last position. If now that dependency was managed by a kotlin-inject factory, this will break the DI setup.

I've seen a few times now, that developers got really confused: "Hey, can you help me? I added that class as a dependency and kotlin-inject says that it can't create the class, but that dependency should be definitely available".

I would really prefer an explicit approach here that makes it clear which arguments of a class are assisted and which should be available in the DI graph.

[evant]

One of the benefits of the current approach is the client gets to decide which dependencies to inject vs providing manually. I'm not sure that actually ever comes up in practice though so may not be worth it over explicit documentation.

[PaulWoitaschek]

That case could still be covered with a secondary constructor if that really ever comes up, right?

[evant]

Ok here's my proposal:

Introduce an @Assisted annotation to make assisted arguments explicit

problem

It can be not obvious which arguments are assisted or which are expected to be provided by the graph. This can make it confusing on why you are unable to inject a certain type. While there is extra flexibility with this on choosing which args you want to inject or not at the usage site, this feature isn't often used.

solution

All assisted arguments must be annotated with an @Assisted annotation. This will limit the usage site to providing exactly those parameters in the same order they are declared. For example with the declaration:

@Inject class AssistedClass(@Assisted arg1: One , arg2: Two, @Assisted arg3: Three)

the only allowed way to inject that class is

@Inject Usage(createAssistedClass: (One, Three) -> AssistedClass)

these will fail to compile with an error about missing or mismatched args

@Inject Usage(createAssistedClass: (One) -> AssistedClass) // missing arg
@Inject Usage(createAssistedClass: (Three, One) -> AssistedClass) // mismatched args
@Inject Usage(createAssistedClass: AssistedClass) // missing args

migration

When this feature is implemented the behavior will be selected based on if there's any args annotated with @Assisted. If so the new strategy will be used, otherwise a warning will be printed to suggest annotating the arg. This warning will then become an error in a future release.

[PaulWoitaschek]

That sounds like a good plan! We should also provide good error messages on this and explain the importance of the ordering. I really like how you take care of this library 💯

[evant]

Yep! Plan to figure out a good way to print out the expected and actual parameter lists on a mismatch.

[PaulWoitaschek]

We can borrow what assertion libraries do. Expected,,, but did not have, ... Different order... That code already exists and is open sourced.

[evant]

You mean something like this? 😉 (in reality I'd probably go for something a bit different since the lists aren't expected to be super long but yea)

[evant]

Just for completeness sake this will apply to function injection too. It already complains about mismatched signatures from the function and typealias so it should be easy to update to complain about missing @Assisted args as well. In other words you'll want to update to:

typealias myFunction = (String) -> String

@Inject
fun myFunction(dep: Dep, @Assisted arg: String): String = ...

[evant]

And one more thing, default args, I propose they work as below:

@Inject class AssistedClass(@Assisted arg1: One = One(), arg2: Two, @Assisted arg3: Three = Three())

you can omit those arguments as long as you keep the order. If they all have defaults you can even inject the type directly. These all work:

@Inject Usage(createAssistedClass: (One, Three) -> AssistedClass)
@Inject Usage(createAssistedClass: (One) -> AssistedClass)
@Inject Usage(createAssistedClass: () -> AssistedClass)
@Inject Usage(assistedClass: AssistedClass)

but this will fail:

@Inject Usage(createAssistedClass: (Three) -> AssistedClass) // mismatch, expected One but got Three

This keeps which param maps to which arg easy to understand even if it's a bit limiting.

[evant]

To round this all out, injected params match by type, assisted params match by position. This makes a lot of sense based on how you use them. You provide them explicitly so making them work the same as regular function params seems the best way to go.