[RFC] Fix append() with negative line numbers.

[choco]

In Vim bound checking is performed from 0 to buffer.linecount, and
returns error if trying to operate outside these bounds. Neovim allows to
use negative indexes but excludes buffer.linecount making its
implementation incompatible with Vim's one.

To see the issue in action

:python import vim; vim.current.buffer.append("Hello there", len(vim.currrent.buffer))

This fixes the incompatibility also maintaining the ability to index using
negative numbers (that would otherwise break python-client
implementation).

This also fixes junegunn/vim-plug#278

[choco]

Failing some tests, gonna take a look later

[choco]

Mmmmm, ok this works. It's not as pretty as the fix I had in mind but I think this part should be rewritten anyway (remove include_start, include_end, etc...). This is just temporary so python plugins work the same in Vim and neovim.

[justinmk]

@choco Thanks. It doesn't matter to me one way or other, but this feature was added to the API in #2846. So, if it's an actually desirable feature, perhaps the API should have this feature, but the specific behavior of :python can be fixed in runtime/autoload/provider/script_host.py.

On the other hand if this is a niche feature, the inconsistency may not be worth it. I need @nhynes to comment here. @fwalch @bfredl @tarruda also need your opinion.

[choco]

I completely understand, I was actually discussing in the vim-plug issue junegunn/vim-plug#278 what the best way was to get around the problem. In the end I though it was better to let you decide :) Fixing it on python side would require adding a request for the lines number, not that it matters from a performance point of view but maybe it's something to consider.

Anyway the second part of the fix is needed because the change I made yesterday breaks append() without arguments in neovim. I didn't notice at first since the append function isn't covered by any test. So if you decide in the end to maintain the current behavior I'll edit the commit and remove the bound checking hack.

[justinmk]

Oh. Someone should write a test for append()...

[nhynes]

@choco Although the command you're trying to issue is semantically valid, there's the underlying problem of not being able to lock a buffer and prevent the line length from changing between the time you fetch it and the time you make the append. Since buffer_line_count in any compound operation is potentially unsafe, I'm not sure if it's a good idea to encourage its use over the atomic (but albeit less idiomatic) -1 index, which is what I think that this change would do.

On the other hand, it would certainly be nice to be consistent with setline. If you or the other participants deem race conditions sufficiently unlikely (I suppose we'd need a larger rplugin ecosystem to know this for sure), then it might be more appropriate to add if (start == nlines) start = -1 to set_line_sice, itself.

[choco]

@nhynes Hi, thank you so much for your comment, I just have a couple of questions if you don't mind. Why is calling buffer_line_count here prone to race conditions? Aren't we simply getting the line count from the buffer directly here, in the same API request as buffer_insert?
Just a little note, adding the check directly in set_line_slice would also change the behavior of set_line, thus again making it inconsistent with Vim implementation.

[nhynes]

@choco Consider the following scenario:

1. client1 calls len(buf) which initiates an async RPC
2. nvim receives client1's request and returns the current length of buf
3. client1 calls append('text', buf_len)
4. at some point before step 3, client2 calls delete buf[1:5]
5. due to the OS scheduler or the implementation of the plugin host, nvim processes client2's request before client1's second request
6. nvim receives client1's second request, which now contains a line number that is out of bounds

To clarify, there's no issue with calling buffer_* methods since, at that point, things are already single threaded. The problem arises from the operations of multiple clients on the same buffer.

[ZyX-I]

Code standard requires using

if (lnum == nlines) {
  lnum = -1
}

: http://neovim.io/develop/style-guide.xml?showone=Conditionals#Conditionals.

[choco]

@nhynes That makes complete sense, I wasn't thinking about multiple clients for some reason (fell ashamed...)
Actually I completely agree that the -1 approach is the best, but I still think that adding Vim compatibility would be beneficial. Obviously I don't wanna force anything so I'll let you guys decide.

[ghost]

IMO, this is just as clear bool end_start = lnum < 0;

If you disagree that's fine; I don't want to waste time bikeshedding ;)

[ZyX-I]

Use Integer here, not linenr_T. Otherwise you will need one (here) or two (here and in condition) casts to pass travis tests. Also note that buffer_set_line_slice accepts Integer as well, though I think it will not complain because sizeof(Integer) >= sizeof(linenr_T) always.

And I think that you need to abort if buffer_line_count returned zero (== aborted with error, but checking for err->set is rather uncommon for some reason).

[tarruda]

@nhynes / @choco there's a way to "lock" nvim for multiple operations by one client: Make nvim call the client via msgpack-rpc and only complete the call when all client operations are complete. It is not very polished but something like this should work:

• client evaluates rpcrequest to some stub method passing it's own channel id
• while the stub method doesn't return, nvim will continue to handle methods from that client alone. so on the handler of the stub method, the client can make multiple rpc calls
  • buffer_line_count
  • some insert/append operation that depends on the count

@choco this means it may be possible to implement this 100% in the python side. When making compatibility changes in the python interface, I prefer if they are made in the compatibility layer so it doesn't affect clients in other languages. In this particular case, it is necessary to add the python client infrastructure to send atomic "transactions" to nvim

[choco]

@tarruda I'm not sure I fully understand, do you mean introducing a new method in the api that allows us to lock vim into executing request from only one client until the client release that lock, i.e.:

vim.lock()
but_len = len(vim.current.buffer)
vim.current.buffer.append(len)
vim.unlock()

or simply implementing this lock and keeping it private and using it in some method to perform multiple atomic operation exposed by the api?
Also would the second case still be really useful? The presence of multiple clients would still mess with the state of nvim, and the single client is already fine right now.
Example:

Client1: but_len = len(vim.current.buffer)
Client2: vim.current.buffer.append("something")
Client1: vim.current.buffer.append(but_len)
                            ^
locks, and checks if but_len==len(vim.current.buffer) by
calling buffer_line_count, it's not, but it's what the user meant.

I still think it makes sense to do it, but it doesn't solve the problem completely.

---

I modified the commit to just fix the regression introduced with #3315 and will leave other changes to another pull request when I have more time to work on it.