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

Fix enum lookup in annotation proxies #685

Merged
merged 4 commits into from Oct 20, 2021
Merged

Conversation

ZacSweers
Copy link
Contributor

Before, this would take the toString() of the KSType, but that's wrong because then you have something like AnnotationRetention.valueOf("kotlin.annotation.AnnotationRetention.RUNTIME"), which obviously won't work. This unpacks the short name from the KSType directly so that it actually looks up just RUNTIME in the given example.

Before, this would take the `toString()` of the KSType, but that's wrong because then you have something like `AnnotationRetention.valueOf("kotlin.annotation.AnnotationRetention.RUNTIME")`, which obviously won't work. This unpacks the short name from the `KSType` directly so that it actually looks up just `RUNTIME` in the given example.
@ZacSweers
Copy link
Contributor Author

Weirdly I can't repro this issue, but it does repro if you use the current KSP 1.5.31 impl in this PR square/moshi#1393

Copy link
Collaborator

@ting-yuan ting-yuan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

returnType.getDeclaredMethod("valueOf", String::class.java)
.invoke(
null,
if (this is KSType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Related discussion; Not for this PR)

Without the type checking it would result in the following error, where the type of this is TestEnum, rather than a KSType.
https://github.com/google/ksp/runs/3911397835#step:9:729

@neetopia Does the need of type checking here indicate a discrepancy in implementations of annotation value? In other words, shouldn't it always be KSType?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example, this was one of the difference among the implementations of .class and sources.

It could also be the reason why it's only reproducible with KSP 1.5.31-1.0.0; The toString()s of class instance and KSType are different.

@ting-yuan ting-yuan merged commit ed96541 into google:main Oct 20, 2021
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.

None yet

2 participants