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 sublimelsp/repository#120 (comment), 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:

LSP/plugin/session_buffer.py

Lines 346 to 356 in 9065e1f

	self._do_color_boxes_async(view, version)
	self.do_document_diagnostic_async(view, version)
	if self.session.config.diagnostics_mode == "workspace" and \
	not self.session.workspace_diagnostics_pending_response and \
	self.session.has_capability('diagnosticProvider.workspaceDiagnostics'):
	self._workspace_diagnostics_debouncer_async.debounce(
	self.session.do_workspace_diagnostics_async, timeout_ms=WORKSPACE_DIAGNOSTICS_TIMEOUT)
	self.do_semantic_tokens_async(view)
	if userprefs().link_highlight_style in ("underline", "none"):
	self._do_document_link_async(view, version)
	self.do_inlay_hints_async(view)

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:

LSP/plugin/documents.py

Lines 369 to 382 in 9065e1f

	for sv in self.session_views_async():
	if sv.code_lenses_needs_refresh:
	sv.set_code_lenses_pending_refresh(needs_refresh=False)
	sv.start_code_lenses_async()
	for sb in self.session_buffers_async():
	if sb.document_diagnostic_needs_refresh:
	sb.set_document_diagnostic_pending_refresh(needs_refresh=False)
	sb.do_document_diagnostic_async(self.view)
	if sb.semantic_tokens.needs_refresh:
	sb.set_semantic_tokens_pending_refresh(needs_refresh=False)
	sb.do_semantic_tokens_async(self.view)
	if sb.inlay_hints_needs_refresh:
	sb.set_inlay_hints_pending_refresh(needs_refresh=False)
	sb.do_inlay_hints_async(self.view)

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 sublimehq/sublime_text#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:

LSP/plugin/session_buffer.py

Line 130 in 9065e1f

self._color_phantoms = sublime.PhantomSet(view, "lsp_color")

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 2b98639 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:

LSP/plugin/session_buffer.py

Line 305 in 9065e1f

debounced(lambda: self.purge_changes_async(view), FEATURES_TIMEOUT,

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:

LSP/plugin/session_buffer.py

Lines 285 to 306 in 9065e1f

	def on_text_changed_async(self, view: sublime.View, change_count: int,
	changes: Iterable[sublime.TextChange]) -> None:
	if change_count <= self._last_synced_version:
	return
	self._last_text_change_time = time.time()
	last_change = list(changes)[-1]
	if last_change.a.pt == 0 and last_change.b.pt == 0 and last_change.str == '' and view.size() != 0:
	# Issue https://github.com/sublimehq/sublime_text/issues/3323
	# A special situation when changes externally. We receive two changes,
	# one that removes all content and one that has 0,0,'' parameters.
	pass
	else:
	purge = False
	if self._pending_changes is None:
	self._pending_changes = PendingChanges(change_count, changes)
	purge = True
	elif self._pending_changes.version < change_count:
	self._pending_changes.update(change_count, changes)
	purge = True
	if purge:
	debounced(lambda: self.purge_changes_async(view), FEATURES_TIMEOUT,
	lambda: view.is_valid() and change_count == view.change_count(), async_thread=True)

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:

LSP/plugin/documents.py

Lines 330 to 333 in 9065e1f

	def on_text_changed_async(self, change_count: int, changes: Iterable[sublime.TextChange]) -> None:
	if self.view.is_primary():
	for sv in self.session_views_async():
	sv.on_text_changed_async(change_count, changes)

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 #1899 (comment):

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 sublimelsp/repository#120 (comment) 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.