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

Proposal to get rid of Java Impls - if possible #177

Open
yigit opened this issue Nov 28, 2020 · 6 comments
Open

Proposal to get rid of Java Impls - if possible #177

yigit opened this issue Nov 28, 2020 · 6 comments
Labels
P2 affects usability but not blocks users

Comments

@yigit
Copy link
Collaborator

yigit commented Nov 28, 2020

Java implementations of KS interfaces cause multiple problems:
#94
#176 (comment)
#170

Also there are some other issues (i couldn't find bugs for) like properties are not implemented properly (e.g. theirs accessors are not available).

Now I'm not sure if it is possible but if so, would you consider removing java implementations in favor of Descriptor implementations?

I'm not even sure if it is possible (e.g. there was this bug #167) but if all java elements are guaranteed to have descriptors, I think moving to them would provide a lot of consistency. When processing kotlin code, the processor does not want to see java classes different than the way kotlin compiler sees so this would help a lot with consistency going forward. Otherwise, we need to make sure all Java implementations treat java source code the way kotlin compiler does.

Moving Java implementations to descriptor could cause problems with KSDeclarationDescriptorImpl.origin but I hope that can be resolved, worst case by passing down the information

@yigit
Copy link
Collaborator Author

yigit commented Nov 28, 2020

some note from prototyping:

  • the order of declarations do not match the java source when reported (not sure it matters but afaik javap preserves that order)
  • types get resolved in kotlin realm (e.g. if you have a java code with foo : Object, it is reported as foo:Any). Could actually be desired not sure. (related: Should getClassDeclarationByName mimic Kotlin Compiler? #126)

@yigit
Copy link
Collaborator Author

yigit commented Nov 28, 2020

another thing that needs to be resolved is declared functions / constructors.
Right now, it is not clear whether KSClassDeclaration.getAllFunctions should include constructors or not.

@neetopia
Copy link
Collaborator

Other than removing them, just delegate all operations to descriptors in Java impls should also work, we already delegate several APIs to underlying descriptors.

The concern here is mainly around performance since in the current solution a fair portion of the operations is psi based which is much faster than resolving to descriptors.

@yigit
Copy link
Collaborator Author

yigit commented Nov 29, 2020

For performance, I thought it would not be very important since this only happens for java source files, hopefully should be limited in any app that wants to take advantage of KSP. On the opposite perspective, when it does not match how kotlin evaluates java sources, it becomes a rather big problem (so even when it is limited usage in app, it should still be correct or consistent)

I also think in some cases, delegating to descriptors might lose some information. I've not hit a very important case right now but one I hit was, when an annotation refers to a non-existing class, descriptor implementation returns null vs java implementation can return error type. Might be fixable, not sure.

I'm trying to make all tests pass to see what behaviors change to be able to evaluate this better.

@yigit
Copy link
Collaborator Author

yigit commented Nov 30, 2020

I have a prototype for this: https://github.com/yigit/ksp/tree/no-java-impls
All tests are passing, had to do some minor changes in tests. Some are bug fixes, some are no-op (like order of outputs) but at least one of them is an undesired behavior change (error types in annotations turn into null).

At least parts of it could turn into a real PRs to fix some other issues but I need to isolate them.

I'll either write a doc on changes or turn it into a PR and to comment on them so it is clear what changes are happening to better evaluate this proposal.

@ting-yuan
Copy link
Collaborator

We cannot overlook the potential performance impact. Without some performance evaluation backing it up, completely switching to the descriptor based implementation really concerns me. I get that quality (in terms of correctness) is at a much higher priority, especially at this moment. However, to achieve it, shouldn't we fix the inconsistencies and java impl bugs?

If fixing the java impl can't keep up to your pace, maybe a runtime flag that allows switching the implementation would work. What do you think?

@ting-yuan ting-yuan added the P2 affects usability but not blocks users label Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 affects usability but not blocks users
Projects
None yet
Development

No branches or pull requests

3 participants