Skip to content
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

Cannot resolve third party annotation with Dagger 2.41. #3256

Closed
vRallev opened this issue Feb 16, 2022 · 7 comments
Closed

Cannot resolve third party annotation with Dagger 2.41. #3256

vRallev opened this issue Feb 16, 2022 · 7 comments

Comments

@vRallev
Copy link

vRallev commented Feb 16, 2022

We have custom annotations in our codebase similar to this:

@ContributesService(AppScope::class, replaces = [AccountStatusService::class])
class MockAccountStatusService @Inject constructor(..) : AccountStatusService

This type MockAccountStatusService is being used in our dependency graph.

With Dagger 2.41 (currently using 2.39.1) we get following error in a module downstream:

error: ComponentProcessingStep was unable to process 'anvil.register.demo.com.squareup.development.shell.demo.login.DemoLoginAppComponent' because 'com.squareup.services.anvil.ContributesService' could not be resolved.

  Dependency trace:
      => element (CLASS): com.squareup.server.accountstatus.MockAccountStatusService
      => annotation: @com.squareup.services.anvil.ContributesService(scope = com.squareup.dagger.AppScope.class, replaces = com.squareup.server.accountstatus.AccountStatusService.class)
      => type (ERROR annotation type): com.squareup.services.anvil.ContributesService

  If type 'com.squareup.services.anvil.ContributesService' is a generated type, check above for compilation errors that may have prevented the type from being generated. Otherwise, ensure that type 'com.squareup.services.anvil.ContributesService' is on your classpath.

It is correct, the annotation isn't on the compile classpath of the final app module where this error is thrown. But in my opinion Dagger should now try to resolve the annotation or at least ignore it if it fails to resolve. Is this new behavior expected?

@bcorso
Copy link

bcorso commented Feb 17, 2022

We should only be validating that annotation in the final app module if you aren't running the Dagger processor in the library module where the class is defined (see InjectValidator.java).

Can you check whether you have the Dagger processor running in the library where MockAccountStatusService is defined?

@vRallev
Copy link
Author

vRallev commented Feb 17, 2022

We're not running the annotation processor in this module. Could Dagger be less strict about requiring all types to be present on the compile classpath? That's definitely a behavior change from previous versions.

@bcorso
Copy link

bcorso commented Feb 17, 2022

We're not running the annotation processor in this module.

Is there a reason you cannot or don't want to run the annotation processor in that module?

That's definitely a behavior change from previous versions.

Yes, we mention it a bit in the 2.41 release notes.

You can go back to the previous behavior using -Adagger.strictSuperficialValidation=DISABLED; however, the old behavior does not guarantee correctness (see #3136) and we also plan to remove the option at some point in the future.

Could Dagger be less strict about requiring all types to be present on the compile classpath?

In general, no, since we want to guarantee correctness.

However, there are a few (relatively rare) cases such as this one where the validation is not technically needed for correctness per-se (e.g. in this case it's just used to check for incorrect placement of qualifiers on the constructor). At the time, we discussed being less strict for this case but decided to keep it mandatory assuming most classpath issues could be avoided by running the annotation processor in the library itself.

I think we'd be willing to reconsider making these cases less strict (or at least add a compiler option to do so) when the validation is not needed for correctness. Still, it would be nice to know what reasons there are to not run Dagger's annotation processor in this case, so please let us know.

@bcorso
Copy link

bcorso commented Feb 17, 2022

Could Dagger be less strict about requiring all types to be present on the compile classpath?

Also, just to clarify, we don't require all types to be present on the compile classpath, only what is needed for us to perform our validation. In particular, in this case we were looking for misplaced qualifiers on the constructor, but we can't tell if @ContributesService is a qualifier unless it's on the classpath (similar to the bug described in #3136), which is why we require it .

@vRallev
Copy link
Author

vRallev commented Feb 17, 2022

Thanks for the long explanation and shame on me for not reading the release notes. What you consider as a bug in the old version is actually something we relied on. I can explain our use case better.

We have a strict separation of :public APIs, :impl modules and :wiring code. We extracted Dagger modules into a :wiring module to avoid the cost of KAPT in the :impl module. Therefore, we only run the Dagger annotation processor in wiring modules. Implementation of the public APIs live in the :impl module. They usually have an @Inject constructor. Since this never required the Dagger annotation processor, we never had to enable it there.

TL;DR for not enabling the Dagger annotation processor is KAPT and the higher build time. It matters with thousands of modules.

The flag to opt-out of this behavior sounds like a good solution for us, but it worries me that you plan to remove it soon.

@Chang-Eric
Copy link
Member

So I think this is going to be a requirement for Dagger that we won't be able to be too flexible on, otherwise we run the risk of missing critical annotations and causing functional issues. Annotations that are in places where we might reasonably care about them will need to be on the classpath for the processor so it can know if they affect functionality or not.

Running the processor over the class is an optimization because we already process it once in the library, so we can pass information down the root, so we don't actually need those classes on the classpath anymore.

With that said, I think there are a few options:

  • Use the flag that opts into the old behavior. This isn't a good option because it opens you up to the functional issues we fixed with qualifiers or scopes missing from the transitive classpath. Presumably, you wouldn't want to accidentally hit this issue, especially because it can be silent.
  • Run the Dagger processor over the classes. As you said, this isn't great because of kapt. I understand the desire to not do this. Maybe one day when we finish KSP support.
  • Add the @ContributesService annotation to the dependencies of the root where your Dagger components are being compiled (or treat it as an api dep, either way just make sure it is available to the root classpath). This is probably the best option, even though it might technically break some encapsulation or increase the classpath of your build, I wouldn't expect it to be too much of a problem.

Hopefully one of those options works for your case.

@vRallev
Copy link
Author

vRallev commented Feb 19, 2022

I totally understand the reasoning. We might go with option 2 or 3. Thanks for all the input!

@vRallev vRallev closed this as completed Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants