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

Order of declarations do not match the source #250

Open
yigit opened this issue Jan 14, 2021 · 12 comments
Open

Order of declarations do not match the source #250

yigit opened this issue Jan 14, 2021 · 12 comments
Assignees
Labels
P1 major features or blocking bugs

Comments

@yigit
Copy link
Collaborator

yigit commented Jan 14, 2021

For .class dependencies, the order KSP reports declarations do not match the class file.
This is the case with Java AP and it helps with consistent output between builds (I've not tested if KSP output is consistent or not though)
https://docs.oracle.com/javase/8/docs/api/javax/lang/model/element/TypeElement.html#getEnclosedElements--

A similar problem existed in KAPT which was solved here: (in 1.5.0)
JetBrains/kotlin@e7f30f3

We should ensure that:

  • the order of elements follows the one from the source
  • the order of elements is deterministic for clean and incremental builds
@ting-yuan ting-yuan added the P2 affects usability but not blocks users label Jan 14, 2021
@ting-yuan ting-yuan added this to the 2021Q1 milestone Jan 14, 2021
@yigit
Copy link
Collaborator Author

yigit commented Jan 15, 2021

I do have a repro for this now.
https://github.com/google/ksp/compare/master...yigit:traversal-order?expand=1#diff-041110caa10bb0996c760d70ec04b2ccddfbeba26cbea9e4661f2b5f32614c5d

very interestingly, it only affects the compiled kotlin classes. Or at least, thats the only one i could reproduce for now.

Their .class representations are almost equal.

➜  support git:(boxed-primitive) javap -p lib/KotlinClass.class
Compiled from "KotlinClass.kt"
public final class lib.KotlinClass {
  private final java.lang.String name;
  private final java.lang.String lastName;
  public final java.lang.String getName();
  public final java.lang.String getLastName();
  public final java.lang.String nameFun();
  public final java.lang.String lastNameFun();
  public lib.KotlinClass();
}
➜  support git:(boxed-primitive) javap -p lib/JavaClass.class
Compiled from "JavaClass.java"
public class lib.JavaClass {
  java.lang.String name;
  java.lang.String lastName;
  public lib.JavaClass();
  void nameMethod();
  void lastNameMethod();
}

@yigit
Copy link
Collaborator Author

yigit commented Jan 15, 2021

in the test, it prints

lib.KotlinClass
lastName
name
lastNameFun
nameFun

instead of:

lib.KotlinClass
name
lastName
nameFun
lastNameFun

@yigit
Copy link
Collaborator Author

yigit commented Jan 16, 2021

@yigit
Copy link
Collaborator Author

yigit commented Jan 16, 2021

looks like CompilerDeserializationConfiguration does not override that field so that option looks like a no-go for now.
https://github.com/JetBrains/kotlin/blob/master/compiler/frontend/src/org/jetbrains/kotlin/resolve/CompilerDeserializationConfiguration.kt

@yigit
Copy link
Collaborator Author

yigit commented Jan 16, 2021

i also debugged the proto in the class declaration and actually it is where the property list does not have the properties in the order they were declared in the code. And as far as I can see, the parser only uses the proto.
That means, even if we could use NoReorderImplementation, it wouldn't help.

@neetopia
Copy link
Contributor

Thanks for doing the investigation. That was my concern that if such ordering info isn't available in both class file and meta data, then there is no way for KSP to restore the ordering, the only feasible solution seems to be to change compiler codegen logic, and also requires a rebuild for libraries.

@yigit
Copy link
Collaborator Author

yigit commented Jan 16, 2021

actually it is available in the class file. in fact, when i debug the class via javap -p, it has the right ordering (this is why we've not seen this issue in kapt).
but as far i understood from reading the code, it only uses the metadata for descriptors (makes sense). "maybe" there is a way to extract that information from the descriptor, find the backing class and get the ordering from there. It would be expensive but for people moving from ksp to kapt, it could matter.

@ting-yuan ting-yuan self-assigned this Jan 19, 2021
@yigit
Copy link
Collaborator Author

yigit commented Jan 20, 2021

i have a prototype:
#260

it is not great as it requires getting jvm signatures etc, but is a possible solution :)

copybara-service bot pushed a commit to androidx/androidx that referenced this issue 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
@ting-yuan ting-yuan added P1 major features or blocking bugs and removed P2 affects usability but not blocks users labels Feb 2, 2021
@ting-yuan ting-yuan modified the milestones: 2021Q1, 2021Q2 Feb 2, 2021
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Apr 6, 2021
BaseDaoTest triggered a bug in XProcessing where we were over-extending
variance resolution to java sources, which is incorrect.

Unfortunately, KSP does not yet tell us if a Origin.CLASS was derived
from a kotlin file or other (java etc).
google/ksp#375

For that, I've expanded KspFieldOrdering to also detect true origin via
reflection (and renamed it to KspClassFileUtility). I also added method
ordering there as order of methods might matter for Auto Value classes.
It adds a performance penalty but we'll eventually get rid of it when
google/ksp#250 is fixed.
Note that method ordering does not check for descriptions so it is not
100% perfect.

Bug: 160322705
Test: BaseDaoTest, MethodSpecHelperTest, KspClassFileUtilityTest

Change-Id: Iddfed0f72c438987a7f68ded62e9653275685189
@ting-yuan ting-yuan assigned neetopia and unassigned ting-yuan Jul 29, 2021
@ting-yuan ting-yuan assigned ting-yuan and unassigned neetopia Sep 1, 2021
@ting-yuan
Copy link
Collaborator

#602 added a KSDeclarationContainer.declarationsInSourceOrder to work this around.

@ting-yuan ting-yuan removed this from the 1.0.0 milestone Sep 2, 2021
copybara-service bot pushed a commit to androidx/androidx that referenced this issue May 16, 2023
This CL fixes an issue with our [TypeName] calculation for types that
are annotated with `@JvmInline`. These types have interesting rules
for when they're actually replaced with the actual type or the
`@JvmInline` type, which I've updated [KSTypeJavaPoetExt] to handle.

In addition, some of the test cases I added to [KspTypeNamesGoldenTest]
caused a breakage in [KspClassFileUtility] when ordering the
methods/fields. However, KSP has since fixed the initial issue
(google/ksp#250), so I went ahead and updated
the code to use the new `Resolver#getDeclarationsInSourceOrder()`
method, which fixed the breakages.

Test: KspTypeNamesGoldenTest.kt
Change-Id: I9ad48edda0fbb518b89be116df2999545d68f33b
@hannomalie
Copy link

Hey guys. I came across this issue here - having a ksp plugin that iterates property declarations. For me, it happens when i don't iterate properties of a containing class directly for a given source file in a given module, but when a class declaration of a nested property should be iterated over.

Don't think it's too important what my usecase is, my question: What is the state of the feature "add the possibility to iterate declarations in order they appear in the source code"? I saw the merge request and that it's already merged to main, but since I use the "symbol-processing-api" artifact in my ksp plugin, i think i can't use it, because it resides in the impl package.

Would you advice to use a different artifact and use the internal api or is there maybe a way to access the "declarationsInSourceOrder" property somehow and I overlooked it? When I can be of any help, let me know :)

(I am using version 1.9.10-1.0.13)

@ianlevesque
Copy link

@hannomalie I see you asked a while back, but for posterity you have to call it from the Resolver:

resolver.getDeclarationsInSourceOrder(myDeclarationContainer)

@hannomalie
Copy link

Using the resolver is a bit of a different api surface that needs to be used, making the code somewhat worse compared to what i had before, but it works and is correct, was able to solve my issue, many thanks @ianlevesque for the hint!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 major features or blocking bugs
Projects
None yet
Development

No branches or pull requests

5 participants