The "workspace/didChangeConfiguration" handler ignores message params

[tmillican]

When a client sends a workspace/didChangeConfiguration notification to the server, the server discards the message parameters entirely.

The LSP specification defines the type of the params property for this notification as:

interface didChangeConfigurationParams {
	/**
	 * The actual changed settings
	 */
	settings: LSPAny
}

LSPAny leaves the door open for a server to handle this data arbitrarily, so I suppose it isn't a bug per se to handle it by discarding the settings . Nevertheless, the comment creates an expectation that the client can send the altered settings in the settings property, and the server will use them to update its configuration.

As near as I can tell, this is how most LSP servers behave, and what most LSP clients are expecting. For instance, this is the behavior of both neovim's built-in LSP client and the ALE plugin for (neo)vim.

Currently, unless --configPath is specified (in which case the notification is ignored), the handler for this notification patches the server configuration from these sources, in this order:

1. The folder-scoped configuration(s), pulled from the client via a workspace.configuration request
2. The .luarc.json(c) file in each folder
3. The global / unscoped configuration (scope.fallback in the source) pulled from the client via a workspace.configuration request

Notably, 1 and 3 only occur if the client advertised the workspace.configuration capability. If it did not, lua-language-server becomes completely unconfigurable via LSP alone. This is specifically a problem for ALE per this issue.

I would propose that between steps 2 and 3, the server should patch scope.fallback with any settings provided in params.settings, using the same sections polled by the workspace.configuration message. In other words, identical to what is returned from loadClientConfig() in the config loader. Ex:

{
  "method": "workspace/didChangeConfiguration",
  "jsonrpc": "2.0",
  "params": {
    "settings": {
      "Lua": {
        "diagnostics": {
          "enable": true,
          "globals": ["vim"]
        }
        // etc.
      },
      "files.associations" = { /* ... */ },
      "editor.semanticHighlighting.enabled" = true
    }
  }
}

This is consistent with how neovim and ALE -- and probably others -- already try to use the notification, and it won't affect anything for existing clients that don't send params.settings or that send then in some other unexpected format (their params.settings were ignored before and will continue to be ignored after).

I've made a fork with this proposed workflow here, and I'd be happy to make a PR if this proposal sounds reasonable.

[tomlau10]

(off topic)

Hi, first of all thanks for taking time trying to contribute to luals 😄 .

I don't have comments on how this protocol should be handled, however by looking at the commit of your fork: tmillican@e1cbd18, seems you have used some formatter on the whole file?

If you're going to open a PR, you should NOT format the whole file, because that will create too much unnecessary changes, and no one can easily see the actual changed lines. Moreover it will heavily affect git blame in the future. This have been explained by collaborator before: #2834 (comment). You may still apply formatter on the actual lines that you have modified if you like, but just don't do it on a whole file base.

Thanks again for your interest in contributing to luals. 😃

[tmillican]

Ah, sorry about that. I didn't notice that had happened. I was popping between a couple different neovim configs to test my changes, and must have accidentally touched it with my main config that uses stylua. I'll get that sorted.

Edit: it's fixed now