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

Implement basic KSP interop #1146

Merged
merged 8 commits into from Sep 13, 2021
Merged

Implement basic KSP interop #1146

merged 8 commits into from Sep 13, 2021

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Sep 12, 2021

This starts a basic KSP interop project, with extensions covering common use cases like...

  • Basic KS type -> KotlinPoet TypeName type conversion
  • Support for originating KSFiles to support KSP's incremental processing
  • Support for writing FileSpec to CodeGenerators
  • Annotated with a new @KotlinPoetKspPreview annotation while we fine-tune the API.

This is largely upstreamed and cleaned up from the Moshi KSP implementation currently being upstreamed over here.

What this does not attempt to do: the full "give me a class and I'll give you a TypeSpec of the API" like kotlinx-metadata interop does. I'm open to considering it if we think it's worthwhile, but unlike with metadata we don't need to run everything from a class's @Metadata annotation for this to work, so I'm currently erring on the side of keeping the interop lightweight. There could be a good use-case for some existing helpers like overriding, AnnotationSpec.get(), ParameterSpec.get(), etc, but we can see if the community asks for that later.

TODO:

  • Tests. Moshi used integration tests as cover for this. We'll need to figure out a good way to test these here. Open to saving this for a followup PR.

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

Dope, thanks! nit: need copyright headers where they're missing.

Tests would definitely be great, mostly to provide usage examples and to make it safer to iterate on this code further. A sample project would be nice too.

interop/ksp/build.gradle.kts Outdated Show resolved Hide resolved
settings.gradle.kts Outdated Show resolved Hide resolved
get() = emptyMap()

override fun get(index: String): TypeVariableName {
error("No TypeParameter found for index $index")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should it throw something like UnsupportedOperationException?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not in this case I think, this is an unexpected condition rather than something unsupported or unimplemented

Copy link
Collaborator

Choose a reason for hiding this comment

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

Called out as based on the message it may seem that this API will return something for a different index, when in fact it will always throw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahhh I didn't realize where you were commenting (was looking on mobile). Good point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Went with NSEE to match what we do in metadata for this 74e323a

*
* @see FileSpec.originatingKSFiles
* @param codeGenerator the [CodeGenerator] to write to
* @param aggregating flag indicating if this is an aggregating
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "aggregating annotation processor"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, though I think we want to say "symbol processor"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*
* @see FileSpec.originatingKSFiles
* @see FileSpec.writeTo
* @param aggregating flag indicating if this is an aggregating
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

} else {
"Function$count"
}
ClassName("kotlin.jvm.functions", functionSimpleName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a multiplatform-friendly alternative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure to be honest. The spec focuses mostly on JVM and agnostic impls, but doesn't explicitly call out how it works on other platforms. Seeing as we do this here and in metadata, mind if we save this for a later day?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For sure, especially since I believe KSP only targets JVM with 1.0? Or it will support MPP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

KSP will support MPP yeah (insofar as it will be able to run code gen for MPP projects)

ZacSweers and others added 5 commits September 12, 2021 22:16
Co-authored-by: Egor Andreevich <egor@squareup.com>
Co-authored-by: Egor Andreevich <egor@squareup.com>
@ZacSweers
Copy link
Collaborator Author

Talked offline, will work at some basic test infra for this in a followup PR. Will likely entail writing a sample processor that parses a class and then generates code that shows it read it correctly.

@ZacSweers ZacSweers merged commit 9952ddc into master Sep 13, 2021
@ZacSweers ZacSweers deleted the z/kspInterop branch September 13, 2021 03:46
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.

None yet

2 participants