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

DRAFT: Very rough in-progress work on Kotlin support in the Gazelle plugin #313

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

Conversation

MorganR
Copy link

@MorganR MorganR commented Dec 1, 2024

Do not merge this! It's for discussion. This has soo many caveats right now. It is ugly, is messy, is incomplete, has debug statements everywhere, has commented out code, etc.

I'm putting this up anyway so that people can take an early look at the overall approach, and give any general comments or guidance.

The overall approach:

  • Add a new Gazelle directive jvm_kotlin_enabled, which defaults to false, to optionally enable Kotlin processing. Use this flag to optionally look for Kotlin files in addition to Java files within packages. If a Kotlin file is present in a package, then we use the kt_jvm_library rule instead of java_library rule. Most remaining Go logic is unchanged.
  • Add a KtParser to the JavaParser gRPC server. This uses the Kotlin compiler's parser to process Kotlin files, and extract the same data that is extracted by the Java-oriented ClasspathParser.
    • This file currently handles detecting a main function, imported types and packages, and class declarations. It has partial coverage for exported types, and has a bunch of commented out code where I'm trying to figure out the best way to tackle detecting fully-qualified types and packages throughout the file. I also haven't yet added support for any annotation handling.

Questions for reviewers

  1. Overall, WDYT? If people think this is worth adding, I'll make a series of smaller, cleaner PRs to actually get this done.
  2. The plugin, its directives, packages, and classes often use the name java. By adding Kotlin, I'm somewhat implicitly changing the interpretation of this to something more like jvm. Any thoughts yay or nay on name changes? That's not an all or nothing thing. For example, maybe we rename the Gazelle Language to jvm, but keep the existing directive names to avoid breaking users. Or, we add new jvm directives, but continue to support the old java ones as well. Lots of other possibilities too.

FAQ

Why do this here when there is another Kotlin plugin?

A few reasons:

  1. If you're using Kotlin + Java, and exclusively using Kotlin with the JVM, then I'm not sure that it actually makes sense to treat Kotlin as a separate language from Gazelle's perspective. That makes sense for two languages like protobuf and Java, which have a strictly one-way dependency graph. However, in my experience, Java and Kotlin code frequently depend on each other, sometimes from separate packages, and sometimes within the same package. If dependencies are only between packages, then I can see separate Gazelle plugins potentially working with Gazelle's cross-resolve feature, though this seems more intended for entirely separate resolution methods, not inter-dependent language plugins. Additionally, I don't see a good way to handle intra-package Java & Kotlin dependencies. If the Java plugin knows nothing about Kotlin, it will try to produce a java_library rule for the Java files, which will lack some Kotlin-specific information. A Kotlin plugin would then need to replace this rule with a Kotlin one, and extend it to handle the extra information from the Kotlin files. This seems a bit hacky and error prone. Conversely, adding Kotlin support directly to the Java plugin is turning out to be relatively painless (minus me trying to learn how to use the Kotlin parser).
  2. There is an open support request for it.

Why add the KtParser to the same gRPC request handler, instead of a separate one for Kotlin?

I'd be happy to change this if others feel strongly, but we're ultimately trying to produce the exact same format of output data for Java and Kotlin, and merge them into a single set of data per package. We could either make two separate gRPC calls from Go land and then merge the data there, or make a single call and merge the data in Java land. That seemed like less boilerplate (defining new RPC requests) and less overhead (fewer network calls, even if they're to localhost).

Why add the KtParser to the same Java parsing server, instead of a separate server entirely?

See above, but also this would be even more resource overhead. No need to kick off multiple JVM processes if we can do it all in one.

@MorganR
Copy link
Author

MorganR commented Dec 1, 2024

I should note: I also haven't yet added support for generating test rules for Kotlin

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.

1 participant