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

Add Resolver.mapToJvmClassName() for KSClassDeclaration #1338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bubenheimer
Copy link

Resolves #1336

Focusing on KSClassDeclaration for now. Other types of KSDeclaration may be added later by changing the parameter type to KSDeclaration.

// TEST PROCESSOR: MapJvmClassNameProcessor
// EXPECTED:
// Cls1
// Cls1$Cls2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any more complicated class names? It looks to me that if only nested class matters, we can actually write this in a format of extension function by recursively checking the parent declaration and build a name?

Copy link
Author

@bubenheimer bubenheimer Mar 8, 2023

Choose a reason for hiding this comment

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

I used signatureMapper.kt (and MapSignatureProcessor) as a template, for the equivalent Resolver.mapToJvmSignature() functionality. Both tests are equally underpowered. However, I don't think can really do much right now to make my test a lot more expressive until #1335 is fixed, which crashes the new Resolver.mapToJvmClassName() in the same manner, so I expect that a fix for #1335 will similarly clear the landscape here.

Once the fix is in place, I'd think the following additional tests will be particularly useful:

  • Local class inside internal function (function name mangling)
  • Local class inside function annotated with @JvmName("xyz")
  • Local class inside overloaded function

Anything else of particular interest that you can think of?

Edit: also: local class inside top level function (public & internal) in file annotated with @file:JvmName("xyz"). Is that possible within the current test infrastructure?

Edit: inline classes & local class in function with inline class parameter (may have special mangling)

@bubenheimer
Copy link
Author

The CI build seems to trip over ResolverAAImpl. Not sure what that means.

@neetopia
Copy link
Contributor

neetopia commented Mar 8, 2023

ResolverAAImpl is a WIP project. Since it inherits Resolver interface you need to implement it to pass compilation, the simplest way is to just put a TODO() in the implementation.

Although, if the logic is simple enough as I asked in the comment, maybe it is easier to just add an extension function in util so that there is no need to add an api to Resolver?

@bubenheimer
Copy link
Author

Gotcha. Logic is not so simple, unfortunately.

@bubenheimer
Copy link
Author

bubenheimer commented Mar 8, 2023

I'm unable to build on my local MacOS for some reason, so I'll have to rely on CI for guidance I guess. I can build the basics locally, but not the whole project.

@neetopia
Copy link
Contributor

neetopia commented Mar 8, 2023

Based on some observations on the behavior of compiled class files, the name mostly gets complicated when it comes to local classes, which is unfortunately not supported as the moment even with this PR.

With local classes though, the behavior seems to be about adding a number specifying the functions that a local class belongs to distinguish and avoid name clashes among multiple local classes, it it seems doable with our getDeclarationsInSourceOrder API. If that is the case (name only matters with the ordering of which the parent function belong to), it might still be fine to just implement as an extension function.

My concern is that without an explicit spec, aforementioned approach won't work, then we still need to rely on compiler to do the work. But at the same time, compiler is throwing exceptions for such queries, we might need to fix compiler before going with current approach in this PR.

@bubenheimer
Copy link
Author

I'm ok with the idea of a recursive extension function for this. However, it does not seem trivial to me to get all possible cases right at each level, and dealing with correct naming & name mangling. Correct implementation may also be tied to specific compiler versions, naming & name mangling might change between versions. Perhaps even specific to JVM version, if it turned out to not be part of an official specification.

Perhaps it seems more daunting to me because I lack a good grasp of it. Are you or KSP team willing to implement this kind of function? I don't think I will dare to do it myself, even though my processor won't work with local classes without this functionality.

I guess, like you said, the next step in any case would be to find out if local class binary name structure is documented/specified.

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.

Provide binary name for KSClassDeclaration in Resolver
2 participants