search

haskell / lsp

Haskell library for the Microsoft Language Server Protocol
364 stars 90 forks source link

Changing the configuration after `workspace/didChangeConfiguration` requests #414

Closed pgujjula closed 1 year ago

pgujjula commented 2 years ago

I notice that lsp now has a function getConfig to get the client configuration "as set via the initialize and workspace/didChangeConfiguration requests." I am a little confused how the configuration is supposed to be updated, however.

Our language server has an implementation of onConfigurationChange that parses the changed configuration. When we try changing the configuration, the lsp library complains with an error lsp:no handler for: SWorkspaceDidChangeConfiguration. I assume this means that we should also provide an implemention for the workspace/didChangeConfiguration notification. However, it's unclear to me what this handler should be doing, since it seems all the relevant work is already done by the onConfigurationChange function.

Therefore, my question is, what functionality should the onConfigurationChange implementation provide, what functionality should the workspace/didChangeConfiguration handler provide, and what is the difference between them?

pgujjula commented 2 years ago

I'd like to bump this question, @michaelpj perhaps?

michaelpj commented 2 years ago

What we currently do with onConfiguraitonChange is:

  1. If the intialize request contains initializationOptions, we call it on that (this is somewhat questionable but some clients expect it).
  2. We put it in the LanguageContextEnv.

So at the moment you need to add your own didChangeConfiguration notification handler, which may well just pull onConfigurationChange from the LanguageContextEnv and call it. I think the reason we don't install a handler by default is that a server may need to do other things in the didChangeConfiguration handler, e.g. invalidating state. So it's something we can't write for people.

pgujjula commented 2 years ago

Thanks for the information! I'll try implementing our configuration management using this info and get back to you if I have any more questions.

pgujjula commented 2 years ago

Ok, I've come across a problem with this setup. When the configuration is changed in VSCode, the client sends a workspace/didChangeConfiguration notification with a null body (see https://github.com/microsoft/vscode-languageserver-node/issues/380#issuecomment-414691493), and expects the server to re-read the configuration from the client.

When the server receives a workspace/didChangeConfiguration message, it seems that it calls the parser specified in onConfigurationChange to parse the config, updates the stored config, and also calls the user-specified handler for SWorkspaceDidChangeConfiguration.

The problem is that in this flow of logic, there is no way to ensure that the config is actually updated properly, if the client is sending a null notification. The onConfigurationChange parser is pure, so it when it's passed a null notification to parse, it can't request the actual config from the client. The user-specified handler for SWorkspaceDidChangeConfiguration can request the actual config, and it can call onConfigurationChange to parse the config, but it can't actually update the config in the server because there's no function in the lsp API to do so.

I think the easiest way to solve this issue would be to add a setConfig function to the lsp API, analogous to the getConfig function that is already present. The setConfig function would be called by the user-specified handler for SWorkspaceDidChangeConfiguration notifications. I can make a PR for this if this plan sounds good to you @michaelpj

michaelpj commented 2 years ago

Ah indeed I was wrong, we do handle this by default.

Having setConfig would be fine. I think this is a further case for unbundling the server implementation so it's easier to this stuff yourself...

pgujjula commented 2 years ago

I created the PR here: https://github.com/haskell/lsp/pull/418

pgujjula commented 2 years ago

Merging https://github.com/haskell/lsp/pull/418 solved this problem, so if there's no objections in a couple of days, I'll close this issue.

michaelpj commented 1 year ago

We now automatically call workspace/configuration when we get a didChangeConfiguration notification, so you should be able to remove your custom logic here. It's still annoying that you have to register an empty didChangeConfiguration handler. I have some thoughts about that: https://github.com/haskell/lsp/issues/520