KTL-3015 implement Kotlin LSP proxy for completions #878

S-furi · 2025-10-01T15:10:06Z

This PR introduces a new way of performing completions requests leveraging Kotlin-lsp. Two approaches are included:

RESTful on api/compiler/lsp/complete;
WebSocket approach.

Currently completions with this approach supports only the latest Kotlin version, but future developments include the support of such feature by using a dedicated client and workspace for each Kotlin version supported by the kotlin-compiler-server.
Moreover, the support for different platforms than Kotlin/JVM is strictly bound to what Kotlin-lsp supports, namely only Kotlin/JVM projects as of today.

dkrasnoff · 2025-10-07T09:55:09Z

Please, resolve conflicts

common/src/main/kotlin/model/Completion.kt

completions/src/test/resources/lsp/lsp-users-projects-root-test/settings.gradle.kts

src/main/kotlin/com/compiler/server/configuration/WebSocketConfig.kt

src/main/kotlin/com/compiler/server/controllers/CompilerRestController.kt

dkrasnoff · 2025-10-07T12:06:56Z

Please add kdocs for all public methods and classes

src/main/kotlin/com/compiler/server/controllers/LspCompletionWebSocketHandler.kt

dkrasnoff · 2025-10-07T12:10:59Z

completions/src/main/kotlin/completions/lsp/KotlinLspProxy.kt

+ }
+
+ fun KotlinLspProxy.onClientConnected(clientId: String) {
+ val project = Project(files = listOf(ProjectFile(name = "$clientId.kt")))


I'd say each project should have clientId, but files can then be called file1-$clientId.kt, file2-$clientId.kt, etc, so that we will have a possibility to work with mutliple files

Is it hard to rewrite it to list of files instead of one file now?

It shouldn't introduce big problems, but right now the LSP logic is implemented considering only a single file, meaning that every file is treated in isolation regardless of the same user. Unfortunately it is not trivial to make the user's files see each other with the current isolated documents LSP mode.

Got it, thanks!
Anyway, let's make multiple files here and pass to lsp only the first one. And leave a comment or todo, with the place where we choose only one file, with the explanation about LSP mode.
Note: if it is a todo, then it should have the number of the task connected to it. If we don't have such a task, then you should create it.

I've added support for multiple documents, let me know if it's ok.
As a sidenote, my main doubt is how we handle which document among the project's documents has to be picked for providing completions. Right now in the KotlinLspProxy I've changed completion functions signatures in order to accept a ProjectFile (which by default is the first file of the project), which now are unused due to the API format, i.e frontend just provide project + line + character. Once multiple files will be supported, the APIs should change in order to include such logic.

dkrasnoff · 2025-10-07T12:11:32Z

completions/src/main/kotlin/completions/lsp/KotlinLspProxy.kt

+ fun KotlinLspProxy.onClientConnected(clientId: String) {
+ val project = Project(files = listOf(ProjectFile(name = "$clientId.kt")))
+ .also { clientsProjects[clientId] = it }
+ val lspProject = LspProject.fromProject(project).also { lspProjects[project] = it }


Not clear for me, why do we have project and lspProject, can it be merged into one entity here?

They can be merged, but I needed a way to keep track of files in the file system, retrieve their lsp-compliant URIs and lsp documents versions. I though it was better to keep them separate being Project a simpler entity and already containing what's needed for most compiler server tasks.

I expect that after extracting completion to a separate subproject, you will not have a Project entity, so that you can only leave a LspProject

Even after extracting the completion logic to a separate subproject, the dependency on Project is still present due to the API contract that we have with frontend. Frontend will send a Project instance to completion endpoints (both REST and WS as of now), so we still need a way to map the incoming project plus the convenient LspProject. If the subproject will be lsp-based only I can merge the two entities, while I suggest to keep them separated in case it must be shared by other completion logics. In the meantime I'll take care in documenting LspProject so that its purpose is clear.

You don't need to copy the whole project from the old API, just save fields, that you need

src/main/kotlin/com/compiler/server/controllers/LspCompletionWebSocketHandler.kt

S-furi · 2025-10-08T14:45:23Z

@dkrasnoff With recent commits I've made some changes that have probably changed the references to the request changes above. In short, I've

Resolved conflicts kotlin-community/dev base branch
Moved completion logic into a new springboot application module
LSP-based completions now leverage WebFlux
- Both REST endpoint (marked as suspendable) and WebSocket Implementation.
Moved LSP related files from project root to its corresponding resources folder (conversation reference)
Applied suggested refactors

I remain available for any eventual question.

completions/src/test/kotlin/AbstractCompletionTest.kt

completions/src/test/resources/lsp/lsp-users-projects-root-test/settings.gradle.kts

completions/build.gradle.kts

completions/src/main/kotlin/completions/controllers/ws/LspCompletionWebSocketHandler.kt

dkrasnoff · 2025-10-15T11:38:57Z

completions/src/main/kotlin/completions/lsp/LspAvailabilityAdvice.kt

+import org.springframework.web.bind.annotation.ExceptionHandler
+
+@ControllerAdvice
+class LspAvailabilityAdvice {


Let's maybe move the controller package and write a class global exception handler for all exceptions, where we can handle all custom exceptions like this.

dkrasnoff · 2025-10-15T12:30:56Z

completions/src/main/kotlin/completions/lsp/LspCompletionParser.kt

+ *
+ * @param query the query the user has typed so far
+ */
+ fun List<CompletionItem>.rankCompletions(query: String): List<CompletionItem> =


nit: I used to conventions, where all public methods go on top and private go down

dkrasnoff · 2025-10-15T12:33:02Z

completions/src/main/kotlin/completions/lsp/LspCompletionParser.kt

+ ?.jsonObject?.get("prefix")
+ ?.jsonPrimitive?.content
+
+ private fun fuzzyScore(query: String, candidate: String): Int {


Was it taken from VS code plugin?

It's inspired and a simplification of how Monaco editor (the actual editor inside VS-Code) performs completion matching. With LSP it's largely up to the editor to determine completion sorting, LSP just provides a filterText: int field that rarely yields good completion results on top.

Got it than, lets make it util and cover this fuzzyScore with differnet test scenarios, to show what do we expect from it in the tests

dkrasnoff · 2025-10-15T12:45:30Z

completions/src/main/kotlin/completions/model/Project.kt

+import com.fasterxml.jackson.annotation.JsonValue
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+data class Project(


lets move it to package completions.dto.api call it CompletionsRequest and remove args and confType as we don't need this fields

dkrasnoff · 2025-10-15T12:53:47Z

completions/src/main/kotlin/completions/lsp/KotlinLspProxy.kt

+ fun KotlinLspProxy.onClientConnected(clientId: String) {
+ val project = Project(files = listOf(ProjectFile(name = "$clientId.kt")))
+ .also { clientsProjects[clientId] = it }
+ val lspProject = LspProject.fromProject(project).also { lspProjects[project] = it }


You don't need to copy the whole project from the old API, just save fields, that you need

dkrasnoff · 2025-10-16T12:03:33Z

completions/src/test/kotlin/ImportTest.kt

 import kotlin.test.Ignore
 import kotlin.test.assertEquals

 // TODO(Dmitrii Krasnov): this test is disabled until KTL-2807 is fixed


I thought we could remove this todo and ignore the annotation here and in the concurrency test

Yes we can, but I wanted to discuss with you about a little detail: having developed basically two ways of performing completion (REST and WebSocket), we could take inspiration from current's CompletionTest, i.e. an abstract test that is reified for both implementations. In this way we reuse current tests, testing both completions approaches adding the least amount of code as possible.

I was trying to implement this with ImportTest, but apparently LSP completions differs a little bit from what is expected in that test (e.g. old completions: Random (kotlin.random.Random), LSP-based completions: Random (kotlin.random)). In these cases, should I change the tests or should I adapt LSP-based completions to make those tests pass? (it is mostly regex and string manipulation).

Yeap, you can manipulate with test data if it doesn't affect the main logic of the check like in your example.

dkrasnoff · 2025-10-16T12:52:34Z

completions/build.gradle.kts

+
+plugins {
+ id("base-kotlin-jvm-conventions")
+ alias(libs.plugins.spring.dependency.management)


What do you think about extracting base spring related things to Grdle convention plugin like base-kotlin-jvm-conventions, so we can share the same spring build setting across different modules.

That's a great idea! I'm going to try and see what I can do after fixing requested changes!

completions/src/main/kotlin/completions/controllers/ws/LspCompletionWebSocketHandler.kt

dkrasnoff · 2025-10-16T12:58:14Z

completions/src/main/kotlin/completions/lsp/LspCompletionParser.kt

+ ?.jsonObject?.get("prefix")
+ ?.jsonPrimitive?.content
+
+ private fun fuzzyScore(query: String, candidate: String): Int {


Got it than, lets make it util and cover this fuzzyScore with differnet test scenarios, to show what do we expect from it in the tests

S-furi · 2025-10-16T13:06:11Z

@dkrasnoff I don't remember if we've already discussed this, but should I move also Completion defined in common subproject into the new completions subproject?

…ackage

Now client's requests are modelled as `CompletionRequest` which will not include args or conftype. This simplifies the logic of `LspProject` and the translation between the two.

fix: fix relative path when running locally fix: use different client id when running stateful test chore: re-introduce `objectReader` fix: propagate refactor in payload of WS tests

… project

Added tests, and improved the following: - improve fuzzy matches - prefer shorter completions first - ultimately follow `sortText`

…line and ch positions

S-furi · 2025-10-17T08:47:05Z

completions/src/test/kotlin/ImportTest.kt

+ """.trimIndent()
+
+ val foundCompletionsTexts = getCompletions(codeWithCaret = code, isJs = isJs).map { it.text }
+ val completions = listOf("kotlin.math.sin(")


@dkrasnoff I think I'm not getting what is the expected behaviour here, and also in current version of playground. Even when looking at the HTTP responses of playground if I insert this code snippet, I'm receiving the following completion:

{ "1": { "text": "kotlin.math.sin(", "displayText": "sin(x: Double) (kotlin.math.sin)", "tail": "Double", "import": "kotlin.math.sin", "icon": "method", "hasOtherImports": true } }

hasOtherImports is marked as true, so the text field is replaced from sin( to the fully qualified name kotlin.math.sin(. However, I think this logic is wrong, because hasOtherImports should not be true due to the fact that the there aren't actually other imports that matches the same function signature, therefore text should be sin(.

On the other hand, if this is a design choice (i.e. prefer fully qualified names over auto-import), please let me know.
I think the best way to understand how I am figuring out this logic can be found in how I cleanup completions to Completion before sending them to clients.

…s already imported fix: fix failing test after completion parsing changes

S-furi self-assigned this Oct 1, 2025

S-furi requested a review from dkrasnoff as a code owner October 1, 2025 15:10

dkrasnoff reviewed Oct 7, 2025

View reviewed changes

src/main/kotlin/com/compiler/server/controllers/LspCompletionWebSocketHandler.kt Outdated Show resolved Hide resolved

src/main/kotlin/com/compiler/server/controllers/LspCompletionWebSocketHandler.kt Outdated Show resolved Hide resolved

S-furi force-pushed the exp/isolated-documents branch 2 times, most recently from d1fd552 to 7731e29 Compare October 8, 2025 14:35

S-furi force-pushed the exp/isolated-documents branch 5 times, most recently from 95cc73a to c4c51ad Compare October 14, 2025 09:50

dkrasnoff reviewed Oct 15, 2025

View reviewed changes

S-furi force-pushed the exp/isolated-documents branch from aa74429 to ff47e3a Compare October 16, 2025 11:26

dkrasnoff reviewed Oct 16, 2025

View reviewed changes

S-furi force-pushed the exp/isolated-documents branch from 0b4bc80 to 0180d35 Compare October 16, 2025 12:59

S-furi added 10 commits October 16, 2025 15:14

feat: implement LSP-based completions

269a5ce

chore: remove unnecessary gradle files for lsp projects roots

2c5502e

chore: use version catalog for springboot deps

d55c535

refactor: bring public function above private ones

57643af

refactor: rename completion test base

9fdbd52

refactor: remove and move completion-related tests into appropriate p…

ebbffdb

…ackage

fix: guard sigle LSPClient creation when multiple requires are invoked

d2040c5

chore: add simple REST completion retrieval extension function

b59c532

chore: rebase fixes

ce6f097

chore: provide jackson's object mapper through a spring bean

cb70fc3

S-furi added 5 commits October 16, 2025 15:15

refactor: move lsp exception handler in exceptions' package

e0d20f2

refactor: move rest controller in rest package

8ac5069

chore: make a global exception handler for completions' exceptions

1a4d629

refactor: rename WS completion request dto

2b7f2b8

refactor: simplify Project and LspProject logic

69ed0ed

Now client's requests are modelled as `CompletionRequest` which will not include args or conftype. This simplifies the logic of `LspProject` and the translation between the two.

S-furi force-pushed the exp/isolated-documents branch from 0180d35 to 69ed0ed Compare October 16, 2025 13:16

S-furi added 5 commits October 16, 2025 16:33

fix: fix failing tests

deb5a8f

fix: fix relative path when running locally fix: use different client id when running stateful test chore: re-introduce `objectReader` fix: propagate refactor in payload of WS tests

refactor: move Completion from common subproject to completions…

5c0e301

… project

refactor: extract FuzzyCompletionRanking into util package

228c44c

chore: improve completion ranking

3c74e66

Added tests, and improved the following: - improve fuzzy matches - prefer shorter completions first - ultimately follow `sortText`

fix: propagate rest endpoint refactor

5ea84b9

S-furi force-pushed the exp/isolated-documents branch from c24863c to 5ea84b9 Compare October 17, 2025 07:11

S-furi added 5 commits October 17, 2025 09:16

chore: use JUnit assumption to ignore JS tests for LSP completions

87dc1db

refactor: name BaseCompletionTest companion object

d87ea04

refactor: use extractCaret function instead of manually specifying …

7eef577

…line and ch positions

refactor: avoid duplicating code for JVM and JS tests

fa4a174

build: force linux/amd64 when running LSP integration tests

61d399e

S-furi commented Oct 17, 2025

View reviewed changes

S-furi added 2 commits October 17, 2025 11:47

fix: adapt import test to LSP-based completions

5811314

chore: do not include package in brackets when completion reference i…

40ad0bb

…s already imported fix: fix failing test after completion parsing changes

S-furi force-pushed the exp/isolated-documents branch from 2cc6d44 to 40ad0bb Compare October 17, 2025 11:34

KTL-3015 implement Kotlin LSP proxy for completions #878

Are you sure you want to change the base?

KTL-3015 implement Kotlin LSP proxy for completions #878

Uh oh!

Conversation

S-furi commented Oct 1, 2025

dkrasnoff commented Oct 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkrasnoff commented Oct 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

S-furi commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

S-furi commented Oct 16, 2025

Choose a reason for hiding this comment

Labels

2 participants

S-furi commented Oct 8, 2025 •

edited

Loading