feat(lsp): support function for client root_dir

[gpanders]

If root_dir is a function it is evaluated when the client is created to determine the root directory.

This enables dynamically determining the root directory based on e.g. project or directory structure (example: finding a parent Cargo.toml file that contains "[workspace]" in a Rust project).

[mfussenegger]

I don't really understand the use-case here. The function would be evaluated within the lsp.start call, which means whoever calls lsp.start could call the root-dir function themselves and they'd have the same kind of evaluation ordering.
What am I missing?

I'd understand this more if it turned root_markers from lsp.config into a function, because there the evaluation would be deferred from definition to on-use.

[justinmk]

Does this resolve neovim/nvim-lspconfig#1827 ?

[lewis6991]

Just to mention, we're exploring the idea on matrix of adding workspace folders instead of a root_dir function.

[gpanders]

Just to mention, we're exploring the idea on matrix of adding workspace folders instead of a root_dir function.

Adding a workspace folder in an on_init function seems to work ok at first glance. Example (for a Rust project):

on_init = function(client)
    vim.system({"cargo", "metadata", "--no-deps", "--format-version", "1"}, {
        cwd = client.root_dir,
    }, function(out)
        if out.code ~= 0 then
            return
        end

        local workspace_root = vim.json.decode(out.stdout).workspace_root
        if workspace_root then
            client:_add_workspace_folder(workspace_root)
        end
    end)
end

The caveat here is that the client will now have 2 workspace folders: one is the workspace root found by on_init (good), the other is the initial root directory when the client was first created. I'm not sure what the consequences of this might be.

To make this more concrete, if I have the following project layout:

root/
  rust/
    Cargo.toml  <-- workspace root
    foo/
      Cargo.toml
      src/
        lib.rs
    bar/
      Cargo.toml
      src/
        lib.rs

If I first open rust/foo/src/lib.rs then my LSP client will have workspace folders root/rust/foo and root/rust. The presence of root/rust/foo as one of the workspace folders is what I'm unsure about, because now even if I open rust/bar/src/lib.rs, the foo directory is still one of the workspace folders, even though this source file is not part of the foo directory.

The definition of "workspace folders" per the spec seems to imply that each folder should be an individual project root, which is not the case in the example above.

So having a "dynamic" root_dir function that can set the client's root directory to the workspace still seems like it might be worth doing.

[mfussenegger]

Mentioned this in chat - posting here for the record too:

I think for dynamic use cases the options are:

1. Function for root_dir or root_markers in lsp.config, because there the is an actual gap between time of definition (init.lua) and evaluation (ftplugin).
2. Do nothing and point users to vim.lsp.start

lsp.start shouldn't support a function root_dir, because you'd basically have a:

local function start(root_dir)
  if type(root_dir) == "function" then
    root_dir = root_dir()
  end
end

Only to support doing a start(root_dir_func) instead of a start(root_dir_func()).
In both cases the evaluation happens at the same time, and there is no later point to which you can defer the evaluation to, because the function immediately needs the string value.

A example for 2)

local cmd = {"cargo", "metadata", "--no-deps", "--format-version", "1"}
vim.system(cmd, {}, function(out)
  if out.code ~= 0 then
    return
  end
  local workspace_root = vim.json.decode(out.stdout).workspace_root
  vim.lsp.start({
    root_dir = workspace_root,
    ...
  })
end)

For 1)

One question is if the root_dir function needs to take a callback, or if it runs within a coroutine to support yielding values to make the sync case easy and the async case possible.

[lewis6991]

One question is if the root_dir function needs to take a callback, or if it runs within a coroutine to support yielding values to make the sync case easy and the async case possible.

For the sync case the function can return the value directly. For the async case, the function must return nil and call the callback, at some point.

[mfussenegger]

For the sync case the function can return the value directly. For the async case, the function must return nil and call the callback, at some point.

Sounds good. Might be a bit odd for when nil is the result - which is a valid value for root_dir given that there are servers that don't require a workspace. Not a dealbreaker, just wanted to have mentioned it.

[lewis6991]

You're right. Ehh, this is getting complicated. I guess we can use some other sentinel value for either case. Maybe returning false means wait for callback. It's a bit weirder but keeps the sync case simple.

[gpanders]

One question is if the root_dir function needs to take a callback, or if it runs within a coroutine to support yielding values to make the sync case easy and the async case possible.

Can we do neither and just force it to be sync?

[mfussenegger]

Can we do neither and just force it to be sync?

Given that the cargo metadata cmd came up as a first use-case I think it should support async.

But I'd tend to either:

1. Limit to callback only, instead of combination with return values:

vim.lsp.config("clangd", {
  root_dir = function(on_root_dir)
    local cmd = {"cargo", "metadata", "--no-deps", "--format-version", "1"}
    vim.system(cmd, {}, function(out)
    if out.code == 0 then
      local workspace_root = vim.json.decode(out.stdout).workspace_root
      on_root_dir(workspace_root)
    else
      on_root_dir(nil)
    end
  end,
})

For synchronous cases it is not so bad either, given that a return myvalue turns into on_root_dir(myvalue)

2. Support it via coroutine

vim.lsp.config("clangd", {
  root_dir = function()
    local cmd = {"cargo", "metadata", "--no-deps", "--format-version", "1"}
    local co = coroutine.running()
    vim.system(cmd, {}, function(out)
      local root = out.code == 0 and vim.json.decode(out.stdout).workspace_root or nil
      coroutine.resume(co, root)
    end)
    return coroutine.yield()
  end,
})

With a future vim.async.resume() (or different name) it could look like:

vim.lsp.config("clangd", {
  root_dir = function()
    local cmd = {"cargo", "metadata", "--no-deps", "--format-version", "1"}
    vim.system(cmd, {}, vim.async.resume())
    local out = corotuine.yield()
    return out.code == 0 and vim.json.decode(out.stdout).workspace_root or nil
  end,
})

Where vim.async.resume() is something like:

---@param co? thread
function vim.async.resume(co)
  co = co or coroutine.running()
  return function(...)
    if coroutine.status(co) == "suspended" then
      coroutine.resume(co, ...)
    else
      local args = {...}
      vim.schedule(function()
        assert(
          coroutine.status(co) == "suspended",
          "Incorrect use of coresume. Callee must have yielded"
        )
        coroutine.resume(co, unpack(args))
      end)
    end
  end
end

This would require a coroutine.wrap within the logic triggering the root function. Upside is that synchronous functions could work with a regular return myvalue.

[lewis6991]

I'm quite strongly against 2. IMO a function should never yield from a coroutine unless it knows where that coroutine was started and what for. The current running coroutine may have requirements about what should be yielded. Additionally some distros build with 5.2 features which makes coroutine.running,() always return a thread. Likewise we should not do any first class integration with a future vim.async, and should allow any async lib to work with this.

vim.async will be designed entirely around wrapping other functions that have continuation parameters.

I don't see any other option other than 1, which is simple and meets the requirements.

To keep the sync case simple the root_dir function can return a sentinel value to indicate that the callback is expected to be called. This will allow us to add a timeout (or not).

[mfussenegger]

IMO a function should never yield from a coroutine unless it knows where that coroutine was started and what for.

Note that we already do this:

af204dd

Although in that case it was a bit more forced because it was the only way to support async operations for reverse-request handlers without breaking BWC.

. Likewise we should not do any first class integration with a future vim.async, and should allow any async lib to work with this.

Can you elaborate on that? I don't see how this pattern would break down with other async libraries - as the first example shows it doesn't even require a vim.async. It's based solely on the contract that

a) it runs within a coroutine and
b) it must yield if it wants to resume, and the resume must happen after the yield. (vim.async.resume() in my example takes care of the latter being always the case via a vim.schedule()). But this is generally the case with any coroutine use - yield and resume must always form a pair.

That said, I don't have a strong opinion on this, I merely wanted to put the option on the table and if there are some issues with it, it would be good to learn about them because I use the patttern in nvim-dap quite a bit and so far I didn't notice any issues (and I think nvim-dap-ui even uses a different async approach on top - making me think that if the approaches were conflicting I should've noticed by now).

[lewis6991]

Can you elaborate on that? I don't see how this pattern would break down with other async libraries - as the first example shows it doesn't even require a vim.async. It's based solely on the contract that

All the different async libs that exist at the moment have certain requirements on what can be yielded.

• With https://github.com/lewis6991/gitsigns.nvim/blob/main/lua/gitsigns/async.lua coroutines, the user must yield a function with the number of arguments.
• https://github.com/nvim-neotest/nvim-nio is similar

Both these async libs require uv functions are executed on the main thread, so the callback can continue within the coroutine.