azdavis / millet

A language server for Standard ML.
https://azdavis.net/posts/millet
Apache License 2.0
196 stars 12 forks source link

Publish diagnostics on DidChangeTextDocument notifications #11

Closed p7p7 closed 1 year ago

p7p7 commented 1 year ago

This PR makes the language server publish diagnostics when it receives a DidChangeTextDocument notification.

Millet already publishes diagnostics on DidChangeWatchedFiles notifications, but client-side support for workspace/didChangeWatchedFiles is optional (contrary to textDocument/didChange), and some LSP clients such as acme-lsp may not support it (yet).

azdavis commented 1 year ago

Thanks for contributing! I think this should be an optional setting, since:

I think we could add a setting, approximately like this, to the VS Code settings and document it:

Then read that in from the VS Code config and send it to the lang-srv binary upon initialization of the client-server connection.

We'd then have to read them in from the server side from the initialization_options and set up the State accordingly.

azdavis commented 1 year ago

I was actually thinking about getting around to adding support for sending vscode settings to the server to add some other options, like showing/not showing doc for tokens on hover, so I went ahead and did that. It's in main now.

I was also looking into how to get the DidChangeTextDocument notifs to be only sent if the init params ask for it, but it seems tricky. The server must declare support for DidChangeTextDocument notifs in the server capabilities, as part of the init handshake, before receiving anything from the client.

azdavis commented 1 year ago

I think maybe it could be dynamically registered, not in the init message.

p7p7 commented 1 year ago

When doing this change I assumed that, when a client would send a DidChangeTextDocument, it would always send a DidChangeWatchedFiles with it, so I thought that millet was already fast enough at producing diagnostics (since it already does when receiving the latter). After reading this, I realized that I was wrong.

As you suggested, I think adding the feature and disabling it by default is the right way, at least until we're faster at producing diagnostics.

About server capabilities, I noticed something weird: lsp_types allows advertising support for didOpen/didClose and not didChange (like millet currently does) or vice-versa, but the LSP spec states that a language server "must either implement all three of them or none". Thoughts on this?

azdavis commented 1 year ago

Not really sure 🤷 I did get this working, though, at least locally with my VS Code installation.

Re: getting faster, I'm waiting for salsa to be more ready for prime-time to attempt having Millet be smarter about not re-computing everything every time. And even when salsa is ready, it'll still be a non-trivial effort on the Millet side.