[AssistedInject] Allow scoping of AssistedFactory with annotations

[ferinagy]

Dagger version: 2.40
Sample project: https://github.com/ghus-raba/dagger-assisted-sample

Currently, annotating an assisted factory with scope like @Singleton takes no effect and the factory can inject also dependencies from other scopes.

In the sample project there is an AppComponent with @Singleton scope and it has a subcomponent ActivityComponent with @ActivityScope. We try to inject dependencies to ViewModel and try to ensure that only unscoped or @Singleton scoped dependencies can be injected, as its lifecycle is longer that the one of Activity.

When we do assisted injection manually, we can annotate the factory as @Singleton and it will correctly be provided by AppComponent. Trying to inject a dependency from @ActivityScope results in compilation error with [Dagger/IncompatiblyScopedBindings]:

class ManuallyAssistedViewModel constructor(
    foo: Foo,
    bar: Bar,
    assisted: Int
) : ViewModel() {

    @Singleton
    class Factory @Inject constructor(
        val fooProvider: Provider<Foo>,
        val barProvider: Provider<Bar>,
    ) {
        fun create(assisted: Int) = ManuallyAssistedViewModel(
            fooProvider.get(),
            barProvider.get(),
            assisted
        )
    }

}

However, if we do the same with @AssistedFactory, the @Singleton annotation is ignored and an Activity instance is injected.

class AssistedViewModel @AssistedInject constructor(
    foo: Foo,
    bar: Bar,
    activity: Activity,
    @Assisted assisted: Int
) : ViewModel() {

    @Singleton // this does nothing
    @AssistedFactory
    interface Factory {
        fun create(assisted: Int): AssistedViewModel
    }
}

Both of the above factories are injected in an activity like this:

private lateinit var component: ActivityComponent

@Inject internal lateinit var manuallyAssistedViewModelFactory: ManuallyAssistedViewModel.Factory
@Inject internal lateinit var assistedViewModelFactory: AssistedViewModel.Factory

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    component = application.component.activityComponentFactory.build(this)
    component.inject(this)

    // ...
}

We can explicitly provide the AssistedViewModel.Factory from AppComponent by adding a provision method val assistedFactory: AssistedViewModel.Factory, even if we do not use it and keep the injection as shown above.

It would be nice, if we could just use a scope annotation the same way as it is possible with manual way.

[bcorso]

Hi @ghus-raba, currently this is working as intended.

Is there a reason why you need the Factory to be scoped?

The factory should never carry state that requires scoping. It's similar to a Provider<Foo> in that you can't tell Dagger to scope the provider. Dagger is free to give you the same provider instance every time, or a new provider instance every time, we don't guarantee either, and your application generally shouldn't care either way.

[ferinagy]

Hi @bcorso, thanks for your response.

Is there a reason why you need the Factory to be scoped?

I do not really care if it is the same instance every time or not. What I do care is that the dependencies injected are coming only from AppComponent and not from the lower ActivityComponent. With the manually created factory, this can be achieved by annotating it with @Singleton, without having to add a provision method to the AppComponent, so I thought it would be nice to be able to do the same also with the @AssistedFactory.

I would prefer to be able to do this by annotating the factory, rather than adding provision method, the same way as we prefer to have @Inject annotated constructor, rather than adding a @Provides method to a module.

[bcorso]

Ah, I see. Typically, we advice against using scoping annotations as a means to pin a binding to a particular component because it introduces an unnecessary performance impact for each unnecessarily scoped binding. In particular, Dagger will have to double-check-lock each scoped binding.

It sounds like what you really want is a way to validate that the binding's dependencies are from a particular component without affecting the scoping. For that, you should be able to use the Dagger SPI plugin.

[bcorso]

Also, taking a step back, I'm interested in why you want to enforce that all of the Factory's dependencies are coming from AppComponent.

[ferinagy]

Thanks, I was not aware of such performance impact. I will take a look at the SPI plugin then.

...why you want to enforce that all of the Factory's dependencies are coming from AppComponent

In our case on Android, I want to avoid accidentally injecting an Activity instance to a ViewModel (possibly through other dependencies). Doing that would cause a memory leak as the Activity can be recreated, while keeping the same ViewModel.

[LeonRa]

Just adding that we've run into the same issue with assisted ViewModels and other classes here. It is currently really easy for engineers to accidentally include more narrowly scoped dependencies in wider scoped assisted classes. I believe this behavior was changed sometime between 2.33 and the present, as we've previously had to scope our assisted factories to guarantee that the assisted classes had access to scoped bindings. Edit: I believe this may have been changed in aff89b1.

[Chang-Eric]

I think maybe there is a feature request here for something more general in Dagger and implicit bindings. It sounds like if you could do @Bind Foo foo() that creates a binding inside a module and just binds it to itself to turn the implicit @Inject binding that floats to different components into an explicit binding in a module/component, that would be what you are looking for?

FWIW there are still complications because actually @Provides methods may float to other components based on if they have dependencies on multibindings, but we could start by adding such a bind feature as a way to make an @Inject behave exactly the same as if you had written an @Provides in the module. WDYT?

[LeonRa]

Thanks for the reply @Chang-Eric! I'm sure you understood the problem I mentioned but I unfortunately didn't quite understand your proposal. So we're speaking the same language, let's try a more concrete example with some code. Just to reiterate, the issue we're seeing currently is that a more narrowly scoped binding can be accidentally used in a broader scope, but only during assisted injection.

More concretely, in Android, if we have some Activity-scoped dependency

@ActivityScoped
class SomeActivityDependency @Inject constructor(
  private val host: Activity,
  // Other dependencies
) { /* Implementation */ }

regular injection will currently prevent us from accidentally including it in a broader scope:

@HiltViewModel
class SomeViewModel @Inject constructor(
  private val someDependency: SomeActivityDependency, // Compilation fails
  // Other dependencies
) { /* Implementation */ }

However, if I try to do the same thing with assisted injection, the code will compile and run, resulting in a source of memory leaks:

class SomeAssistedViewModel @AssistedInject constructor(
  @Assisted private val someAssistedParam: AssistedParam,
  private val someDependency: SomeActivityDependency, // Should not be allowed
  // Other dependencies
) {

  @AssistedFactory
  interface Factory {
    fun create(someAssistedParam: AssistedParam): SomeAssistedViewModel
  }
}

The above did used to fail before aff89b1, and required us to annotate the assisted factory with @ActivityScoped, which would be a clear implementation error. So holistically, the feature request from my end is to treat assisted factories the same way as normal injection: Disallow the use of a scoped dependency in an unscoped or more broadly scoped component.

Would love to better understand your proposal through code and/or get your thoughts on the above. Thanks again!

[Chang-Eric]

In your example, where is SomeAssistedViewModel.Factory injected? I just tried it with the factory injected into an activity before that change and it didn't fail for me, but I might not be replicating the scenario correctly. I didn't really expect it to though, since there's not actually a ViewModel in that code, it is just in name, so there's no reason Dagger or anything should know that an activity dependency isn't really allowed.

With that said, my proposal was actually trying to approach something more broadly. The factory, assisted or not, is something that only by specific rules of the app you want to limit dependencies. For example, you might have a regular Foo with an @Inject constructor, and just by the rules of what Foo does, you want it to only use dependencies from SingletonComponent. There are two ways to limit that right now.

One is to not use @Inject and use @Provides instead, so that you can install the Foo binding in a SingletonComponent module and thereby limit its dependencies (caveat about multibindings). This is kind of annoying though because writing the @Provides is more boilerplate and a bit more error prone if you're just piping a bunch of stuff into a constructor.

The other is to scope Foo with @Singleton, but that only works if you actually want Foo to be scoped.

The same applies to @AssistedFactory types, except your options are even more limited because you can't scope them and you also can't really @Provide them. Those methods of using @Provides and scoping are already kind of not great solutions though, so instead of opening them up to assisted types, my proposal is to add a new feature that better handles it for both.

@AssistedFactory and @Inject types are both what we call implicit bindings. They are found not through modules, but through usages. The opposite are explicit bindings which are bindings through modules. Explicit bindings have an extra attribute to them because we know which component they are installed in by virtue of the module, which can limit dependencies because now they have to be provided from that component (caveat about multibindings). My suggestion is to add a new feature that can take an implicit binding and bind it to itself in a module to make it explicit. This would help for both @Inject types and @AssistedFactory types.

[LeonRa]

In your example, where is SomeAssistedViewModel.Factory injected? I just tried it with the factory injected into an activity before that change and it didn't fail for me, but I might not be replicating the scenario correctly. I didn't really expect it to though, since there's not actually a ViewModel in that code, it is just in name, so there's no reason Dagger or anything should know that an activity dependency isn't really allowed.

Apologies for missing that in the above. We do in fact have SomeAssistedViewModel as a ViewModel in the code. The factory is injected inside an Activity and then passed to the ViewModel creation flow as a factory.

@AndroidEntryPoint
class SomeActivity : AppCompatActivity() {
 
  @Inject lateinit var viewModelFactory: SomeViewModel.Factory

  // Using a custom extension for this, but this is the gist
  private val viewModel: SomeViewModel by viewModels(
    factoryProducer = {
      object : ViewModelProvider.Factory {
        @Suppress("UNCHECKED_CAST")
        override fun <T : ViewModel> create(modelClass: Class<T>): T = viewModelFactory.create(someAssistedParam) as T
      }
    }
  )
}

Ah, okay, I see what you mean now. Thank you for clarifying! Completely agree about having to manually pipe dependencies being pretty boilerplate-heavy and error-prone. I like the idea of being able to make implicit bindings explicit. Probably out of scope, but I wonder if being able to support such binding can help solve for the verbosity of this workaround as well.

[Chang-Eric]

I'm surprised that example ever failed still, since even with the type extending ViewModel, from a Dagger perspective, there's still nothing different about this from any other factory type that depends on an activity binding.

Probably out of scope, but I wonder if being able to support such binding can help solve for the verbosity of #2301 (comment) as well.

I don't think so. We'd probably need to special case some of the checks to allow binding to the original implicit binding, but in that bug's case, they wanted to control the definition to come from another component. Unfortunately at that point, we can't distinguish between it coming from another Dagger component manually vs it being a different definition entirely.

[LeonRa]

I'm surprised that example ever failed still, since even with the type extending ViewModel, from a Dagger perspective, there's still nothing different about this from any other factory type that depends on an activity binding.

IIRC the failure was not at the binding site, but instead when trying to bind a scoped dependency into an unscoped assisted factory. I'll see if I can get this to fail again by downgrading Dagger before the commit I'm suspecting introduced the change in behavior