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.

Sign up for GitHub

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

Jump to bottom

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

Closed
vinistock opened this issue Dec 13, 2024 · 7 comments
Closed

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

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

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?
@vinistock vinistock mentioned this issue Dec 13, 2024
Merged
@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
@vinistock
Move RuboCop file watching to the server (#2982)
516cff3 
### 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.
@vinistock
Copy link
Contributor Author

I tried debugging in the spots you indicated. Here's a video of what I'm seeing happening. You can see that the diagnostics are an empty array in both the places you mentioned, but it still won't go away.

Some other things I tried verifying:

  1. The URI matches the document that I'm trying to clear diagnostics for
  2. The server does not receive a textDocument/diagnostic (pull diagnostics) request after trying to clear via publish diagnostics. I thought maybe clearing was working, but that then we received a pull request and returned stale information. That doesn't seem to be the case

Any other things I can try debugging to understand what's going on?

diagnostics_not_cleared.mov

@dbaeumer
Copy link
Member

I think I know what is going on: you report the diagnostics via pull right which I overread in the description. You can't clear diagnostics reported by pull with a publish since they are distinct collections (unless you explicitly name them the same). Instead when recognizing the configuration change on the server side you should ask for a refresh of the pull diagnostics using workspace/diagnostic/refresh which is a message sent from the server to the client.

Hope that helps.

@vinistock vinistock mentioned this issue Jan 10, 2025
Merged
@vinistock
Copy link
Contributor Author

Thank you for the explanation! That did it Shopify/ruby-lsp#3041.

The solution was indeed to request a diagnostic refresh and clearing our own cache, so that the new wave of pull diagnostics were all using the new configs. Appreciate the help!

@dbaeumer
Copy link
Member

Happy to hear you got it working.

vinistock added a commit to Shopify/ruby-lsp that referenced this issue Jan 16, 2025
@vinistock
Request diagnostic refresh when RuboCop configs change (#3041)
dcb4bbe 
### Motivation

The explanation in microsoft/vscode-languageserver-node#1594 (comment) solved our issue clearing diagnostics. Push and pull diagnostics are two different collections, so you cannot clear diagnostics acquired via pull using a push with an empty array.

The correct solution is to request a diagnostic refresh to the editor, which will then trigger pull diagnostics for all open documents.

### Implementation

Changed the implementation to clear our cache and then request a diagnostic refresh, which now correctly recomputes the diagnostics using the new RuboCop configs.

### Automated Tests

Added a test to verify that we're request the refresh.

### Manual Tests


https://github.com/user-attachments/assets/2a29ce82-4b23-47cf-8af0-8267980feec4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Milestone
Next
Development

No branches or pull requests
2 participants
@dbaeumer @vinistock