refactor(lsp): extract resolve_bufnr to vim.lsp.util

[ofseed]

When trying to do some stuff for codelens, I found that the local function resolve_bufnr is slightly different from the others which have the same name but in other files.

It might be better if we put it into vim.lsp.utils so that we can use the name as expected.

[ofseed]

Not sure if it should be private.

[MariaSolOs]

I'm unsure about this change. Although I admit that we repeat this logic in multiple places, some LSP functions interpret the meaning of nil differently and so there's no global agreement on what resolving the buffer number is (e.g. vim.lsp.codelens.clear will treat a nil buffer parameter refreshing all buffers, while 0 means the current buffer).

[ofseed]

@MariaSolOs Thanks for pointing this out, I hadn't noticed that vim.lsp.codelens.clear also uses this function. But in my opinion, if the functions have the same name (even if they are defined in different places), it's best if they do the same thing. So far, it seems that the most useful use of this function is to treat nil as the current buffer, not all buffers.

I think the problem you mentioned is essentially due to the design of this API. The current codelens API is designed to be easily triggered manually. In this case, we may need a method to indicate that it is effective for all buffers. However, for other LSP-related APIs, such as semantic token or inlay hint, nil represents the current buffer, not all buffers. So for the consistency of the API, I even suggest changing this behavior, but this will be another breaking change, so it is better not to do it.

By the way, the stuff I am trying to work on is rewriting codelens to provide something like vim.lsp.codelens.enable like vim.lsp.inlay_hint.enable, I see vim.diagnostic made a similar change recently, so it should be acceptable, and the we have no more special case like this.

[mfussenegger]

Nothing of resolve_bufnr is specific to lsp, so it imho doesn't really fit into vim.lsp.util (which we kinda want to get rid of #25272).

If useful enough, it should be in a general purpose module - but given how trivial it is and the aspects @MariaSolOs mentioned I'm not convinced it's worth having as public API (but open to more inputs, together with suggestions where it would fit)

[mfussenegger]

I should probably also point out that its current implementation contradicts the dev guidelines:

neovim/runtime/doc/develop.txt

Line 315 in a1550db

- When accepting a buffer id, etc., 0 means "current buffer", nil means "all

[justinmk]

ome LSP functions interpret the meaning of nil differently and so there's no global agreement on what resolving the buffer number is (e.g. vim.lsp.codelens.clear will treat a nil buffer parameter refreshing all buffers, while 0 means the current buffer).

we should fix that though. having common functions helps with that. vim._util module for internal use would be better than ad-hoc duplication everywhere, to start with. and will help avoid the inconsistencies.

the preferred convention for bufnr/winnr/etc is:

• nil means "all"
• 0 means "current"

[gpanders]

the preferred convention for bufnr/winnr/etc is:

• nil means "all"
• 0 means "current"

Which means we really shouldn't need these "resolve" helper functions at all, since most (all?) API functions take 0 as "current buffer" anyway, and if bufnr is nil then the function has to handle that independently anyway.

[ofseed]

the preferred convention for bufnr/winnr/etc is:

nil means "all"
0 means "current"

If it needs to be implemented in this way, it will be convenient for users. But it's a bit hard to implement. There are a few problems we may encounter.

1. Sometimes the bufnr is not a direct parameter for these API functions, it might be wrapped in a table, e.g. opts.bufnr in vim.lsp.codelens.refresh and filter.bufnr in vim.lsp.inlay_hint.enable, should we do this in these places?
2. The current implementation of many of these functions, like vim.lsp.inlay_hint.enable, needs a lot of work to make it take effect on all buffers, because
   1. 0 or any specific number is for one buffer, this is how these functions are designed;
   2. Intuitively, "all buffers" should not only refer to "all current buffers," but also include all future buffers. For vim.lsp.inlay_hint.enable, all current buffers should be enabled, and the same for the future buffers.

Then like @gpanders said, we do not need these "resolve" helper functions at all. So the best way I can think of at the moment is, to delete all resolve_bufnr functions. If a function can handle the "all buffer" situation, then bufnr is optional; otherwise at least 0 must be passed as bufnr. Since we have lua_ls and a lot of annotations, this should not be a big problem. Then we can gradually add support for processing "all buffers" to the functions in need, or think that this function should only handle one buffer, and then mark bufnr as required instead of optional.

However, this approach maintains API consistency.

[justinmk]

So the best way I can think of at the moment is, to delete all resolve_bufnr functions. If a function can handle the "all buffer" situation, then bufnr is optional; otherwise at least 0 must be passed as bufnr

sounds like a reasonable cleanup task. could be risky so should wait until 0.10 is released in a couple weeks.