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(lsp): use the same lsp server for filetype if it exists #2287

Merged
merged 1 commit into from
Dec 4, 2022

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Dec 3, 2022

fix #2285

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

I don't know if there is a spec or something to follow, this is just guessing what vscode does.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

Helix handles this, so I got curious whats the trick, turns out there is no trick, they never spawn an lsp server in response to a goToDefinition request, which now I think about actually make sense. Why do we have that code in the first place, are there actual use cases ?

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

If we take a step back, theoretically in response to a gotodefiniton, the lsp server that sent us there should be the one to continue to work

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

It might be only really solvable at neovim level though

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 3, 2022

but helix has a sanity check to compare the original lsp that sent us id with the one that we found https://github.com/helix-editor/helix/blob/bcdb475b71b0fbabce57344ac8d2575c23b1bbe0/helix-view/src/editor.rs#L958 and it doesn't spawn a different filetype server in any case

This can only be done inside nvim itself

-- Check if there is an active server already for this same file type
local active_clients_list = util.get_active_clients_list_by_ft(vim.bo[bufnr].filetype)
if #active_clients_list ~= 0 then
id = active_clients_list[1].id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there can use index 1 if there has multiple clients for this filetype . like efm tsserver for ts file the 1 can be efm or tsserver .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the original lsp client id at this point, so we can't tell which client to choose

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to remove root_dir in lsp start client in core. maybe can solve this. there is no root_dir definition in lsp doc. use wrokspace folder would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a correct approach , helix also have this concept , (emacs as well?)

I'm not sure what vscode does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked a bit , seems like vscode sets the root workspace as root but it have more logic to add more workspaces (vscode can handle multiple workspaces at the time)

Honestly I can't answer here as I'm not familiar that much with lsp protocol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that works I updated the pr with your code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need add the new root dir into the workspaceFolder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure when I tested with jonhoo original rust problem your snippet fixes it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe them belong to one workspace.

Copy link
Contributor

@xiantang xiantang Dec 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe them belong to one workspace.

NO, they belong different workspace when you try to open another project file

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.

go-to-definition spawns new LSP server with root at definition
3 participants