chore(lsp): remove capabilities sanitization

[mjlbach]

Closes #16200

[mjlbach]

@mfussenegger I removed the vim.tbl_get implementation and rebased this on that PR. One thing, should we just get rid of resolved_capabilities and update server_capabilities in place? The main thing is sanitizing the textDocumentSync capabilities.

[mjlbach]

Also I think I want to squeeze this in before 0.7.

[mfussenegger]

One thing, should we just get rid of resolved_capabilities and update server_capabilities in place?

I'd tend to get rid of them altogether - or what kind of advantage does it offer to maintain the resolved_capabilities? I'd think with dynamic capabilities it would be easier if there is only a single source of truth instead of two?

[mjlbach]

Those were my thoughts as well, done.

[mjlbach]

@tjdevries Not sure if you have an opinion on this, since I think this was original your code, but LMK if you disagree with the change! It's mostly to simplify adding new capabilities (plugin authors can directly check the raw response for off-protocol stuff, which they technically could before) and for adding dynamic capabilities (we can naively join the tables instead of having to track the sanitized name for each property). I also think it makes the code a bit cleaner.

[mjlbach]

This is also a pretty big breaking change, maybe should soft-deprecate instead...

[mfussenegger]

This is also a pretty big breaking change, maybe should soft-deprecate instead...

That would be good. Any idea how to go about it without it being too much effort?

[mjlbach]

We could add an __index method to required_capabilities that calls notify_once with a deprecation method, and keep the old thing around?

[mjlbach]

@mfussenegger added the warning, want to test it out with one of the offending plugins? It worked locally for me.

[kylo252]

How about creating a small wrapper for the lookup function? It will help (slightly) with readability, and make any future changes easier

diff --git a/runtime/lua/vim/lsp.lua b/runtime/lua/vim/lsp.lua
index bd315ce67..0af464167 100644
--- a/runtime/lua/vim/lsp.lua
+++ b/runtime/lua/vim/lsp.lua
@@ -306,6 +306,11 @@ local function once(fn)
   end
 end
 
+function lsp._get_server_capability(client, name)
+ local keys = vim.split(name, "%.")
+ return vim.tbl_get(client.server_capabilities, unpack(keys))
+end
+
 local changetracking = {}
 do
   --@private
@@ -328,7 +333,7 @@ do
   function changetracking.init(client, bufnr)
     local use_incremental_sync = (
       if_nil(client.config.flags.allow_incremental_sync, true)
-      and vim.tbl_get(client.server_capabilities, "textDocumentSync", "change") == protocol.TextDocumentSyncKind.Incremental
+      and lsp._get_server_capability(client, "textDocumentSync.change") == protocol.TextDocumentSyncKind.Incremental
     )
     local state = state_by_client[client.id]
     if not state then
@@ -447,7 +452,7 @@ do
     end)
     local uri = vim.uri_from_bufnr(bufnr)
     return function(client)
-      if vim.tbl_get(client.server_capabilities, "textDocumentSync", "change")  == protocol.TextDocumentSyncKind.None then
+      if lsp._get_server_capability(client, "textDocumentSync.change")  == protocol.TextDocumentSyncKind.None then
         return
       end
       local state = state_by_client[client.id]
@@ -526,7 +531,7 @@ end
 ---@param client Client object
 local function text_document_did_open_handler(bufnr, client)
   changetracking.init(client, bufnr)
-  if not vim.tbl_get(client.server_capabilities, "textDocumentSync", "openClose") then
+  if not lsp._get_server_capability(client, "textDocumentSync.openClose") then
     return
   end
   if not vim.api.nvim_buf_is_loaded(bufnr) then
@@ -983,7 +988,7 @@ function lsp.start_client(config)
         if not required_capability then
           return true
         end
-        if vim.tbl_get(client.server_capabilities, unpack(required_capability)) then
+        if lsp._get_server_capability(client, unpack(required_capability)) then
           return true
         else
           return false
@@ -1207,9 +1212,9 @@ function lsp._text_document_did_save_handler(bufnr)
   local uri = vim.uri_from_bufnr(bufnr)
   local text = once(buf_get_full_text)
   for_each_buffer_client(bufnr, function(client, _client_id)
-    if vim.tbl_get(client.server_capabilities, "textDocumentSync", "save") then
+    if lsp._get_server_capability(client, "textDocumentSync.save") then
       local included_text
-      if vim.tbl_get(client.server_capabilities, "textDocumentSync", "save", "includeText") then
+      if lsp._get_server_capability(client, "textDocumentSync.save.includeText") then
         included_text = text(bufnr)
       end
       client.notify('textDocument/didSave', {
@@ -1262,7 +1267,7 @@ function lsp.buf_attach_client(bufnr, client_id)
         local params = { textDocument = { uri = uri; } }
         for_each_buffer_client(bufnr, function(client, _)
           changetracking.reset_buf(client, bufnr)
-          if vim.tbl_get(client.server_capabilities, "textDocumentSync", "openClose") then
+          if lsp._get_server_capability(client, "textDocumentSync.openClose") then
             client.notify('textDocument/didClose', params)
           end
           text_document_did_open_handler(bufnr, client)
@@ -1272,7 +1277,7 @@ function lsp.buf_attach_client(bufnr, client_id)
         local params = { textDocument = { uri = uri; } }
         for_each_buffer_client(bufnr, function(client, _)
           changetracking.reset_buf(client, bufnr)
-          if vim.tbl_get(client.server_capabilities, "textDocumentSync", "openClose") then
+          if lsp._get_server_capability(client, "textDocumentSync.openClose") then
             client.notify('textDocument/didClose', params)
           end
         end)
@@ -1322,7 +1327,7 @@ function lsp.buf_detach_client(bufnr, client_id)
 
   changetracking.reset_buf(client, bufnr)
 
-  if vim.tbl_get(client.server_capabilities, "textDocumentSync", "openClose") then
+  if lsp._get_server_capability(client, "textDocumentSync.openClose") then
     local uri = vim.uri_from_bufnr(bufnr)
     local params = { textDocument = { uri = uri; } }
     client.notify('textDocument/didClose', params)

[mfussenegger]

It will help (slightly) with readability, and make any future changes easier

Do you have a rationale for this? This assessment sounds rather subjective and I would claim the opposite is the case as it adds some additional indirection that obfuscates what's happening

added the warning, want to test it out with one of the offending plugins? It worked locally for me.

I'll give it a go. Do you want to get this into 0.7?

[kylo252]

It will help (slightly) with readability, and make any future changes easier

Do you have a rationale for this? This assessment sounds rather subjective and I would claim the opposite is the case as it adds some additional indirection that obfuscates what's happening

I agree that this is rather subjective, so no argument from me there! One other protentional (readability) benefit that I didn't mention, is default value support, which should trim some boiler-plate code

function lsp._get_server_capability(client, name, default)
  local keys = vim.split(name, "%.")
  return vim.F.if_nil(vim.tbl_get(client.server_capabilities, unpack(keys)), default)
end

[mjlbach]

Doesn't that add boiler plate code?

[kylo252]

Doesn't that add boiler plate code?

I feel like I'm starting to sound like I'm giving the snippet too much credit, while it's a basic "extract method" refactor 😅

Just for the sake of the argument

• it's intentionally adding an abstraction about the how capabilities (and their hierarchy) are stored with relation to the client.
• it's also handy for testing, since you can just mock the return value of that function instead of setting up a custom server_capabilities table every time.

At the end of the day, it's most probably an internal function anyway, so choose what you feel is best for maintainability.

[tjdevries]

(I have no opinion, since I was asked)

[kylo252]

should this actually be info level? wouldn't it be better to have the info to be status message "initialized", and then dump the capabilities in debug or trace level?

[mfussenegger]

Gave it a quick try and seems to work and diff looks good to me

[mjlbach]

@mfussenegger should be good to merge