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

Kapt: Properly fix annotation ordering issue. #75

Closed
wants to merge 1 commit into from

Conversation

Jeffset
Copy link
Contributor

@Jeffset Jeffset commented Sep 4, 2023

As per KT-51087, kapt generated annotations in
reverse order, which was important for Yatagan.
This CL adds logic to reverse annotations based
on kotlin metadata version check.

The ultimate reason for the fix was that
room-compiler-processing-testing in 2.6.0-beta01 migrated to 1.9.x Kotlin compiler and some tests were broken.

Closes #74

As per KT-51087, kapt generated annotations in
 reverse order, which was important for Yatagan.
This CL adds logic to reverse annotations based
 on kotlin metadata version check.

The ultimate reason for the fix was that
`room-compiler-processing-testing` in `2.6.0-beta01`
migrated to 1.9.x Kotlin compiler and some tests were broken.

Closes #74
@Jeffset Jeffset requested a review from bacecek September 4, 2023 15:37
@Jeffset Jeffset marked this pull request as ready for review September 4, 2023 15:46
@@ -170,6 +170,7 @@ abstract class CompileTestDriverBase private constructor(
"-opt-in=com.yandex.yatagan.ConditionsApi",
"-opt-in=com.yandex.yatagan.VariantApi",
"-P", "plugin:org.jetbrains.kotlin.kapt3:correctErrorTypes=true",
"-jvm-target=11",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, why tests are running on Java 11, but target is 8?

Copy link
Contributor Author

@Jeffset Jeffset Sep 5, 2023

Choose a reason for hiding this comment

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

Testing framework requires 11+, but yatagan itself is built with 8 to maximize compatibility, as far as I remember.

@@ -21,14 +21,16 @@ import com.yandex.yatagan.lang.compiled.CtAnnotationBase
import javax.lang.model.element.AnnotationMirror
import javax.lang.model.element.Element

// See https://youtrack.jetbrains.com/issue/KT-51087, which was fixed in [1, 9, 0] metadata version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Target version in KT-51087 is 1.8.20-Beta. Is this version already have 1.9.0 metadata version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, It seemed to me, but I better double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out, metadata version in the code, generated by 1.8.20 compiler, is still [1, 8, 0], so such check is not sound.

Moreover, if the kotlin.Metadata is present, it doesn't mean the code is from KAPT (a stub), it may well be an already compiled bytecode, which always had correct annotation order. And there's no way to determine whether the element is backed by source or bytecode.

So, the task to make a sound decision here seems impossible :(

@Jeffset Jeffset closed this Sep 5, 2023
@Jeffset Jeffset deleted the wp/kapt-annotations-ordering-fix branch September 12, 2023 14:36
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