Semantic highlighting (v2)

[jwortmann]

This is an alternative to #1751, but with support for more request types and additional features.

There is still a lot to optimize, for example there is a short flickering because all regions are redrawn after each request. Also I'm not happy yet with how token modifiers are taken into account - there probably should be a way for the helper plugins to consider them too, and for color schemes to target them. I left some "TODO" comments in the code where I got stuck a bit; suggestions or help appreciated.
I haven't yet considered the region draw order (so that it works together with diagnostics etc.) and there might be some other bugs around - just wanted to open it here already, so that those who are interested could take a look or provide suggestions. And in general, I'm not sure if this add_regions approach for semantic highlighting is something we want, or if we shouldn't better wait for a dedicated API (might possibly never be implemented though).

In general it seems to work with clangd, but I have only tested with very simple examples because I'm not very familiar with C++. Maybe some tweaks for the default scopes are needed too, probably requires some experience with more servers about how token types are used.
I couldn't get it really to work with the Lua server, because that server immediately unregisters the semantic token capability after it initializes.

[rwols]

Interestingly, I see no request for semantic tokens for clangd with this pull request. It does a workspace/semanticTokens/refresh immediately after opening the document.

::  -> clangd textDocument/didOpen: {'textDocument': {'languageId': 'cpp', 'version': 0, 'uri': 'file:///home/raoul/dev/tonic/src/CancelledException.cpp', 'text': '#include <tonic/CancelledException.hpp>\n\nnamespace tonic {\n\nconst char* CancelledException::what() const noexcept\n{\n    return "task was cancelled";\n}\n\n} // namespace tonic\n'}}
:: <-- clangd workspace/semanticTokens/refresh(3): None
:: >>> clangd 3: None
:: <-  clangd textDocument/publishDiagnostics: {'version': 0, 'uri': 'file:///home/raoul/dev/tonic/src/CancelledException.cpp', 'diagnostics': []}
:: --> clangd textDocument/codeAction(14): {'range': {'start': {'line': 0, 'character': 0}, 'end': {'line': 0, 'character': 0}}, 'context': {'diagnostics': []}, 'textDocument': {'uri': 'file:///home/raoul/dev/tonic/src/CancelledException.cpp'}}
:: <<< clangd 14: []

[jwortmann]

That is strange. clangd doesn't send any workspace/semanticTokens/refresh at all for me. But I've only tested with a single file, and with the default configuration from LSP.sublime-settings. What version of clangd do you use? However, LSP should still send a request for semantic tokens after that refresh request. Do you have added the required rule into your color scheme, as described in the docs entry?

[rwols]

Ah right, I don’t have the color scheme customization on the Linux box I’m testing this with. It’s late and I’ll get back to this some other time :)

[rwols]

Please fix the merge conflicts :)

[jwortmann]

Not sure how to proceed with this PR. Have you tried it already?

It seems to works relatively well with clangd and rust-analyzer, but I haven't thoroughly tested because I neither use C++ nor Rust. Of course there is a little bit of lag from the communication with the servers, which can make the syntax highlighting feel a bit sluggish. E.g. if you comment out a line of code, then it will take a fraction of a second until the tokens on this line are cleared by the server too, which I find quite noticeable and I don't like it.
clangd seems to have cool features that are not possible with normal syntax highlighting, for example it can dynamically evaluate preprocessor directives and mark inactive code as comment:

But for rust-analyzer I saw for example that it will mark argument names in a function declaration just as "variable" token type (instead of variable.parameter scope), which will basically remove the highlighting color for it in most color schemes. I would consider that as a regression compared to the highlighting from ST syntaxes. So I think it can vary a lot how much of an advantage or even disadvantage semantic highlighting is, compared to regular highlighting from ST.

I guess something with possible improvements would be the token-to-scope mapping, if we can figure out how exactly the tokens are used by each server. It is already possible in this PR to override this mapping in the client config for the servers, or add scopes for custom token types. If I remember correctly, rust-analyzer uses a lot of custom types for example.

[rwols]

I added the special color scheme rule for the built-in color schemes in 2aadb1a, are you okay with that? With this, I would recommend to keep the boolean setting and leave it disabled by default. Otherwise we would force users to use semantic highlighting (it could still be disabled via "disabled_capabilities", but probably not many users know about it or use it).

I have doubts about treating the built-in color schemes special. It shouldn't be that hard to run UI: Customize Color Scheme from the command palette and copy-and-paste the magic rule, no? Right now, users with a color scheme downloaded from package control (I think there's many of them) must go through two steps (enabling a boolean option, and customizing their color scheme).

[rwols]

Also please merge with upstream :)

[rchl]

Sorry if that was already discussed/reported but do squiggly error highlights are supposed to work correctly with those changes or it's still an unsolved issue?

Because what I can see now is that if a given word has both semantic region and error region applied then the squiggly underline is missing completely.

[jwortmann]

They are supposed to work correctly even with underlines from diagnostics and also with documentHighlights. This is what the SessionView._initialize_region_keys() method is for. But if there is a special color scheme rule for a certain custom token type, then it can happen that its region key didn't get initialized, so that diagnostic underlines might not work together with this token type. But otherwise the region keys used in the initializing method should correspond to what is actually used in the code, so they should work unless I've missed a bug (I've noticed that I had forgotten to update the initializing method when I changed the scopes recently, but it should be fixed already). During some quick tests with clangd it seemed to work correctly for me. Are the squiggly underlines not shown for you all the time when there is a semantic token, or only in certain cases?

[rchl]

But if there is a special color scheme rule for a certain custom token type, then it can happen that its region key didn't get initialized,

Sorry, I think that was the case. My custom rule for highlighting unused variables was actually the reason why the error highlight didn't show up.

[jwortmann]

Sorry, I think that was the case. My custom rule for highlighting unused variables was actually the reason why the error highlight didn't show up.

This is good to know :)
It should work correctly, if you instead declare a scope for the token/modifier in the LSP-typescript.sublime-settings file in a "semantic_tokens" mapping. Then the region key for this token should be initialized correctly.

But yes, it can still be considered to be a bug that it might not work for when there are only color scheme rules for custom token types. But I don't really know how this could be solved.

[jwortmann]

To fix the bug I described in #1839 (comment), I think we need better support for request cancellation. From looking at the code base, the only thing we can do right now, is

Session.send_notification(Notification("$/cancelRequest", {"id": request_id}))

But according to the LSP specs, the server still must send a response for the request. If I understand it correctly, it is advised to return an error response with code ErrorCodes.RequestCancelled then, but the response seems not required to be an error response and can still be a valid normal response to the request. If that would happen for a cancelled textDocument/semanticTokens/full/delta reqeuest, the unexpected response could still mess up our internally stored token data.

Therefore I would suggest to add a new method Session.cancel_request(self, request_id: int) -> None, which will then send the $/cancelRequest notification, as well as remove the response handlers for that request, to ignore a possible response. Also Session.send_request() and Session.send_request_async() should return an int with the request_id, so that the requests with pending responses for a certain buffer can be tracked. I think currently the only possible alternative way would be to iterate over all active requests and filter for a certain method, like in

LSP/plugin/session_view.py

Lines 312 to 314 in 8807689

	for request_id, request in self.active_requests.items():
	if request.method == "codeAction/resolve":
	self.session.send_notification(Notification("$/cancelRequest", {"id": request_id}))

but this is not sufficient if I want to cancel only one or multiple particular requests, but leave other open requests of the same method untouched (for example for other buffers).

[jwortmann]

I've added what I meant with the request cancellation & ignored response (rust-analyzer indeed still sends a response and just ignores the cancel notification) in 5014576. I'm not sure if this implementation is a good way to do it, and perhaps it would fit better for a separate PR, ideally including a test case (I looked into the tests, but couldn't add one for it without help). Also I didn't see a good way to return the request_id from Session.send_request() and Session.send_request_task().

[rwols]

I think the cancellation stuff is better suited as a separate PR because this one is getting quite large. Do you want to put it on “ready for review” or is there something that requires work for this to be on draft status?

[jwortmann]

I think the cancellation stuff is better suited as a separate PR because this one is getting quite large.

Compared to the commit I linked above, it would be nice for consistency to also return the request id at least from Session.send_request(). I tried it like the following, but I think it doesn't work together with Promise in Session.execute_command() and Pyright returns an error.

Click to expand

diff --git a/plugin/core/sessions.py b/plugin/core/sessions.py
index dd61e6d..c417f9a 100644
--- a/plugin/core/sessions.py
+++ b/plugin/core/sessions.py
@@ -1646,15 +1646,13 @@ class Session(TransportCallbacks):
 
     # --- RPC message handling ----------------------------------------------------------------------------------------
 
-    def send_request_async(
+    def _send_request_async(
             self,
+            request_id: int,
             request: Request,
             on_result: Callable[[Any], None],
             on_error: Optional[Callable[[Any], None]] = None
     ) -> None:
-        """You must call this method from Sublime's worker thread. Callbacks will run in Sublime's worker thread."""
-        self.request_id += 1
-        request_id = self.request_id
         if request.progress and isinstance(request.params, dict):
             request.params["workDoneToken"] = _WORK_DONE_PROGRESS_PREFIX + str(request_id)
         self._response_handlers[request_id] = (request, on_result, on_error)
@@ -1664,14 +1662,29 @@ class Session(TransportCallbacks):
         self._logger.outgoing_request(request_id, request.method, request.params)
         self.send_payload(request.to_payload(request_id))
 
+    def send_request_async(
+            self,
+            request: Request,
+            on_result: Callable[[Any], None],
+            on_error: Optional[Callable[[Any], None]] = None
+    ) -> int:
+        """You must call this method from Sublime's worker thread. Callbacks will run in Sublime's worker thread."""
+        self.request_id += 1
+        request_id = self.request_id
+        self._send_request_async(request_id, request, on_result, on_error)
+        return request_id
+
     def send_request(
             self,
             request: Request,
             on_result: Callable[[Any], None],
             on_error: Optional[Callable[[Any], None]] = None,
-    ) -> None:
+    ) -> int:
         """You can call this method from any thread. Callbacks will run in Sublime's worker thread."""
-        sublime.set_timeout_async(functools.partial(self.send_request_async, request, on_result, on_error))
+        self.request_id += 1
+        request_id = self.request_id
+        sublime.set_timeout_async(functools.partial(self._send_request_async, request_id, request, on_result, on_error))
+        return request_id
 
     def send_request_task(self, request: Request) -> Promise:
         task = Promise.packaged_task()  # type: PackagedTask[Any]
@@ -1679,6 +1692,12 @@ class Session(TransportCallbacks):
         self.send_request_async(request, resolver, lambda x: resolver(Error.from_lsp(x)))
         return promise
 
+    def cancel_request(self, request_id: int, ignore_response: bool = True) -> None:
+        self.send_notification(Notification("$/cancelRequest", {"id": request_id}))
+        if ignore_response and request_id in self._response_handlers:
+            request, _, _ = self._response_handlers[request_id]
+            self._response_handlers[request_id] = (request, lambda *args: None, lambda *args: None)
+
     def send_notification(self, notification: Notification) -> None:
         if self._plugin:
             self._plugin.on_pre_send_notification_async(notification)

And a test with something like the following would be useful, but idk really how to start:

1. define a callback function, which modifies a global variable or so
2. send request to server with that callback function on response
3. cancel the request
4. server sends back a normal response
5. assert that the global variable is not modified (i.e. callback function was not called)

If you know how to fix it and how to add a test, feel free to use the diff or and/or improve it for a separate PR :)

---

Do you want to put it on “ready for review” or is there something that requires work for this to be on draft status?

OK. Some remaining limitations of the PR:

• the code assumes that there is only a single language server which provides semantic tokens
• highlighting for tokens with more than 1 token modifier could have unexpected results and can't be styled reliably via the color scheme
• I didn't add the language ID from the server to the scope name yet, so unlike VSCode we can't adjust styles per language via the color scheme

---

Btw, I updated my clangd version to 13.0.0 and it seems to work noticeably better than the previous version (now uses token modifiers & syncronization bug fixed).

[rwols]

Amazing work. Thanks for spending time on this :) Let's move on with the cancellation stuff.