Skip to content

Add support to specify server name and initialization options in server command #1116

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

Merged
merged 7 commits into from
Dec 9, 2020

Conversation

martskins
Copy link
Collaborator

@martskins martskins commented Oct 25, 2020

This is a draft PR with a proposal of a way of addressing an issue we have with how we handle the initializationOptions. Currently, the value we send on initializationOptions on the initialize request is the value for the key initializationOptions on the settings file, but that doesn't seem correct to me for two reasons:
- it assumes that it's ok to send the same initialization options to two different servers (if you are working on a multi-language project with two different servers)
- it makes things difficult for the workspace/configuration implementation, as the servers (at least rust-analyzer, gopls and pyls) seem to assume that the root section is a section with the name of the server. So for example if a server wants the configuration for rust-analyzer.inlayHints.chainingHints it would fail to get that now as there is no section named rust-analyzer (as we use a section named initializationOptions instead).

Fixing this issue is a little tricky, because we don't know the name of the server prior to issuing the initialize request, so we wouldn't know what section to get from the settings file. Other clients don't have this problem because they either are a client for a specific server (for example rust-analyzer's VSCode extension, it knows it should send the section rust-analyzer in the settings file), or because they implement some other way to populate the initialization options (vim-lsc uses a pre-request hook for that).

Fixing this will also enable users to use a single global settings file, as each server's settings will be under it's own section. For example, users will be able to have a settings.json file like this one:

{
    "gopls": {
    },
    "rust-analyzer": {
    }
}

The above is currently impossible to configure, as we don't differentiate settings for different servers.

To fix this breaking as little as possible, I propose that we add a new format for specifying the serverCommands. So the idea is to accept what we accept now (a list of strings with the command and arguments) or an object with a commands key which is the same list of strings we accept now and an optional server_name key in that object which will be used to identify the root section for that language server. For example something like this would be a valid config for serverCommands:

let g:LanguageClient_serverComands = {
    \ "rust": {
    \   "command": ["/path/to/rust-analyzer"],
    \   "name": "rust-analyzer",
    \   "initializationOptions": { "inlayHints": { "enable": v:true } },
    \  },
    \ "go": ["gopls"]
    \ }

To avoid breaking too many things, in the case where the user specifies the command in a list of strings like we do now, I attempted to make a best effort guess of the name of the server by assuming the first item in the list is a binary (or a path to one) with the name of the server (/path/to/rust-analyzer would yield rust-analyzer as server name). This is a little nasty, but to remove this I think we would have to introduce a breaking change in the configuration (maybe that's better?).

Note that the server command now also includes an optional initializationOptions key, that acts as global settings for that specific server and will be merged with the default server settings we define in code (java only) and with the workspace settings defined by the user. The order in which they are merged are default options -> server options -> workspace options, so the workspace local options will be preferred over the server ones and the server ones over the default ones.

Also, to aid existing users in identifying issues with their current configuration, an error message will be shown to the user when we detect a settings file with an initializationOptions key, the message will direct them to see the help for serverCommands which will indicate the proper configuration.


I considered a few other alternatives to how we specify the server commands. There was one that I was very tempted to implement, which was basically to have servers keyed by their server name instead of the filetype they apply to.

let g:LanguageClient_servers = {}
let g:LanguageClient_servers.gopls = {
    \ 'command': ['gopls'],
    \ 'filetypes': ['go', 'gomod'],
    \ }

This would enable running more than one server for the same filetype, which maybe is something we want to do in the future? I didn't go through with it because it seemed like it would introduce a whole new set of problems to solve that I wasn't planning to address with this PR, but maybe it's something that we can look into after this one.

Related: #1090
Closes #1107

@martskins martskins requested a review from autozimu October 25, 2020 19:42
@martskins martskins force-pushed the fix-initialization-options branch 3 times, most recently from a94e9f5 to aeefef9 Compare October 25, 2020 19:53
@martskins martskins force-pushed the fix-initialization-options branch from aeefef9 to 7551525 Compare November 19, 2020 18:11
@martskins martskins changed the title Add support to specifiy server name to send the correct initializationOptions Add support to specify server name and initialization options in server command Nov 19, 2020
@martskins martskins force-pushed the fix-initialization-options branch 3 times, most recently from 68d590e to 0282b22 Compare November 26, 2020 18:11
@martskins martskins force-pushed the fix-initialization-options branch from 0282b22 to 585a956 Compare December 6, 2020 03:46
@martskins martskins force-pushed the fix-initialization-options branch 2 times, most recently from 27fa471 to f5d29af Compare December 6, 2020 04:20
@martskins martskins force-pushed the fix-initialization-options branch from f5d29af to 29a8e68 Compare December 6, 2020 04:22
@martskins martskins marked this pull request as ready for review December 6, 2020 04:27
@martskins martskins force-pushed the fix-initialization-options branch from c100216 to 652d5c2 Compare December 6, 2020 05:13
@martskins martskins force-pushed the fix-initialization-options branch from fb189e3 to 7ad8e1d Compare December 8, 2020 21:55
@martskins martskins force-pushed the fix-initialization-options branch from 7ad8e1d to 81ec338 Compare December 8, 2020 21:57
@martskins martskins merged commit 8010205 into autozimu:dev Dec 9, 2020
@martskins martskins deleted the fix-initialization-options branch December 9, 2020 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant