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

vim.lsp.definition race condition with multiple servers #22318

Open
antonk52 opened this issue Feb 18, 2023 · 2 comments · May be fixed by #26507
Open

vim.lsp.definition race condition with multiple servers #22318

antonk52 opened this issue Feb 18, 2023 · 2 comments · May be fixed by #26507
Labels
bug issues reporting wrong behavior complexity:low Low-risk, self-contained. Do NOT ask "can I work on this", just read CONTRIBUTING.md lsp

Comments

@antonk52
Copy link
Sponsor

Neovim version (nvim -v)

v0.8.2

Language server name/version

tsserver v0.6.4 and cssmodules-language-server v1.2.1

Operating system/version

macOS 13.0.1

Steps to reproduce using "nvim -u minimal_init.lua"

This issue can be reproduced when two language servers provide a single result for symbol.

minimal init.lua

-- install lspconfig.nvim

local function on_attach()
    vim.keymap.set('n', 'gd', vim.lsp.buf.definition)
end

local lspconfig = require('lspconfig')

lspconfig.cssmodules_ls.setup({on_attach = on_attach})
lspconfig.tsserver.setup({on_attach = on_attach})

To repro the issue with these servers a typescript project with cssmodules should be available. For example swaggerlint-playground.

To setup project

git clone https://github.com/antonk52/swaggerlint-playground.git
cd swaggerlint-playground
npm install

Reproduce issue

  • nvim . from swaggerlint-playground
  • open component/Button/index.tsx
  • call vim.lsp.buf.definition (directly or via a keymap) while the cursor is over btn on line 23

Aside

As a temporary workaround a custom solution to merge results can be used gist. It would be nice to have this behaviour be built in. It is worth to mention that VS Code merges the results from multiple sources by default. I cannot think

Expected behavior

See a location list with locations to jump to, similar to when a single server returns multiple results.

Actual behavior

See both files open one after another based on which server return a result first. Files open instantly so it may seem like only one result was used. If you use <C-o> to jump to a previous location you will see the result from an language server that was first to response and only then the line where you invoked vim.lsp.buf.definition.

Log file

No response

@antonk52 antonk52 added bug issues reporting wrong behavior lsp labels Feb 18, 2023
@mfussenegger
Copy link
Member

As a workaround you can use the tag function - which is set by default (vim.lsp.tagfunc, used via ctrl+], ctrl + mouse-click or commands like :tjump)

To fix this we've to rework the implementation and move way from the global handler model and instead use something like buf_request_all (or manually iterate over the clients) to merge the results.

See:

function M.definition(options)
local params = util.make_position_params()
request_with_options(ms.textDocument_definition, params, options)
end

And here is how the tagfunc merges the results:

local results_by_client, err = lsp.buf_request_sync(0, ms.textDocument_definition, params, 1000)
if err then
return {}
end
local results = {}
local add = function(range, uri, offset_encoding)
table.insert(results, mk_tag_item(pattern, range, uri, offset_encoding))
end
for client_id, lsp_results in pairs(assert(results_by_client)) do
local client = lsp.get_client_by_id(client_id)
local offset_encoding = client and client.offset_encoding or 'utf-16'
local result = lsp_results.result or {}
if result.range then -- Location
add(result.range, result.uri)
else
result = result --[[@as (lsp.Location[]|lsp.LocationLink[])]]
for _, item in pairs(result) do
if item.range then -- Location
add(item.range, item.uri, offset_encoding)
else -- LocationLink
add(item.targetSelectionRange, item.targetUri, offset_encoding)
end
end
end
end
return results

(See also vim.lsp.buf.code_action or vim.lsp._completion.omnifunc for examples of functions which merge results from mulitple clients)

@mfussenegger mfussenegger added the complexity:low Low-risk, self-contained. Do NOT ask "can I work on this", just read CONTRIBUTING.md label Dec 9, 2023
@antonk52
Copy link
Sponsor Author

antonk52 commented Dec 9, 2023

To fix this we've to rework the implementation and move way from the global handler model and instead use something like buf_request_all (or manually iterate over the clients) to merge the results.

This is pretty much what I ended up doing in my personal config. Here is what I have been using since creating this issue. check all clients that support the lsp method, merge responses from them and jump to definition if there is a single response or quickfix/telescope for multiple ones.

Please let me know if this would be a welcome contribution.

local function cross_lsp_definition()
    local util = require('vim.lsp.util')
    local req_params = util.make_position_params()
    local all_clients = vim.lsp.get_active_clients()

    ---@table (Location | Location[] | LocationLink[] | nil)
    local raw_responses = {}
    -- essentially redoing Promise.all with filtering of empty/nil values
    local responded = 0

    local function make_cb(client)
        return function(err, response)
            if err == nil and response ~= nil then
                table.insert(raw_responses, { response = response, encoding = client.offset_encoding })
            end

            responded = responded + 1

            if responded == #all_clients then
                local flatten_responses = {}
                local flatten_responses_encoding = {}
                for _, v in ipairs(raw_responses) do
                    -- first check for Location | LocationLink because
                    -- tbl_islist returns `false` for empty lists
                    if v.response.uri or v.response.targetUri then
                        table.insert(flatten_responses, v.response)
                        table.insert(flatten_responses_encoding, v.encoding)
                    elseif vim.tbl_islist(v.response) then
                        for _, v2 in ipairs(v.response) do
                            table.insert(flatten_responses, v2)
                            table.insert(flatten_responses_encoding, v.encoding)
                        end
                    end
                end

                if #flatten_responses == 0 then
                    return
                end

                -- if there is only one response, jump to it
                if #flatten_responses == 1 and not vim.tbl_islist(flatten_responses[1]) then
                    return util.jump_to_location(flatten_responses[1], flatten_responses_encoding[1])
                end

                local items = util.locations_to_items(flatten_responses, nil)

                vim.fn.setqflist({}, ' ', { title = 'LSP locations', items = items })
                vim.api.nvim_command('botright copen')
                -- or use Telescope vim.api.nvim_command('Telescope quickfix')
            end
        end
    end

    for _, client in ipairs(all_clients) do
        if client.supports_method('textDocument/definition') then
            client.request('textDocument/definition', req_params, make_cb(client))
        else
            responded = responded + 1
        end
    end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues reporting wrong behavior complexity:low Low-risk, self-contained. Do NOT ask "can I work on this", just read CONTRIBUTING.md lsp
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants