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

Beta testing 'mini.snippets': expanding snippets from lsp using 'nvim-cmp' or 'blink.cmp' #9

Closed
abeldekat opened this issue Jan 12, 2025 · 16 comments

Comments

@abeldekat
Copy link
Owner

abeldekat commented Jan 12, 2025

mini_snippets_lsp_expansion.mp4

@echasnovski,

I created this issue in order to avoid too much "noice" inside the actual beta testing issue, although the problem is not related to this cmp-mini-snippets repository.

Functions with parameters annotated with fun() fail to expand correctly when the user chooses the fun completion item from lua_ls.
Apart from vim.schedule, there are potentially lots of cases where the expansion is not correct.

Happens with both blink and nvim-cmp.

Related:

  1. This issue in nvim-cmp
  2. This comment

Test setup:
See reproduction in mini.nvim

Test config:

init.lua
-- Clone latest 'mini.nvim' (requires Git CLI installed)
vim.cmd('echo "Installing `mini.nvim`" | redraw')
local mini_path = vim.fn.stdpath("data") .. "/site/pack/deps/start/mini.nvim"
local clone_cmd = { "git", "clone", "--depth=1", "https://github.com/echasnovski/mini.nvim", mini_path }
vim.fn.system(clone_cmd)
vim.cmd('echo "`mini.nvim` is installed" | redraw')

-- Make sure 'mini.nvim' is available
vim.cmd("packadd mini.nvim")
require("mini.deps").setup()

-- Add extra setup steps needed to reproduce the behavior
-- Use `MiniDeps.add('user/repo')` to install another plugin from GitHub

local use_cmp = true -- switch between blink and cmp

local function add_cmp()
  MiniDeps.add({ source = "hrsh7th/nvim-cmp", depends = { "hrsh7th/cmp-nvim-lsp", "hrsh7th/cmp-buffer" } })
  local cmp = require("cmp")
  require("cmp").setup({
    snippet = {
      expand = function(args) -- use mini.snippets to expand snippets from lsp
        local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
        insert({ body = args.body }) -- Insert at cursor
      end,
    },
    sources = cmp.config.sources({ { name = "nvim_lsp" }, { name = "buffer" } }),
    mapping = cmp.mapping.preset.insert(),
    completion = { completeopt = "menu,menuone,noinsert" },
  })
end

local function add_blink()
  local function build_blink(params)
    vim.notify("Building blink.cmp", vim.log.levels.INFO)
    local obj = vim.system({ "cargo", "build", "--release" }, { cwd = params.path }):wait()
    if obj.code == 0 then
      vim.notify("Building blink.cmp done", vim.log.levels.INFO)
    else
      vim.notify("Building blink.cmp failed", vim.log.levels.ERROR)
    end
  end

  MiniDeps.add({
    source = "saghen/blink.cmp", -- no friendly snippets, just lsp expand with mini.snippets
    hooks = { post_install = build_blink, post_checkout = build_blink },
  })
  require("blink.cmp").setup({
    sources = { default = { "lsp", "path", "buffer" }, cmdline = {} }, -- no cmdline, no snippets
    snippets = {
      expand = function(snippet)
        local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
        insert({ body = snippet })
      end,
      active = function(_) return MiniSnippets.session.get(false) ~= nil end,
      jump = function(direction) MiniSnippets.session.jump(direction == -1 and "prev" or "next") end,
    },
    keymap = { preset = "default" },
    completion = { documentation = { auto_show = true } },
  })
end

require("mini.notify").setup() -- lsp loading indicator

local mini_snippets = require("mini.snippets")
mini_snippets.setup({
  snippets = { -- just one dummy custom snippet:
    { prefix = "dummy", body = "T1=fu$1", desc = "fu before $1" },
  }
})

if use_cmp then
  add_cmp()
else
  add_blink()
end

MiniDeps.add("williamboman/mason.nvim")
MiniDeps.add("williamboman/mason-lspconfig.nvim")
MiniDeps.add("neovim/nvim-lspconfig")
require("mason").setup()
require("mason-lspconfig").setup({ ensure_installed = { "lua_ls" } })

local capabilities = vim.lsp.protocol.make_client_capabilities()
if use_cmp then
  capabilities = vim.tbl_deep_extend("force", capabilities, require("cmp_nvim_lsp").default_capabilities() or {})
else
  capabilities = vim.tbl_deep_extend("force", {}, require("blink.cmp").get_lsp_capabilities(capabilities))
end
require("lspconfig").lua_ls.setup({
  capabilities = capabilities,
  settings = {
    Lua = {
      runtime = { version = "LuaJIT" },
      completion = { callSnippet = "Replace" },
      workspace = {
        checkThirdParty = false,
        library = { vim.env.VIMRUNTIME },
      },
    },
  },
})

Test scenarios:

--[[
TLDR: functions with parameters annotated with `fun()`
fail to expand correctly when the user choses the `fun` completion item from lua_ls
Happens with both blink and nvim-cmp

-- NOTE: Mini.snippets is setup to only provide lsp expansion. No completion sources.

-- NOTE: Completion keys: `<c-y> <c-n> <c-p>``

-- NOTE: In all "wrong" cases, if `callback` is not nested(press <c-c>), expansion is correct!
  Conclusion: the lsp snippet `{body = "function ()\n\t$0\nend}"`, having kind "function", is not wrong itself

-- NOTE: After initial snippet expand no completions should be suggested.
  However, `nvim-cmp` does suggest completion items, but **only** after
  expansion of `demo4` and `demo5`, where callback is the first parameter.
  
--]]

-- cmp: correct
-- blink: correct
---@param message string
---@param callback function
local function demo1(message, callback) end

-- cmp: correct
-- blink: correct
---@param callback function
---@param message string
local function demo2(callback, message) end

-- cmp: select "fun" completion item -> wrong
-- blink: select "fun" completion item -> wrong
---@param message string
---@param callback fun() -- Two lsp snippet completions: fun and function
local function demo3(message, callback) end

-- cmp: select "fun" completion item -> correct!
-- blink: select "fun" completion item -> wrong
---@param callback fun() -- Two lsp snippet completions: fun and function
---@param message string
local function demo4(callback, message) end

-- cmp: select "fun" completion item -> wrong
-- blink: select "fun" completion item -> wrong
---@param callback fun() -- Two lsp snippet completions: fun and function
local function demo5(callback) end

I could "duplicate" the nvim-cmp issue to blink.cmp.
In this comment, and in this comment, @xzbdmw provides an explanation.

I also tried to find an explanation as to why this happens, and did not succeed yet. What is difference between the handling of those lsp snippets in luasnip and mini.snippets?

If this is not an issue in mini.snippets, I think the "why" would be very helpful for the maintainers of nvim-cmp and blink.cmp.

@echasnovski
Copy link

I also tried to find an explanation as to why this happens, and did not succeed yet. What is difference between the handling of those lsp snippets in luasnip and mini.snippets?

If this is not an issue in mini.snippets, I think the "why" would be very helpful for the maintainers of nvim-cmp and blink.cmp.

This sounds plausible.

@xzbdmw
Copy link

xzbdmw commented Jan 12, 2025

The problem exists on gopls too. Snippets contains a textedits will do.

iShot_2025-01-12_20.49.49.mp4

@abeldekat
Copy link
Owner Author

abeldekat commented Jan 12, 2025

@xzbdmw, thank you for this comment. Especially for this line:

... nvim clears the placeholder, by the definition of select mode ...

Now that I understand a bit more, I am able to succesfully complete demo3, demo4 and demo5. The workaround: <c-o>cw

Demo:

mini_snippets_lsp_expansion_ctrl_o.mp4

@echasnovski, regarding your comment:

... then 'nvim-cmp' delaying sending request ...

That would also need a change in blink.cmp.

I wonder, would it be possible to hook into the expected select mode behavior of both completion engines, using <c-o>cw:

  1. mini.snippets detect that user is in a tabstop
  2. user types first character
  3. mini.snippets somehow issues a <c-o>cw, effectively mimicking select mode.

Perhaps:

--- cmp opts
...
local snippet = { -- configure mini.snippets to handle `lsp snippets`
  expand = function(args)
    local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
    insert({ body = args.body }) -- TODO: Add a flag "from_lsp" ?
  end,
}
...

@echasnovski
Copy link

3. mini.snippets somehow issues a <c-o>cw, effectively mimicking select mode.

'mini.snippets' does nothing wrong here: it "immediately" (i.e. in the same event loop, not through vim.schedule) removes placeholder text. Changing mode through <C-o> is a very bad idea and goes against core design of "always stay in Insert mode".

@abeldekat
Copy link
Owner Author

Just to be sure: I did not say that mini.snippets is doing something wrong... I do understand now that <c-o> is a bad idea. Acting as beta tester, I hope you don't mind...)

As a POC, and perhaps whilst waiting for the issue in nvim-cmp to be resolved, I took the init.lua provided in this issue and made some changes:

-- ...

local mini_snippets = require("mini.snippets")
mini_snippets.setup({
  snippets = { -- just one dummy custom snippet:
    { prefix = "dummy", body = "T1=fu$1", desc = "fu before $1" },
  },
  expand = {
    insert = function(snippet)
      -- NOTE: Prevent any completion directly after snippet insert:
      if use_cmp then require("cmp.config").set_onetime({ sources = {} }) end
      MiniSnippets.default_insert(snippet)
    end,
  },
})

if use_cmp then
  add_cmp()
  -- NOTE: Make sure the config prevents any completion directly after snippet insert
  -- NOTE: Now, it's possible to prevent outdated completion items:
  local cmp = require("cmp")
  vim.api.nvim_create_autocmd("User", {
    group = vim.api.nvim_create_augroup("mini_snippets_cmp_nvim_lsp", { clear = true }),
    pattern = "CmpRegisterSource",
    callback = function(ev)
      local id = ev.data.source_id
      for _, source in ipairs(cmp.get_registered_sources()) do
        if source.id == id and source.name == "nvim_lsp" then
          local cmp_nvim_lsp = source.source
          local org_complete = cmp_nvim_lsp.complete
          cmp_nvim_lsp.complete = vim.schedule_wrap(org_complete)
        end
      end
    end,
  })
else
  add_blink()
end

-- ...

I also tested the autocommand with LazyVim and the luasnip extra. It's working fine.

@xzbdmw
Copy link

xzbdmw commented Jan 12, 2025

Right 👍 you may consider #6 (comment) to conditionally schedule.

@abeldekat
Copy link
Owner Author

abeldekat commented Jan 13, 2025

I managed to address all nvim-cmp issues in the add_cmp function. The config of mini.snippets does not need the expand.insert override anymore.

local function add_cmp()
  local group = vim.api.nvim_create_augroup("mini_snippets_nvim_cmp", { clear = true })

  -- NOTE: Firstly, make sure that nvim-cmp never provides completion items directly after snippet insert
  -- See https://github.com/abeldekat/cmp-mini-snippets/issues/6
  vim.api.nvim_create_autocmd("User", {
    group = group,
    pattern = "MiniSnippetsSessionStart",
    callback = function(_) require("cmp.config").set_onetime({ sources = {} }) end,
  })

  -- HACK: Secondly, it's now possible to prevent outdated completion items
  -- See https://github.com/hrsh7th/nvim-cmp/issues/2119
  local function make_complete_override(complete_fn)
    return function(self, params, callback)
      local override_fn = complete_fn
      -- TODO: Perhaps mini.snippets can expose a public function to allow the wrapping to be more specific,
      -- according to the example @xzbdmw provided in this comment:
      -- https://github.com/abeldekat/cmp-mini-snippets/issues/6#issuecomment-2583446264
      --
      if MiniSnippets.session.get(false) ~= nil then override_fn = vim.schedule_wrap(override_fn) end
      override_fn(self, params, callback)
    end
  end
  local function find_cmp_nvim_lsp(id)
    for _, source in ipairs(require("cmp").get_registered_sources()) do
      if source.id == id and source.name == "nvim_lsp" then return source.source end
    end
  end
  vim.api.nvim_create_autocmd("User", {
    group = group,
    pattern = "CmpRegisterSource",
    callback = function(ev)
      local cmp_nvim_lsp = find_cmp_nvim_lsp(ev.data.source_id)
      if cmp_nvim_lsp then
        local org_complete = cmp_nvim_lsp.complete
        cmp_nvim_lsp.complete = make_complete_override(org_complete)
      end
    end,
  })

  MiniDeps.add({ source = "hrsh7th/nvim-cmp", depends = { "hrsh7th/cmp-nvim-lsp", "hrsh7th/cmp-buffer" } })
  local cmp = require("cmp")
  require("cmp").setup({
    snippet = {
      expand = function(args) -- use mini.snippets to expand snippets from lsp
        local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
        insert({ body = args.body }) -- Insert at cursor
      end,
    },
    sources = cmp.config.sources({ { name = "nvim_lsp" }, { name = "buffer" } }),
    mapping = cmp.mapping.preset.insert(),
    completion = { completeopt = "menu,menuone,noinsert" },
  })
end

I think we need:

  1. one PR for nvim-cmp in order to avoid completion suggestions directly after snippet expand.
  2. one PR for cmp-nvim-lsp to address outdated completion items
  3. one PR for blink.cmp to address outdated completion items

@echasnovski, would you like to create these PR's yourself?

@echasnovski
Copy link

@echasnovski, would you like to create these PR's yourself?

If I wanted to play with other completion engines (with or without 'mini.snippets' integration), I'd already done it :) I'd rather add 'mini.snippets' support for 'mini.completion' (which is in the process). It is totally fine for you to do this.

One thing I have in mind is that if it is indeed the issue of 'nvim-cmp'/'blink.cmp' making text edits on the outdated buffer, then this is a more general problem than 'mini.snippets' removing placeholder while in the Insert mode. In theory, a slow language server can take significant amount of time to process the request. If the text edit is done by the server, then user can have already updated buffer manually by the time text edit is executed. Maybe cancelling text edit or some kind of undo/repo can be a better idea.

@xzbdmw
Copy link

xzbdmw commented Jan 13, 2025

I agree, there are some sort of “smart” treatment of TextEdits based on context (diff between when requests sending and when user really confirming) on both nvim-cmp and blink, they failed to do smartly in this case. Given that nvim-cmp author is rather inactive to issues, maybe blink author has more insight.

@abeldekat
Copy link
Owner Author

abeldekat commented Jan 14, 2025

@echasnovski, for the past few days, I have been thinking really hard about this issue. So, I hope you'll allow me another suggestion.

One thing I have in mind is that if it is indeed the issue of 'nvim-cmp'/'blink.cmp' making text edits on the outdated buffer, then this is a more general problem than 'mini.snippets' removing placeholder while in the Insert mode. In theory, a slow language server can take significant amount of time to process the request. If the text edit is done by the server, then user can have already updated buffer manually by the time text edit is executed. Maybe cancelling text edit or some kind of undo/repo can be a better idea.

Would that happen when a user manually updates the buffer? I experimented a bit having the explanation of @xzbdmw in mind:

  1. Focus is on tabstop with placeholder( a nested snippet )
  2. User presses a key
  3. Responding to TextChangedI, nvim-cmp starts completion
  4. Responding to TextChangedI, mini.snippets removes the placeholder, using vim.api.nvim_buf_set_text
  5. nvim-cmp is not notified of this change, and the issue arises

Other snippet engines avoid the manual removal of the placeholder via selectmode.
I do think the design decision to not use selectmode is an excellent choice. The UI does not display diagnostics that are about to be resolved, code is not dimmed, and undo works on the entire edit.

As suggested, we can PR nvim-cmp. Somehow, cmp-nvim-lsp would need to know whether it's call needs to be scheduled or not. An extra item(ie. complete_needs_to_be_scheduled) in it's config seems to be required. That extra item would only be needed for mini.snippets.

Alternatively, we can solve the issue in the config of nvim-cmp, forcing a "refresh":

local function add_cmp()
  local group = vim.api.nvim_create_augroup("mini_snippets_nvim_cmp", { clear = true })

  -- NOTE: First, make sure that nvim-cmp never provides completion items directly after snippet insert
  -- See https://github.com/abeldekat/cmp-mini-snippets/issues/6
  vim.api.nvim_create_autocmd("User", {
    group = group,
    pattern = "MiniSnippetsSessionStart",
    callback = function(_) require("cmp.config").set_onetime({ sources = {} }) end,
  })

  -- NOTE: Second, it's now possible to prevent outdated completion items
  -- See https://github.com/hrsh7th/nvim-cmp/issues/2119
  -- TODO: after line 2264, the call to nodes_del, add to mini.snippets
  -- H.trigger_event('MiniSnippetsSessionPlaceholderDel', {})
  vim.api.nvim_create_autocmd("User", {
    group = group,
    pattern = "MiniSnippetsSessionPlaceholderDel",
    callback = function(_)
      local core = require("cmp").core
      local ctx = core.context -- the context is good, keep it
      core:reset()
      core:complete(ctx) -- now we have the up-to-date completion items!
    end,
  })

  MiniDeps.add({ source = "hrsh7th/nvim-cmp", depends = { "hrsh7th/cmp-nvim-lsp", "hrsh7th/cmp-buffer" } })
  local cmp = require("cmp")
  require("cmp").setup({
    snippet = {
      expand = function(args) -- use mini.snippets to expand snippets from lsp
        local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
        insert({ body = args.body }) -- Insert at cursor
      end,
    },
    sources = cmp.config.sources({ { name = "nvim_lsp" }, { name = "buffer" } }),
    mapping = cmp.mapping.preset.insert(),
    completion = { completeopt = "menu,menuone,noinsert" },
  })
end

These instructions specific to nvim-cmp could be added to the wiki of mini.nvim, via a link in the help of mini.snippets. Or perhaps in the readme of this repo.

@echasnovski
Copy link

  • Responding to TextChangedI, mini.snippets removes the placeholder, using vim.api.nvim_buf_set_text

  • nvim-cmp is not notified of this change, and the issue arises

'nvim-cmp' is notified of this change via separate 'TextChangedI' which is triggered during removing placeholder in 'mini.snippets' (I just checked). And this is a more general approach as 'TextChangedI' can be triggered by any source (plugin or user action) and is an indication that buffer text has changed (thus in theory making any pending text edit outdated).

@abeldekat
Copy link
Owner Author

I submitted the PR

@echasnovski
Copy link

I submitted the PR

If it solves the 'mini.snippets' issue, then that one-line fix does look to me like a good solution indeed.

@abeldekat
Copy link
Owner Author

if it solves the 'mini.snippets' issue, then that one-line fix does look to me like a good solution indeed.

Two lines...) I forgot the piece of code that prevents completion popup directly after snippet expand.

I am closing this issue.

@abeldekat
Copy link
Owner Author

FYI:
Blink PR

@abeldekat
Copy link
Owner Author

The nvim-cmp PR will not be accepted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants