api: set_text: fix validation and some issues

fix double free because intermediary lines weren't xmemdup'd.

NL-for-NUL dance.

Normalize row indices and perform more validation.

Adjust the cursor position if it's on the right side of the replacement.

Tests and documentation.

src/nvim/api/buffer.c

@@ -453,7 +453,7 @@ void nvim_buf_set_lines(uint64_t channel_id,
 
   // Adjust marks. Invalidate any which lie in the
   // changed range, and move any in the remainder of the buffer.
-  // Only adjust mapks if we managed to switch to a window that holds
+  // Only adjust marks if we managed to switch to a window that holds
   // the buffer, otherwise line numbers will be invalid.
   mark_adjust((linenr_T)start,
               (linenr_T)(end - 1),
@@ -474,6 +474,23 @@ end:
   try_end(err);
 }
 
+/// Sets (replaces) a range in the buffer.
+///
+/// Indexing is zero-based, end-exclusive.
+///
+/// To insert text at a given index, set `start` and `end` ranges to the same
+/// index. To delete a range, set `replacement` to an empty array.
+///
+/// Prefer nvim_buf_set_lines when modifying entire lines.
+///
+/// @param channel_id
+/// @param buffer           Buffer handle, or 0 for current buffer
+/// @param start_row        First line index
+/// @param start_column     Last column
+/// @param end_row          Last line index (exclusive)
+/// @param end_column       Last column
+/// @param replacement      Array of lines to use as replacement
+/// @param[out] err         Error details, if any
 void nvim_buf_set_text(uint64_t channel_id,
                        Buffer buffer,
                        Integer start_row,
@@ -490,29 +507,48 @@ void nvim_buf_set_text(uint64_t channel_id,
   if (!buf) {
     return;
   }
-  size_t new_len = replacement.size;
 
-  // TODO: do the nl-for-NUL dance as well?
+  bool oob = false;
-  bool disallow_nl = (channel_id != VIML_INTERNAL_CALL);
+
-  if (!check_string_array(replacement, disallow_nl, err)) {
+  // check range is ordered and everything!
+  // start_row, end_row within buffer len (except add text past the end?)
+  start_row = normalize_index(buf, start_row, &oob);
+  if (oob) {
+    api_set_error(err, kErrorTypeValidation, "start_row out of bounds");
     return;
   }
 
-  // TODO: check range is ordered and everything!
+  end_row = normalize_index(buf, end_row, &oob);
-  // start_row, end_row within buffer len (except add text past the end?)
+  /* if (oob) { */
-  char *str_at_start = (char *)ml_get_buf(buf, start_row+1, false);
+  /*   api_set_error(err, kErrorTypeValidation, "end_row out of bounds"); */
+  /*   return; */
+  /* } */
+
+  char *str_at_start = (char *)ml_get_buf(buf, start_row, false);
   if (start_col < 0 || (size_t)start_col > strlen(str_at_start)) {
     api_set_error(err, kErrorTypeValidation, "start_col out of bounds");
     return;
   }
 
-  char *str_at_end = (char *)ml_get_buf(buf, end_row+1, false);
+  char *str_at_end = (char *)ml_get_buf(buf, end_row, false);
   size_t len_at_end = strlen(str_at_end);
   if (end_col < 0 || (size_t)end_col > len_at_end) {
     api_set_error(err, kErrorTypeValidation, "end_col out of bounds");
     return;
   }
 
+  if (start_row > end_row || (end_row == start_row && start_col > end_col)) {
+    api_set_error(err, kErrorTypeValidation, "start is higher than end");
+    return;
+  }
+
+  bool disallow_nl = (channel_id != VIML_INTERNAL_CALL);
+  if (!check_string_array(replacement, disallow_nl, err)) {
+    return;
+  }
+
+  size_t new_len = replacement.size;
+
   String first_item = replacement.items[0].data.string;
   String last_item = replacement.items[replacement.size-1].data.string;
   size_t firstlen = (size_t)start_col+first_item.size;
@@ -523,18 +559,26 @@ void nvim_buf_set_text(uint64_t channel_id,
   char *first = xmallocz(firstlen), *last = NULL;
   memcpy(first, str_at_start, (size_t)start_col);
   memcpy(first+start_col, first_item.data, first_item.size);
+  memchrsub(first+start_col, NUL, NL, first_item.size);
   if (replacement.size == 1) {
     memcpy(first+start_col+first_item.size, str_at_end+end_col, last_part_len);
   } else {
     last = xmallocz(last_item.size+last_part_len);
     memcpy(last, last_item.data, last_item.size);
+    memchrsub(last, NUL, NL, last_item.size);
     memcpy(last+last_item.size, str_at_end+end_col, last_part_len);
   }
 
   char **lines = (new_len != 0) ? xcalloc(new_len, sizeof(char *)) : NULL;
   lines[0] = first;
   for (size_t i = 1; i < new_len-1; i++) {
-    lines[i] = replacement.items[i].data.string.data;
+    const String l = replacement.items[i].data.string;
+
+    // Fill lines[i] with l's contents. Convert NULs to newlines as required by
+    // NL-used-for-NUL.
+    lines[i] = xmemdupz(l.data, l.size);
+    memchrsub(lines[i], NUL, NL, l.size);
+    /* lines[i] = replacement.items[i].data.string.data; */
   }
   if (replacement.size > 1) {
     lines[replacement.size-1] = last;
@@ -549,13 +593,11 @@ void nvim_buf_set_text(uint64_t channel_id,
     goto end;
   }
 
-  // TODO: dubbel KOLLA KOLLA indexen här
+  if (u_save((linenr_T)start_row - 1, (linenr_T)end_row) == FAIL) {
-  if (u_save((linenr_T)start_row, (linenr_T)end_row+2) == FAIL) {
     api_set_error(err, kErrorTypeException, "Failed to save undo information");
     goto end;
   }
 
-
   ptrdiff_t extra = 0;  // lines added to text, can be negative
   size_t old_len = (size_t)(end_row-start_row+1);
 
@@ -564,7 +606,7 @@ void nvim_buf_set_text(uint64_t channel_id,
   // repeatedly deleting line "start".
   size_t to_delete = (new_len < old_len) ? (size_t)(old_len - new_len) : 0;
   for (size_t i = 0; i < to_delete; i++) {
-    if (ml_delete((linenr_T)start_row+1, false) == FAIL) {
+    if (ml_delete((linenr_T)start_row, false) == FAIL) {
       api_set_error(err, kErrorTypeException, "Failed to delete line");
       goto end;
     }
@@ -579,7 +621,7 @@ void nvim_buf_set_text(uint64_t channel_id,
   // less memory allocation and freeing.
   size_t to_replace = old_len < new_len ? old_len : new_len;
   for (size_t i = 0; i < to_replace; i++) {
-    int64_t lnum = start_row + 1 + (int64_t)i;
+    int64_t lnum = start_row + (int64_t)i;
 
     if (lnum >= MAXLNUM) {
       api_set_error(err, kErrorTypeValidation, "Index value is too high");
@@ -597,7 +639,7 @@ void nvim_buf_set_text(uint64_t channel_id,
 
   // Now we may need to insert the remaining new old_len
   for (size_t i = to_replace; i < new_len; i++) {
-    int64_t lnum = start_row + (int64_t)i;
+    int64_t lnum = start_row + (int64_t)i - 1;
 
     if (lnum >= MAXLNUM) {
       api_set_error(err, kErrorTypeValidation, "Index value is too high");
@@ -617,7 +659,7 @@ void nvim_buf_set_text(uint64_t channel_id,
 
   // Adjust marks. Invalidate any which lie in the
   // changed range, and move any in the remainder of the buffer.
-  // Only adjust mapks if we managed to switch to a window that holds
+  // Only adjust marks if we managed to switch to a window that holds
   // the buffer, otherwise line numbers will be invalid.
   mark_adjust((linenr_T)start_row,
               (linenr_T)end_row,
@@ -626,17 +668,24 @@ void nvim_buf_set_text(uint64_t channel_id,
               kExtmarkNOOP);
 
   colnr_T col_extent = (colnr_T)(end_col
-                                 - ((end_row > start_col) ? start_col : 0));
+                                 - ((end_col > start_col) ? start_col : 0));
-  extmark_splice(buf, (int)start_row, (colnr_T)start_col,
+  extmark_splice(buf, (int)start_row-1, (colnr_T)start_col,
                  (int)(end_row-start_row), col_extent,
                  (int)new_len-1, (colnr_T)last_item.size, kExtmarkUndo);
 
-  changed_lines((linenr_T)start_row+1, 0, (linenr_T)end_row+1, (long)extra, true);
+  changed_lines((linenr_T)start_row, 0, (linenr_T)end_row, (long)extra, true);
 
-  // TODO: adjust cursor like an extmark ( i e it was inside last_part_len)
+  // adjust cursor like an extmark ( i e it was inside last_part_len)
-  fix_cursor((linenr_T)start_row+1, (linenr_T)end_row+1, (linenr_T)extra);
+  if (curwin->w_cursor.lnum == end_row && curwin->w_cursor.col > end_col) {
+    curwin->w_cursor.col -= col_extent - (colnr_T)last_item.size;
+  }
+  fix_cursor((linenr_T)start_row, (linenr_T)end_row, (linenr_T)extra);
 
 end:
+  for (size_t i = 0; i < new_len; i++) {
+    xfree(lines[i]);
+  }
+  xfree(lines);
   aucmd_restbuf(&aco);
   try_end(err);
 }

test/functional/api/buffer_spec.lua

@@ -392,6 +392,89 @@ describe('api/buf', function()
     end)
   end)
 
+  describe('nvim_buf_get_lines, nvim_buf_set_text', function()
+    local get_lines, set_text = curbufmeths.get_lines, curbufmeths.set_text
+    local line_count = curbufmeths.line_count
+
+    it('works', function()
+      insert([[
+      hello foo!
+      text
+      ]])
+
+      eq({'hello foo!'}, get_lines(0, 1, true))
+
+
+      -- can replace a single word
+      set_text(0, 6, 0, 9, {'world'})
+      eq({'hello world!', 'text'}, get_lines(0, 2, true))
+
+      -- can replace with multiple lines
+      local err = set_text(0, 6, 0, 11, {'foo', 'wo', 'more'})
+      eq({'hello foo', 'wo', 'more!', 'text'}, get_lines(0,  4, true))
+
+      -- will join multiple lines if needed
+      set_text(0, 6, 3, 4, {'bar'})
+      eq({'hello bar'}, get_lines(0,  1, true))
+    end)
+
+    it('updates the cursor position', function()
+      insert([[
+      hello world!
+      ]])
+
+      -- position the cursor on `!`
+      curwin('set_cursor', {1, 11})
+      -- replace 'world' with 'foo'
+      set_text(0, 6, 0, 11, {'foo'})
+      eq('hello foo!', curbuf_depr('get_line', 0))
+      -- cursor should be moved left by two columns (replacement is shorter by 2 chars)
+      eq({1, 9}, curwin('get_cursor'))
+    end)
+
+    it('can handle NULs', function()
+      set_text(0, 0, 0, 0, {'ab\0cd'})
+      eq('ab\0cd', curbuf_depr('get_line', 0))
+    end)
+
+    it('adjusts extmarks', function()
+      local ns = request('nvim_create_namespace', "my-fancy-plugin")
+      insert([[
+      foo bar
+      baz
+      ]])
+      local id1 = curbufmeths.set_extmark(ns, 0, 0, 1, {})
+      local id2 = curbufmeths.set_extmark(ns, 0, 0, 7, {})
+      local id3 = curbufmeths.set_extmark(ns, 0, 1, 1, {})
+      set_text(0, 4, 0, 7, {"q"})
+
+      -- TODO: if we set text at 0,3, what happens to the mark at 0,3
+
+      eq({'foo q', 'baz'}, get_lines(0, 2, true))
+      -- mark before replacement point is unaffected
+      rv = curbufmeths.get_extmark_by_id(ns, id1)
+      eq({0, 1}, rv)
+      -- mark gets shifted back because the replacement was shorter
+      rv = curbufmeths.get_extmark_by_id(ns, id2)
+      eq({0, 5}, rv)
+      -- mark on the next line is unaffected
+      rv = curbufmeths.get_extmark_by_id(ns, id3)
+      eq({1, 1}, rv)
+
+      -- replacing the text spanning two lines will adjust the mark on the next line
+      set_text(0, 3, 1, 3, {"qux"})
+      rv = curbufmeths.get_extmark_by_id(ns, id3)
+      eq({'fooqux', ''}, get_lines(0, 2, true))
+      eq({0, 6}, rv)
+      -- but mark before replacement point is still unaffected
+      rv = curbufmeths.get_extmark_by_id(ns, id1)
+      eq({0, 1}, rv)
+      -- and the mark in the middle was shifted to the end of the insertion
+      rv = curbufmeths.get_extmark_by_id(ns, id2)
+      eq({0, 6}, rv)
+    end)
+  end)
+
   describe('nvim_buf_get_offset', function()
     local get_offset = curbufmeths.get_offset
     it('works', function()