fix: Outdated completion item with mini.snippets

[abeldekat]

Fixes #2119

The issue as described by @xzbdmw has been debated in several other places:

.. triggers completion when it should not ..
beta testing mini.snippets: expanding snippets from lsp

@xzbdmw provided a clear explanation:

I'll try to explain more detail:
mini.snippets: insert mode -> type 'f' -> both mini.snippets and cmp detects TextChangedI -> cmp-nvim-lsp sends request -> lua_ls receives the buffer content, which is vim.schedule(ffn) -> mini.snippets clears the placeholder fn, it becomes vim.schedule(f) -> "fun" completion_item contains TextEdits that want to clear the fn , now what in fn's original place is now the right pair -> right pair gone, before snippets expanding. Neither mini.snippets or cmp is to blame here IMO, they all do what they supposed to do.

luasnip: select mode -> type 'f' -> nvim clears the placeholder, by the definition of select mode -> cmp-nvim-lsp sends request -> lua_ls receives the buffer content, which is vim.schedule(f) -> all good.

@hrsh7th

I considered solving the problem in cmp-nvim-lsp, introducing some kind of flag indicating whether the call to complete should be scheduled or not.

Then I saw that TextChangedI and TextChangedP do not yet use async.debounce_next_tick.

I think this solves the issue on a more appropriate level.

[abeldekat]

It is possible to solve the problem in the user's config:

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
      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

[hrsh7th]

In my career as a front-end developer, solving any kind of asynchronous timing problem with things like setTimeout or vim.schedule has always been an anti-pattern.

I think this problem should be solved by mini.snippets.

[echasnovski]

I think this problem should be solved by mini.snippets.

As far as I understand, there is no problem in 'mini.snippets' to solve. The way it works mostly served as a source to reproduce an issue in completion plugins, which is roughly as follows: there can be other text changes in between completion plugin reacting to 'TextChangedI' / 'TextChangedP' and async action they do as a result. This might lead to action being outdated as the buffer text state has already changed.

The 'mini.snippets' showed this issue because it can react on text change by removing tabstop placeholder immediately in the same tick (otherwise there will be flicker) inside 'TextChanged{I,P}' callback. Other snippet engines utilize Select mode here: its replacing effect is done internally by Neovim and is already in place for 'TextChanged{I,P}' events.

[hrsh7th]

I didn't know about that behavior of mini.snippets.

I'm guessing that mini.snippets changes the text during TextChangedI/P, which then causes TextChangedI/P to fire again and complete correctly, but let's look into it.

[echasnovski]

I'm guessing that mini.snippets changes the text during TextChangedI/P, which then causes TextChangedI/P to fire again and complete correctly, but let's look into it.

Yes, removing placeholder in 'mini.snippets' indeed triggers either TextChangedI or TextChangedP (after ones caused by the user typing a character which causes placeholder removal). They are not suppressed because there is a text change in Insert mode which those events are meant monitor.

[hrsh7th]

I found that this doesn't happen with the new completion engine I'm developing, but it does happen with nvim-cmp.

So this is something nvim-cmp wants to solve somehow.

But it seems like it's going to be pretty difficult to solve...

[echasnovski]

I found that this doesn't happen with the new completion engine I'm developing, but it does happen with nvim-cmp.

Another evidence to a saying "Third time is a charm" :)

[hrsh7th]

nvim-cmp has a lot of problems...

Plus, rivals have emerged, so I'm motivated.

• core logic are library, not a plugin.
• views are pluggable
• macro support (there are asynchronous issues in some cases, but it works in most cases)
• tests have been written

etc.

It might be a good idea to release it as a beta test, but I'm afraid the interface will freeze.

[abeldekat]

But it seems like it's going to be pretty difficult to solve...

It also happens with blink.

When mini expands an lsp snippet, it also starts listening for TextChangedI and TextChangedP.
However, it has no way of saying: I, mini.snippets, would like to be notified first, because I, mini.snippets, know that there might be outdated completion items.

How about this:

cmp - init.lua:

-- Resubscribe to events if needed
-- Forces cmp to be "last"
cmp.resubscribe = function(events)
  autocmd.resubscribe(events)
end

cmp - utils - autocmd.lua:

autocmd.resubscribe = function(events)
  -- Delete the autocommands
  local candidates = vim.api.nvim_get_autocmds({
    group = autocmd.group,
    event = events,
  })
  for _, candidate in ipairs(candidates) do
    vim.api.nvim_del_autocmd(candidate.id)
  end

  -- And recreate....
  for _, event in ipairs(events) do
    if autocmd.events[event] then
      vim.api.nvim_create_autocmd(event, {
        desc = ('nvim-cmp: autocmd: %s'):format(event),
        group = autocmd.group,
        callback = function()
          autocmd.emit(event)
        end,
      })
    end
  end
end

Integration: The user's config for nvim-cmp:

local group = vim.api.nvim_create_augroup("mini_snippets_nvim_cmp", { clear = true })
vim.api.nvim_create_autocmd("User", {
  group = group,
  pattern = "MiniSnippetsSessionStart",
  callback = function()
    -- Mini.snippets stays in insert mode
    -- Make sure no completion suggestions are offered directly after snippet expand
    require("cmp.config").set_onetime({ sources = {} })
  end,
})

-- Part of opts nvim-cmp:
snippet = {
  expand = function(args)
     ---@diagnostic disable-next-line: undefined-global
    local insert = MiniSnippets.config.expand.insert or MiniSnippets.default_insert
    insert({ body = args.body }) -- insert at cursor

    -- TODO: Only needs to happen on first snippet expand, not for nested expands....
    -- The trick:
    require("cmp").resubscribe({ "TextChangedI", "TextChangedP" })
  end
}

[hrsh7th]

@abeldekat Could you try this?
#2128

[abeldekat]

@hrsh7th,

Sure!

minimal init

-- 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

require("mini.notify").setup() -- lsp loading indicator
local mini_notify = MiniNotify.make_notify() -- opts
---@diagnostic disable-next-line: duplicate-set-field
vim.notify = function(msg, level, _) mini_notify(msg, level) end

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" },
  },
})

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(),
  view = { docs = { auto_open = false } },
})

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()
capabilities = vim.tbl_deep_extend("force", capabilities, require("cmp_nvim_lsp").default_capabilities() or {})
require("lspconfig").lua_ls.setup({
  capabilities = capabilities,
  settings = {
    Lua = {
      runtime = { version = "LuaJIT" },
      completion = { callSnippet = "Replace", keywordSnippet = "Replace" },
      workspace = {
        checkThirdParty = false,
        library = { vim.env.VIMRUNTIME },
      },
    },
  },
})

test file

--[[
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

video result

fix-mini.mp4

Unfortunately, the issue is still there.

I would like to hear your thoughts on the idea I wrote down in my last comment.
As "pro" I can see:

1. No fixing code in nvim-cmp. Just a new function, resubscribe
2. No fixing code in mini either.
3. No vim.schedule. I do agree that this is an anti pattern.

As "con"

1. One extra line in the user's config: The call to cmp.resubscribe.

[hrsh7th]

There would be no problem if nvim-cmp were simply equipped with resubscribe. I would accept it.

However, one thing that concerns me is that it may not be a fundamental fix, but that's a topic for another discussion.

(my words almost translated by google 😅

[abeldekat]

@hrsh7th,

I added two new commits. It's a very concrete fix, not fundamental indeed.

@echasnovski

Could you also have a look? I would like to conditionally call cmp.resubscribe, seeing this code. Using MiniSnippets.session.get(true) does a full clone and is perhaps too expensive.

[echasnovski]

Using MiniSnippets.session.get(true) does a full clone and is perhaps too expensive.

On a typical use case when there is no snippet session it will just by vim.deepcopy(nil), which is very fast. Even with active session, it is typically on the order of 0.01ms.

[abeldekat]

On a typical use case when there is no snippet session it will just by vim.deepcopy(nil), which is very fast. Even with active session, it is typically on the order of 0.01ms.

Thanks. I think it's not even necessary. I benchmarked the call to cmp.resubscribe, as you explained in this issue.

The median is 0.0475465 on an old laptop. I think that's not too bad, even when added after every expand.

[hrsh7th]

Thank you.

[abeldekat]

My pleasure!