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

Handle unsaved files in main language server client #2124

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

vinistock
Copy link
Member

Motivation

With the changes in #2101, we regressed with an interesting bug. Because of the narrower document selector, we were simply ignoring unsaved files.

Theoretically, that shouldn't case any issues since we're just ignoring them. However, received reports of #1897 (comment) happening again and indeed it's easy to reproduce on main.

That issue only happens when we receive an unexpected request for a document before a textDocument/didOpen notification gets sent for it (which should never happen).

The reason it's happening is the bug we identified microsoft/vscode-languageserver-node#1487, where the LSP package is sending unexpected semantic token requests without respecting the document selector.

So, the order of the events were:

  1. You open an unsaved Ruby file
  2. We didn't receive a textDocument/didOpen notification for it, because we were accidentally ignoring it in the document selector
  3. The LSP package mistakenly sends a semantic token request anyway
  4. Since we never got notified that the file was opened in the first place, it breaks the server

Quite an interesting interaction of different issues.

Implementation

For unsaved files, it's not possible to know to which workspace they belong to. That information only exists after the user chooses where to save the file.

What we need to do is have the main language client (the first one) handle all files while they are unsaved. If the user decides to save the file in a workspace that does not belong to that main client, that is completely fine. VS Code will actually send didClose notifications for the untitled:Untitled-1 uri and then send a didOpen notification for the file that was just saved to the correct language client.

The implementation is not the prettiest, but I don't know how else we could go about it.

@vinistock vinistock added bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes labels Jun 3, 2024
@vinistock vinistock self-assigned this Jun 3, 2024
@vinistock vinistock requested a review from a team as a code owner June 3, 2024 18:29
@vinistock vinistock requested review from andyw8 and st0012 June 3, 2024 18:29
@vinistock vinistock force-pushed the vs/handle_unsaved_files_in_main_client branch from e1fb043 to db82758 Compare June 4, 2024 20:21
vscode/src/client.ts Outdated Show resolved Hide resolved
vscode/src/client.ts Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs/handle_unsaved_files_in_main_client branch from db82758 to ef1d09f Compare June 5, 2024 13:41
@vinistock vinistock requested a review from st0012 June 5, 2024 13:42
@vinistock vinistock merged commit 467e0d9 into main Jun 5, 2024
34 checks passed
@vinistock vinistock deleted the vs/handle_unsaved_files_in_main_client branch June 5, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants