Sub component implementation inconsistent between different parent components in regards to scopes.

[trevjonez]

Version 2.7, & 2.8 Consumption is in a large android project, the issue only manifests itself in my androidTest src set component.

Component and scope layout is: AppComponent/ActivityComponent/ViewComponent where App component is @Singleton the activity component has a custom scope, and the view component has a custom scope.

Inside of my androidTest src set I create a ActivityTestComponent that is @Singleton and includes the ActivityComponent as a sub component. This allows me replace the majority of my object graph with mocks and give the activity the test component version of its component in its test run.

Problem that is occurring is the ViewComponent generated as a sub class of the test component is not correctly wrapping the objects provided in the double check provider.

AppCompImpl.java.txt TestCompImpl.java.txt

I've tried to replicate in a small sample project but the trivial version of this component layout has not produced the issue.

ActivityComponent is added as a sub component of the AppComponent via the @Module(subcomponent) and a @Binds @IntoMap @ClassKey on the component builder type. ActivityComponent is added as a sub component of the ActivityTestComponent via a method directly exposing the ActivityComponent.Builder on ActivityTestComponent.

[gk5885]

I think we're going to need a little more information here. What you're describing here isn't a testing strategy that I think we've ever seen, so it's a little tricky to infer exactly what behavior you're expecting.

In your AppCompImplfile I see 8 bindings that are scoped (DoubleCheck.provider). In the TestCompImpl I see none. Are you expecting all 8 of them to still be scoped or just a subset? For the ones that are misbehaving, can you show how they're being bound in both your app and test components? Can you also show where the modules that bind them are being installed?

Thanks.

[trevjonez]

I am expecting the scoping in the TestCompImpl to be exactly as it is in the AppCompImpl

The application component is responsible for providing all things model in an offline first MVP setup. Database access, syncing. Network clients for sync jobs. RPC implementations for online only behavior. etc...

Rather than providing the application component with the full list of test modules that would return mocked network clients and database queries, I create a new root singleton component and include a bare minimum set of modules to fulfill only the needs of the activity to be tested. The activity component is then added as a sub component of the new test component as well as remaining a sub component of the application component. When the activity reaches out to the application to get a component builder for its self the test application returns DaggerOnboardingTestComponent.AccountCreateComponentBuilder which implements AccountCreationComponent.Builder. Under normal run configurations it would get DaggerAppComponent.AccountCreateComponentBuilder.

This particular activity is about as bad as it gets in a single context as I have nested view pagers and am trying to depend on dagger to manage the creation and distribution of different views. The number of mocked dependencies is low here but there is an extra level of sub component here compared to my other test cases.

[trevjonez]

Doing some further investigation I have pinned it to an inappropriate retention policy on my custom scope.

Setting a conditional break point at dagger.internal.codegen.Scope#uniqueScopeOf(Element) I was able to show that the element metadata was missing the annotation meta info in the compileAndroidTest invocation for the effected module methods. From there it was just a matter of working backwards as to why the annotation was being discarded.

Would it be appropriate to add a validation on custom scopes that ensure they have at least a class retention? Or at least something in the docs indicating that it can be a source of very subtle bugs?

[ronshapiro]

Error Prone does actually do just that

[trevjonez]

@ronshapiro After "configuring" my project to use the error prone compiler, it is not warning me about this potential issue. I would like to make the argument that a defense strategy based on a non standard compiler with a potentially non standard config that could also be riddled with other subtleties is no defense strategy at all.

Using this plugin to configure my android project to use error prone version 2.0.15

Given that a primary goal of dagger2 is to provide compile time safety I implore you to add this check to the processor rather than depending on the error prone compiler being in place and configured correctly.

[gk5885]

@trevjonez, the problem is two-fold:

1. Nothing about your scope is Dagger-specific. The Dagger processor only runs on Dagger types and @Scope is part of JSR 330, not Dagger.
2. If Dagger were to validate things like @Scope annotations, it would have had to have run on that class in order to have caught the error (because source retention wouldn't have been retained in the class file).

That creates a semi-awkward situation in which you would potentially run the Dagger processor on build targets that contain no Dagger types just to get the extra static analysis. If you're going to commit to running static analysis in the first place, Error Prone is the better choice because it will give you more complete analysis and suggested fixes.

[tbroyer]

That creates a semi-awkward situation in which you would potentially run the Dagger processor on build targets that contain no Dagger types just to get the extra static analysis.

Well, you sort-of have to do it already, for classes with @Inject, if you don't want the message telling you to "run Dagger on that class". Not for static analysis only, but still.

That said, I agree that it's not Dagger's role to check the retention of scope annotations.