Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish diagnostics on DidChangeTextDocument notifications #11

Closed
wants to merge 1 commit into from

Conversation

p7p7
Copy link

@p7p7 p7p7 commented Sep 3, 2022

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
Copy link
Owner

azdavis commented Sep 3, 2022

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

  • Some users may not want diagnostics to be re-calculated between typing new code and saving
  • Millet is not especially efficient with re-calculating diagnostics, so doing it more often may be expensive

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

  • Name: millet.server.diagnostics.onChange
  • Type: boolean
  • Default: false
  • Description: Whether to send diagnostics when files change without saving.

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
Copy link
Owner

azdavis commented Sep 4, 2022

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
Copy link
Owner

azdavis commented Sep 4, 2022

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

@p7p7
Copy link
Author

p7p7 commented Sep 4, 2022

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 azdavis closed this in 606e04d Sep 4, 2022
@azdavis
Copy link
Owner

azdavis commented Sep 4, 2022

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.

azdavis added a commit that referenced this pull request Sep 4, 2022
Co-authored-by: =?UTF-8?q?L=C3=A9o=20Larnack?= <[email protected]>

Close #11
azdavis added a commit that referenced this pull request Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants