feat: add args.client for LspAttach callback

[adoyle-h]

Many LSP modules (aerial, navic, lsp-format) need client as on_attach function argument. It is convenient that returns args.client for users.

callback = function(args)
  -- local client = vim.lsp.get_client_by_id(args.data.client_id)
  local client = args.client
  require('aerial').on_attach(client, args.buf)
end,

[gpanders]

Client ID is already passed in the data field (which is where arbitrary data should go, not at the top level). We discussed passing the client directly but decided to stick with the client ID because the client object is not always serializable (important for RPC clients).

[adoyle-h]

@gpanders

Return client directly doesn't affect RPC clients. The data.client_id is not removed in the PR.

client = client,
data = { client_id = client.id },

client field could be skipped for RPC and used for native. RPC clients can get clientId from data.client_id. Native user can get client from client field.

[glepnir]

Maybe some examples at the beginning use the on_attach field? . So these plugins use the on_attach field? It's not the only way. They can totally do without on_attach.

[adoyle-h]

@glepnir

These plugins do not use on_attach field. They just need client and buffer number. For example, see aerial. But LspAttach callback only pass the clientId not client object.

And the usage of on_attach follows its definition. We don't need another way.

https://github.com/neovim/neovim/blob/e6f7e038b8bbca487e78ebfc6fe21d6852330623/runtime/doc/lsp.txt#L641-L642

[glepnir]

there has api can get client object by id . is it not enough ?

We discussed passing the client directly but decided to stick with the client ID because the client object is not always serializable (important for RPC clients).

[adoyle-h]

there has api can get client object by id . is it not enough ?

Every plugin use LspAttach autocmd should call vim.lsp.get_client_by_id(args.data.client_id) to get client. It is redundant.

It is convenient that nvim_exec_autocmds pass the client.

[clason]

Your convenience, at the cost of our inconvenience. Two lines instead of one does not justify the additional implementation, documentation, maintenance, and testing effort.

[adoyle-h]

@clason I will try to cover all these costs. Current PR has bug. I will write test cases and submit another PR.

I think the problem is not the costs, but whether LspAttach return client directly is acceptable or not in design.

[clason]

I think the problem is not the costs, but whether LspAttach return client directly is acceptable or not in design.

And whether it is depends exactly on the cost...

But the cost may have changed since LspAttach was implemented (due to changes to core), so it's indeed reasonable to revisit this decision now.

But the point that this needs to be a member of data, not its own top-level field, remains.

[adoyle-h]

But the cost may have changed since LspAttach was implemented (due to changes to core), so it's indeed reasonable to revisit this decision now.

So where to talk about the decision? Is there any relevant issue?

But the point that this needs to be a member of data, not its own top-level field, remains.

Yes, thanks for reminding me. I'm reading the codes.

[gpanders]

I had to go back and revisit some of the discussion around this. There was an issue with serializability for the client object, but that should not be a real problem in practice since Lua autocommand callbacks are only usable from Lua, not from RPC clients.

So it looks like a (small) mistake on my part. Passing the client object directly is the better design choice, I think. Thanks @adoyle-h for bringing this up so we could re-visit it.

[github-actions[bot]]

This PR has been closed since a request for changes has not been answered for 30 days. It can be reopened when the requested changes are provided.