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

fix: check server supported workspaceFolders #2418

Merged
merged 10 commits into from
Jan 27, 2023
Merged

fix: check server supported workspaceFolders #2418

merged 10 commits into from
Jan 27, 2023

Conversation

glepnir
Copy link
Member

@glepnir glepnir commented Jan 23, 2023

rewrite the spawn the server instance logic.

clients cache the server instance id with root , key is root dir value is all clients id which attached this root dir .

  1. find the client from clients
  2. if find exist client check this client support workspaceFolders by
    vim.tbl_get(client, 'server_capabilities', 'workspace', 'workspaceFolders', 'supported') if true mean client support
    https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
  3. otherwise spawn a new one.

notice there has a timer for some situation .like load from session .

@glepnir glepnir marked this pull request as draft January 23, 2023 14:24
@jedrzejboczar
Copy link
Contributor

Looks like there's some infinite loop or something similar? Had to kill nvim process.

Error executing vim.schedule lua callback: .../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: attempt to index local 'client_instance' (a nil value)
stack traceback:
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: in function 'attach_or_spawn_new'
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:367: in function ''
	vim/_editor.lua: in function <vim/_editor.lua:0>
Error executing vim.schedule lua callback: .../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: attempt to index local 'client_instance' (a nil value)
stack traceback:
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: in function 'attach_or_spawn_new'
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:367: in function ''
	vim/_editor.lua: in function <vim/_editor.lua:0>
Error executing vim.schedule lua callback: .../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: attempt to index local 'client_instance' (a nil value)
stack traceback:
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: in function 'attach_or_spawn_new'
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:367: in function ''
	vim/_editor.lua: in function <vim/_editor.lua:0>
Error executing vim.schedule lua callback: .../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: attempt to index local 'client_instance' (a nil value)
stack traceback:
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:357: in function 'attach_or_spawn_new'
	.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:367: in function ''
	vim/_editor.lua: in function <vim/_editor.lua:0>
...

@glepnir
Copy link
Member Author

glepnir commented Jan 23, 2023

fixed.

@jedrzejboczar
Copy link
Contributor

jedrzejboczar commented Jan 23, 2023

Now LspInfo shows that clients are not attached to any buffers:

 Client: clangd (id: 2, bufnr: [17])
...
 
 4 active client(s) not attached to this buffer: 
 
 Client: clangd (id: 3, bufnr: [])
...
 
 Client: clangd (id: 4, bufnr: [])
...
 
 Client: clangd (id: 5, bufnr: [])
...

But there should be one client for buffers (18, 21) and one for (17, 26).

Note that I haven't changed cmd to function. But the issue is that in my setup I am changing cmd in on_new_config (it's actually reading my .nvim/lsp.json file under root directory, something similar to nlsp-settings.nvim but not only for settings). So even if I use cmd as function, then it would fail if I put different cmd_env or settings in lsp.json).

@glepnir
Copy link
Member Author

glepnir commented Jan 23, 2023

something wrong mabye I didn't test it. I will fix it tomorrow. about cmd or on_new_config it's okay . this way will spawn multiple server if server does not have workspace.workspaceFolders.supported . clangd does not have because it does not support

@glepnir
Copy link
Member Author

glepnir commented Jan 23, 2023

a small mistake .sorry..

@glepnir glepnir marked this pull request as ready for review January 24, 2023 06:57
@jedrzejboczar
Copy link
Contributor

I tried with the new version but still getting the same behavior. I tried debugging but haven't found the reason. It looks like all servers are spawned, just are not being attached to buffers. Only the first spawned clangd is attached to a buffer, others have bufnr: [] and no LSP features work in these buffers.

My test case is:

  • I have 2 project roots: /projectA and /projectB
  • Edit file /projectA/file1.c
  • Split edit file /projectB/file1.c
  • Split edit file /projectA/file2.c
  • Split edit file /projectB/file2.c

Result: there are 4 seprate clangd processes, and only the first clangd is attached to any buffer (to /projectA/file1.c).

@glepnir
Copy link
Member Author

glepnir commented Jan 25, 2023

fixed ..I forget attached it after spawn new one.

@jedrzejboczar
Copy link
Contributor

Now it works in the sense that clangd processes are attached to buffers, but they are always spawned 1 per buffer. So for example opening 4 buffers I have:

Buffers:
/projectA: 17, 44
/projectB: 24, 29

Clients:
clangd (id: 5, bufnr: [44])
clangd (id: 2, bufnr: [17])
clangd (id: 3, bufnr: [24])
clangd (id: 4, bufnr: [29])

Then when I e.g. use "go to definition" to another file a new clangd process is spawned. So for e.g. 10 open files = 10 clangd processes running.

But it should look like this:

Clients:
clangd (id: 1, bufnr: [17, 44])
clangd (id: 2, bufnr: [24, 29])

@glepnir
Copy link
Member Author

glepnir commented Jan 26, 2023

I noticed that will try to fix it later .

@glepnir
Copy link
Member Author

glepnir commented Jan 26, 2023

fixed.

1 similar comment
@glepnir
Copy link
Member Author

glepnir commented Jan 26, 2023

fixed.

@glepnir
Copy link
Member Author

glepnir commented Jan 26, 2023

notice this pr just fix the workspaceFolder field and server spawn or attach based on workspaceFolder. about your issue spawn according the on_new_config.cmd is not relate too much. but it can solve one of your issue ...

@jedrzejboczar
Copy link
Contributor

I think there is still some problem as now it looks like this:

Buffers:
/projectA: 29, 56
/projectB: 36, 41

Clients:
clangd (id: 1, bufnr: [56, 41, 29])
clangd (id: 2, bufnr: [36])

If it helps the buffers were opened in order: 29, 36, 41, 56.

notice this pr just fix the workspaceFolder field and server spawn or attach based on workspaceFolder. about your issue spawn according the on_new_config.cmd is not relate too much. but it can solve one of your issue ...

I know, it makes sense to check workspaceFolder, and I know that it just so happens that it may fix my issue. This is why I think that merging #2417 (or providing alternative solution) still makes sense.

@glepnir
Copy link
Member Author

glepnir commented Jan 26, 2023

fixed.

@jedrzejboczar
Copy link
Contributor

Thanks, now it works.

@glepnir glepnir merged commit 01185a8 into neovim:master Jan 27, 2023
@glepnir glepnir deleted the workspace branch January 27, 2023 07:57
@sibouras
Copy link

this commit broke tsserver, it spawns a new server whenever i use go to definition with tsserver, this happens only on windows, tested the same project with wsl and it works without spawning a new server

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

Successfully merging this pull request may close these issues.

None yet

3 participants