-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support KSP in BindsMethodValidator #831
Support KSP in BindsMethodValidator #831
Conversation
@ZacSweers anvil/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt Line 272 in 540ce10
anvil/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt Line 318 in 540ce10
anvil/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt Line 373 in 540ce10
As far as I can see, dagger compilation with ksp is not yet supported in tests (according to
Do I need to do something with these three tests? |
Oh, I forgot to write a test for this issue: #750 |
For those three tests, what you wanna do is enable the KSP validator on the first compilation and then just use the regular dagger processing on the subsequent compilation. Hoping that could work? Sidenote - there are some related changes I've made in #830 to support Dagger's new behavior in 2.48, but you're welcome to cherry-pick those into this PR since they sorta go together and I can update my PR after this is in |
eh actually the more I think about it, the more I think it'll be smoother for me to just update that PR and get this one through first. I can manage any conflicts. Just know that it's there :) |
Ok, now I will fix those three tests and add a test for #750, after that I will wait for your PR to be merged 🙂 |
@ZacSweers regarding the #750 - I've added test which is correctly passes in KSP mode. |
Nevermind, I've fixed #750 🙁 |
compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt
Outdated
Show resolved
Hide resolved
dc8c9d7
to
8f0cdf6
Compare
if it's not too difficult, do you think it's possible to split out the fix for #750 to a separate PR first? Might make this easier to review |
Sure! |
8f0cdf6
to
e153081
Compare
@ZacSweers I've extracted #750 fix to #833 |
e153081
to
d37d6ca
Compare
d37d6ca
to
52e0a97
Compare
52e0a97
to
593833a
Compare
Let's start reviewing this once #833 is in and this branch is updated. Suspect there will be a decent amount of overlap :) |
@IlyaGulya ping me when you've pulled latest main and we can start review here! |
fbc6d57
to
f087463
Compare
@ZacSweers done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
@@ -352,17 +353,43 @@ class BindsMethodValidatorTest( | |||
} | |||
} | |||
|
|||
@Test | |||
fun `binding inside interface is valid`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
private fun compile( | ||
@Language("kotlin") vararg sources: String, | ||
previousCompilationResult: JvmCompilationResult? = null, | ||
enableDagger: Boolean = useDagger, | ||
forceEmbeddedMode: Boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a small comment explaining why we need this for posterity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to explain it in a comment.
I've added this flag to fix tests according to your suggestion: #831 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a simple comment like "Used to enable the dagger compiler even in KSP mode, which is necessary for some multi-round tests"
|
||
clazz to constructors[0] | ||
} | ||
} | ||
|
||
private fun requireSingleInjectConstructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
.resolveSuperTypesExcludingAny() | ||
.toList() | ||
|
||
if (returnType !in parameterSuperTypes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use KSP's built-in assignability check? KSType.isAssignableFrom(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I did not know about this api. Will fix that =)
) | ||
} | ||
|
||
internal inline fun <reified T> Resolver.getAnnotatedSymbols(): Sequence<KSAnnotated> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires all these classes to be on the KSP classpath. This could cause unexpected errors if this processor is run on a source set that doesn't contain them, and we should gracefully degrade in this case. This is why we usually use the fqcn. No need to fix here as we depend directly on these annotations in the compiler artifact anyway, but just a note
private fun compile( | ||
@Language("kotlin") vararg sources: String, | ||
previousCompilationResult: JvmCompilationResult? = null, | ||
enableDagger: Boolean = useDagger, | ||
forceEmbeddedMode: Boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a simple comment like "Used to enable the dagger compiler even in KSP mode, which is necessary for some multi-round tests"
Haven't forgotten about this, we just had to revert the embedded impl temporarily due to issues :/. Will revisit once that one is in again, unless you want to rebase this one |
@ZacSweers you mean this revert? 5a04d60 |
c4d1f71
to
d062920
Compare
@ZacSweers done 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long on this, appreciate your patience!
Part of the issue #751