haskell / lsp

Haskell library for the Microsoft Language Server Protocol
360 stars 89 forks source link

Support `workspace/configuration` request for every server initilize #510

Closed July541 closed 10 months ago

July541 commented 11 months ago

In the latest release of vscode-language-server-node, they don't send workspace/didChangeConfiguration more when the server is initialized, which causes https://github.com/haskell/vscode-haskell/issues/920.

Following the comment of the SynchronizeOptions below, they suggest using workspace/configuration, so I'm thinking to extend ServerDefinition with a new field to send workspace/configuration while the server is initiated.

https://github.com/haskell/lsp/blob/97c76604bd5cb77dc3c683a2c4cdee0282e3e9c1/lsp/src/Language/LSP/Server/Core.hs#L280

https://github.com/microsoft/vscode-languageserver-node/blob/f2ff7d55464a1f58f978cb6635bd8865f050553c/client/src/common/configuration.ts#L132

michaelpj commented 11 months ago

This is actually pretty bad. We can't send workspace/configuration requests during initialize. The "solution" seems to be "use dynamic registration for everything so you can request the configuration"?? This is very much not the model that we use, and probably requires significant re-architecting.

Possibly we can get away with doing this work when we receive the initialized notification from the client, but I'm really not sure.

I have read several of the issues about this and honestly I'm very confused about how this is supposed to work :(

michaelpj commented 11 months ago

Okay, so I think we could try and do something like what is described here as what rust-analyzer does: https://github.com/microsoft/language-server-protocol/issues/972#issuecomment-626668243

Namely:

July541 commented 11 months ago

Okay, so I think we could try and do something like what is described here as what rust-analyzer does

That is what I want.

I think the only problem is I'm not sure if we can send workspace/configuration after initialized. I'll try this several days later if you don't have time:)

Night is long, let me try it now:)

michaelpj commented 11 months ago

You're explicitly allowed to send workspace/configuration in the handler for initialized: https://github.com/microsoft/language-server-protocol/issues/567#issuecomment-953772465

The only thing is that I guess the client might start sending you requests during that time, so you probably have to be responsive. But we do already have the facility to change the config dynamically, so it shouldn't be too bad.

michaelpj commented 11 months ago

It just means that e.g. if you somehow hit a formatting request immediately after initialization you might get the default behaviour. But that probably just means that the default behaviour should be "no formatting", I guess.

michaelpj commented 11 months ago

Sounds like indeed the answer is "tough, accept it": https://github.com/microsoft/language-server-protocol/issues/1006#issue-627241635

July541 commented 11 months ago

BTW, I'm trying to find a way to ensure the client will send workspace/didChangConfiguration after restarting.

michaelpj commented 11 months ago

I think we can't count on that, though, so we probably need to handle the other case anyway.

July541 commented 11 months ago

After reading some lsp code I found that patching handlers for SMethod_Initialized seems very hard in lsp side.

Is it possible to add a field about workspace/configuration handler, and then run it after SMethod_Initialized handler?

michaelpj commented 11 months ago

So, we already have this callback that should give us all the information we need to process config: https://github.com/haskell/lsp/blob/97c76604bd5cb77dc3c683a2c4cdee0282e3e9c1/lsp/src/Language/LSP/Server/Core.hs#L284

I think the only thing we need to be told additionally is the "section".

I agree it's a little tricky in lsp: the idea of having multiple handlers for things and then gluing them together is a HLS thing. So maybe we just want to provide people with helper functions to call in doInitialize and an Initialized handler. But I'm not really sure. It would be very nice if it just magically worked...

July541 commented 11 months ago

So, we already have this callback that should give us all the information we need to process config:

Seems not this, onConfigurationChange is for handling the didChangeConfiguration notification. I think we need a function to send workspace/configuration request.

Left this up and see if https://github.com/haskell/haskell-language-server/pull/3745 works firstly...

michaelpj commented 11 months ago

Seems not this, onConfigurationChange is for handling the didChangeConfiguration notification

I mean, that tells us what to do when we get a new config. Actually implementing it we can probably pull out a part of handleConfigChange.

Sarrus1 commented 5 months ago

Sounds like indeed the answer is "tough, accept it": https://github.com/microsoft/language-server-protocol/issues/1006#issue-627241635

Hello, I have also struggled with this issue on my LSP implementation. My workaround was to:

This is hacky, but it seems better than updating the config after we received some requests. I reasoned that if the server has not received the config yet, it is not properly initialized.

I see that this is closed, but maybe this can help someone with the same issue.

michaelpj commented 5 months ago

I think we now handle this pretty well, we send workspace/configuration during the handling of initialized. We also generally handle notification sequentially and don't process anything else concurrently with them, since they often update state, so we basically don't do anything until we're done getting the initial configuration.