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

Extra semanticTokens requests in a multiroot workspace #1487

Closed
andyw8 opened this issue May 29, 2024 · 13 comments
Closed

Extra semanticTokens requests in a multiroot workspace #1487

andyw8 opened this issue May 29, 2024 · 13 comments
Labels
bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster
Milestone

Comments

@andyw8
Copy link

andyw8 commented May 29, 2024

👋

I'm a maintainer on Ruby LSP.

I have a multiroot repo with two apps, app1 and app2. https://github.com/andyw8/multiroot_monorepo

While a file from app1 is open, the logs show all requests going to app1, as expected, except for one type of request, semanticTokens:

Screenshot 2024-05-29 at 12 06 39 PM

This happens even if I have no files from app2 open.

Is there something special about semanticTokens that might cause it to behave that way?

(I can provide further information to help troubleshoot, but wanted to ask this up-front in case there's a simple answer).

cc @vinistock

@vinistock
Copy link
Contributor

Worth pointing out that the Ruby LSP spawns separate language servers for each configured workspace. Each one of the clients has their document selector configured to only handle requests for the specific workspace they are related to.

All other requests work as expected: each language server only receives requests for files that are inside their workspace. Except for semantic highlighting, which seems to be sent to all servers despite the document selector.

@dbaeumer
Copy link
Member

Is this still an issue? If so, what document selectors are you using for the different workspace folders?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Dec 12, 2024
@vinistock
Copy link
Contributor

Yes, it still happens. In fact, we started noticing that code snippets in Copilot Chat also trigger a semantic highlighting request before ever sending a textDocument/didOpen notification. It kept erroring for us because the server wasn't aware of that document's existence.

Just recently, we tried to limit our document selector to only the file and git schemes (PR), but we still see the Copilot Chat window firing semantic highlighting requests.

Our document selector is configured like this (simplified):

const selector = [
  // Selectors for documents inside the workspace for `ruby` and `erb`
  { scheme: "file", language: "ruby", pattern: `${workspaceFolder.uri.fsPath}/**/*` },
  { scheme: "git", language: "ruby", pattern: `${workspaceFolder.uri.fsPath}/**/*` },
  { scheme: "file", language: "erb", pattern: `${workspaceFolder.uri.fsPath}/**/*` },
  { scheme: "git", language: "erb", pattern: `${workspaceFolder.uri.fsPath}/**/*` },

  // Selectors for unsaved files
  { scheme: "untitled", language: "ruby" },
  { scheme: "untitled", language: "erb" },

  // Selectors for bundled gem dependencies
  { scheme: "file", language: "ruby", pattern: `${home}/.gem/ruby/${rubyVersion}/gems/**/*` },

  // Selectors for default gem dependencies
  { scheme: "file", language: "ruby", pattern: `${home}/.rubies/${rubyVersion}/lib/gems/**/*` },
]

Each workspace can have its own set of dependencies and it's possible to configure their installation path completely differently. For example, you can have a workspace that installs all dependencies under ~/.gem and another one that installs inside the workspace under vendor/bundle.

However, we see these extra semantic highlighting requests for files inside the workspaces or for the Copilot Chat snippets, not for files in dependencies.

@dbaeumer
Copy link
Member

Regarding textDocument/didOpen and request: according to the spec it is legal to send requests for documents that are not open. Open is a bad name since it is more about ownership.

I will look into why the sematic color request is not scoped correctly

@dbaeumer dbaeumer added bug Issue identified by VS Code Team member as probable bug and removed info-needed Issue requires more information from poster labels Dec 16, 2024
@dbaeumer dbaeumer added this to the Next milestone Dec 16, 2024
@dbaeumer
Copy link
Member

I tried to reproduce this but failed. Can you check in semanticTokens.js around here:

Image

that this is called twice with different document selectors. Can you also let me know how these selectors look like.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Dec 18, 2024
@vinistock
Copy link
Contributor

Okay, I just debugged in the place you mentioned and I got the exact values for what we declare as document selectors and what reaches that point in the code.

The document selector we pass as a client option, has the following entries.

[
  // ERB files inside the workspace for file and git
  { scheme: 'file', language: 'erb', pattern: '/workspace/path/ruby-lsp/**/*' },
  { scheme: 'git', language: 'erb', pattern: '/workspace/path/ruby-lsp/**/*' },
  
  // Ruby files inside the workspace for file and git
  { scheme: 'file', language: 'ruby', pattern: '/workspace/path/ruby-lsp/**/*' },
  { scheme: 'git', language: 'ruby', pattern: '/workspace/path/ruby-lsp/**/*' },

  // Ruby and ERB files for the untitled scheme
  { scheme: 'untitled', language: 'ruby' },
  { scheme: 'untitled', language: 'erb' },

  // Ruby files in bundled gems
  { scheme: 'file', language: 'ruby', pattern: '$HOME/.gem/ruby/3.4.1/**/*' },
  { scheme: 'git', language: 'ruby', pattern: '$HOME/.gem/ruby/3.4.1/**/*' },

  // Ruby files in default gems
  { scheme: 'file', language: 'ruby', pattern: '/RUBY_PATH/3.4.1/lib/ruby/gems/3.4.0/**/*' },
  { scheme: 'git', language: 'ruby', pattern: '/RUBY_PATH/3.4.1/lib/ruby/gems/3.4.0/**/*' },

  // Ruby files in bundled gems stored inside of the Ruby installation
  { scheme: 'file', language: 'ruby', pattern: '/RUBY_PATH/3.4.1/lib/ruby/3.4.0/**/*' },
  { scheme: 'git', language: 'ruby', pattern: '/RUBY_PATH/3.4.1/lib/ruby/3.4.0/**/*' },
]

Debugging in the spot that you indicated, registerLanguageProvider is invoked only once when a single workspace is opened in VS Code, but the value of const selector = options.documentSelector; is the following:

[
  { language: "ruby" },
  { language: "erb" }
]

Could the lack of the schemes and patterns be the reason here? If I ask Copilot Chat to solve a simple problem in Ruby, I see the code blocks in the chat fire a bunch of requests for vscode-chat-code-block:// scheme URIs (despite we not including that in the selector).

Let me know if I can provide more information to help.

@dbaeumer
Copy link
Member

@vinistock do you by any chance specify a document selector on the server side when returning from the initialize request? Newer requests (like semantic tokens) support that and it would override the client side default selector.

@dbaeumer
Copy link
Member

@vinistock
Copy link
Contributor

You once again cleared things up. It was indeed the server that was incorrectly overriding our document selectors and not scoping it to any scheme or patterns. This is fixed in Shopify/ruby-lsp#3074. Thank you very much for the help, I appreciate it.

Do you believe it would make sense for the client package to emit a warning if the original document selector was overridden by the server?

Also, please feel free to close this issue.

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

I finally understood the mysterious requests we were receiving from URIs we didn't care about or the duplicate requests thanks to microsoft/vscode-languageserver-node#1487 (comment).

We were overriding the document selectors we define in the client in the server, with a much more simplistic approach. In the client, we take steps to ensure that we're handling only certain schemes with specific patterns and not duplicating client handling for the `untitled` scheme.

All of that configuration was lost for many requests because the server was simply broadcasting the document selector as `{ language: "ruby" }`, which is not scoped to any scheme or pattern.

This results in every language server running in the current VS Code window advertise that they handle any Ruby file, which is why we kept receiving duplicate requests and why we kept receiving requests for schemes we are not interested in handling - like the `vscode-chat-code-block://` scheme.

### Implementation

Since our document selector logic is so complex and requires knowledge of default gem paths, bundled gem paths, workspace paths and so on, we should not override it from the server. Trying to implement the same document selector logic on both ends would probably lead to unnecessary headache.

All of our requests already specify in `server.rb` if they need to return early for a certain document type, so there's no risk of receiving undesired requests that we don't know how to handle.

Considering that, I just set all document selectors in requests to `nil`, which [makes the editor prefer the one configured by the client](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocumentRegistrationOptions).
@andyw8 andyw8 closed this as completed Jan 17, 2025
@dbaeumer
Copy link
Member

What we could add is a trace entry saying under which document selector a feature got registered. Making this a warning is IMO to hard since servers might really do this on purpose.

@vinistock
Copy link
Contributor

Yeah, that makes sense. Would you like to have that added? I can put a PR up if you think it's worth doing it.

@dbaeumer
Copy link
Member

PRs are always welcome!

@vinistock
Copy link
Contributor

I looked a little bit into this. I think the spot where server capabilities are assigned is where the selectors are overridden for static registration. Is that correct?

Logging something in this case would require going over all registered features that support text document registration options and checking if they passed a document selector, which may honestly not be worth it.

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 info-needed Issue requires more information from poster
Projects
None yet
Development

No branches or pull requests

3 participants