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

KSName doesn't offer a deterministic naming API #61

Closed
ZacSweers opened this issue Jun 11, 2020 · 6 comments
Closed

KSName doesn't offer a deterministic naming API #61

ZacSweers opened this issue Jun 11, 2020 · 6 comments
Assignees

Comments

@ZacSweers
Copy link
Contributor

KSName isn't ideal for handling non-trivial class names, such as heavily nested types. KotlinPoet's ClassName.bestGuess() API is used in the Glide example project, but that API should really only be used as a last resort.

In Kotlin's internals and metadata, it has a pattern for its names that is deterministic (e.g. "org/foo/bar/Baz.Nested"). It would be ideal if this name was exposed or otherwise surfaced a more concrete API.

Here's an implementation we have in KotlinPoet for piecing together a name from it: https://github.com/square/kotlinpoet/blob/06aad8e024aaa453477d420ad65cfe4af9e84f3a/kotlinpoet-metadata-specs/src/main/kotlin/com/squareup/kotlinpoet/metadata/specs/internal/ClassInspectorUtil.kt#L171-L209

Example of where KSName currently falls short:

  • Given this name: dev.zacsweers.moshisealed.sample.test.MessageTest.MessageWithNullDefault
  • The result of getQualifier is dev.zacsweers.moshisealed.sample.test.MessageTest 🤔
@neetopia
Copy link
Contributor

neetopia commented Jun 11, 2020

KSName is meant to align with qualified names therefore you are seeing this result, but it is also reasonable to provide correct logic to get the qualifier. We will discuss to see what's the path forward for KSName.
The Glide example is not using KotlinPoet in a very practical way, mainly due to there is no KSP support built in KotlinPoet so we took a workaround to address the ClassName, ideally, there should be some extension functions around KSP properties to work with KotlinPoet.

@ZacSweers
Copy link
Contributor Author

sounds good. I'm one of the maintainers of KotlinPoet, we'd be interested in seeing what a kotlinpoet-ksp artifact could like on our end. Some obvious ones come to mind, like FileSpec.writeTo(CodeGenerator) support and KSName.toClassName(). Open to other suggestions as well for things you'd want to see in an artifact like that, let us know 👍. Will file an issue on our end soon to track ideas

@neetopia neetopia self-assigned this Jul 31, 2020
@neetopia
Copy link
Contributor

neetopia commented Aug 7, 2020

Current KSName implementation is based on the qualified name spec.

To address this issue I will add a new property/function to KSName.

@ting-yuan
Copy link
Collaborator

A KSName returned by compiler/KSP is guaranteed to be unambiguous. As for getQualifier, did you mean to get its package name? If so, KSDeclaration.getPackage (to be added) might serve the purpose.

Forcing the notion of package in KSName would require all creation sites to supply package and the remaining identifier(s) separately. Although I agree that specifying package explicitly is much clearer, it is also less convenient.

As for the / separator, it looks like an encoding / implementation choice of Kotlin metadata. Would getPackage be more human friendly?

@neetopia
Copy link
Contributor

To add more context, on Kotlin language perspective names are separated by dot, and the case when you see names separated by slash is platform dependent (in this case, it is the JVM runtime name). In my opinion the right way to solve this is to provide KSDeclaration with a getPackage API so that you can inspect package name for a nested class. However, you will have to make some adjustments to the KotlinPoet code you linked with this approach.

@neetopia
Copy link
Contributor

Alternative solution for this is shipped with android/kotlin#98

@davidjwiner davidjwiner transferred this issue from android/kotlin Sep 25, 2020
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

No branches or pull requests

3 participants