Skip to content

Fix compatibility issues + bugs #556

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.

Sign up for GitHub

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

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

Fix compatibility issues + bugs #556

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
faf492e
Fix compatibility issues + bugs
Feb 19, 2024
2ae1252
Fallback to declaration text range
Feb 20, 2024
1abe6bf
Fix detekt violations
Mar 13, 2024
fb2550b
Requested changes
Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions server/src/main/kotlin/org/javacs/kt/Configuration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ public data class CompilerConfiguration(
val jvm: JVMConfiguration = JVMConfiguration()
)

public data class SymbolResolveSupport(
val enabled: Boolean = false,
val properties: List<String> = emptyList()
)

public data class WorkspaceConfiguration(
var symbolResolveSupport: SymbolResolveSupport = SymbolResolveSupport()
)

public data class IndexingConfiguration(
/** Whether an index of global symbols should be built in the background. */
var enabled: Boolean = true
Expand Down Expand Up @@ -108,4 +117,5 @@ public data class Configuration(
val externalSources: ExternalSourcesConfiguration = ExternalSourcesConfiguration(),
val inlayHints: InlayHintsConfiguration = InlayHintsConfiguration(),
val formatting: FormattingConfiguration = FormattingConfiguration(),
val workspace: WorkspaceConfiguration = WorkspaceConfiguration()
)
7 changes: 7 additions & 0 deletions server/src/main/kotlin/org/javacs/kt/KotlinLanguageServer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ class KotlinLanguageServer(
serverCapabilities.renameProvider = Either.forRight(RenameOptions(false))
}

config.workspace.symbolResolveSupport = clientHasWorkspaceSymbolResolveSupport(clientCapabilities)
Copy link
Owner

@fwcd fwcd Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configuration classes are intended to map (1-to-1) to the user-provided configuration. I'm not sure how I feel about adding a special case for a value derived from the client capabilities. IMO it would probably be better to just pass the client capabilities to KotlinWorkspaceService directly.

Copy link
Author

@lorenzhawkes lorenzhawkes Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, I change it to pass the client capabilities into the KotlinWorkspaceService


@Suppress("DEPRECATION")
val folders = params.workspaceFolders?.takeIf { it.isNotEmpty() }
?: params.rootUri?.let(::WorkspaceFolder)?.let(::listOf)
Expand Down Expand Up @@ -141,6 +143,11 @@ class KotlinLanguageServer(
InitializeResult(serverCapabilities, serverInfo)
}

private fun clientHasWorkspaceSymbolResolveSupport(clientCapabilities: ClientCapabilities) =
clientCapabilities?.workspace?.symbol?.resolveSupport?.properties?.let { properties ->
if (properties.size > 0) SymbolResolveSupport(true, properties) else null
} ?: SymbolResolveSupport(false, emptyList())
Copy link
Owner

@fwcd fwcd Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If the function does not return a boolean, its name should not be clientHas.... Though if we pass the client capabilities to KotlinWorkspaceService, I would either

  • remove this function (since we only use it in one place anyway)
  • move SymbolResolveSupport to a separate file (e.g. in org.javacs.kt.symbols) and make this an extension property on ClientCapabilities, i.e. something along the lines of 
    val ClientCapabilities.symbolResolveSupport
  get() = ...

Copy link
Author

@lorenzhawkes lorenzhawkes Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for the SymbolResolveSupport. I think technically more properties can me masked in the symbol calls in the future although the only example of this is the location property, based on the types available for return.


private fun connectLoggingBackend() {
val backend: (LogMessage) -> Unit = {
client.logMessage(MessageParams().apply {
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/kotlin/org/javacs/kt/KotlinWorkspaceService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class KotlinWorkspaceService(

@Suppress("DEPRECATION")
override fun symbol(params: WorkspaceSymbolParams): CompletableFuture<Either<List<SymbolInformation>, List<WorkspaceSymbol>>> {
val result = workspaceSymbols(params.query, sp)
val result = workspaceSymbols(!config.workspace.symbolResolveSupport.enabled, params.query, sp)

return CompletableFuture.completedFuture(Either.forRight(result))
}
Expand Down
38 changes: 25 additions & 13 deletions server/src/main/kotlin/org/javacs/kt/symbols/Symbols.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package org.javacs.kt.symbols

import com.intellij.psi.PsiElement
import org.eclipse.lsp4j.Location
import org.eclipse.lsp4j.SymbolInformation
import org.eclipse.lsp4j.SymbolKind
import org.eclipse.lsp4j.DocumentSymbol
Expand All @@ -11,9 +12,9 @@ import org.eclipse.lsp4j.WorkspaceSymbolLocation
import org.eclipse.lsp4j.jsonrpc.messages.Either
import org.javacs.kt.SourcePath
import org.javacs.kt.position.range
import org.javacs.kt.position.toURIString
import org.javacs.kt.util.containsCharactersInOrder
import org.javacs.kt.util.preOrderTraversal
import org.javacs.kt.util.toPath
import org.jetbrains.kotlin.psi.*
import org.jetbrains.kotlin.psi.psiUtil.parents

Expand All @@ -33,10 +34,10 @@ private fun doDocumentSymbols(element: PsiElement): List<DocumentSymbol> {
} ?: children
}

fun workspaceSymbols(query: String, sp: SourcePath): List<WorkspaceSymbol> =
fun workspaceSymbols(locationRequired: Boolean, query: String, sp: SourcePath): List<WorkspaceSymbol> =
Copy link
Owner

@fwcd fwcd Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: I would pass the locationRequired parameter last, to maintain the ordering of decreasing significance.

doWorkspaceSymbols(sp)
.filter { containsCharactersInOrder(it.name!!, query, false) }
.mapNotNull(::workspaceSymbol)
.mapNotNull(workspaceSymbol(locationRequired))
.toList()

private fun doWorkspaceSymbols(sp: SourcePath): Sequence<KtNamedDeclaration> =
Expand All @@ -56,10 +57,28 @@ private fun pickImportantElements(node: PsiElement, includeLocals: Boolean): KtN
else -> null
}

private fun workspaceSymbol(d: KtNamedDeclaration): WorkspaceSymbol? {
val name = d.name ?: return null
private fun workspaceSymbol(locationRequired: Boolean): (KtNamedDeclaration) -> WorkspaceSymbol? {
Copy link
Owner

@fwcd fwcd Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another style nit: Most functions in this codebase are not curried, therefore I would prefer the uncurried versions here too (even though it requires using a lambda at the call site):

Suggested change
private fun workspaceSymbol(locationRequired: Boolean): (KtNamedDeclaration) -> WorkspaceSymbol? {
private fun workspaceSymbol(declaration: KtNamedDeclaration, locationRequired: Boolean): WorkspaceSymbol? {

This would keep the naming pattern more consistent too (most singular thing-named functions also return the thing in question and not a function).

fun location(d: KtNamedDeclaration): Location? {
val content = d.containingFile?.text
val locationInContent = (d.nameIdentifier?.textRange ?: d.textRange)
return if (content != null && locationInContent != null) {
Location(d.containingFile.toURIString(), range(content, locationInContent))
} else {
null
}
}

return { d ->
d.name?.let { name ->
val location: Either<Location, WorkspaceSymbolLocation>? = if (locationRequired) {
location(d)?.let { l -> Either.forLeft(l) }
} else {
Either.forRight(WorkspaceSymbolLocation(d.containingFile.toURIString()))
}

return WorkspaceSymbol(name, symbolKind(d), Either.forRight(workspaceLocation(d)), symbolContainer(d))
location?.let { WorkspaceSymbol(name, symbolKind(d), it, symbolContainer(d)) }
}
}
}

private fun symbolKind(d: KtNamedDeclaration): SymbolKind =
Expand All @@ -73,13 +92,6 @@ private fun symbolKind(d: KtNamedDeclaration): SymbolKind =
else -> throw IllegalArgumentException("Unexpected symbol $d")
}

private fun workspaceLocation(d: KtNamedDeclaration): WorkspaceSymbolLocation {
val file = d.containingFile
val uri = file.toPath().toUri().toString()

return WorkspaceSymbolLocation(uri)
}

private fun symbolContainer(d: KtNamedDeclaration): String? =
d.parents
.filterIsInstance<KtNamedDeclaration>()
Expand Down
3 changes: 1 addition & 2 deletions shared/src/main/kotlin/org/javacs/kt/Logger.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package org.javacs.kt
import java.io.PrintWriter
import java.io.StringWriter
import java.util.*
import java.util.logging.Formatter
import java.util.logging.LogRecord
import java.util.logging.Handler
import java.util.logging.Level
Expand Down Expand Up @@ -136,7 +135,7 @@ class Logger {

fun connectStdioBackend() {
connectOutputBackend { println(it.formatted) }
connectOutputBackend { System.err.println(it.formatted) }
connectErrorBackend { System.err.println(it.formatted) }
}

private fun insertPlaceholders(msg: String, placeholders: Array<out Any?>): String {
Expand Down