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

Traversal order #260

Closed
wants to merge 4 commits into from
Closed

Traversal order #260

wants to merge 4 commits into from

Conversation

yigit
Copy link
Collaborator

@yigit yigit commented Jan 20, 2021

Prototype for keeping declaration order in .class files that come from compiled kotlin code.

@yigit
Copy link
Collaborator Author

yigit commented Jan 20, 2021

this ended up requiring producing jvm signatures, which is not a great added cost and does not feel great to turn on by default.
I thought about making it an optional parameter OR possibly, provide another method that will sort the descriptors.
e.g.

KSClassDeclaration.getDeclarationsInDeclarationOrder() : List<KSDeclaration>

which developer needs to call? That could help avoid the cost in generic case but let processor owners call it when needed.

copybara-service bot pushed a commit to androidx/androidx that referenced this pull request Jan 29, 2021
This CL fixes a bug in KSP where the order of fields returned in a
KSClassDeclaration does not necessarily match the order they were
declared in the class. It only impacts compiled kotlin classes because
Kotlin compiler orders field names when serializing the metadata.
google/ksp#250

This particularly hurts Room as we generate the table structure based on
this order. Even though Room is clever enough to consider the schema the
same, the effects are still visible in both schema files and generated
sql database.

This CL is a port of a possible implementation in KSP:
google/ksp#260
Unfortunately, merging that CL would mean KSP doing a lot more work for
each class. The plan there is to first fix it in the kotlin compiler if
possible and if not, add an API to get sorted fields (to avoid the cost
when not necessary). Meanwhile, this CL is a band aid solution in Room
so that developers won't have unexpected schema file changes when they
move to KSP from KAPT.

As this solution is best effort, it doesn't fail compilation if it fails
to read fields. I've added a strict mode to XProcessing so that we can
run it in tests with a requirement to always find the order but on
developer's machine, it won't require this. This strict mode can be
turned on via a system property, which we can use to collect more bug
information when it doesn't work.

Fixes: 177471948
Test: OrderOfFieldsTest
Change-Id: If0cf346a7f92dbbf2094e0076ea9ed02de4e32fc
@yigit yigit marked this pull request as draft February 1, 2021 20:29
ting-yuan added a commit to ting-yuan/ksp that referenced this pull request Sep 2, 2021
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from google#260, whose
author is Yigit Boyar <yboyar@google.com>.
ting-yuan added a commit to ting-yuan/ksp that referenced this pull request Sep 2, 2021
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from google#260, which is
authored by Yigit Boyar <yboyar@google.com>.
ting-yuan added a commit that referenced this pull request Sep 2, 2021
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from #260, which is
authored by Yigit Boyar <yboyar@google.com>.
@ting-yuan ting-yuan closed this Sep 2, 2021
github-actions bot pushed a commit that referenced this pull request Sep 2, 2021
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from #260, which is
authored by Yigit Boyar <yboyar@google.com>.

(cherry picked from commit f710c5f)
github-actions bot pushed a commit to ting-yuan/ksp that referenced this pull request Sep 15, 2021
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from google#260, which is
authored by Yigit Boyar <yboyar@google.com>.

(cherry picked from commit f710c5f)
pritisnivv added a commit to pritisnivv/ksp that referenced this pull request Aug 24, 2024
which is same as KSDeclarationContainer.declarations, except that the
order of retrived declarations match their positions in source.

This is adapted from google/ksp#260, which is
authored by Yigit Boyar <yboyar@google.com>.
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