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

feat(lsp): add vim.lsp.server to create in-process server #24338

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mfussenegger
Copy link
Member

@mfussenegger mfussenegger commented Jul 13, 2023

Early WIP/draft:

There are occasionally requests for dedicated APIs for code actions or codelens, (also formatter, but there's formatexpr, formatprg and !cmd and afaik so far no explanation of why they are not sufficient). Concrete request for codelens: #20181

Or potential use-cases for code-action:

For vim.diagnostic, a dedicated module made a lot of sense as linters have existed long before LSP and there are lots of standalone tools.

For the others (codelens, code actions, hover, inline-hints, etc.) the value proposition is more questionable. What would a dedicated API improve over just using LSP?

One problem with "just use LSP" is that you either need a separate process, or some boilerplate.
This PR would address the boilerplate problem.

E.g. a implementation for hover could look like this:

local api = vim.api
local server = vim.lsp.server({
  capabilities = {
    hoverProvider = true
  },
  handlers = {
    ---@param params lsp.HoverParams
    ["textDocument/hover"] = function(_, params)
      local bufnr = vim.uri_to_bufnr(params.textDocument.uri)
      local lnum = params.position.line
      local col = params.position.character
      -- use bufnr/lnum/col to provide actual text
      return {
        contents = {
          kind = "plaintext",
          value = "just some dummy text",
        }
      }
    end,
  }
})
vim.lsp.start({ name = "dummy-ls", cmd = server })

Advantages:

  • We don't need to figure out a provider pull/push model for plugins to register code-actions, hover, etc.
  • We don't need a ton of new APIs, documentations and tests
  • Also provides answers for progress notification and cancellation (although with open construction sites, e.g. Expose a simple way to cancel LSP requests from vim.lsp.buf.* #22852)
  • We already use something like that in the tests. Even if we decide to add dedicated APIs later, this might be low cost due do that
  • We can maybe convince Lewis to improve multi-client support, which we want anyway, instead of working on https://github.com/lewis6991/hover.nvim
  • Would also be useful for plugins implementing off-spec LSP extensions or similar functions for testing. (E.g lsp-compl has something like this in its tests)

Disadvantages:

  • LSP allows to make requests for buffers that are not visible and not "current": That means in the handlers you cannot make good use of functions like vim.fn.expand("<cexpr>"), but you'd currently have to create a dummy window, and open the buffer in that windows
  • Slightly more verbose than dedicated APIs might be
  • Limited to LSP functionality

TBD:

  • Decide if we want this
  • Finetune API
    • is the method param required for each handler, it's kinda redundant.
    • How about push from server to client?
  • Finish example in the docs

@github-actions github-actions bot added the lsp label Jul 13, 2023
@mfussenegger mfussenegger added the needs:discussion For PRs that propose significant changes to some part of the architecture or API label Jul 13, 2023
@mfussenegger
Copy link
Member Author

We could reduce the boilerplate even more. For example:

local server = vim.lsp.server({
  ---@param params lsp.HoverParams
  ["on_textDocument/hover"] = function(_, params)
    local bufnr = vim.uri_to_bufnr(params.textDocument.uri)
    local lnum = params.position.line
    local col = params.position.character
    -- use bufnr/lnum/col to provide actual text
    return {
      contents = {
        kind = "plaintext",
        value = "just some dummy text",
      }
    }
  end,
})

capability is implicitly set, based on provided handlers. (We already have a method <-> provider mapping internally)

Maybe even with a auto_start = true or something like that, to also imply the vim.lsp.start()

@justinmk
Copy link
Member

justinmk commented Jul 14, 2023

Wonderful! I think we definitely want this, can you confirm the following:

  • It extends the usefulness of our LSP support by allowing LSP concepts to be re-used even when a server is not available.
    • So non-langserver things like cscope can be adapted to Nvim LSP interface.
  • Formalizes the general idea behinds null-ls
  • Analogous to vscode's in-process language features

@justinmk justinmk removed the needs:discussion For PRs that propose significant changes to some part of the architecture or API label Jul 14, 2023
@mfussenegger
Copy link
Member Author

mfussenegger commented Jul 14, 2023

It extends the usefulness of our LSP support by allowing LSP concepts to be re-used even when a server is not available.

Yes

Formalizes the general idea behinds null-ls

Yes. null-ls used the same mechanism. (At least since Neovim 0.8 made it possible, before it hacked into internals))

Analogous to vscode's in-process language features

It goes into that direction in the sense that it lets plugins provide that kind of functionality, yes - but also with a key difference.
Afaik in VSCode, the core editor and the language server client share the same primitives like Position, Location, TextDocument. The core editor also has higher level concepts like ReferenceProvider, DefinitionProvider and the concrete language server client is just one implementation of these providers. Any extension can also implement providers without going through a lsp-client.

That makes a lot of sense for vscode since they already share the common structures, but for Neovim we have different core primitives. bufnr, lnum and col instead of TextDocument and Position. So we'd need a translation layer (like vim.lsp.diagnostic is for vim.diagnostic)

Following the vscode model closely would mean creating vim.references, vim.definitions, vim.code_actions, vim.codelenses, and re-building LSP on top of that.
What this does instead is using the lsp-client as ReferenceProvider/DefinitionProvider/CodeActionProvider

@justinmk
Copy link
Member

Following the vscode model closely would mean creating vim.references, vim.definitions, vim.code_actions, vim.codelenses`, and re-building LSP on top of that.
What this does instead is using the lsp-client as ReferenceProvider/DefinitionProvider/CodeActionProvider

👍 The latter approach (this PR) is 100% fine and (for reference) we definitely don't need/want to take the "inverted" approach.

@pmizio
Copy link

pmizio commented Jul 15, 2023

@mfussenegger I'm author of https://github.com/pmizio/typescript-tools.nvim and this is s great addition to have which can really simplify process of spawning this tsserver mess. Tsserver is lsp like but not compatible on "message structure" level. I see how this addition can simplify my code, but to use this I would need few extensions to this:

  • pass request_id into handler thanks to that I can drop code related to synchronization ids between tsserver and nvim
  • incorporate into api async handlers - for me it will be waiting for pong from tsserver, for others it can be running some process and waiting for stout(null-lsish plugins)
  • get access to dispatchers - just useful thing to have

I know this pr is meant to be for simpler use cases but this three changes can make it more universal even for more complicated projects. Of course I give only my insight from building off-spec in process server plugin and it is ok if this is out of scope of this pr. But anyway I want to put this on the table.

@lewis6991
Copy link
Member

lewis6991 commented Jul 15, 2023

I'm pretty confident we need to pass a callback to the handlers to better support async applications as opposed to them returning data.

local api = vim.api
local server = vim.lsp.server({
  capabilities = {
    hoverProvider = true
  },
  handlers = {
    ---@param params lsp.HoverParams
    ["textDocument/hover"] = function(_, params, callback)
      local bufnr = vim.uri_to_bufnr(params.textDocument.uri)
      local lnum = params.position.line
      local col = params.position.character
      -- use bufnr/lnum/col to provide actual text
      callback({
        contents = {
          kind = "plaintext",
          value = "just some dummy text",
        }
      })
    end,
  }
})
vim.lsp.start({ name = "dummy-ls", cmd = server })

@mfussenegger
Copy link
Member Author

mfussenegger commented Jul 15, 2023

I'm pretty confident we need to pass a callback to the handlers to better support async applications as opposed to them returning data.

It could run in a coroutine to allow yielding.
The same approach is already used in global handlers - when a server sends a request and the client returns a response.
Downside is that it requires some yield/resume boilerplate if the components used internally lack coroutine awareness. But if they do, it would avoid callback nesting.

For example, if we extended vim.system with something like this:

--- a/runtime/lua/vim/_system.lua
+++ b/runtime/lua/vim/_system.lua
@@ -22,10 +22,11 @@ local uv = vim.uv
 --- @field handle uv_process_t
 --- @field timer uv_timer_t
 --- @field pid integer
 --- @field timeout? integer
 --- @field done boolean
+--- @field co thread
 --- @field stdin uv_stream_t?
 --- @field stdout uv_stream_t?
 --- @field stderr uv_stream_t?
 --- @field cmd string[]
 --- @field result? SystemCompleted
@@ -78,10 +79,20 @@ local MAX_TIMEOUT = 2 ^ 31
 --- @param timeout? integer
 --- @return SystemCompleted
 function SystemObj:wait(timeout)
   local state = self._state

+  if state.done then
+    return state.result
+  end
+
+  local co, is_main = coroutine.running()
+  if co and not is_main then
+    state.co = co
+    -- TODO: spawn timer for timeout?
+    return coroutine.yield()
+  end
+
   vim.wait(timeout or state.timeout or MAX_TIMEOUT, function()
     return state.done
   end)

   if not state.done then
@@ -294,14 +305,16 @@ function M.run(cmd, opts, on_exit)
         code = code,
         signal = signal,
         stdout = stdout_data and table.concat(stdout_data) or nil,
         stderr = stderr_data and table.concat(stderr_data) or nil,
       }
-
       if on_exit then
         on_exit(state.result)
       end
+      if state.co then
+        coroutine.resume(state.co, state.result)
+      end
     end)
   end, function()
     close_handles(state)
   end)

One could write code as if synchronous without blocking and without the 2-color problem of explicit async/await

@lewis6991
Copy link
Member

lewis6991 commented Jul 15, 2023

I don't think there's any need for that and just over complicates things as there are too many edge cases that need to be considered with coroutines. Plus an async interface is already planned (#19624) so it should just be handled with that. All we need here is the handlers to have access to a callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants