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

Built-in LSP completion not filtering on label #29287

Closed
kristijanhusak opened this issue Jun 11, 2024 · 3 comments · Fixed by #29491
Closed

Built-in LSP completion not filtering on label #29287

kristijanhusak opened this issue Jun 11, 2024 · 3 comments · Fixed by #29491
Labels
enhancement feature request lsp
Milestone

Comments

@kristijanhusak
Copy link
Contributor

Problem

According to LSP specification, completion items should be filtered by filterText property, and if it does not exist, it should filter by label. Quoting from https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem:

	/**
	 * A string that should be used when filtering a set of
	 * completion items. When omitted the label is used as the
	 * filter text for this item.
	 */
	filterText?: string;

New built-in LSP completion filters only by filterText if it is defined, but it does not apply filtering on the label as a fallback.

https://github.com/neovim/neovim/blob/master/runtime/lua/vim/lsp/completion.lua#L241-L243

Steps to reproduce

  1. Install vtsls language server (this one was easiest to reproduce)
npm install -g @vtsls/language-server
  1. Use this minimal init:
vim.api.nvim_create_autocmd('FileType', {
  pattern = 'javascript',
  callback = function()
    vim.lsp.start({
      cmd = { 'vtsls', '--stdio' },
      on_attach = function(client, bufnr)
        vim.lsp.completion.enable(true, client.id, bufnr, { autotrigger = true })
      end,
    })
  end,
})

vim.keymap.set('i', '<C-space>', function()
 vim.lsp.completion.trigger()
end)
  1. nvim -u minimal_init.lua
  2. :edit test.js
  3. Type Math.mi, this should show correct completion since it's doing "live" filtering
  4. Press <C-space>, it will show a lot of unmatched items
    screenshot_2024_06_11_18_14_41

Expected behavior

Expected to properly filter by the prefix, which is in my example mi.

Neovim version (nvim -v)

NVIM v0.11.0-dev-37bf4c5

Vim (not Nvim) behaves the same?

N/A

Operating system/version

Arch Linux

Terminal name/version

kitty 0.35.1

$TERM environment variable

xterm-kitty

Installation

bob (https://github.com/MordechaiHadad/bob

@kristijanhusak kristijanhusak added the bug issues reporting wrong behavior label Jun 11, 2024
@github-actions github-actions bot added the lsp label Jun 11, 2024
@mfussenegger mfussenegger removed the bug issues reporting wrong behavior label Jun 12, 2024
@mfussenegger
Copy link
Member

This is kinda intentional to support the fuzzy completeopt and also to have the option to select snippets where the word/label won't have a prefix match. All subsequent filtering is left up to the complete() handling.

But we could add an option to make it stricter

@kristijanhusak
Copy link
Contributor Author

From my understanding, fuzzy completeopt is using the same code as matchfuzzy, so those should behave the same.

But we could add an option to make it stricter

Yes, this would be great. It can be added as an option on vim.lsp.completion.enable together with autotrigger.

@justinmk justinmk added the enhancement feature request label Jun 12, 2024
@justinmk justinmk added this to the backlog milestone Jun 12, 2024
@kristijanhusak
Copy link
Contributor Author

@mfussenegger I'd like to address this, but want to discuss it before I start.
Do you have any preference how to approach it? I have these suggestions:

  1. Allow custom function for filtering
    Accept the filter(or some other name, I'm not sure what would be the most suitable) option when enabling the completion from on_attach. It would accept the lsp.CompletionItem and a prefix. Something along these lines:
vim.lsp.completion.enable(true, client.id, bufnr, {
  autotrigger = true,
  filter = function(item, prefix)
    ---@cast item lsp.CompletionItem
    ---@cast prefix string
    return vim.fn.matchfuzzy({ item.label }, prefix)
  end
})

This would either be a complete substitution for the current filter that filters by filterText, or it could be used as a fallback filter. I'd prefer the latter, since filterText is part of the LSP specification, and it is always expected to be filtered by this.

  1. Add a flag that will additionally filter by label
    This one is a simpler but less flexible option. Flag would just additionally filter by label as the LSP specification mentions:
vim.lsp.completion.enable(true, client.id, bufnr, {
  autotrigger = true,
  fallbackFilterByLabel = true
})

Personally, I'd prefer option 1. since it gives more flexibility, but option 2. is also perfectly fine since it solves this issue.

mfussenegger added a commit to mfussenegger/neovim that referenced this issue Jun 26, 2024
Although the built-in pum completion mechanism will filter anyway on the
next input it is odd if the initial popup shows entries which don't
match the current prefix.

Using fuzzy match on the label/prefix is compatible with
`completeopt+=fuzzy` and also doesn't seem to break postfix snippet
cases

Closes neovim#29287
mfussenegger added a commit to mfussenegger/neovim that referenced this issue Jun 27, 2024
Although the built-in pum completion mechanism will filter anyway on the
next input it is odd if the initial popup shows entries which don't
match the current prefix.

Using fuzzy match on the label/prefix is compatible with
`completeopt+=fuzzy` and also doesn't seem to break postfix snippet
cases

Closes neovim#29287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature request lsp
Projects
None yet
3 participants