-
Notifications
You must be signed in to change notification settings - Fork 84
KTL-3015 implement Kotlin LSP proxy for completions #878
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
base: kotlin-community/dev
Are you sure you want to change the base?
KTL-3015 implement Kotlin LSP proxy for completions #878
Conversation
Please, resolve conflicts |
@@ -0,0 +1 @@ | |||
rootProject.name = "lsp-users-projects-root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this empty lsp-users-projects-root
committed in this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the base project from which LSP will retrieve base libraries/sdks/project sources. As of now, this project just sets up a simple kotlin project with version 2.2.20
, but it is thanks to this base project that we could define libraries available in playground (e.g. kotlinx.coroutines
) or common configurations.
The LSP needs a gradle project in its local filesystem, and I though it could be handy to have the base project here in the compiler server repo in order to reflect the current completion's approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, then it looks more like a folder in resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove gradle
,.gitattributes
, and .gitignore
from this test project?
src/main/kotlin/com/compiler/server/configuration/WebSocketConfig.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/compiler/server/configuration/WebSocketConfig.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/compiler/server/configuration/WebSocketConfig.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/compiler/server/controllers/CompilerRestController.kt
Outdated
Show resolved
Hide resolved
Please add kdocs for all public methods and classes |
src/main/kotlin/com/compiler/server/controllers/LspCompletionWebSocketHandler.kt
Outdated
Show resolved
Hide resolved
incomingCompletionsRequests[session.id] = it | ||
} | ||
|
||
val responseFlow = MutableSharedFlow<Response>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can extract lines from 59 to 75 to a separate method called setupResponseFlow
(you can think about a better name)
} | ||
|
||
fun KotlinLspProxy.onClientConnected(clientId: String) { | ||
val project = Project(files = listOf(ProjectFile(name = "$clientId.kt"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it hard to rewrite it to list of files instead of one file now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear for me, why do we have project and lspProject, can it be merged into one entity here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
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
adece87
to
d1fd552
Compare
# Conflicts: # build.gradle.kts
temporary solution until the new version of kotlin LSP is released publicly. Right now a manual building of the zipped version is performed in order to guarantee the usage of a koltin-lsp version that supports custom "isolated documents mode" running option.
This tests still do not pass due to some internal errors in resolving imported workspace. An investigation is being carried out in order to assess why this behaviour. Note that this tests will pass using a local running language server instance.
This version helps in all those cases where the LS is busy serving clients and some completion requests can fail due to the server not keeping up with `didOpen` notifications which should arrive before the completion request.
c8c6ebc
to
8a0db31
Compare
testcontiner uses `docker-compose` APIs which aren't present in github actions runners. This fix should let testcontainer run in its environment without depending on host's APIs version.
8a0db31
to
c6e36ac
Compare
refactor: remove unnecessary completable future extension function for cancelling events refactor: add uri's guards for each notification type docs: add client capabilties issue
* chore: remove logging when for build workflow
95cc73a
to
c4c51ad
Compare
@@ -0,0 +1,95 @@ | |||
import org.junit.jupiter.api.Test | |||
|
|||
interface AbstractCompletionTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove CompletionTest
, ImportTest
and completion connected tests from ConcurrencyRunnerTest
and move deleted tests to the new completions module, please
@@ -0,0 +1 @@ | |||
rootProject.name = "lsp-users-projects-root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove gradle
,.gitattributes
, and .gitignore
from this test project?
completions/build.gradle.kts
Outdated
} | ||
|
||
dependencies { | ||
implementation("org.springframework.boot:spring-boot-starter-webflux") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets put in libs.versions.toml
as well
completions/build.gradle.kts
Outdated
testImplementation(libs.kotlin.test) | ||
testImplementation(libs.bundles.testcontainers) | ||
testImplementation(libs.rector.test) | ||
testImplementation("org.springframework.boot:spring-boot-starter-test") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets put in libs.versions.toml
as well
} | ||
} | ||
.flatMap({ payload -> | ||
val req = runCatching { objectReader.readValue<CompletionRequest>(payload) }.getOrNull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need objectReader
? Not clear for me, why just not use: objectMapper.readValue(payload, CompletionRequest::class.java)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object reader is an instance of object writer that is used only for instances of CompletionRequest::class.java
, which in theory should improve a little bit deserialization performance. Even if we have to explicitly mark the generic parameter in readValue<CompletionRequest>
, this is just for enabling type inference, that object reader can be used only for parsing CompletionRequest
.
If it's confusing I think that I can remove it without any noticeable performance loss.
import org.springframework.web.bind.annotation.ExceptionHandler | ||
|
||
@ControllerAdvice | ||
class LspAvailabilityAdvice { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* | ||
* @param query the query the user has typed so far | ||
*/ | ||
fun List<CompletionItem>.rankCompletions(query: String): List<CompletionItem> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I used to conventions, where all public methods go on top and private go down
?.jsonObject?.get("prefix") | ||
?.jsonPrimitive?.content | ||
|
||
private fun fuzzyScore(query: String, candidate: String): Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it taken from VS code plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import com.fasterxml.jackson.annotation.JsonValue | ||
|
||
@JsonIgnoreProperties(ignoreUnknown = true) | ||
data class Project( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets move it to package completions.dto.api call it CompletionsRequest and remove args
and confType
as we don't need this fields
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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to copy the whole project from the old API, just save fields, that you need
This PR introduces a new way of performing completions requests leveraging Kotlin-lsp. Two approaches are included:
api/compiler/lsp/complete
;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.