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

Clearing existing diagnostics when a watched configuration file is modified #1594

Open
vinistock opened this issue Dec 13, 2024 · 3 comments
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@vinistock
Copy link
Contributor

We implemented the following scenario in the Ruby LSP (PR):

  1. A user is modifying files. Diagnostics are served using pull diagnostics (textDocument/diagnostic)
  2. The user decides to change the configurations for the linter. This is done by editing .rubocop.yml
  3. We then reload the linter with the new configurations and clear the existing diagnostics, since they may no longer be valid

The flow of clearing the diagnostics is simply:

  1. The server receives a workspace/didChangeWatchedFiles notification including .rubocop.yml
  2. We fire textDocument/publishDiagnostics notifications with empty arrays to clear the diagnostics on all currently opened files

However, this does not seem to work. We use the same technique to clear diagnostics on textDocument/didClose and that works properly, but trying to clear the diagnostics when a watched file notification comes in doesn't do anything.

I tried debugging the code for vscode-languageclient to see if our notifications were being received and processed and everything seemed fine, but the diagnostics still won't go away.

Are we doing something incorrectly? Or is this a bug somewhere?

@dbaeumer
Copy link
Member

That should work that way. I use the same for ESLint

@dbaeumer dbaeumer added the bug Issue identified by VS Code Team member as probable bug label Dec 16, 2024
@dbaeumer dbaeumer added this to the Next milestone Dec 16, 2024
@vinistock
Copy link
Contributor Author

The fact that the client doesn't include the configuration file's extension as part of its document selector shouldn't matter, right? That is, the configuration file is YAML, but the client doesn't include YAML in the selector.

Do you have any tips for trying to debug this? It's easy for me to reproduce, so if you can point me to where the clearing of the diagnostics happen I can check what's going on.

@dbaeumer
Copy link
Member

dbaeumer commented Jan 7, 2025

The fact that the client doesn't include the configuration file's extension as part of its document selector shouldn't matter, right? That is, the configuration file is YAML, but the client doesn't include YAML in the selector.

That is correct.

Do you have any tips for trying to debug this? It's easy for me to reproduce, so if you can point me to where the clearing of the diagnostics happen I can check what's going on.

You could debug the code here: https://github.com/microsoft/vscode-languageserver-node/blob/dbaeumer/lexical-skunk-aquamarine/client/src/common/client.ts#L1711

This is where the published diagnostics are handled and then passed to the VS Code API here: https://github.com/microsoft/vscode-languageserver-node/blob/dbaeumer/lexical-skunk-aquamarine/client/src/common/client.ts#L1743

Let me know how it goes.

vinistock added a commit to Shopify/ruby-lsp that referenced this issue Jan 7, 2025
### Motivation

Closes #1457

This was originally attempted in #1921 and #2510. This PR moves watching `.rubocop.yml` to the server, so that we can avoid a full server restart and instead just create a new instance of the RuboCop runner.

This PR still suffers from the same issue reported in both previous attempts: trying to clear existing diagnostics after changing `.rubocop.yml` simply does not work. I believe this must be some sort of bug in the client or some assumption we're not satisfying. Clearing diagnostics works normally when closing files on `textDocument/didClose` notifications, so I see no reason why it wouldn't work while processing a did change watched files notification.

Despite the bug, since we're trying to improve stability of the LSP, I think it's still worth moving forward. Launching the server is still the most critical part in our entire life cycle and reducing the amount of times we restart helps prevent unnecessary pain for developers. Additionally, simply editing the file with the stale diagnostics already makes them go away immediately, so the bug is not that terrible.

I created an issue to discuss the bug with the maintainers of the protocol microsoft/vscode-languageserver-node#1594.

### Implementation

Stopped restarting the server on `.rubocop.yml` and `.rubocop` modifications. Instead, we watch the files in the server and simply re-create the instance of the runner.

Note that this has the added benefit of working on any editor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

2 participants