[AssistedInject] Validate that parameters names match between factory method and the constructor

[lwasyl]

With the following code:

package com.example

import dagger.Component
import dagger.assisted.Assisted
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject

class NamingRepro @AssistedInject constructor(
    @Assisted private val foo: String,
    @Assisted private val bar: String,
) {

    @AssistedFactory
    interface Factory {
        fun create(
            bar: String,
            foo: String
        ): NamingRepro
    }

    fun print() {
        println("""
            Passed foo: $foo,
            passed bar: $bar
        """.trimIndent())
    }
}

@Component
interface NamingComponent {

    fun repro(): NamingRepro.Factory
}

Dagger will ignore the fact that foo and bar parameters order is different in the create method and the constructor. The generated factory will then pass arguments from create to the constructor in unexpected (or expected, but clearly not intentional) order. So:

DaggerNamingComponent.create()
    .repro()
    .create(bar = "bar", foo = "foo")
    .print()

will print

Passed foo: bar,
passed bar: foo

This is error prone and difficult to prevent using automated tools. It would be great if Dagger verified the parameters names as well, requiring them to match between the factory method and the constructor.

Also as far as I understand, square/AssistedInject doesn't verify the parameters order, but forces matching names (if there were more than one parameter with the same type). This can lead to some confusing migration errors, as now the behavior is the opposite.

[bcorso]

Thanks! that's a good point.

Originally, we decided not to rely on parameter names because the .class files don't include parameter names by default. However, I think you're right that we should validate the parameter names to avoid subtle issues like you describe above. This change may require adding -parameters flag when compiling with javac, but at least anyone using Android Gradle Plugin will get it added automatically. We will still require that the parameters be in the same order as the @Assisted parameters though.

I'll start working on a fix for this.

[frett]

square AssistedInject actually reverted the requirement on parameter name matching, and Jake Wharton posted this comment regarding it: cashapp/InflationInject#157 (comment)

[lwasyl]

Good catch, I wasn't aware of that one as it seems like it never went live 😕 Although the behavior since the last public square/AssistedInject release changes, so it still might be a surprising migration for some.

Do you know why incap means parameter names sometimes aren't present? Would that be the case for when factory and the injected types are in separate files, or there's some more common scenario?

edit: Gradle docs suggest the -parameters flag should solve the issue you're mentioning?

"Aggregating" processors have the following limitations:

• (...)
• They can only read parameter names if the user passes the -parameters compiler argument.

[JakeWharton]

I am strongly opposed to parameter name matching, as was said. Once you start using base interfaces for factories and incremental compilation it will break. Our (square's) AssistedInject is simply broken in current form and that change needs released.

Using qualifiers, like regular Dagger matching, is a logical and conceptually easy concept. You'll hear the typical calls of "boilerplate" at least once, but correctness trumps the slight added verbosity in this case which isn't even all that common.

[lwasyl]

This all makes sense. I'm not familiar with the technical limitations, but if qualifiers are really the only way, then validation should be adjusted to fail if there are unqualified parameters of the same type. My primary motivation for this issue was to require more effort to create wrong bindings, if qualifiers are the only technically appropriate way to fix this then 🤷

[Chang-Eric]

Hm, I'm currently still leaning towards just keeping the current behavior of relying on the parameter order for disambiguation. The issue with using qualifiers is that I think it can cause confusion because from someone using the factory, the qualifier does nothing. Someone might look at a factory that has a method create(@Foo Object, @Bar Object) and mistakenly think that there's going to be some type of check that those passed in values somehow came from some binding. It would get especially confusing because when there isn't a conflict like create(Object) we want to prevent you from using a qualifier for exactly this reason, so whether a qualifier is allowed on a parameter would depend on the existence of other parameters. Also, from reading the @AssistedInject side of the code, I personally tend to always take a second to remember which params are coming from Dagger and which are passed in (I've always found it ambiguous if Dagger is assisting you or you're assisting Dagger), and having qualifiers on both I think would add to that confusion.

While relying on parameter order isn't my favorite thing either, since adding a new parameter to the @AssistedFactory class requires a change to the @AssistedInject class at the same time, I wouldn't expect them to get out of sync very often (other than in this migration case, which is kind of unfortunate).

[JakeWharton]

Qualifier annotations may be present on component builder and factory methods which are exposed to user values in the same way, right? I mean assisted injection itself is basically just a scope-less component factory that directly returns an instance rather than a component.

Additionally, when there isn't a conflict for graph types you are still able to use a qualifier.

The need to use qualifiers to disambiguate assisted types should be rare. We are designing an edge case here, but doing things like parameter matching and placing significance on order have implications to every assisted inject usage rather than just the disambiguation situation. Nowhere else in Dagger do parameter names matter. Nowhere else in Dagger does position matter. Everywhere else in Dagger the qualifier concept used for disambiguation.

There's a high cognitive load to learning Dagger and to using Dagger day-to-day. My argument is for self-consistency so that the mechanisms you use for other parts of Dagger apply the same way here rather than having to learn and remember a different set of rules.

[Chang-Eric]

Qualifier annotations may be present on component builder and factory methods which are exposed to user values in the same way, right? I mean assisted injection itself is basically just a scope-less component factory that directly returns an instance rather than a component.

I think those cases are okay though because they actually create bindings. Assisted injection is different from a scope-less component factory because the assisted values aren't bindings. You can't, for example, move an @Assisted param into an @Inject type because it isn't a binding. Similarly, we aren't going to check for conflicts with bindings and assisted params. I'm afraid of someone seeing a create(@Foo Bar) method on a factory and thinking it is going to override the binding or something.

I think we disagree because before assisted injection, the statements "qualifiers are used for all disambiguation" and "qualifiers are only used for bindings" could be consistent and so it is easy to come up with different mental models of what a qualifier should be. I'm probably influenced by Guice where @Qualifier started as @BindingAnnotation. With that said, I agree that parameter order isn't great and doesn't feel very Dagger-like.

How about this? If we created an @AssistedParam("foo") annotation kind of like @Named, I think it would satisfy both of our requirements. You would use it like a qualifier annotation, but because it is specially named, it is clear what it is used for and that it is separate from other qualified bindings. Because of how restricted assisted injection is, especially with the exact behavior of not being bindings so we know @Assisted parameters can only be used in a single class, I think it is probably overkill to allow arbitrary annotations. @Named is bad in general Dagger usage because of the global namespace, but I think here the drawbacks don't apply.

(We can fiddle with how this looks exactly. One option is to put the string on @Assisted, but it might look weird to have @Assisted on the factory methods.)

[JakeWharton]

@Named is actually the qualifier that I recommend when a disambiguation is needed because no one really wants to maintain actual qualifiers solely for this case. So I would be supportive of either a standalone annotation for supplying a string or attaching it to @Assisted. I haven't thought about adding it to @Assisted in depth.

[tbroyer]

For the record, Guice attaches the disambiguation name to the @Assisted annotation: https://google.github.io/guice/api-docs/latest/javadoc/com/google/inject/assistedinject/Assisted.html
See “Making parameter types distinct” in https://google.github.io/guice/api-docs/latest/javadoc/com/google/inject/assistedinject/FactoryModuleBuilder.html

[Chang-Eric]

Thanks for the all of the input, it sounds like we have a plan =)

[IlyaGulya]

So you are going to support qualifiers in assisted injection, right?

[bcorso]

@IlyaGulya, we plan to support adding a String value to the @Assisted annotation to disambiguate the type, e.g. @Assisted("qualifier"), rather than the full javax.inject.Qualifier support.

[IlyaGulya]

Aren't qualifiers more natural for Dagger than some custom parameter for @Assisted annotation?
In this case, it would be much more flexible. Those who want to use String constants to disambiguate the types could use the @Named qualifier in this case. And those who like a more declarative (IMO) way of using custom qualifiers would not need to switch to String constants.
Also, switch to assisted injection for existing classes would be much easier if qualifiers already there.

[Chang-Eric]

@IlyaGulya I have some thoughts on why we don't want to support qualifiers in my comments above.

[JakeWharton]

And if it wasn't clear, I am supportive of this solution.

Since we'll likely do another Square AssistedInject release I am considering dropping the qualifier annotation support which was re-added and instead switching to this approach.