Merge pull request #4083 from bfredl/oob

api/buffer: add get/set_lines with more flexible out-of-bounds handling and deprecate the line_slice functions

7 files changed, 278 insertions, 41 deletions

diff --git a/src/nvim/api/buffer.c b/src/nvim/api/buffer.c
index fa4b8e5f7d..c25a9789c5 100644
--- a/src/nvim/api/buffer.c
+++ b/src/nvim/api/buffer.c
@@ -45,14 +45,22 @@ Integer buffer_line_count(Buffer buffer, Error *err)
 
 /// Gets a buffer line
 ///
+/// @deprecated use buffer_get_lines instead.
+///             for positive indices (including 0) use
+///                 "buffer_get_lines(buffer, index, index+1, true)"
+///             for negative indices use
+///                 "buffer_get_lines(buffer, index-1, index, true)"
+///
 /// @param buffer The buffer handle
 /// @param index The line index
 /// @param[out] err Details of an error that may have occurred
 /// @return The line string
 String buffer_get_line(Buffer buffer, Integer index, Error *err)
 {
-  String rv = {.size = 0};
-  Array slice = buffer_get_line_slice(buffer, index, index, true, true, err);
+  String rv = { .size = 0 };
+
+  index = convert_index(index);
+  Array slice = buffer_get_lines(buffer, index, index+1, true, err);
 
   if (!err->set && slice.size) {
     rv = slice.items[0].data.string;
@@ -65,6 +73,12 @@ String buffer_get_line(Buffer buffer, Integer index, Error *err)
 
 /// Sets a buffer line
 ///
+/// @deprecated use buffer_set_lines instead.
+///             for positive indices use
+///                 "buffer_set_lines(buffer, index, index+1, true, [line])"
+///             for negative indices use
+///                 "buffer_set_lines(buffer, index-1, index, true, [line])"
+///
 /// @param buffer The buffer handle
 /// @param index The line index
 /// @param line The new line.
@@ -72,23 +86,34 @@ String buffer_get_line(Buffer buffer, Integer index, Error *err)
 void buffer_set_line(Buffer buffer, Integer index, String line, Error *err)
 {
   Object l = STRING_OBJ(line);
-  Array array = {.items = &l, .size = 1};
-  buffer_set_line_slice(buffer, index, index, true, true, array, err);
+  Array array = { .items = &l, .size = 1 };
+  index = convert_index(index);
+  buffer_set_lines(buffer, index, index+1, true,  array, err);
 }
 
 /// Deletes a buffer line
 ///
+/// @deprecated use buffer_set_lines instead.
+///             for positive indices use
+///                 "buffer_set_lines(buffer, index, index+1, true, [])"
+///             for negative indices use
+///                 "buffer_set_lines(buffer, index-1, index, true, [])"
 /// @param buffer The buffer handle
 /// @param index The line index
 /// @param[out] err Details of an error that may have occurred
 void buffer_del_line(Buffer buffer, Integer index, Error *err)
 {
   Array array = ARRAY_DICT_INIT;
-  buffer_set_line_slice(buffer, index, index, true, true, array, err);
+  index = convert_index(index);
+  buffer_set_lines(buffer, index, index+1, true, array, err);
 }
 
 /// Retrieves a line range from the buffer
 ///
+/// @deprecated use buffer_get_lines(buffer, newstart, newend, false)
+///             where newstart = start + int(not include_start) - int(start < 0)
+///                   newend = end + int(include_end) - int(end < 0)
+///                   int(bool) = 1 if bool is true else 0
 /// @param buffer The buffer handle
 /// @param start The first line index
 /// @param end The last line index
@@ -103,16 +128,48 @@ ArrayOf(String) buffer_get_line_slice(Buffer buffer,
                                  Boolean include_end,
                                  Error *err)
 {
+  start = convert_index(start) + !include_start;
+  end = convert_index(end) + include_end;
+  return buffer_get_lines(buffer, start , end, false, err);
+}
+
+
+/// Retrieves a line range from the buffer
+///
+/// Indexing is zero-based, end-exclusive. Negative indices are interpreted
+/// as length+1+index, i e -1 refers to the index past the end. So to get the
+/// last element set start=-2 and end=-1.
+///
+/// Out-of-bounds indices are clamped to the nearest valid value, unless
+/// `strict_indexing` is set.
+///
+/// @param buffer The buffer handle
+/// @param start The first line index
+/// @param end The last line index (exclusive)
+/// @param strict_indexing whether out-of-bounds should be an error.
+/// @param[out] err Details of an error that may have occurred
+/// @return An array of lines
+ArrayOf(String) buffer_get_lines(Buffer buffer,
+                                 Integer start,
+                                 Integer end,
+                                 Boolean strict_indexing,
+                                 Error *err)
+{
   Array rv = ARRAY_DICT_INIT;
   buf_T *buf = find_buffer_by_handle(buffer, err);
 
-  if (!buf || !inbounds(buf, start)) {
+  if (!buf) {
     return rv;
   }
 
-  start = normalize_index(buf, start) + (include_start ? 0 : 1);
-  include_end = include_end || (end >= buf->b_ml.ml_line_count);
-  end = normalize_index(buf, end) + (include_end ? 1 : 0);
+  bool oob = false;
+  start = normalize_index(buf, start, &oob);
+  end = normalize_index(buf, end, &oob);
+
+  if (strict_indexing && oob) {
+    api_set_error(err, Validation, _("Index out of bounds"));
+    return rv;
+  }
 
   if (start >= end) {
     // Return 0-length array
@@ -152,8 +209,14 @@ end:
   return rv;
 }
 
+
 /// Replaces a line range on the buffer
 ///
+/// @deprecated use buffer_set_lines(buffer, newstart, newend, false, lines)
+///             where newstart = start + int(not include_start) + int(start < 0)
+///                   newend = end + int(include_end) + int(end < 0)
+///                   int(bool) = 1 if bool is true else 0
+///
 /// @param buffer The buffer handle
 /// @param start The first line index
 /// @param end The last line index
@@ -170,20 +233,52 @@ void buffer_set_line_slice(Buffer buffer,
                       ArrayOf(String) replacement,
                       Error *err)
 {
+  start = convert_index(start) + !include_start;
+  end = convert_index(end) + include_end;
+  buffer_set_lines(buffer, start, end, false, replacement, err);
+}
+
+
+/// Replaces line range on the buffer
+///
+/// Indexing is zero-based, end-exclusive. Negative indices are interpreted
+/// as length+1+index, i e -1 refers to the index past the end. So to change
+/// or delete the last element set start=-2 and end=-1.
+///
+/// To insert lines at a given index, set both start and end to the same index.
+/// To delete a range of lines, set replacement to an empty array.
+///
+/// Out-of-bounds indices are clamped to the nearest valid value, unless
+/// `strict_indexing` is set.
+///
+/// @param buffer The buffer handle
+/// @param start The first line index
+/// @param end The last line index (exclusive)
+/// @param strict_indexing whether out-of-bounds should be an error.
+/// @param replacement An array of lines to use as replacement
+/// @param[out] err Details of an error that may have occurred
+void buffer_set_lines(Buffer buffer,
+                      Integer start,
+                      Integer end,
+                      Boolean strict_indexing,
+                      ArrayOf(String) replacement,
+                      Error *err)
+{
   buf_T *buf = find_buffer_by_handle(buffer, err);
 
   if (!buf) {
     return;
   }
 
-  if (!inbounds(buf, start)) {
+  bool oob = false;
+  start = normalize_index(buf, start, &oob);
+  end = normalize_index(buf, end, &oob);
+
+  if (strict_indexing && oob) {
     api_set_error(err, Validation, _("Index out of bounds"));
     return;
   }
 
-  start = normalize_index(buf, start) + (include_start ? 0 : 1);
-  include_end = include_end || (end >= buf->b_ml.ml_line_count);
-  end = normalize_index(buf, end) + (include_end ? 1 : 0);
 
   if (start > end) {
     api_set_error(err,
@@ -457,6 +552,8 @@ Boolean buffer_is_valid(Buffer buffer)
 
 /// Inserts a sequence of lines to a buffer at a certain index
 ///
+/// @deprecated use buffer_set_lines(buffer, lnum, lnum, true, lines)
+///
 /// @param buffer The buffer handle
 /// @param lnum Insert the lines after `lnum`. If negative, it will append
 ///        to the end of the buffer.
@@ -467,8 +564,9 @@ void buffer_insert(Buffer buffer,
                    ArrayOf(String) lines,
                    Error *err)
 {
-  bool end_start = lnum < 0;
-  buffer_set_line_slice(buffer, lnum, lnum, !end_start, end_start, lines, err);
+  // "lnum" will be the index of the line after inserting,
+  // no matter if it is negative or not
+  buffer_set_lines(buffer, lnum, lnum, true, lines, err);
 }
 
 /// Return a tuple (row,col) representing the position of the named mark
@@ -632,20 +730,26 @@ static void fix_cursor(linenr_T lo, linenr_T hi, linenr_T extra)
 }
 
 // Normalizes 0-based indexes to buffer line numbers
-static int64_t normalize_index(buf_T *buf, int64_t index)
+static int64_t normalize_index(buf_T *buf, int64_t index, bool *oob)
 {
+  int64_t line_count = buf->b_ml.ml_line_count;
   // Fix if < 0
-  index = index < 0 ?  buf->b_ml.ml_line_count + index : index;
+  index = index < 0 ? line_count + index +1 : index;
+
+  // Check for oob
+  if (index > line_count) {
+    *oob = true;
+    index = line_count;
+  } else if (index < 0) {
+    *oob = true;
+    index = 0;
+  }
   // Convert the index to a vim line number
   index++;
-  // Fix if > line_count
-  index = index > buf->b_ml.ml_line_count ? buf->b_ml.ml_line_count : index;
   return index;
 }
 
-// Returns true if the 0-indexed `index` is within the 1-indexed buffer bounds.
-static bool inbounds(buf_T *buf, int64_t index)
+static int64_t convert_index(int64_t index)
 {
-  linenr_T nlines = buf->b_ml.ml_line_count;
-  return index >= -nlines && index < nlines;
+  return index < 0 ? index - 1 : index;
 }
diff --git a/test/functional/api/buffer_spec.lua b/test/functional/api/buffer_spec.lua
index c924988d06..a15d489a1f 100644
--- a/test/functional/api/buffer_spec.lua
+++ b/test/functional/api/buffer_spec.lua
@@ -1,8 +1,8 @@
 -- Sanity checks for buffer_* API calls via msgpack-rpc
 local helpers = require('test.functional.helpers')
-local clear, nvim, buffer, curbuf, curwin, eq, ok =
-  helpers.clear, helpers.nvim, helpers.buffer, helpers.curbuf, helpers.curwin,
-  helpers.eq, helpers.ok
+local clear, nvim, buffer = helpers.clear, helpers.nvim, helpers.buffer
+local curbuf, curwin, eq = helpers.curbuf, helpers.curwin, helpers.eq
+local curbufmeths, ok = helpers.curbufmeths, helpers.ok
 
 describe('buffer_* functions', function()
   before_each(clear)
@@ -35,10 +35,11 @@ describe('buffer_* functions', function()
       eq('', curbuf('get_line', 0))
     end)
 
-    it('get_line: out-of-bounds returns empty string', function()
+    it('get_line: out-of-bounds is an error', function()
       curbuf('set_line', 0, 'line1.a')
-      eq('', curbuf('get_line', 1))
-      eq('', curbuf('get_line', -2))
+      eq(1, curbuf('line_count')) -- sanity
+      eq(false, pcall(curbuf, 'get_line', 1))
+      eq(false, pcall(curbuf, 'get_line', -2))
     end)
 
     it('set_line, del_line: out-of-bounds is an error', function()
@@ -68,14 +69,16 @@ describe('buffer_* functions', function()
       eq({}, curbuf('get_line_slice', -4, -5, true, true))
     end)
 
-    it('set_line_slice: out-of-bounds is an error', function()
+    it('set_line_slice: out-of-bounds extends past end', function()
       curbuf('set_line_slice', 0, 0, true, true, {'a', 'b', 'c'})
       eq({'a', 'b', 'c'}, curbuf('get_line_slice', 0, 2, true, true)) --sanity
 
       eq({'c'}, curbuf('get_line_slice', -1, 4, true, true))
       eq({'a', 'b', 'c'}, curbuf('get_line_slice', 0, 5, true, true))
-      eq(false, pcall(curbuf, 'set_line_slice', 4, 5, true, true, {'d'}))
-      eq(false, pcall(curbuf, 'set_line_slice', -4, -5, true, true, {'d'}))
+      curbuf('set_line_slice', 4, 5, true, true, {'d'})
+      eq({'a', 'b', 'c', 'd'}, curbuf('get_line_slice', 0, 5, true, true))
+      curbuf('set_line_slice', -4, -5, true, true, {'e'})
+      eq({'e', 'a', 'b', 'c', 'd'}, curbuf('get_line_slice', 0, 5, true, true))
     end)
 
     it('works', function()
@@ -101,6 +104,136 @@ describe('buffer_* functions', function()
     end)
   end)
 
+  describe('{get,set}_lines', function()
+    local get_lines, set_lines = curbufmeths.get_lines, curbufmeths.set_lines
+    local line_count = curbufmeths.line_count
+
+    it('has correct line_count when inserting and deleting', function()
+      eq(1, line_count())
+      set_lines(-1, -1, true, {'line'})
+      eq(2, line_count())
+      set_lines(-1, -1, true, {'line'})
+      eq(3, line_count())
+      set_lines(-2, -1, true, {})
+      eq(2, line_count())
+      set_lines(-2, -1, true, {})
+      set_lines(-2, -1, true, {})
+      -- There's always at least one line
+      eq(1, line_count())
+    end)
+
+    it('can get, set and delete a single line', function()
+      eq({''}, get_lines(0, 1, true))
+      set_lines(0, 1, true, {'line1'})
+      eq({'line1'}, get_lines(0, 1, true))
+      set_lines(0, 1, true, {'line2'})
+      eq({'line2'}, get_lines(0, 1, true))
+      set_lines(0, 1, true, {})
+      eq({''}, get_lines(0, 1, true))
+    end)
+
+    it('can get a single line with strict indexing', function()
+      set_lines(0, 1, true, {'line1.a'})
+      eq(1, line_count()) -- sanity
+      eq(false, pcall(get_lines, 1, 2, true))
+      eq(false, pcall(get_lines, -3, -2, true))
+    end)
+
+    it('can get a single line with non-strict indexing', function()
+      set_lines(0, 1, true, {'line1.a'})
+      eq(1, line_count()) -- sanity
+      eq({}, get_lines(1, 2, false))
+      eq({}, get_lines(-3, -2, false))
+    end)
+
+    it('can set and delete a single line with strict indexing', function()
+      set_lines(0, 1, true, {'line1.a'})
+      eq(false, pcall(set_lines, 1, 2, true, {'line1.b'}))
+      eq(false, pcall(set_lines, -3, -2, true, {'line1.c'}))
+      eq({'line1.a'}, get_lines(0, -1, true))
+      eq(false, pcall(set_lines, 1, 2, true, {}))
+      eq(false, pcall(set_lines, -3, -2, true, {}))
+      eq({'line1.a'}, get_lines(0, -1, true))
+    end)
+
+    it('can set and delete a single line with non-strict indexing', function()
+      set_lines(0, 1, true, {'line1.a'})
+      set_lines(1, 2, false, {'line1.b'})
+      set_lines(-4, -3, false, {'line1.c'})
+      eq({'line1.c', 'line1.a', 'line1.b'}, get_lines(0, -1, true))
+      set_lines(3, 4, false, {})
+      set_lines(-5, -4, false, {})
+      eq({'line1.c', 'line1.a', 'line1.b'}, get_lines(0, -1, true))
+    end)
+
+    it('can handle NULs', function()
+      set_lines(0, 1, true, {'ab\0cd'})
+      eq({'ab\0cd'}, get_lines(0, -1, true))
+    end)
+
+    it('works with multiple lines', function()
+      eq({''}, get_lines(0, -1, true))
+      -- Replace buffer
+      for _, mode in pairs({false, true}) do
+        set_lines(0, -1, mode, {'a', 'b', 'c'})
+        eq({'a', 'b', 'c'}, get_lines(0, -1, mode))
+        eq({'b', 'c'}, get_lines(1, -1, mode))
+        eq({'b'}, get_lines(1, 2, mode))
+        eq({}, get_lines(1, 1, mode))
+        eq({'a', 'b'}, get_lines(0, -2, mode))
+        eq({'b'}, get_lines(1, -2, mode))
+        eq({'b', 'c'}, get_lines(-3, -1, mode))
+        set_lines(1, 2, mode, {'a', 'b', 'c'})
+        eq({'a', 'a', 'b', 'c', 'c'}, get_lines(0, -1, mode))
+        set_lines(-2, -1, mode, {'a', 'b', 'c'})
+        eq({'a', 'a', 'b', 'c', 'a', 'b', 'c'},
+          get_lines(0, -1, mode))
+        set_lines(0, -4, mode, {})
+        eq({'a', 'b', 'c'}, get_lines(0, -1, mode))
+        set_lines(0, -1, mode, {})
+        eq({''}, get_lines(0, -1, mode))
+      end
+    end)
+
+    it('can get line ranges with non-strict indexing', function()
+      set_lines(0, -1, true, {'a', 'b', 'c'})
+      eq({'a', 'b', 'c'}, get_lines(0, -1, true)) --sanity
+
+      eq({}, get_lines(3, 4, false))
+      eq({}, get_lines(3, 10, false))
+      eq({}, get_lines(-5, -5, false))
+      eq({}, get_lines(3, -1, false))
+      eq({}, get_lines(-3, -4, false))
+    end)
+
+    it('can get line ranges with strict indexing', function()
+      set_lines(0, -1, true, {'a', 'b', 'c'})
+      eq({'a', 'b', 'c'}, get_lines(0, -1, true)) --sanity
+
+      eq(false, pcall(get_lines, 3, 4, true))
+      eq(false, pcall(get_lines, 3, 10, true))
+      eq(false, pcall(get_lines, -5, -5, true))
+      -- empty or inverted ranges are not errors
+      eq({}, get_lines(3, -1, true))
+      eq({}, get_lines(-3, -4, true))
+    end)
+
+    it('set_line_slice: out-of-bounds can extend past end', function()
+      set_lines(0, -1, true, {'a', 'b', 'c'})
+      eq({'a', 'b', 'c'}, get_lines(0, -1, true)) --sanity
+
+      eq({'c'}, get_lines(-2, 5, false))
+      eq({'a', 'b', 'c'}, get_lines(0, 6, false))
+      eq(false, pcall(set_lines, 4, 6, true, {'d'}))
+      set_lines(4, 6, false, {'d'})
+      eq({'a', 'b', 'c', 'd'}, get_lines(0, -1, true))
+      eq(false, pcall(set_lines, -6, -6, true, {'e'}))
+      set_lines(-6, -6, false, {'e'})
+      eq({'e', 'a', 'b', 'c', 'd'}, get_lines(0, -1, true))
+    end)
+
+  end)
+
   describe('{get,set}_var', function()
     it('works', function()
       curbuf('set_var', 'lua', {1, 2, {['3'] = 1}})
diff --git a/test/functional/api/server_requests_spec.lua b/test/functional/api/server_requests_spec.lua
index c0099e44c4..1b33275803 100644
--- a/test/functional/api/server_requests_spec.lua
+++ b/test/functional/api/server_requests_spec.lua
@@ -165,8 +165,8 @@ describe('server -> client', function()
 
       eq('SOME TEXT', eval("rpcrequest(vim, 'buffer_get_line', "..buf..", 0)"))
 
-      -- Call get_line_slice(buf, range [0,0], includes start, includes end)
-      eq({'SOME TEXT'}, eval("rpcrequest(vim, 'buffer_get_line_slice', "..buf..", 0, 0, 1, 1)"))
+      -- Call get_lines(buf, range [0,0], strict_indexing)
+      eq({'SOME TEXT'}, eval("rpcrequest(vim, 'buffer_get_lines', "..buf..", 0, 1, 1)"))
     end)
 
     it('returns an error if the request failed', function()
diff --git a/test/functional/autocmd/textyankpost_spec.lua b/test/functional/autocmd/textyankpost_spec.lua
index 965b19581a..0e46aa5641 100644
--- a/test/functional/autocmd/textyankpost_spec.lua
+++ b/test/functional/autocmd/textyankpost_spec.lua
@@ -14,7 +14,7 @@ describe('TextYankPost', function()
     execute('autocmd TextYankPost * let g:event = copy(v:event)')
     execute('autocmd TextYankPost * let g:count += 1')
 
-    curbufmeths.set_line_slice(0, -1, true, true, {
+    curbufmeths.set_lines(0, -1, true, {
       'foo\0bar',
       'baz text',
     })
diff --git a/test/functional/helpers.lua b/test/functional/helpers.lua
index 55c97d451b..4e294029ab 100644
--- a/test/functional/helpers.lua
+++ b/test/functional/helpers.lua
@@ -320,7 +320,7 @@ local function curbuf_contents()
   -- previously sent keys are processed(vim_eval is a deferred function, and
   -- only processed after all input)
   wait()
-  return table.concat(curbuf('get_line_slice', 0, -1, true, true), '\n')
+  return table.concat(curbuf('get_lines', 0, -1, true), '\n')
 end
 
 local function curwin(method, ...)
diff --git a/test/functional/plugin/shada_spec.lua b/test/functional/plugin/shada_spec.lua
index 4100a30452..a3f617eeb0 100644
--- a/test/functional/plugin/shada_spec.lua
+++ b/test/functional/plugin/shada_spec.lua
@@ -2215,7 +2215,7 @@ describe('In plugin/shada.vim', function()
   describe('event BufWriteCmd', function()
     it('works', function()
       nvim('set_var', 'shada#add_own_header', 0)
-      curbuf('set_line_slice', 0, 0, true, true, {
+      curbuf('set_lines', 0, 1, true, {
         'Jump with timestamp ' .. epoch .. ':',
         '  % Key________  Description  Value',
         '  + n            name         \'A\'',
@@ -2271,7 +2271,7 @@ describe('In plugin/shada.vim', function()
   describe('event FileWriteCmd', function()
     it('works', function()
       nvim('set_var', 'shada#add_own_header', 0)
-      curbuf('set_line_slice', 0, 0, true, true, {
+      curbuf('set_lines', 0, 1, true, {
         'Jump with timestamp ' .. epoch .. ':',
         '  % Key________  Description  Value',
         '  + n            name         \'A\'',
@@ -2310,7 +2310,7 @@ describe('In plugin/shada.vim', function()
   describe('event FileAppendCmd', function()
     it('works', function()
       nvim('set_var', 'shada#add_own_header', 0)
-      curbuf('set_line_slice', 0, 0, true, true, {
+      curbuf('set_lines', 0, 1, true, {
         'Jump with timestamp ' .. epoch .. ':',
         '  % Key________  Description  Value',
         '  + n            name         \'A\'',
@@ -2512,7 +2512,7 @@ describe('syntax/shada.vim', function()
   it('works', function()
     nvim_command('syntax on')
     nvim_command('setlocal syntax=shada')
-    curbuf('set_line_slice', 0, 0, true, true, {
+    curbuf('set_lines', 0, 1, true, {
       'Header with timestamp ' .. epoch .. ':',
       '  % Key  Value',
       '  + t    "test"',
diff --git a/test/functional/terminal/edit_spec.lua b/test/functional/terminal/edit_spec.lua
index 6da1521121..dcc4a54610 100644
--- a/test/functional/terminal/edit_spec.lua
+++ b/test/functional/terminal/edit_spec.lua
@@ -70,6 +70,6 @@ describe(':edit term://*', function()
     end
     exp_screen = exp_screen .. (' '):rep(columns) .. '|\n'
     scr:expect(exp_screen)
-    eq(bufcontents, curbufmeths.get_line_slice(1, -1, true, true))
+    eq(bufcontents, curbufmeths.get_lines(1, -1, true))
   end)
 end)