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

Allow consumption of Anvil-KSP contribution annotations #68

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mrmans0n
Copy link

@mrmans0n mrmans0n commented Oct 31, 2024

After making Anvil-KSP annotations KMP ready, it's now possible to have some level of interop between Anvil and kotlin-inject-anvil working.

This PR adds support to (Dagger) Anvil annotations to generate code similar to what kotlin-inject-anvil's own annotations would generate.

Anvil kotlin-inject-anvil
com.squareup.anvil.annotations.ContributesTo(scope, ...) ContributesTo(scope)
com.squareup.anvil.annotations.ContributesBinding(scope, boundType, ...) ContributesBinding(scope, boundType, multibinding = false)
com.squareup.anvil.annotations.ContributesMultibinding(scope, boundType, ...) ContributesBinding(scope, boundType, multibinding = true)

The unsupported parameters (parameters in Anvil annotations that kotlin-inject-anvil doesn't have a counterpart for) are logged as a warning. These warnings can be disabled by setting the ksp arg kotlin-inject-anvil-ignore-dagger-anvil-unsupported-param-warnings to true.

To be able to reuse some of the processor code, some utilities were forked to their own artifact, compiler-utils. This includes things like ContextAware or LOOKUP_PACKAGE.

Same story with the tests, there were some test utilities that would be very necessary for extra modules, and those were moved to a compiler-test module. It's been 0 days since I've missed testFixtures 😬

Copy link
Contributor

@vRallev vRallev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, the more I would have liked seeing a separate module that generates bindings that kotlin-inject-anvil can understand instead of mixing the annotations into the existing :compiler module. That makes the code harder to maintain long term. There is also the classpath issue I called out in other comments.

For example, for @ContributesBinding and @ContributesTo coming from the original Anvil, a separate symbol processor could have generated a component interface with the binding. kotlin-inject-anvil is extensible and that's how we handle custom annotations internally. I don't see a reason why the same mechanism cannot be used here. This would keep the core free of the original Anvil annotations.

@mrmans0n
Copy link
Author

mrmans0n commented Nov 5, 2024

Sure, I'll go the separate module route then. That would make it inherently opt-in. I hope that would still have place in this repo?

@vRallev
Copy link
Contributor

vRallev commented Nov 5, 2024

Sure, I'll go the separate module route then. That would make it inherently opt-in. I hope that would still have place in this repo?

Yes, I'm okay with keeping it in this repo. Any suggestion for the module name? anvil-dagger-interop? I'm not very creative.

@mrmans0n
Copy link
Author

mrmans0n commented Nov 5, 2024

I am locally calling it anvil-compat for now, but open to suggestions. It's weird naming this as the name Anvil is in both 😅

Also, the changes are kinda forcing me to create a compiler-utils and compiler-test packages, to share stuff like ContextAware / LOOKUP_NAME or the compile method for tests. I tried having the "anvil-compat" module as isolated as possible (wrt the rest of the project) but there was so much duplication of code that I had to go back to this small module refactor.

@vRallev
Copy link
Contributor

vRallev commented Nov 5, 2024

Yes, that's why I wanted to throw in "dagger" into the module name.

The supporting modules make sense. I meant to do this work earlier, because I had ideas about further optional extensions myself. Thanks for doing this :-)

@mrmans0n mrmans0n force-pushed the nacho/anvil-ksp-contributesto-support branch from 82ef6e7 to 5da5a0c Compare November 5, 2024 15:44
@mrmans0n
Copy link
Author

mrmans0n commented Nov 5, 2024

Just pushed an update with the support done in its own module, dagger-anvil-compat.

@mrmans0n mrmans0n force-pushed the nacho/anvil-ksp-contributesto-support branch from 5a3673e to db388f6 Compare November 6, 2024 15:46
@mrmans0n
Copy link
Author

mrmans0n commented Nov 6, 2024

Added bindings/components that use Anvil-KSP annotations to the :sample:app to make sure it works. It found some issues with ContributesTo codegen, so I swapped it to generate an interface that uses the kotlin-inject-anvil's ContributesTo instead, which worked as expected.

I think I'll create a separate sample that uses the Anvil-KSP annotations instead though, to not add noise to the current one (in case people are using it as an implementation example).

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

Successfully merging this pull request may close these issues.

2 participants