Add Kotlin BSP integration test

[alexarchambault]

This PR adds non-regression integration tests for Kotlin support via BSP. Kotlin modules are currently advertized as Java ones, and IntelliJ seems to handle fine Kotlin sources in those (using non-experimental BSP support).

[alexarchambault]

Includes #3608 for now

[alexarchambault]

Should preserve bin compat, unlike #3619 and #3644

[lefou]

TBH, to accept these weird kind of workaround, I'd require at least an upstream issue and/or discussion in the BSP project. Especially since this is a IDEA specific thing whereas BSP is a more general protocol, also serving other IDEs and editors.

We already have some BSP client specific handling (e.g. enable semanticDB for metals, or if you dig in the git history handling of the build.sc for IDEA), so we should guard workaround behind a BSP client check.

[alexarchambault]

@lefou #3644 restricts that workaround to IntelliJ, and only so if it seems IntelliJ lacks explicit Kotlin support over BSP. But it requires changes that break binary compatibility in BspModule.

[alexarchambault]

In a similiar fashion, I'd like to propose to add the synthetic root module only when we detect we're connected to IntelliJ (that one could be made without breaking bin compat).

[lefou]

Can you a bit more specific? How do you detect the missing support in IDEA and why can't it be binary compatible. Or if this was discussed before plz provide a pointer.

[lefou]

The whole BSP implementation is far from simple, so a bit of context and explanation helps a lot.

[alexarchambault]

Can you a bit more specific? How do you detect the missing support in IDEA and why can't it be binary compatible. Or if this was discussed before plz provide a pointer.

Right now, my understanding is that BSP support in IntelliJ is provided by intellij-scala. Intellij-scala advertizes support only for Java and Scala in its InitializeBuildParams during BSP server initialization. Yet, in practice, it supports Kotlin sources in Scala modules, as I was able to check with this project. The PR here makes Kotlin works in IntelliJ for Mill projects by relying on this (and I confirm it works fine by checking manually). If you're checking manually, note that I use the non-experimental BSP import in IntelliJ (the EAP version I use offers an experimental and a non-experimental BSP import - I'm not sure what the experimental one is, and why it doesn't work fine here).

Yet, advertizing Kotlin modules as Scala ones doesn't allow to pass kotlinc arguments (and other Kotlin-specific details) to the BSP client. It seems Jetbrains is working on a better solution, with explicit support for Kotlin via BSP, in the intellij-bsp project (I mean, it's formerly called "intellij-bsp", it's now part of a bigger repo). Note that intellij-scala has its own BSP implementation, that differs from the one of intellij-bsp.

@jastice said in #3619 that intellij-bsp would probabaly be used mid-term by intellij-scala too.

So in #3644, we try to detect which IntelliJ BSP implementation we talk to, and advertize Kotlin modules either as Scala ones (for intellij-scala) or as Kotlin ones (for intellij-bsp). To detect which intellij BSP implementation we talk to, inspecting the BSP InitializeBuildParams seems to work: both implementation use Intellij-BSP as "display name", but they claim support for different language sets: only Java and Scala for intellij-scala, Java, Scala, Kotlin, and more for intellij-bsp.

[alexarchambault]

About the binary compatibility issue, we need details of the client InitializeBuildParams (at least, "display name" and supported languages) in BspModule.{bspBuildTarget, bspBuildTargetData}, in order to detect which IntelliJ BSP implementation we talk to. Passing those details to these methods breaks bin compat in #3644.

[alexarchambault]

The idea with the PR here is to advertize Kotlin modules as Scala ones via BSP, rather than Java ones as is currently the case (as KotlinModule doesn't currently override BspModule.{bspBuildTarget, bspBuildTargetData}). That seemed to be an acceptable short-term workaround to me, until the bin compat of BspModule can be broken. But if you'd rather wait a bit for something like the cautious BSP implem detection of #3644, that's fine by me too.

[lefou]

Understood.

Regarding the detection logic. The data in BspModule isn't necessarily the one we need to send to the client. I think the server could simply translate it depending on its knowledge about the client. So we could be more BSP client specific without breaking BspModule.

Technically, I'd rather avoid investing into the current monolithic BSP implementation in favor of a more modular approach, but it's currently blocked by bsp4j not being leightweight enough,

• Split artifacts into a bsp4j.protocol and a bsp4j.server  build-server-protocol/build-server-protocol#668
• Can the bsp4j classes be more lightweight? build-server-protocol/build-server-protocol#651

[lefou]

I think KotlinModule should definitely override BspModule defs to reflect its nature. This shouldn't break any compatibility.

Please note, that KotlinModule is currently unfinished. Also, I'd expect a rather poor behavior in any BSP client, since compilation of Kotlin modules isn't incremental and takes very long compared to any other languages supported by Mill. Hence, the GenIdea approach is still the go-to solution for Kotlin projects. I can't say much more about it, since I haven't finished the review of all the changes introduced lately to Mill in depth. This also includes the synthetic BSP module you mentioned.

[alexarchambault]

One last note: I can't recall if I checked whether Kotlin sources alongside a Java module (as should be the case currently without this PR) were fine in Intellij. I checked as Kotlin module as done in #3644 in the general case, and as a Scala one as done here, for sure. We should able to do #3644 without breaking bin compat if it works fine alongside a Java module, by changing the KotlinBuildTarget data to JvmBuildTarget in MillBuildServer itself, rather than from the BspModule methods overrides.

[lefou]

So is intelli-scala ignoring modules which support Kotlin as BSP language? Or is just the Kotlin part not working, but they still work as Java modules?

[lefou]

This repeatedly rebasing makes the review process harder than necessary. I'm going to set this PR to draft mode until PR #3608 is merged.

[alexarchambault]

This repeatedly rebasing makes the review process harder than necessary. I'm going to set this PR to draft mode until PR #3608 is merged.

The last commit is the only one specific to this PR. You can just look at this one. You'll have no diff between pushes, but that commit isn't that large.

[lihaoyi]

One nit but looks good. What's the relationship between this and #3644? Does one depend on the other? Or are they unrelated?

[lihaoyi]

What's the relation between this bspBuildTarget and the jvmBuildTarget you introduced in #3681?

[alexarchambault]

bspBuildTarget is defined in BspModule, and was there before my PRs. It contains details about the module, passed to the BSP clients via the workspace/buildTargets request. It goes along bspBuildTargetData, that contains language-specific data passed along in the response to this request too.

bspJvmBuildTarget contains JVM-specific details about the module. These are passed to clients via bspBuildTargetData for JavaModule, and as a field of what's passed in bspBuildTargetData for ScalaModule (same for KotlinModule in #3644). So ScalaModule (and KotlinModule in #3644) also use bspJvmBuildTarget, but uses it differently in bspBuildTargetData.

[alexarchambault]

What's the relationship between this and #3644? Does one depend on the other? Or are they unrelated?

#3644 includes the PR here. It adds things to it (it actually adds Kotlin-specific data in some BSP responses, to pass kotlinc arguments to clients in particular). It adds what intellij-bsp expects for Kotlin modules, although I wasn't able to use it in practice, and it breaks binary compatibility in order to detect which IntelliJ BSP implementation it's talking to (the one of intellij-scala or the new one of intellij-bsp).