Fix compatibility issues + bugs #556
Conversation
1) From LSP spec 3.16 to 3.17, dropping the location attribute from the response to `workspace/symbol` is only permitted if the client capability `workspace.symbol.resolveSupport` is advertized. Without this setting, the `location` attribute should be present. 2) `findDeclarationCursorSite` constructs a location using a non-uri path in the uri property. This leads to document symbol rename for example to not work. 3) THe stdio backend for logging accidentally overrides the stdout backend with the one meant for stderr.
Not all named declarations have a name identifier. In those cases fallback to using the range that encloses the whole named declaration rather than returning nothing.
lorenzhawkes
force-pushed
the
fix-some-compatibility-issues
branch
from
ec4e302
to
1abe6bf
Compare
| @@ -108,6 +108,8 @@ class KotlinLanguageServer( | |||
| serverCapabilities.renameProvider = Either.forRight(RenameOptions(false)) | |||
| } | |||
|
|
|||
| config.workspace.symbolResolveSupport = clientHasWorkspaceSymbolResolveSupport(clientCapabilities) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
no problem, I change it to pass the client capabilities into the KotlinWorkspaceService
| private fun clientHasWorkspaceSymbolResolveSupport(clientCapabilities: ClientCapabilities) = | ||
| clientCapabilities?.workspace?.symbol?.resolveSupport?.properties?.let { properties -> | ||
| if (properties.size > 0) SymbolResolveSupport(true, properties) else null | ||
| } ?: SymbolResolveSupport(false, emptyList()) |
There was a problem hiding this comment.
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
SymbolResolveSupportto a separate file (e.g. inorg.javacs.kt.symbols) and make this an extension property onClientCapabilities, i.e. something along the lines ofval ClientCapabilities.symbolResolveSupport get() = ...
There was a problem hiding this comment.
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.
| @@ -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> = | |||
There was a problem hiding this comment.
Style nit: I would pass the locationRequired parameter last, to maintain the ordering of decreasing significance.
| @@ -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? { | |||
There was a problem hiding this comment.
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):
| 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).
1) pass client capabilities into KotlinWorkspaceService 2) Moved SymbolResolveSupport to a new file and added extension property to ClientCapabilities for it 3) Added a test for the location fetching required by the symbol resolve client capability
response to
workspace/symbolis only permitted if the clientcapability
workspace.symbol.resolveSupportis advertized. Withoutthis setting, the
locationattribute should be present.findDeclarationCursorSiteconstructs a location using a non-uripath in the uri property. This leads to document symbol rename
for example to not work.
backend with the one meant for stderr.