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

Equality check fails with KSP2, but not KSP1 #2196

Open
vRallev opened this issue Nov 8, 2024 · 6 comments
Open

Equality check fails with KSP2, but not KSP1 #2196

vRallev opened this issue Nov 8, 2024 · 6 comments

Comments

@vRallev
Copy link

vRallev commented Nov 8, 2024

This is related to evant/kotlin-inject#447 and likely #2091.

The kotlin-inject processor compares two KSAnnotation instances for equality here. This check works as expected with KSP1, but with KSP2 it fails and produces the error mentioned in the original ticket as side effect.

This issue still happens with 2.0.21-1.0.27.

@eygraber
Copy link

@ting-yuan with KSP1 the following works, but with KSP2 the KSPropertyDeclaration.annotations is empty:

@Qualifier
@Target(
    CLASS,
    ANNOTATION_CLASS,
    TYPE_PARAMETER,
    PROPERTY,
    FIELD,
    LOCAL_VARIABLE,
    VALUE_PARAMETER,
    CONSTRUCTOR,
    FUNCTION,
    PROPERTY_GETTER,
    PROPERTY_SETTER,
    TYPE,
    FILE,
    TYPEALIAS,
)
annotation class QualifiedString

@Component
abstract class MainComponent {

    @Provides
    @QualifiedString
    fun provideString(): String = ""

    @QualifiedString
    abstract val otherString: String
}

To get it to work with KSP2 either VALUE_PARAMETER needs to be removed from @Target or the @QualifiedString annotation on otherString has to be moved to the type (i.e. abstract val otherString: @QualifiedString String).

@eygraber
Copy link

eygraber commented Nov 21, 2024

Looks like this is intentional?

#1216

@evant
Copy link
Contributor

evant commented Nov 21, 2024

only happening for properties declared in constructor parameter

Seems like that behavior should only be happening for constructor parameters? You example doesn't use them there.

@ting-yuan
Copy link
Collaborator

I checked out to amzn/kotlin-inject-anvil@c8b7009 and enabled KSP2 in gradle.properties as well as the line of useKsp2=... in compiler/src/test/kotlin/software/amazon/lastmile/kotlin/inject/anvil/processor/ContributesSubcomponentProcessorTest.kt. It builds fail as expected. The error message looks like I'm using the right test:

e: Error occurred in KSP, check log for detail
e: [ksp] Cannot find an @Inject constructor or provider for: Int
/tmp/Kotlin-Compilation2841034303579867967/sources/src/main/java/software/amazon/test/Source0.kt:39: int: Int

So far so good.

Then I tried to break at https://github.com/evant/kotlin-inject/blob/6ab3df82aab1bdb4cdd72b259338f12e39f47d12/ast/ksp/src/main/kotlin/me/tatarka/kotlin/ast/KSUtil.kt#L104 and there are only two pairs of annotations (@SingleIn and @QualifiedString) got compared, and both of the call return true. Should any one of them fail?

Or am I using the wrong reproducer?

@eygraber
Copy link

Seems like that behavior should only be happening for constructor parameters? You example doesn't use them there.

@evant that PR changed KSPropertyDeclarationImpl, presumably because a contractor parameter can also be a property.

@vRallev it looks like evant/kotlin-inject#447 is caused by the changes in #1216.

Given that, is the equality check failing not related to evant/kotlin-inject#447?

@eygraber
Copy link

OK I checked out the test in ContributesSubcomponentProcessorTest and the repro case in evant/kotlin-inject#447 and as far as I can tell the issue has nothing to do with KSAnnotation.eqv.

The underlying problem for both cases is that #1216 made a change to KSPropertyDeclarationImpl to filter out annotations that include VALUE_PARAMETER in the annotation's Target.

However, I believe there's a bug (or bugs) in the implementation.

  1. As @evant pointed out:

Seems like that behavior should only be happening for constructor parameters

and this is happening even on non constructor parameters

  1. The original issue requesting this change states:

I think it should not inherit the annotations of the KSValueParameter, especially in this case where the @target is only meant for value parameters

However, the behavior change strips out the annotation if any of the targets are VALUE_PARAMETER.

@ting-yuan it might make more sense to strip the annotation if the targets contains VALUE_PARAMETER and does not contain any property targets.

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

4 participants