Neovim mode doesn't handle some > and < characters properly

[dbarnett]

Neovim mode still doesn't handle special characters properly. I found that this code from https://github.com/google/maktaba/blob/master/examples/helloworld/vroom/mappings.vroom doesn't work properly:

  :execute 'norm' '<Leader>Hh'

It reports a "No mapping found" message even though the mapping should exist. I expect the angle braces are getting escaped but shouldn't be when they're inside a string literal.

I doubt this is the only bad corner case. We can maybe add a few more levels of cleverness to it, but before long we'll probably need some changes in neovim and/or some rethinking to get it fully correct.

[dbarnett]

@equalsraf Found more escaping issues.

[dbarnett]

Another case I'm expecting to be an issue is a quote character inside quotes.

Looks like push_keys is supposed to support roughly the usage we need, except that I'm not sure it accepts <> notation like . @tarruda, do you know if push_keys or anything else in the python client will accept <> notation the same way vim --remote-send does?

Alternatively, we could see if we could invoke (n)vim itself to interpret the keys somehow.

[tarruda]

Actually vim_push_keys wasn't implemented yet, I think @equalsraf simulated it using vim.command and feedkeys. He's working on it in neovim/neovim#914.

As for parsing special key strings such as <cr>, I don't think it would be a problem since we can simply use the same function that backs feedkeys implementation

[dbarnett]

@equalsraf Sorry, I think I just deleted your comment somehow and can't figure out how to restore it. Here's a copy (I might have messed up some of the formatting. If so and you want to re-post it, I can just delete this copy):

Here is a big patch for the feedkeys logic - still doesn't fix this issue though - it fixes wrongfully escaping < twice and avoids escaping < inside single quotes. The call issued to neovim for that case

call feedkeys(":execute 'norm' '<Leader>Hh'\<CR>")

Which seems correct. Unfortunately I think --remote-send was the one translating . Time to build something equivalent for the Neovim API :D.

Here is the patch -- apologies in advance for the funky re

diff --git a/vroom/neovim_mod.py b/vroom/neovim_mod.py
index e762c13..eefd02b 100644
--- a/vroom/neovim_mod.py
+++ b/vroom/neovim_mod.py
@@ -3,7 +3,7 @@ from vroom.vim import Communicator as VimCommunicator
 import subprocess
 import time
 import neovim
-import os
+import os, re
 try:
   from StringIO import StringIO
 except ImportError:
@@ -44,6 +44,22 @@ class Communicator(VimCommunicator):
         time.sleep(0.01)
     self.conn = neovim.connect(self.args.servername)

+  @staticmethod
+  def EscapeInput(inp):
+    def replace_angle(m):
+        if not m.group().startswith("'"):
+            return re.sub(r"(?<=[^\\])<|^<", r"\\<", m.group())
+        return m.group()
+
+    # Backslashes
+    inp = inp.replace('\\', '\\\\')
+    # Quotes
+    inp = inp.replace('"', '\\"')
+    # Escape <Expr> unless already escaped or within
+    # single quotes ''
+    inp = re.sub(r"'[^']*'|[^']+", replace_angle, inp)
+    return inp
+
   def Communicate(self, command, extra_delay=0):
     """Sends a command to Neovim

@@ -53,11 +69,10 @@ class Communicator(VimCommunicator):
     Raises:
       Quit: If vim quit unexpectedly.
     """
+    neovimcmd = 'call feedkeys("%s")' % self.EscapeInput(command)
-    command = command.replace('\\', '\\\\')
-    command = command.replace('"', '\\"')
-    command = command.replace('<', '\\<')
-    self.conn.command('call feedkeys("%s")' % command)
+    self.writer.Log(neovimcmd)
+    self.conn.command(neovimcmd)
     self._cache = {}
     time.sleep(self.args.delay + extra_delay)

[ZyX-I]

As for parsing special key strings such as , I don't think it would be a problem since we can simply use the same function that backs feedkeys implementation

@tarruda Replying here as well: feedkeys() does not do any <…> transformations. It is a feature of VimL parser if you use double-quoted strings or :map command family parser. Maybe something else is doing this processing as well, but feedkeys() is not on that list.

[equalsraf]

@ZyX-I right you are. Apparently I got confused because --remote-send is doing something else. Namely calling replace_termcodes() ... still investigating.

[dbarnett]

It's hard for me to reason about all these levels of escaping, but couldn't a backslash inside single quotes have issues as well?

I think r"(?<=[^\\])<|^<" can be replaced with r"(?<!\\)<", BTW. And that probably isn't technically right since I think it would get tripped up on things like \\< (which is unescaped but treated as if it's escaped). But as long as we have a plan for fixing it properly, I don't mind a compromise that's slightly less correct and slightly less hideous in the meantime. =)

[equalsraf]

Mmmm, I got tired of fighting over string escaping and backported the original Vim remote-send as a Neovim API function. @tarruda is something like this acceptable? equalsraf/neovim@dd599a9

[dbarnett]

Looks good to me!

BTW, we can update vroom as soon as the changes are in the pypi version. This stuff is still experimental status and I have no big concerns about version compatibility.

[dbarnett]

Looks like the new feedkeys function was merged a few weeks ago. We ready to proceed here?

[equalsraf]

Actually the saga continues in neovim/neovim#920. I've been pretty busy for the past weeks, sorry for the delay.

[dbarnett]

No worries. Thanks for the update!

[equalsraf]

This should be fixed since #45.

[dbarnett]

I see failures in maktaba from vroom --neovim maktaba/vroom/pluginsignals.vroom that seem to be a result of escaping problems. If I use vroom's --interactive and check :let g:weirdpath for neovim, I see

g:weirdpath            /home/dbarnett/.vim/vim-addons/maktaba/vroom/fakeplugins/
weird¬pâ<80>¦l✓u↓g⏎iâ<80>½n

whereas in vim (using --interactive and adding a :throw 'BOOM' to trigger it) I see the expected

g:weirdpath            /home/dbarnett/.vim/vim-addons/maktaba/vroom/fakeplugins/
weird¬p…l✓u↓g⏎i‽n

I can close this issue and open a separate one if you think it's an unrelated problem, but it looks like we're still having trouble getting exactly the right level of escaping.

[equalsraf]

Not quite an escaping error, more like a bug - 'replace_termcodes()' did something unexpected with the character … in utf8 this would be \xe2\x80\xa6 but replace_termcodes turned into '\xe2\x80\xfeX\xa6' i.e. it replaced \x80 with \x80\xfeX. This is basically what Vim uses internally for escaping chars - but it is clearly not being well handled by feedkeys()

I'll need to have a deeper look.

[tarruda]

Yes, the wrong arguments were being passed to replace_termcodes. The following patch should fix the problem:

diff --git a/vroom/neovim_mod.py b/vroom/neovim_mod.py
index f50ec42..3c6fcb2 100644
--- a/vroom/neovim_mod.py
+++ b/vroom/neovim_mod.py
@@ -52,7 +52,7 @@ class Communicator(VimCommunicator):
       Quit: If vim quit unexpectedly.
     """
     self.writer.Log(command)
-    parsed_command = self.conn.replace_termcodes(command, True, True, True)
+    parsed_command = self.conn.replace_termcodes(command, False, True, False)
     self.conn.feedkeys(parsed_command)
     self._cache = {}
     time.sleep(self.args.delay + extra_delay)

[tarruda]

Nevermind the patch above, it fixes only that test because it disables escaping, but fails all others

[equalsraf]

@tarruda I think the way to go about it is to disable the K_SPECIAL escaping in term.c:replace_termcodes() (only when it is called from the API) - I'll get a PR going shortly.

[equalsraf]

Looking at it it easier to alter vim_feedkeys to skip escaping.

[tarruda]

@tarruda I think the way to go about it is to disable the K_SPECIAL escaping in term.c:replace_termcodes() (only when it is called from the API) - I'll get a PR going shortly

I just looked at replace_termcodes and the code which takes care of skipping multibyte characters seems to be wrong. I have no idea why this works on vim though

[equalsraf]

@tarruda not quite, replace_termcodes() escapes K_SPECIAL(0x80) in the string once and returns the internal Vim representation BUT vim_feedkeys will escape 0x80 a second time. As far as I can see we can

1. Change vim_replace_termcodes to make escaping of K_SPECIAL optional - this seems tricky in some corner cases (from_part=True)
2. Change vim_feedkeys() to make escaping optional() just add a new parameter to skip vim_strsave_escape_csi()

Both seem to fix the issue

[tarruda]

@tarruda not quite, replace_termcodes() escapes K_SPECIAL(0x80) in the string once and returns the internal Vim representation BUT vim_feedkeys will escape 0x80 a second time.

👍