Duplicate color provider references

[niksy]

Describe the bug

When using two or more language servers which have color provider functionality, there are situations when duplicate references are displayed. This is most recently visible when using LSP-volar and LSP-sass for Vue SFC files.

To Reproduce

Steps to reproduce the behavior:

1. Create Vue SFC file (.vue) with following content

   <template>
   <div></div>
   </template>
   <style lang="scss" scoped>
   $color: #f00;
   body {
   color: $color;
   }
   </style>

2. Follow instructions on how to use LSP-sass for Vue SFC files
3. Observe $color variable

Expected behavior

Only one color provider reference is displayed.

Screenshots

Actual:

Expected:

Logs

N/A

Environment (please complete the following information):

• OS: macOS 14.6.1
• Sublime Text version: 4180
• LSP version: 2.2.0
• Language servers used: LSP-volar, LSP-sass

Additional context

As mentioned in https://github.com/sublimelsp/repository/pull/120#issuecomment-2422619311, this is probably related to Sublime LSP. My initial reaction was that it’s related to how Volar packages language servers and since there are no hooks with which you can control those servers (at least I haven’t found them), there isn’t really much you can do except disable colorProvider in one of the servers, but that relates to all server functionalities. What you actually want is to disable color provider on SFC Sass style blocks.

[jwortmann]

Thanks for the report.

I believe it happens because this entire block in the SessionBuffer class is faulty: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/session_buffer.py#L346-L356

This runs after document changes, but for each SessionBuffer (i.e. for each language server). I think that

• color boxes should only be requested from one session
• pull diagnostics are fine
• semantic tokens should only be requested from one session
• I'm unsure about document links, but probably it's okay to request from all sessions because the underline decorations (if enabled) won't conflict with each other
• inlay hints should only be requested from one session

For the mentioned features that are rendered in the view (e.g. via phantoms) it should be handled in DocumentSyncListener instead to choose the best_session and then distributed only to the relevant SessionBuffer.

I also saw that this part is buggy: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/documents.py#L369-L382

Again pull diagnostics are fine but the rest should probably not iterater over all SessionBuffers / SessionViews but instead select only the relevant one.

Edit: this part might be fine because the "pending refresh" is stored per SessionBuffer. Unless some server sends workspace/.../refresh requests even when we never requested that feature from that server.

I might take a look over the weekend and see if I can fix it.

Related recent discussion also at #2526

[niksy]

I'm unsure about document links, but probably it's okay to request from all sessions because the underline decorations (if enabled) won't conflict with each other

I think I also saw similar issues for document links. For example, you could have @import or @use Sass references where if you click on them you’re presented with multiple options. Don’t know if that makes sense, usually I’m using key binding which opens first result.

[rchl]

For the mentioned features that are rendered in the view (e.g. via phantoms) it should be handled in DocumentSyncListener instead to choose the best_session and then distributed only to the relevant SessionBuffer.

I remember working on that in the past but I was stopped in tracks due to https://github.com/sublimehq/sublime_text/issues/6188.

[jwortmann]

sublimehq/sublime_text#6188.

Okay but that still looks like a different issue to me because here we don't have any cloned views. I would think the cause of the bug described in this issue here is that color boxes, inlay hints, etc. are requested and stored per SessionBuffer: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/session_buffer.py#L130

There is always exactly one DocumentSyncListener per view, but if you have two language servers running there will be two SessionBuffer objects. Although the phantom sets seem to share the same key "lsp_color", so I assume they should just overwrite each other... but perhaps this is also part of the cause for the bug. If the self._color_phantoms were stored in DocumentSyncListener, it would be guaranteed that there is only one single PhantomSet object per view for the color boxes.

Similarly if semantic tokens were stored in DocumentSyncListener instead of SessionBuffer, I could imagine that your recent PR fix #2517 might not have been necessary. (well the cleanup of semantic tokens regions probably would still not work, I guess)

I might be wrong but iirc some time ago more of the code was initially in DocumentSyncListener, but then it was refactored and moved into the purge_changes_async method in SessionBuffer.

[jwortmann]

This is a bit tricky to fix. It seems doing the color boxes was moved from DocumentSyncListener to SessionBuffer in https://github.com/sublimelsp/LSP/commit/2b9863912636257283dd85e915f87a599af15d64 to ensure proper ordering between the didChange notification and the subsequent requests (which is of course good). But in SessionBuffer there is also debouncing happen first, before didChange and the requests are sent: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/session_buffer.py#L305

1. If we leave it like that, the proper ordering of didChange and the color box request could not be ensured directly if we move that request back into DocumentSyncListener.
2. I would say the design to have the debouncing happen in SessionBuffer is not good, because with multiple sessions the debouncing unnecessarily happens in parallel, implemented by scheduling via sublime.set_timeout_async.

So I would say that in SessionBuffer there must be no debouncing; instead this whole block should be moved to DocumentSyncListener, and then didChange and the requests could be distributed separately from DocumentSyncListener to the relevant SessionBuffers: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/session_buffer.py#L285-L306

This should only run for the DocumentSyncListener of the primary view, so that the methods in SessionBuffer won't get additionally called by listeners from cloned views. Currently we have that condition here: https://github.com/sublimelsp/LSP/blob/9065e1f5f9546caf35b329f831626a895318f1ce/plugin/documents.py#L330-L333

And then the PhantomSet could still be stored in the SessionBuffer (if necessary due to ST API behavior) because the corresponding update method would only be called for one of the SessionBuffers.

---

By the way, @rchl already recognized this problem in https://github.com/sublimelsp/LSP/pull/1899#issue-1055470521:

As a side effect, we'll request color boxes from all active sessions that support them rather then only the first one. This might be good or bad but if it's an issue, the user can disable relevant capability for one of the sessions.

I would say that it's not the right thing to request features which are rendered in the view (color boxes, inlay hints, ...) from all sessions. And the majority of users probably just use the default configuration and are not aware that something like "disabled_capabilities" exists (besides that, as pointed out in https://github.com/sublimelsp/repository/pull/120#issuecomment-2422810624 it is not always desired to disable it completely). So I think we should try to give the best default experience and only request those features from the best_session.

[rchl]

I kinda feel like technically handling it from SessionBuffer is the correct thing to do since the result is specific to the particular session.

We might want to instead try to figure out how to make the SessionBuffer dynamically decide whether it should perform the request or not and not move any logic from there.

EDIT: Sorry, that's what you said more or less. Did not read your (big) comment properly.