[reverted] fix(lsp): should support vim.lsp.Config.root_dir as fun, not only string|nil

[atusy]

This PR improves type annotation of vim.lsp.Config.root_dir by allowing function as documented in https://neovim.io/doc/user/lsp.html#lsp-root_dir().

The fix also required changes in vim.lsp._get_workspace_folders and :checkhealth vim.lsp.

I ran :cheackhealth vim.lsp and confirmed root_dir as function works in monorepo with ts_ls.

vim.lsp: Active Clients ~
- ts_ls (id: 2)
  - Version: ? (no serverInfo.version response)
  - Root directory: ~/ghq/github.com/xxx/xxx/xxx
  - Command: { "typescript-language-server", "--stdio" }
  - Settings: {}
  - Attached buffers: 1
- ts_ls (id: 3)
  - Version: ? (no serverInfo.version response)
  - Root directory: ~/ghq/github.com/xxx/xxx/yyy
  - Command: { "typescript-language-server", "--stdio" }
  - Settings: {}
  - Attached buffers: 2

[MariaSolOs]

@atusy you need to run make doc to update the text files.

[atusy]

thanks! I will

[atusy]

I also need to fix type inconsistencies...

I will make some time later.

  local config_folders = lsp._get_workspace_folders(config.workspace_folders or config.root_dir)

[atusy]

Okay, seems like everything is clear.

[justinmk]

Are you able to write a test to exercise this

[atusy]

I'll try

[atusy]

@justinmk
I added test cases to vim.lsp.start which relies on the unexported _get_workspace_folder.

[justinmk]

need a similar vim.wait approach here, in case root_dir is async.

[justinmk]

forgot return here. i will fix this in a push.

[justinmk]

if there are many attached_buffers all resolving to the same root, this will show many redundant root dirs.

i will fix this in a push.

[atusy]

thanks a lot!

[mfussenegger]

Isn't the annotation change sort of wrong?

While lsp.config() can handle a root_dir as a function (vim.lsp.Config), once the client is initialized and started it must be resolved, because it gets used. So vim.lsp.ClientConfig doesn't have to support the function case.

Resolving happens in:

neovim/runtime/lua/vim/lsp.lua

Lines 503 to 508 in c9b74f8

	config.root_dir(bufnr, function(root_dir)
	config.root_dir = root_dir
	vim.schedule(function()
	start_config(bufnr, config)
	end)
	end)

With this change it is implied that the root_dir on a vim.lsp.Client can also be a function, which is unnecessary and would also be a breaking change.

[justinmk]

once the client is initialized and started it must be resolved, because it gets used.

true, and very important.
now I am wondering about other parts of this entire PR.

[mfussenegger]

now I am wondering about other parts of this entire PR.

On a closer look, none of the changes here make sense to me but maybe I'm missing the problem

[atusy]

The change I made is related to switching code formatters per project.
I will explan more later, although my explanation may not justify the change. I mean there might be an alternative.

[atusy]

I think @mfussenegger 's suggestion make sense. vim.lsp.Config.root_dir should not be a function, and we should fix the case where vim.lsp.Config.root_dir becomes function.

Just for clarifications, let me explain the backgrounds to open this PR.

I am switching formatter by looking at files in the root_dir.
Recently, ts_ls started to return vim.lsp.Client with root_dir as a function.

-- This used to be string|nil, but recently it became function.
local root_dir = vim.lsp.get_clients({name = "ts_ls", bufnr = vim.api.nvim_get_current_buf()})[1].root_dir

I was surprised because LSP showed vim.lsp.Client.root_dir is not a function.
That is why I made a PR.

[mfussenegger]

Recently, ts_ls started to return vim.lsp.Client with root_dir as a function.

Sounds like there is a call to vim.lsp.start() with a config where root_dir isn't resolved. That's not allowed.
The proper fix would be to tighten the validation to ensure it results in an error.

[mfussenegger]

One thing to add here: We could change vim.lsp.start() to resolve root_dir. It feels a bit strange to me because the evaluation can't be deferred further but needs to happen right away but it would make the vim.lsp.start(vim.lsp.config.myserver) use-case a bit more convenient. So far users need to do something like this (untested):

local conf = vim.lsp.config.myserver
if type(conf.root_dir) == "function" then
  conf.root_dir(bufnr, function(root_dir)
    conf.root_dir = root_dir
    vim.lsp.start(conf)
  end)
else
  vim.lsp.start(conf)
end

What we shouldn't do because it is breaking is the example you gave with:

-- This used to be string|nil, but recently it became function.
local root_dir = vim.lsp.get_clients({name = "ts_ls", bufnr = vim.api.nvim_get_current_buf()})[1].root_dir

The exposed root_dir from the client must not become a function to avoid breaking plugins which use the root_dir property. (And also because it would be nonsense to have to re-evaluate it on each use when it was already evaluated)

But there's also still in the air to revert root_dir being a function on vim.lsp.Config altogether, and instead make the whole config a function, then you'd have:

local conf = vim.lsp.config.myserver
if type(conf) == "function" then
  conf(bufnr, function(fullconf)
    vim.lsp.start(fullconf)
  end)
else
  vim.lsp.start(conf)
end

Or maybe something like:

vim.lsp.get_config(function(conf)
  vim.lsp.start(conf)
end)