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

enum constants in java sources are reported as properties #234

Closed
yigit opened this issue Jan 6, 2021 · 3 comments
Closed

enum constants in java sources are reported as properties #234

yigit opened this issue Jan 6, 2021 · 3 comments
Labels
enhancement New feature or request P1 major features or blocking bugs
Milestone

Comments

@yigit
Copy link
Collaborator

yigit commented Jan 6, 2021

For the following two inputs:

            enum class KotlinEnum(val x:Int) {
                VAL1(1),
                VAL2(2);
            }
            public enum JavaEnum {
                VAL1(1),
                VAL2(2);
                private int x;
                JavaEnum(int x) {
                    this.x = x;
                }
            }

When traversing declarations in KotlinEnum and JavaEnum, enum constants of JavaEnum are returned as KSPropertyDeclaration instances while enum constants of KotlinEnum are returned as KSClassDeclarations with the proper kind (ENUM_ENTRY)

Java Annotation Processing also returns them as fields for the java source code but i think for KSP, to be consistent, it is better to report them as KSClassDeclarations.

I've not checked what happens with descriptors, will update the bug.

@yigit
Copy link
Collaborator Author

yigit commented Jan 6, 2021

it works fine for the descriptor implementation (they are reported as KSClassDeclaration instances with type ENUM_ENTRY)

@yigit
Copy link
Collaborator Author

yigit commented Jan 7, 2021

this seems not very trivial because PsiEnumConstant does not have a PsiClass that represents that entry :/.
I can work around it by resolving to the descriptor instead. In the test, it resolves to an instance of: EnumEntrySyntheticClassDescriptor (notice the Synthetic in name)
https://github.com/JetBrains/kotlin/blob/master/core/descriptors/src/org/jetbrains/kotlin/descriptors/impl/EnumEntrySyntheticClassDescriptor.java

We could also possibly implement KSClassDeclaration from a PsiEnumConstant but that adds more variety.

yigit added a commit to yigit/ksp that referenced this issue Jan 7, 2021
This PR fixes a bug in KSClassDeclarationJavaImpl where it would return
properties for its enum constants instead of returning class
declarations.

Unfortunately, PsiEnumConstant does not have an API to resolve it as a
class. So instead, this PR delegates to the descriptor implementation
for these fields.

Fixes: google#234
Test: classKinds
@ting-yuan ting-yuan added enhancement New feature or request P1 major features or blocking bugs labels Jan 8, 2021
@ting-yuan ting-yuan added this to the 2021Q1 milestone Jan 8, 2021
@ting-yuan
Copy link
Collaborator

Fixed in #238

copybara-service bot pushed a commit to androidx/androidx that referenced this issue Jan 11, 2021
Enum classes are slightly different than other classes where they have
inner classes/properties that are actually enum constants.

Moreoever, KSP returns those constants as class declarations whereas
java AP returns them as fields.

To avoid possibly confusions, this CL adds a new XEnumTypeElement with a
field to receive enum constant names.
Subsequently, java implementation of XTypeElement was returning enum
constants as if they are fields, I've removed them from that list as
well.

As part of this change, I've removed XType.isEnum and instead added
XTypeElement.isEnum which can also do the auto cast to XEnumTypeElement.
Also refactored EnumColumnTypeAdapter to use the new APIs.

Note that there is an existing bug in KSP where it does not report enum
constants properly from java sources so for now, I've excluded that case
from tests: google/ksp#234

I've copied the EnumColumnTypeAdapterTest to also run in room kotlin
test app so that we can ensure KSP is working propery (it was failing).

Bug: 160322705
Bug: 173236324
Test: XTypeElementTest, EnumColumnTypeAdapterTest.kt

Change-Id: Ie6d74cc5ccd93ed6be2687cdb382baf9dee16a0f
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Apr 12, 2021
This issue (google/ksp#234) was already fixed
in an earlier version but looks like i forgot to remove the TODO.

Test exclusion was removed here:
https://android-review.googlesource.com/c/platform/frameworks/support/+/1543326/7/room/compiler-processing/src/test/java/androidx/room/compiler/processing/XTypeElementTest.kt

Bug: 160322705
Test: n/a

Change-Id: Ib36361b8d6598ea18d5f332ee38f62af1b532efc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 major features or blocking bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants