Handle LSP Initialize request synchronously

[pepeiborra]

The LSP spec. has strong restrictions around what messages are allowed
in between the Initialize request and response. Handling the message
synchronously goes a long way towards satisfying those invariants.

[wz1000]

This is dead code - handleInit is the actual initialize request handler. lsp will never actually call a InitializeRequest handler defined the regular way since initialize is special. Instead it defines its own handler, into which custom logic can be inserted using doInitialize

[pepeiborra]

In that case it sounds like InitializeRequest is already being handled synchronously, is that right?

[wz1000]

Yes.

I've also been confused by the out of order progress messages and something non-obvious is going on. It might even be a bug in lsp-test.

I did try to investigate the ghcide code path and I couldn't figure out how it was possible for us to send progress messages before the initialize response.

[pepeiborra]

The second part seems easy to explain: we call shakeOpen inside the doInitialize handler, shakeOpen will evaluate the rules and call kick, and kick may trigger progress reporting.

[wz1000]

Right, I thought shakeOpen was only initializing state.

[ndmitchell]

Will this cause progress begin/end messages to be generated, but they won't make it back to the client until after the initialise response returns? If they arrive later, what will happen? What about messages which are allowed during initialise, like logging - are they going to be delayed too?

[pepeiborra]

Will this cause progress begin/end messages to be generated, but they won't make it back to the client until after the initialise response returns? If they arrive later, what will happen? What about messages which are allowed during initialise, like logging - are they going to be delayed too?

Turns out this change won't make any difference, since the Initialize request is already being handled synchronously.

It's not obvious to me how to block progress reporting during initialise, since we call shakeOpen inside the initialise handler. I think this might require a change in the lsp package to allow doInitialize to return an IO action to run after sending the response. @wz1000 what do you think?

[ndmitchell]

We already have a flag to say we want testing messages. How about making that a mode. ProgressAlways (testing), ProgressSometimes (normal), ProgressNever (init).

[wz1000]

doInitialize to return an IO action to run after sending the response

Maybe that could work, but I'm not sure how all the data dependencies will work out. We already use mdo in both shakeOpen and lsps initialize handler, and there is that trick with unsafeInterleaveIO readMVar in handleInit, so it is already a bit of a circular tangled mess.

We need the result of doInitialize to construct the monadic environment all the other handlers will be running in.

The only way to find out is by prototyping the change both in lsp and ghcide.

[wz1000]

Usually we would use the initialized notification as a trigger to execute actions after initialization, but I guess that can't work right now because we need the result of shakeOpen for the environment the notification handler will be running in.

[pepeiborra]

doInitialize to return an IO action to run after sending the response

Maybe that could work, but I'm not sure how all the data dependencies will work out. We already use mdo in both shakeOpen and lsps initialize handler, and there is that trick with unsafeInterleaveIO readMVar in handleInit, so it is already a bit of a circular tangled mess.

We need the result of doInitialize to construct the monadic environment all the other handlers will be running in.

The only way to find out is by prototyping the change both in lsp and ghcide.

The easiest thing would be to change doInitialize to just send the response immediately and then evaluate the handler.

[ndmitchell]

My understanding is the flow is initialize request/response, where we are super limited. Then we get initialized. Why not move all our initialisation to initialized, and just stick with the minimum reporting of capabilities back? Then we wouldn't call the kick or anything.

Even if we did do a shakeOpen, I don't see why we need to do a shakeRun inside it, but I'm super unclear about the flow there.

[pepeiborra]

Right, we could move some of the initialization to the Initialized notification handler.

shakeOpen sets up the ShakeExtras environment which includes the ShakeSession, but we could delay creating the session until the first LSP file event.