g:AutoPairsMapCR conflicts against coc.nvim

[Frederick888]

OS: Arch Linux (rolling)
What vim? (+ version): Neovim 0.7.2
Reproduced in other variants of Vim? (Optional): Yes. VIM - Vi IMproved 9.0 (2022 Jun 28, compiled Jun 28 2022 16:22:51)

Autopairs config (if applicable):

set runtimepath^=~/.vim runtimepath+=~/.vim/after
let &packpath = &runtimepath

call plug#begin('~/.vim/plugged')
Plug 'neoclide/coc.nvim', { 'branch': 'release' }
Plug 'LunarWatcher/auto-pairs'
call plug#end()

set nocompatible
set updatetime=300
set cmdheight=2

" auto-pairs
let g:AutoPairsCenterLine = 0
" let g:AutoPairsMapCR = 0

inoremap <expr> <CR> coc#pum#visible() ? coc#_select_confirm() : "\<CR>"

Describe the bug

After inserting a new pair, if coc.nvim's popup menu is visible, <CR> inserts a line above current line instead of selecting highlighted item from coc.nvim popup.

Steps to reproduce

1. Save above minimal vimrc as mini.vim
2. Install the plugins, then :CocInstall coc-tsserver
3. Save this snippet as src.js

function foo(req) {
    let {user} = req
    let {user} = b
}

4. nvim -u mini.vim src.js
5. Start a new line right below the first line
6. Type let {us<CR>
7. It sometimes becomes

function foo(req) {
    
    let {us}
    let {user} = req
    let {user} = b
}

sometimes

function foo(req) {
    user
    let {us}
    let {user} = req
    let {user} = b
}

neoclide/coc.nvim#3031 and neoclide/coc.nvim#2204 are probably the same issue.

(Btw thanks for maintaining this project!)

[LunarWatcher]

I'm aware this could happen in practice, and it's an unfortunate consequence of how Vim's mapping system is built.

Essentially, only two maps of a given type for a given key can exist in a given mode; a global variant, and a buffer-only variant. For insert mode, that means you can have one global mapping for enter, and one buffer mapping for enter. The auto-pairs map and the coc map conflict.

Now, jiangmiao's initial code made some assumptions about the integrity of <CR> that doesn't seem to hold in practice, but is necessary to avoid conflicts with other buffer mappings, but it doesn't extend into global mappings because the mapping system is weird.

I'm not entirely sure why <CR> isn't included; my best guess is some weirdness related to coc.nvim being async and triggering a late <BS>, but the code is too much of a mess for me to figure that out quickly on the fly, so I won't. The last example you have happens when precisely that happens; coc.nvim's mapping is invoked first, because global maps are invoked first. Before the timer expires, auto-pairs' callback has been invoked, which only does O and drops the cr for some reason. (Maybe related to coc's use of <ignore>? Not sure how that works anyway). This creates a line above, coc.nvim then does its thing and inserts user, and voila, you have user where you didn't want it.

Alternatively, it's related to <expr>, but I haven't done much to test and I didn't have much to go on when I inherited the code:

                " remap <expr> to `name` to avoid mix expr and non-expr mode
                execute 'inoremap <buffer> <expr> <script> '. wrapper_name . ' ' . old_cr
                let old_cr = wrapper_name

However, if cr had been included, you would've ended up with a linebreak within the brackets anyway, because the execution of both still takes place.

At this time, I have no better suggestions than to make the else portion of the ternary be \<cr>\<C-r>=autopairs#Keybinds#IgnoreInsertEnter('autopairs#AutoPairsReturn')\<cr>. It's a nasty hack, and I'm aware of that, but it should make sure everything is invoked properly. Disable AutoPairsCR as well, or course

[Frederick888]

Thanks for all the details!

Personally I've only encountered this issue when there are popup menus so may I know how you feel about tackling this case by case? For example,

diff --git a/autoload/autopairs.vim b/autoload/autopairs.vim
index 173333f..25d858d 100644
--- a/autoload/autopairs.vim
+++ b/autoload/autopairs.vim
@@ -482,9 +482,9 @@ fun! autopairs#AutoPairsDetermineCRMovement()
     endif
 endfun
 
 func! autopairs#AutoPairsReturn()
-    if b:autopairs_enabled == 0 || b:AutoPairsIgnoreSingle
+    if b:autopairs_enabled == 0 || b:AutoPairsIgnoreSingle || pumvisible() || (exists('*coc#pum#visible') && coc#pum#visible())
         let b:AutoPairsIgnoreSingle = 0
         return ''
     end
 

[LunarWatcher]

The thing is, that conflicts with people who don't want to make <CR> to coc.nvim, including myself. I got used to ctrl-n and ctrl-y to save pain surrounding <CR> ambiguity (and that was purely from a UX POV, and not from a Vim mapping weirdness one). There's also no guarantee that works consistently; coc.nvim is async, and like I said, that can result in coc.nvim's mapping not being done executing by the time auto-pairs' mapping kicks in.

The beset way to solve it is to make sure there isn't a conflict, but auto-pairs also can't hijack global <CR> on the off chance it has some auto-complete mappings that may or may not conflict.

An option C of sorts to the trigger, actually, though that depends on the execution order of mappings; if vim can stop the mapping from executing in the middle to deal with the timer, it could consistently be stopping immediately after dispatching <CR>, but prior to running AutoPairsReturn. Some weird coc.nvim internals could then treat the newline as it would any character it has to remove, and destroy it. By the time the auto-pairs function is called, the CR is gone and the text inserted. That's the working theory anyway, actually verifying it requires a bit lower level debugging that I can't be arsed to do, because the system is annoying.

The third option is offering mappings compatible with various plugins by default, but as evidenced by coc.nvim recently introducing coc#pum#visible, this results in version incompatibilities between plugins. This doesn't even begin to account for other auto-complete plugins, and there's an obnoxious quantity of them at the moment, or other plugins that add global mappings to <CR>.

And of course, to add insult to injury, there's personal preference on top of that; some people might want to map <CR> to going down the list of completions instead, or do other weird stuff with the map.

So, sorting out the mapping itself in auto-pairs isn't an option, because that's what vimrcs are for, and adding the code you proposed introduces race conditions. Adding a variable for enabling the conditions does solve the user preference bit, but it doesn't solve the race condition.

What would be nice is if Vim had facilities for dealing with conflicting mappings at the editor level rather than on a per-plugin with different practices basis, but that's massively complex onto itself, and I imagine a place where neovim and vim once again fork if it's attempted implemented, which itself isn't stonks. However, that's a pipedream and pipenightmare, and not something we need to think too much about when the problem remains

[LunarWatcher]

... alternatively, if Vim had an equivalent of JavaScript's e.preventDefault(), a bit of code could be amended to the coc bit of the ternary operator to stop any further mappings from executing, but I don't believe that exists at this time either. I imagine it's primarily useful if there's a larger number of mappings, but that typically isn't the case with vim.

Conflict avoidance on keys is preferred as much as possible in Vim. Space and <CR>, along with <BS> to an extent, all represent common keys many plugins and many people use for lots of different things, and Vim's mapping system simply isn't designed to deal with them being used as much as they are. These keys often represent semantic meanings as well, where other keys, subjectively, don't make sense. For auto-pairs, all three keys include extensions to base functionality, and consequently have to be on their equivalent keys. This is with the exception of <CR>, which supports a different key to trigger a "modded" expanding <CR> (IIRC, I haven't touched that system in ages)

[Frederick888]

Understood. Thanks again for the explanation.

Btw you mentioned 'I got used to ctrl-n and ctrl-y', was this talking about assigning them to e.g. g:AutoPairsCRKey?

[Frederick888]

Oh TIL Ctrl-y confirms current pum selection. That's indeed better!

[LunarWatcher]

And ctrl-n goes to the next entry in the popup, yeah. They're default Vim keybinds for operating with the completion menu, so there's strictly speaking no need to remap <CR> to get the completions. It's all about preference; a lot of people have gotten used to <CR> to accept the auto-complete suggestion, and want to keep that in Vim, rather than using Vim's default keybinds. While I sympathise with that, it sure does cause compatibility issues when multiple maps are expected to just work :')

There's also a mapping for going up in the list, but I keep forgetting what it is. The docs are probably more useful than me here. I also believe there's even more keybinds for interacting with the popup, but I just remember the ones I press daily. In either case, if manually invoking auto-pairs yourself in the mapping helps, that's still an option if you prefer <CR>

[Frederick888]

a mapping for going up in the list

That's Ctrl-p I believe. n = next, p = previous :D

there's even more keybinds for interacting with the popup

Yup I just checked :verbose imap output and discovered things like Ctrl-e. Years in Vim still learning new things every day!

[LunarWatcher]

I'll just close this for now; I'll rather reopen if creative ideas for working around incompatible plugins, incompatible plugin versions, and async behavior appear. Until then, amending the map to include an explicit call to auto-pairs (+ disabling the built-in mapping), or preferring ctrl-y for completion insertion are viable workarounds. Imperfect solutions for an imperfect system

[LunarWatcher]

For the record, via #72, I've figured out I was wrong. I assumed async because it sounds plausible for this bug, but I now highly doubt that. I'm unable to reproduce locally now, and it should work fine anyway. The c-y workaround is still solid, but my initial thought on the global vs buffer mappings are blatantly wrong. Buffer maps are always preferred, and auto-pairs includes global maps in its buffer map. That's why coc.nvim's remap works at all

[LunarWatcher]

Leaving a final comment here: if someone is able to reproduce this again at any point in time, please consider leaving a comment (or opening a new issue) with the result of imap <cr>. If imap <cr> only contains auto-pairs, run imap <SNR>1234_AutoPairsOldCRWrapper1234, where both instances of 1234 are replaced with the numbers that appear in the map.

For instance, if imap <CR> yields:

i  <CR>        &@<SNR>129_AutoPairsOldCRWrapper73<SNR>129_AutoPairsReturn
    Last set from ~\vimfiles\plugged\auto-pairs\autoload\autopairs\Keybinds.vim line 245

Run imap <SNR>129_AutoPairsOldCRWrapper73, and include the output of that instead. The numbers will vary. There's a lot of possible causes here, and diagnostic steps will be listed in :help autopairs-bad-cr when 4.0.0 is released soon:tm: (it's already available on the 4.0.0 branch, and can be viewed on GitHub without having to switch)

[LunarWatcher]

Good news; finally tracked down the source thanks to an issue from jiangmiao/auto-pairs.

I was partly right. Async stuff causes bad behavior, but this is because both <SNR>s execute in spite of the conditional in the first SNR. It objectively makes sense, just not in the way I thought

The only workaround is

let g:AutoPairsMapCR = 0
inoremap <silent><expr> <cr> coc#pum#visible() ? coc#_select_confirm() : "\<C-g>u\<CR>\<Plug>AutoPairsReturn"

I'll be expanding the documentation with this.

[Frederick888]

Thank you! And this apparently also fixed the conflict in Neo-Tree pop-up somehow. I used to get 'buffer not modifiable' warnings after hitting Enter in Neo-Tree fuzzy search pop-up (by default /), so I had to use inoremap <expr> <CR> coc#pum#visible() ? coc#pum#confirm() : (&filetype == "neo-tree-popup" ? "\<CR>" : "\<CR>\<C-r>=autopairs#Keybinds#IgnoreInsertEnter('autopairs#AutoPairsReturn')\<CR>") Now using your mapping alone everything works as expected :)

…

On 4/2/23 00:33, Olivia (Zoe) wrote: Good news; finally tracked down the source thanks to an issue from jiangmiao/auto-pairs. I was partly right. Async stuff causes bad behavior, but this is because both |<SNR>|s execute in spite of the conditional in the first SNR. It objectively makes sense, just not in the way I thought The only workaround is |let g:AutoPairsMapCR = 0 inoremap <silent><expr> <cr> coc#pum#visible() ? coc#_select_confirm() : "\<C-g>u\<CR>\<Plug>AutoPairsReturn" | I'll be expanding the documentation with this. — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCMP75L47CJY4H42EK2DBDWVUCJFANCNFSM55OWYPNQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Frederick Zhang PGP: 8BFB EA5B 4C44 BFAC C8EC 5F93 1F92 8BE6 0D8B C11D

[LunarWatcher]

Funny how things work out :) That said, you can also add neo-tree-popup to the filetype blacklist