Fix code formatting with ktfmt (fixes #304)

[rami3l]

Following previous discussion in #303 (comment), this PR is an attempt to resolve #304:

• Fix the existing code formatting with gradle ktfmtFormat using the official style configuration:

  val KOTLINLANG_FORMAT = FormattingOptions(style = GOOGLE, blockIndent = 4, continuationIndent = 4)

• Adjust default KLS formatting option to match KOTLINLANG_FORMAT:
  

  kotlin-language-server/server/src/main/kotlin/org/javacs/kt/formatting/Formatter.kt

  Lines 15 to 19 in 55f58e8

  	KtfmtOptions(
  	style = KtfmtOptions.Style.GOOGLE,
  	blockIndent = options.tabSize,
  	continuationIndent = options.tabSize
  	),

• Make gradle build depend on gradle ktfmtCheck.
• Add corresponding VSCode formatting task.

Questions

• Why do we need to vendor ktfmt for this project?
• CI check should have been triggered on PR?

[fwcd]

We're vendoring ktfmt (here) since it uses the embedded Kotlin compiler rather than the standard one, which we use. The embedded Kotlin compiler uses other package paths (e.g. org.jetbrains.kotlin.com.intellij instead of com.intellij), which causes weird clashes at runtime, for example:

java.lang.NoSuchMethodError: 'org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.createForProduction(org.jetbrains.kotlin.com.intellij.openapi.Disposable, org.jetbrains.kotlin.config.CompilerConfiguration, org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles)'
            at com.facebook.ktfmt.format.Parser.parse(Parser.kt:45)
            at com.facebook.ktfmt.format.Formatter.sortedAndDistinctImports(Formatter.kt:141)
            at com.facebook.ktfmt.format.Formatter.format(Formatter.kt:84)
            at org.javacs.kt.formatting.FormatterKt.formatKotlinCode(Formatter.kt:10)
            at org.javacs.kt.KotlinTextDocumentService$formatting$1.invoke(KotlinTextDocumentService.kt:204)
            at org.javacs.kt.KotlinTextDocumentService$formatting$1.invoke(KotlinTextDocumentService.kt:199)
            at org.javacs.kt.util.AsyncExecutor.compute$lambda-2(AsyncExecutor.kt:19)
            at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700)
            at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
            at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
            at java.base/java.lang.Thread.run(Thread.java:829)

While this is non-ideal, it's currently the best solution I could find that works for our setup. A better solution would be to automate this as part of our Gradle build (i.e. rewrite the package paths and replace the transitive kotlin-compiler-embeddable dependency with kotlin-compiler - basically do this automatically), but I couldn't figure out a clean way of doing so yet.

[rami3l]

@fwcd
Thanks a lot for your reply!

Just being curious: given the context in the official thread, what would possibly be the downside if we also switch to kotlin-compiler-embeddable? It seems that ktfmt has done exactly that in facebook/ktfmt#58.

I'm trying my best to get familiar with the current codebase, please inform me if I missed anything :)

[fwcd]

We tried this and it didn't work since there were other dependencies which needed the non-embedded compiler and didn't have an embedded version themselves (I think it was the kotlin-scripting-compiler or perhaps the IDEA plugin, which we no longer depend on though, can't remember). Might be worth trying again, but I haven't gotten around to doing so (though feel free to experiment).

[rami3l]

@fwcd
I see. I would like to see what I can to to solve this dependency problem.

However, due to the fact that the incoherent formatting might cause frequent merge conflicts, can we first merge what has been changed for now (if you agree, of course), and we will continue our discussion in another issue (#338)?

Thanks a lot :)

[rami3l]

@fwcd
Hi again! I would really like to help, but It seems to me that formatting the code base and changing the project config at the same time gets way too hairy.

So this got me thinking, maybe a better approach to get this merged is to first change all the configs without changing the actual formatting (probably need to disable the formatting check in gradle build first), and then somehow prompt the open PRs to use the config to format the code on their side...

I believe this could possibly reduce the number of conflicts for the authors of unmerged PRs at the moment. What do you think?

---

cc @kievitsp from #318 (comment)