bug(lua_ls): incorrect root_dir due to early return when root_files is found which ignore longer root_git (and root_lua)

[mangkoran]

Description

The issue is similar to #3165 but now it seems it's caused by root_files. The issue occurs with the following directory structure:

HOME/
├─ workspaces/ # `root`, actual `root_dir`
│  ├─ stylua.toml # one of `root_files`
│  ├─ lua/
│  ├─ python/
│  ├─ work/
│  │  ├─ SomeProjectWithLuaFiles/ # `root_git`, expected `root_dir`
│  │  │  ├─ .git/

Whenever I open a file under ../SomeProjectWithLuaFiles/, I expect root_dir is set to ../SomeProjectWithLuaFiles/. However somehow root_dir is set to HOME/workspaces/. After quick investigation, it seems stylua.toml makes the LSP set root_dir to HOME/workspaces/ because after I removed stylua.toml the root_dir is now ../SomeProjectWithLuaFiles/. Looking at lua_ls config, it seems this is caused by early return from the following lines:

nvim-lspconfig/lua/lspconfig/configs/lua_ls.lua

Lines 19 to 21 in 3cb6c05

	if root and root ~= vim.env.HOME then
	return root
	end

#3322 suggest setting root_dir to the longer root path if both a .git- and a lua-ancestor are found, which is not satisfied in this case.

Please let me know if this is confirmed to be an issue. I am happy to help with a PR.

[justinmk]

Thanks for the very clear report.

#3322 suggest setting root_dir to the longer root path if both a .git- and a lua-ancestor are found, which is not satisfied in this case.

Good point. Are you suggesting that we remove or reorder the condition, or what exactly?

Glancing at the logic, it does seem strange that we try root_files first and then special-case only root_lua and root_git after that.

Also, if "choose the longest (i.e. innermost) path" is the right approach, why don't we do that for all configs instead of only this one?

What are your thoughts @pitkling @mangkoran

[mangkoran]

Thank you for your reply.

Good point. Are you suggesting that we remove or reorder the condition, or what exactly?

I'm thinking of comparing these 3 (root, root_lua, root_git) and pick the longest one. Currently the comparison is only for root_lua and root_git, which will also be skipped if root is found.

Also, if "choose the longest (i.e. innermost) path" is the right approach, why don't we do that for all configs instead of only this one?

If by "all configs" you mean all LSP configs, honestly I'm not really sure if this is also applicable for all configs as I don't use every LSP in this repo. It's just coincidence that this issue happened when I used lua_ls. Although from my opinion, the longest/innermost path could be the most suitable as it's the most "specific" path. Perhaps nvim-lspconfig team have more capacity to answer the question better.

For this particular issue/lsp, my suggestion has been stated above. What do you think?

[pitkling]

Good point. Are you suggesting that we remove or reorder the condition, or what exactly?

I'm thinking of comparing these 3 (root, root_lua, root_git) and pick the longest one. Currently the comparison is only for root_lua and root_git, which will also be skipped if root is found.

Also, if "choose the longest (i.e. innermost) path" is the right approach, why don't we do that for all configs instead of only this one?

[…] I'm not really sure if this is also applicable for all configs as I don't use every LSP in this repo. […] Although from my opinion, the longest/innermost path could be the most suitable as it's the most "specific" path. Perhaps nvim-lspconfig team have more capacity to answer the question better. […]

I completely agree with @mangkoran here. From my experience with Lua, I would also expect that the LSP simply takes the longest among all candidate paths as root dir. So I think the suggested change makes sense for the Lua LSP.

And similarly, while I personally think it's probably a very good approximation for almost all LSPs to use the longest path as root dir if there are multiple candidates (due to different types of indicator files), I could imagine that

• maybe there are some languages where this is not intended and
• even if not, making this the default for all LSPs will likely break some people's configs.

So not sure whether the (maybe) better default behavior is worth the potential problems it might cause to change this for all LSPs.

[lithammer]

A counter point for longest path is rust-analyzer where you most likely want to use the top-level Cargo.toml file as root (i.e. shortest path). In this case, it has custom logic so it's not an issue in practice. Another one is gopls, but it's using a different marker in those cases (go.work vs go.mod).

[mangkoran]

@justinmk @lithammer I think for now we could fix this for lua_ls first while we evaluate for other LSP (maybe in separate ticket too?). What do you think?

[lithammer]

Sounds reasonable to me.

[dundargoc]

Whenever I open a file under ../SomeProjectWithLuaFiles/

What does this even mean? Is the .. supposed to be the parent directory?

[mangkoran]

What does this even mean? Is the .. supposed to be the parent directory?

Yes it's supposed to be parent directory. It's just to cut short/abbreviate the full parent directory path 😅

You could refer to the tree diagram for the full directory structure.

[dundargoc]

Yes it's supposed to be parent directory. It's just to cut short/abbreviate the full parent directory path

Oh, as in ellipsis. So .../SomeProjectWithLuaFiles, not ../SomeProjectWithLuaFiles. OK, gotcha.