feat(lsp): support connect via named pipes/unix domain sockets #26032

TheLeoP · 2023-11-13T18:05:38Z

This would solve #26031.

This PR is functional, but (since this is my first PR to the project), I would like to receive feedback regarding things like:

~~Should I check possibly nil values, if yes, which is the appropiate way to show this erros to the user (is using error preffered over something like vim.notify using the error log level?).~~
Suggestions for naming the new function (maybe also renaming vim.lsp.rpc.connect to something regarding tcp?)
The policy regarding lua typings (is a specific type like fun(dispatchers: Dispatchers): StartServer? prefered over a general one like function? Is it the oposit way because of how docs are generated?) (as fas as I can tell, neither CONTRIBUTING.MD nor :help dev-doc-lua talk about this).
I'm using vim.schedule on the write transport as a workaround because, on my tests using Powershell Editor Services, Neovim sended the initialization requets before the pipe connection was stablished. I think that, ideally, public_client(client) should be returned from inside the callback on pipe:connect, but I'm not sure if I'm using the previously existing functions in an unexpected way. (Since I'm using vim.schedule here, I also had to add it to the test)

Edit:
~~I'm also not sure about how to test if the method received is initialize because of the vim.schedule thing :/~~
~~Turns out that busted supports async tests~~
Nevermind, it looks like async test aren't supported currently

MariaSolOs · 2023-11-13T21:25:38Z

The policy regarding lua typings (is a specific type like fun(dispatchers: Dispatchers): StartServer? prefered over a general one like function?

There's nothing written in stone about it, but personally I believe that the more accurate the type annotations, the better (and so I prefer fun(param: paramType): returnType).

dundargoc · 2023-11-13T21:33:32Z

The doc generation becomes kinda goofy when using fun(param: paramType): returnType though because it has not been written to handle that case.

runtime/lua/vim/lsp/rpc.lua

MariaSolOs · 2023-11-13T21:29:40Z

runtime/lua/vim/lsp/rpc.lua

+--- Unix and name on Windows)
+---
+---@param pipe_path  string
+---@return fun(dispatchers: Dispatchers): StartServer?


Feels a bit odd to have a "plural" type like Dispatchers. I find more intuitive to have a singular vim.rpc.Dispatcher type and make this parameter a table of those.

I took these types from the private dispatchers that are passed to this function by vim.lsp.start_client. Not all of them share the same signature, so this type can't be an array of vim.lsp.rpc.Distpacher, what do you suggest for this case?

runtime/lua/vim/lsp/rpc.lua

test/functional/plugin/lsp_spec.lua

mfussenegger · 2023-11-15T14:26:18Z

runtime/lua/vim/lsp/rpc.lua

+---
+---@param pipe_path string file path of the domain socket (Unix) or name of the named pipe (Windows) to connect to
+---@return fun(dispatchers: vim.lsp.rpc.PrivateDispatchers): vim.lsp.rpc.StartServer? #function intended to be passed to |vim.lsp.rpc.start_client| or |vim.lsp.start| on the field cmd
+function M.domain_socket_connect(pipe_path)


Maybe something like:

Suggested change

function M.domain_socket_connect(pipe_path)

function M.connect_to_pipe(pipe_path)

Merging it into connect could also be an option, if we change the args to opts and distinguish based on the keys. With a type(opts) == "table" check we could even make it backward compatible (if it is a string, it's a host name)

cc @justinmk do you have inputs on this?

mfussenegger · 2023-11-23T16:33:11Z

@TheLeoP could you also add an entry to the news.txt?

@MariaSolOs Do you have any other inputs on this? Otherwise I'd go ahead and merge this. We can still follow up with renaming the method before it get's released.

runtime/lua/vim/lsp/rpc.lua

MariaSolOs · 2023-11-23T17:44:24Z

@MariaSolOs Do you have any other inputs on this? Otherwise I'd go ahead and merge this. We can still follow up with renaming the method before it get's released.

Left some minor comments but it LGTM.

TheLeoP · 2023-11-24T15:38:59Z

Is the entry on news.txt ok?

runtime/doc/news.txt

mfussenegger · 2023-12-08T16:32:34Z

@TheLeoP it looks like some of the test failures are introduced by this PR. Could you take a look?

TheLeoP · 2023-12-08T21:55:15Z

@mfussenegger Sorry, I hadn't realized. I'll look into it.

mfussenegger · 2023-12-22T11:16:26Z

@TheLeoP sorry I haven't realized that you had fixed the test failures. Could you resolve the conflicts and then ping me, I'll then merge the PR

TheLeoP · 2023-12-23T05:45:57Z

@mfussenegger done :3. The current failling test may have something to do with the fact that I rebased in top of master? Because I dind't touch any c code in this PR

wookayin

This looks like a great feature. I've left some minor style suggestion comments. You'll also need to fix linting errors as checked by CI.

wookayin · 2023-12-23T05:02:44Z

runtime/lua/vim/lsp/rpc.lua

---@param callback fun(err: lsp.ResponseError|nil, result: any) Callback to invoke
---@param notify_reply_callback (function|nil) Callback to invoke as soon as a request is no longer pending
---@return boolean success, integer|nil request_id true, request_id if request could be sent, `false` if not
+---@param params (table?) Parameters for the invoked LSP method


Suggested change

---@param params (table?) Parameters for the invoked LSP method

---@param params (table|nil) Parameters for the invoked LSP method

And for other many places: see https://github.com/neovim/neovim/blob/master/CONTRIBUTING.md#lua-docstrings

I thought that using this syntax for optional values was preffered over the old |nil. Is it not?

Throughout neovim codebase, table|nil is used instead of table? But if you meant an optional parameter, it should be @param params? table (without nil).

Throughout neovim codebase, table|nil is used instead of table?

Remember that Neovim's codebase is vast and written over years. Guidance can change without wanting to add churn by rewriting all old code. Nevertheless, new(ly touched) code should follow current guidance (:h develop; I don't think we've explicitly formalized this specific aspect -- @lewis6991 @mfussenegger @gpanders @MariaSolOs ?).

In this specific case @param params? table is indeed correct (no parens!)

runtime/lua/vim/lsp/rpc.lua

runtime/doc/lsp.txt

runtime/lua/vim/lsp/rpc.lua

TheLeoP · 2023-12-23T06:14:47Z

This looks like a great feature. I've left some minor style suggestion comments. You'll also need to fix linting errors as checked by CI.

The link problem is because I used a commit message that is not valid for the CI. Is it possible to squash all the commits into a single commit when merging to handle this? I'm not very well versed with git, so I didn't know of other ways of modifying past commit messages.

@wookayin

wookayin · 2023-12-23T06:51:07Z

Is it possible to squash all the commits into a single commit when merging to handle this? I'm not very well versed with git, so I didn't know of other ways of modifying past commit messages.

You can squash the commits into one using "interactive rebase". And then push-force.

https://github.com/neovim/neovim/blob/master/CONTRIBUTING.md#pull-requests-prs

clason · 2023-12-23T11:23:25Z

But we can also squash-merge if the PR should just be a single commit.

runtime/lua/vim/lsp/rpc.lua

TheLeoP · 2023-12-23T14:45:19Z

I think it's ok for this PR to be a single feature commit, but I could also manually squash commits if it's necessary. Is there something else that should be adressed?

wookayin · 2024-01-02T13:27:07Z

The generated vimdoc still contains some mess and incorrect formatting due to the LuaCATS-vimdoc incompatibility, but this something gen_vimdoc is responsible for so let's clean up that as a future work. Anyway nice feature!

github-actions bot added the lsp label Nov 13, 2023

github-actions bot requested review from folke, MariaSolOs and mfussenegger November 13, 2023 18:08

MariaSolOs reviewed Nov 13, 2023

View reviewed changes

mfussenegger reviewed Nov 15, 2023

View reviewed changes

MariaSolOs approved these changes Nov 23, 2023

View reviewed changes

runtime/lua/vim/lsp/rpc.lua Outdated Show resolved Hide resolved

runtime/lua/vim/lsp/rpc.lua Outdated Show resolved Hide resolved

runtime/lua/vim/lsp/rpc.lua Show resolved Hide resolved

clason reviewed Nov 24, 2023

View reviewed changes

runtime/doc/news.txt Outdated Show resolved Hide resolved

mfussenegger mentioned this pull request Dec 9, 2023

Support vim.lsp.start_client parameter to avoid uv.spawn(...) #15847

Closed

wookayin reviewed Dec 23, 2023

View reviewed changes

lewis6991 reviewed Dec 23, 2023

View reviewed changes

runtime/lua/vim/lsp/rpc.lua Outdated Show resolved Hide resolved

TheLeoP added 6 commits January 2, 2024 09:04

feat(lsp): support connect via named pipes/unix domain sockets

6c57de4

test(lsp): vim.lsp.rpc.domain_socket_connect

f48943a

chore: delete unnecessary vim.print on test

510f21f

docs(lsp): improve lua-docs rpc.connect and rpc.domain_socket_connect

2083d0c

feat(lsp): log errors on rpc.conect and rpc.domain_socket_connect

930f023

test(lsp): make domain_socket_connect test async

eda1fd6

TheLeoP and others added 13 commits January 2, 2024 09:04

doc(lsp): fix links on rpc.lua

298cf49

docs: update generated docs

3c276ba

docs: re-add info to news.txt

8fc2320

docs: fix tags references

0b3fdfa

docs: generate docs

63d6b3a

docs(lsp rpc): remove redundant Rpc on type names

2d842c1

docs(lsp): remove unnecesary parens from type annotations

f9ec70c

docs(lsp): use ? in param name, not type

4029383

docs(lsp): do not use ? for return values

adabff3

docs(lsp): remove space before #

8c53ef7

docs(lsp): add reference to vim.lsp.protocol.ErrorCodes

b3750e2

docs(lsp): add autogenerated docs

dcc6579

fixup! docs(lsp): add autogenerated docs

07b4471

mfussenegger merged commit 3f788e7 into neovim:master Jan 2, 2024
24 of 25 checks passed

github-actions bot removed the request for review from folke January 2, 2024 09:08

wookayin mentioned this pull request Jan 10, 2024

fix(lsp): fix incorrect typing and doc for vim.lsp.rpc #26971

Merged

wookayin added this to the 0.10 milestone Jan 10, 2024

616b2f mentioned this pull request Apr 4, 2024

vscode-csharp - new lsp server neovim/nvim-lspconfig#2657

Closed

TheLeoP mentioned this pull request Apr 25, 2024

refactor(lsp): merge rpc.domain_socket_connect into rpc.connect #28398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lsp): support connect via named pipes/unix domain sockets #26032

feat(lsp): support connect via named pipes/unix domain sockets #26032

TheLeoP commented Nov 13, 2023 •

edited

Loading

MariaSolOs commented Nov 13, 2023

dundargoc commented Nov 13, 2023

MariaSolOs Nov 13, 2023

TheLeoP Nov 13, 2023 •

edited

Loading

mfussenegger Nov 15, 2023

mfussenegger commented Nov 23, 2023

MariaSolOs commented Nov 23, 2023

TheLeoP commented Nov 24, 2023

mfussenegger commented Dec 8, 2023

TheLeoP commented Dec 8, 2023 •

edited

Loading

mfussenegger commented Dec 22, 2023

TheLeoP commented Dec 23, 2023

wookayin left a comment •

edited

Loading

wookayin Dec 23, 2023

TheLeoP Dec 23, 2023

wookayin Dec 23, 2023

clason Dec 23, 2023 •

edited

Loading

TheLeoP commented Dec 23, 2023 •

edited

Loading

wookayin commented Dec 23, 2023

clason commented Dec 23, 2023

TheLeoP commented Dec 23, 2023

wookayin commented Jan 2, 2024

	function M.domain_socket_connect(pipe_path)
	function M.connect_to_pipe(pipe_path)

	---@param params (table?) Parameters for the invoked LSP method
	---@param params (table\|nil) Parameters for the invoked LSP method

feat(lsp): support connect via named pipes/unix domain sockets #26032

feat(lsp): support connect via named pipes/unix domain sockets #26032

Conversation

TheLeoP commented Nov 13, 2023 • edited Loading

MariaSolOs commented Nov 13, 2023

dundargoc commented Nov 13, 2023

MariaSolOs Nov 13, 2023

Choose a reason for hiding this comment

TheLeoP Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

mfussenegger Nov 15, 2023

Choose a reason for hiding this comment

mfussenegger commented Nov 23, 2023

MariaSolOs commented Nov 23, 2023

TheLeoP commented Nov 24, 2023

mfussenegger commented Dec 8, 2023

TheLeoP commented Dec 8, 2023 • edited Loading

mfussenegger commented Dec 22, 2023

TheLeoP commented Dec 23, 2023

wookayin left a comment • edited Loading

Choose a reason for hiding this comment

wookayin Dec 23, 2023

Choose a reason for hiding this comment

TheLeoP Dec 23, 2023

Choose a reason for hiding this comment

wookayin Dec 23, 2023

Choose a reason for hiding this comment

clason Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

TheLeoP commented Dec 23, 2023 • edited Loading

wookayin commented Dec 23, 2023

clason commented Dec 23, 2023

TheLeoP commented Dec 23, 2023

wookayin commented Jan 2, 2024

TheLeoP commented Nov 13, 2023 •

edited

Loading

TheLeoP Nov 13, 2023 •

edited

Loading

TheLeoP commented Dec 8, 2023 •

edited

Loading

wookayin left a comment •

edited

Loading

clason Dec 23, 2023 •

edited

Loading

TheLeoP commented Dec 23, 2023 •

edited

Loading