Skip to content
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

workspace/configuration is impossible to implement on the client side #972

Open
bstaletic opened this issue May 7, 2020 · 8 comments
Open

Comments

@bstaletic
Copy link
Contributor

For reference: https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#workspace_configuration

The configuration section ask for is defined by the server and doesn’t necessarily need to correspond to the configuration store used be the client.

This makes it impossible for the client to implement workspace/configuration in a generic way. It absolutely requires server-specific knowledge, which turns the "LSP is the solution to the matrix problem" mantra on its head. More specifically:

Rust-analyzer

  • section is rust-analyzer
  • Client is supposed to return server_settings[ 'rust-analyzer' ]

Gopls

  • section is gopls
  • Client is supposed to return server_settings

Lua-language-server

  • sections are Lua, files.associations and files.exclude
  • Client is supposed to return server_settings, and no clue what for the other two

 

Since a generic implementation is impossible, can we define what section actually means?

@puremourning
Copy link

To clarify the question a little: as a client implementor, when the server requests workspace/configuration and specifies a section <section>, what is the expected behaviour of the client ?

If the expected behaviour is for the client to have intrinsic knowledge of what <section> means in the context of the specific and particular language server, then the protocol is no longer client/server agnostic - you can't build a generic client with no knowledge of particular servers.

I realise that the actual configuration is server-specific (and user-supplied), that that's different. We're talking about how a generic client should handle a request from a server for configuration. The protocol seems to say that 'it's the client's responsibility to do the mapping', but mapping of what to what exactly.

So, let's assume (as the protocol does) that there is a client-side mechanism for allowing user-supplied configuration in some manner. I feel that the under-specification of this in the protocol is perhaps a mistake, but nonetheless given that it's a JSON protocol, let's assume that there must be some configuration mechanism that can specify an arbitrary JSON value. That seems not impossible.

That assumed, there are a number of places in which the client must supply configuration to the server:

  • the client-to-server initialisation request
  • the did-change-configuration notification
  • the server-to-client configuration request

The later appears to introduce a new concept 'sections'. Given the above, what is the sort of "prototypical" expectation of a client ? As Boris says, it appears that the expectation of at least 3 servers surveyed differs:

Rust-analyzer:

  • On initialisation, expects an object like configuration: { rust-analyzer: { config... } }
  • On didChangeConfiguration ???
  • For server-to-client configuration request, requests the rust-analyzer section, and expects to receive { config... }

Gopls:

  • On initialization expects an object like { config ... }
  • For server-to-client request: requests the (fictional?) gopls section and expects to receive { config... }; it also requests a bunch of other keys gopls-<something> which seem impossible for the client to know about.

Lua-Language-Server:

  • for server-to-client, requests the Lua section along with files.assocaitions and other sections which seem utterly unrelated. It's unclear how a client is expected to interpret that.

I think we'd all agree that clients should not have intrinsic knowledge of servers and vice-versa, as the whole "solving the matrix problem" requires that.

In summary, what we're asking for is clarity on what a client implementation should or must do and should or must provide in terms of user-specified configuration that allows it to implement a LSP client without having a priori knowledge of (or require a posteriori implementation for) specific language server implementations.

@dbaeumer
Copy link
Member

The implementation I use inside VS Code can be found here: https://github.com/microsoft/vscode-languageserver-node/blob/master/client/src/configuration.ts#L46

This is a generic implementation that simply treats every section as an entry in the configuration store. Since the VS Code store is JSON as well converting the properties is fairly simple.

However I also know that for some settings a generic mapping is not enough especially if a server needs to read settings it is not necessarily owning. In the VS Code libs I therefore allow clients to plugin into the settings reading using a middleware layer.

I do agree that this might require to ship some specific code or configuration per [IDE, LS] tuple but it is far away from the n x m matrix that we had before where each language features needed to be implemented for every IDE.

Some additional words to configuration. There are two model:

  • PUSH: which sends the settings in the initialize request and on didChangeConfiguration. This model is basically deprecated
  • PULL: nothing is sent during initialize. didChangeConfiguration fires when the configuration changes. In the simplest implementation it can have no payload indicating that all settings changed. Settings are always fetched using workspace/configuration request.

If anyone has an idea how to better spec section I am fully open for recommendations. What I do want to avoid is to spec a settings model inside LSP since this is something every client has its own view on :-).

@bstaletic
Copy link
Contributor Author

@dbaeumer Thanks for replying.

This is a generic implementation that simply treats every section as an entry in the configuration store.

Alright, I see the problem now. The same, generic implementation, in our client would work only for rust-anlyzer. For gopls, we don't store the gopls key, only the value of it, as that is what the server expects in, say, the initializationOptions. For the lua server it's even worse, since files.* sections are vscode specific.

I do agree that this might require to ship some specific code or configuration per [IDE, LS] tuple but it is far away from the n x m matrix

I have to disagree here. As long as sections are "arbitrary" in the specs and server implementers tailor them exactly to vscode, there are exactly two options:

  • Be vscode, or implement the exact vscode configuration store.
  • Poke each and every server and figure out what it needs.

I'm sure you can see how neither of the two are doable.

Some additional words to configuration. There are two model:

  • PUSH: which sends the settings in the initialize request and on didChangeConfiguration. This model is basically deprecated

This model was perfectly implementable by clients and >80% of servers still rely on it, thankfully. The deprecation, if I'm not mistaken, is not mentioned in the specs.

What I do want to avoid is to spec a settings model inside LSP

That sounds reasonable. Rigid config is not necessary for client-side implementability.

If anyone has an idea how to better spec section I am fully open for recommendations.

What rust-analyzer is doing is conforming and implementable, so let's go with that.

On initialize request

{
  "rust-analyzer": {
    "foo": 123,
    "bar": [1]
  }
}

The foo and bar keys are there just to illustrate my point. The above JSON object is sent as initializationOptions.

This is the server_settings object that our client knows about.

Config update

First, the client sends an empty didChangeConfiguration notification. Rust-analyzer ignores everything inside, even if the notification were not empty.

Then RA sends the workspace/configuration request, with {"section":"rust-analyzer"}. The client responds with:

{
    "foo": 123,
    "bar": [1]
}

The client has to do server_setting[ section ].

Note that I'm not saying that sections should only be those keys that were in the initialize request.

What I am saying is that sections should only be keys that could have been in the initialize request.

What gopls does differently?

Initialization

{
  "foo": 123,
  "bar": [1]
}

Again, the key are for illustrative purposes. Again, this object is sent as initializationOptions and is the one the client knows about. However, in vscode, this object is inside the gopls section.

Configuration update

The client sends a didChangeConfiguration notification with {"section":"gopls"}, expecting

{
  "foo":123,
  "bar": [1]
}

However, if the client tries to do server_settings[ section ] the result would be None. Instead, the client is expected to just return the entire server_settings.

What the lua server does differently?

They go one step beyond what gopls does and request files.associations which is a complete mystery for anything but vscode.

 


To restate, the simplest way to make the "pull" model implementable on the non-vscode-client side would be to require that sections can only be all the possible keys that could be found in the initializationOptions. Not some arbitrary vscode config, be it the parent section of the initializationOptions or not.

@h-michael
Copy link
Contributor

I have additional opinions.

Currently in the specification, "section" is defined as a string type, and there is only a comment "The configuration section asked for."
Clients other than VSCode probably implement the implementation of VSCode by dividing the string of "section" by ".". I want the specification to show how to handle the section value.

For example, ESlint language server sends an empty string as section.
https://github.com/microsoft/vscode-eslint/blob/master/server/src/eslintServer.ts#L668

@krobelus
Copy link

krobelus commented Sep 3, 2021

I worked around this problem by changing the structure of server settings in the kak-lsp LSP client, see kakoune-lsp/kakoune-lsp#511. Previously, kak-lsp users could simply set an arbitrary object for initializationOptions / didChangeConfiguration. Now we wrap this object in a "section".

So instead of

{
  "go": {
    "command": "/path/to/gopls",
    "settings": {
      "build.buildFlags": []
    }
  },
  "rust": { "command": "rls" }
}

kak-lsp users need to write

{
  "go": {
    "command": "/path/to/gopls",
    "settings_section": "gopls",
    "settings": {
      "gopls": {
        "build.buildFlags": []
      }
    }
  },
  "rust": { "command": "rls" }
}

which is unfortunate, but it's simplest way to support all language servers.

The value of settings_section tells kak-lsp to send the contents of the "gopls" section as initializationOptions / didChangeConfiguration.

To restate, the simplest way to make the "pull" model implementable on the non-vscode-client side would be to require that sections can only be all the possible keys that could be found in the initializationOptions. Not some arbitrary vscode config, be it the parent section of the initializationOptions or not.

I very much agree with that.
For my example, this would mean that the language server may request sections like build.buildFlags but never gopls.

This would work nicely for servers like gopls that group all their settings in a JSON object.
Unfortunately, there are some language servers like the Julia one that may want a settings.json like this:

{
  "julia.format.indent": 4,
  "julia.lint.call": true,
  "some unrelated config": 123
}

where we can't send correct initializationOptions generically; we don't know which keys the server accepts and can't just send the entire settings object because that would also send "some unrelated config".
However, in my experience this won't matter because those servers ignore they payloads of initializationOptions / didChangeConfiguration and instead just request workspace/configuration.

So my suggested solution would be add this small contract to the LSP spec:

  • If a language server uses the settings that are sent by initializationOptions / didChangeConfiguration, then:
    • whenever the server requests a section with workspace/configuration, it must also accept that section if it were sent via either of initializationOptions or didChangeConfiguration.

The nice part is that this change does not affect servers who only ever use settings from workspace/configuration (but not initializationOptions / didChangeConfiguration).

It seems fine to make workspace/configuration the standard and deprecate others, but we should use a solution like this if it can ease the transition period.

This would require changes to language servers like gopls, to stop requesting the gopls section.
The Go VSCode plugin should take care of choosing the gopls section from settings.json as target for the requests.

What I do want to avoid is to spec a settings model inside LSP since this is something every client has its own view on :-).

yeah, but it is only reasonable to allow creating a single object that contains all of a server's settings, independently of which requests the server supports

Of course an alternative solution would be to just force deprecation of initializationOptions and didChangeConfiguration, then there would be no more incompability with workspace/configuration.

@bstaletic
Copy link
Contributor Author

Previously, kak-lsp users could simply set an arbitrary object for initializationOptions / didChangeConfiguration. Now we wrap this object in a "section".

That's not a bad API, but isn't backwards compatible. Our starting point was the same - users could pass anything through initializationOptions. Then, there are two possibilities:

  1. A language server enjoys extra support from our client (clangd, gopls, rust-analyzer) and their wishes for sections are handled on a case-by-case basis.
  2. A language server is provided by the user and there's zero server-specific code - only user conf. This is ugly. We added two other config fields - capabilities and config_sections. Former can override our client's capabilities and users have to opt-in to workspace/configuration. Latter makes the user responsible for catering to server's wishes regarding sections.
    2.1. Note that we have also kept ls field, where the users where the users are allowed to provide initializationOptions.

I had warned you this was ugly. I just couldn't think of a better way that is backwards compatible and allows for potential catering to server's wishes.

If it weren't for rust-analyzer forcing us to support workspace/configuration, I'm pretty sure we never would have.

deprecation of initializationOptions and didChangeConfiguration, then there would be no more incompability with workspace/configuration.

Can you really deprecate/remove things in LSP? No protocol version is negotiated, so you have no idea what version of specification a server (or client) has been written against. More or less, this makes you remain both forward and backward compatible. For example, our client is "pushing" config, yet can claim workspace/configuration capability and respect "pull" config model. Another example is the textDocument/hover response. I just don't know which server will a user try to combine with my client.

then there would be no more incompability with workspace/configuration

That still doesn't help with underspecification. What is files.exclude in lua server's case?

@krobelus
Copy link

krobelus commented Sep 4, 2021

Right, for compatibility we'd need a different name, say workspace/configuration2 or workspace/settings.

If it weren't for rust-analyzer forcing us to support workspace/configuration, I'm pretty sure we never would have.

Same, we mainly added it for Lua/Julia/gopls. It would be nice if all servers implemented the old configuration methods because they don't have this problem, and they are enough for most use cases.

@astoff
Copy link

astoff commented Mar 8, 2023

The solution to the n x m matrix problem in this case would be a section of the initialization message where the server announces its configuration schema (option names, types and descriptions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants