fix(api): wrong return value with reverse range + overlap #32956

Problem:  When iterating in reverse with {start} > {end} in
          `nvim_buf_get_extmarks()`, marks that overlap {start} and are
          greater than {end} are included in the return value twice.
          Marks that overlap {end} and do not overlap {start} are not
          not included in the return value at all. Marks are not
          actually returned in a meaningful "traversal order".

Solution: Rather than actually iterating in reverse, (also possible but
          requires convoluted conditions and would require fetching
          overlapping marks for both the {start} and {end} position,
          while still ending up with non-traversal ordered marks),
          iterate normally and reverse the return value.
(cherry picked from commit 65170e8dad)

runtime/doc/api.txt

@@ -2777,8 +2777,11 @@ nvim_buf_get_extmarks({buffer}, {ns_id}, {start}, {end}, {opts})
         vim.api.nvim_buf_get_extmarks(0, my_ns, {0,0}, {-1,-1}, {})
 <
 
-    If `end` is less than `start`, traversal works backwards. (Useful with
-    `limit`, to get the first marks prior to a given position.)
+    If `end` is less than `start`, marks are returned in reverse order.
+    (Useful with `limit`, to get the first marks prior to a given position.)
+
+    Note: For a reverse range, `limit` does not actually affect the traversed
+    range, just how many marks are returned
 
     Note: when using extmark ranges (marks with a end_row/end_col position)
     the `overlap` option might be useful. Otherwise only the start position of

runtime/lua/vim/_meta/api.lua

@@ -369,8 +369,11 @@ function vim.api.nvim_buf_get_extmark_by_id(buffer, ns_id, id, opts) end
 --- vim.api.nvim_buf_get_extmarks(0, my_ns, {0,0}, {-1,-1}, {})
 --- ```
 ---
---- If `end` is less than `start`, traversal works backwards. (Useful
---- with `limit`, to get the first marks prior to a given position.)
+--- If `end` is less than `start`, marks are returned in reverse order.
+--- (Useful with `limit`, to get the first marks prior to a given position.)
+---
+--- Note: For a reverse range, `limit` does not actually affect the traversed
+--- range, just how many marks are returned
 ---
 --- Note: when using extmark ranges (marks with a end_row/end_col position)
 --- the `overlap` option might be useful. Otherwise only the start position

src/nvim/api/extmark.c

@@ -241,8 +241,11 @@ ArrayOf(Integer) nvim_buf_get_extmark_by_id(Buffer buffer, Integer ns_id,
 /// vim.api.nvim_buf_get_extmarks(0, my_ns, {0,0}, {-1,-1}, {})
 /// ```
 ///
-/// If `end` is less than `start`, traversal works backwards. (Useful
-/// with `limit`, to get the first marks prior to a given position.)
+/// If `end` is less than `start`, marks are returned in reverse order.
+/// (Useful with `limit`, to get the first marks prior to a given position.)
+///
+/// Note: For a reverse range, `limit` does not actually affect the traversed
+/// range, just how many marks are returned
 ///
 /// Note: when using extmark ranges (marks with a end_row/end_col position)
 /// the `overlap` option might be useful. Otherwise only the start position
@@ -327,8 +330,6 @@ Array nvim_buf_get_extmarks(Buffer buffer, Integer ns_id, Object start, Object e
     limit = INT64_MAX;
   }
 
-  bool reverse = false;
-
   int l_row;
   colnr_T l_col;
   if (!extmark_get_index_from_obj(buf, ns_id, start, &l_row, &l_col, err)) {
@@ -341,17 +342,31 @@ Array nvim_buf_get_extmarks(Buffer buffer, Integer ns_id, Object start, Object e
     return rv;
   }
 
-  if (l_row > u_row || (l_row == u_row && l_col > u_col)) {
-    reverse = true;
+  size_t rv_limit = (size_t)limit;
+  bool reverse = l_row > u_row || (l_row == u_row && l_col > u_col);
+  if (reverse) {
+    limit = INT64_MAX;  // limit the return value instead
+    int row = l_row;
+    l_row = u_row;
+    u_row = row;
+    colnr_T col = l_col;
+    l_col = u_col;
+    u_col = col;
   }
 
   // note: ns_id=-1 allowed, represented as UINT32_MAX
   ExtmarkInfoArray marks = extmark_get(buf, (uint32_t)ns_id, l_row, l_col, u_row,
-                                       u_col, (int64_t)limit, reverse, type, opts->overlap);
+                                       u_col, (int64_t)limit, type, opts->overlap);
 
-  rv = arena_array(arena, kv_size(marks));
-  for (size_t i = 0; i < kv_size(marks); i++) {
-    ADD_C(rv, ARRAY_OBJ(extmark_to_array(kv_A(marks, i), true, details, hl_name, arena)));
+  rv = arena_array(arena, MIN(kv_size(marks), rv_limit));
+  if (reverse) {
+    for (int i = (int)kv_size(marks) - 1; i >= 0 && kv_size(rv) < rv_limit; i--) {
+      ADD_C(rv, ARRAY_OBJ(extmark_to_array(kv_A(marks, i), true, details, hl_name, arena)));
+    }
+  } else {
+    for (size_t i = 0; i < kv_size(marks); i++) {
+      ADD_C(rv, ARRAY_OBJ(extmark_to_array(kv_A(marks, i), true, details, hl_name, arena)));
+    }
   }
 
   kv_destroy(marks);

src/nvim/extmark.c

@@ -259,11 +259,9 @@ bool extmark_clear(buf_T *buf, uint32_t ns_id, int l_row, colnr_T l_col, int u_r
 ///
 /// if upper_lnum or upper_col are negative the buffer
 /// will be searched to the start, or end
-/// reverse can be set to control the order of the array
 /// amount = amount of marks to find or INT64_MAX for all
 ExtmarkInfoArray extmark_get(buf_T *buf, uint32_t ns_id, int l_row, colnr_T l_col, int u_row,
-                             colnr_T u_col, int64_t amount, bool reverse, ExtmarkType type_filter,
-                             bool overlap)
+                             colnr_T u_col, int64_t amount, ExtmarkType type_filter, bool overlap)
 {
   ExtmarkInfoArray array = KV_INITIAL_VALUE;
   MarkTreeIter itr[1];
@@ -281,29 +279,21 @@ ExtmarkInfoArray extmark_get(buf_T *buf, uint32_t ns_id, int l_row, colnr_T l_co
   } else {
     // Find all the marks beginning with the start position
     marktree_itr_get_ext(buf->b_marktree, MTPos(l_row, l_col),
-                         itr, reverse, false, NULL, NULL);
+                         itr, false, false, NULL, NULL);
   }
 
-  int order = reverse ? -1 : 1;
   while ((int64_t)kv_size(array) < amount) {
     MTKey mark = marktree_itr_current(itr);
     if (mark.pos.row < 0
-        || (mark.pos.row - u_row) * order > 0
-        || (mark.pos.row == u_row && (mark.pos.col - u_col) * order > 0)) {
+        || (mark.pos.row > u_row)
+        || (mark.pos.row == u_row && mark.pos.col > u_col)) {
       break;
     }
-    if (mt_end(mark)) {
-      goto next_mark;
-    }
-
-    MTKey end = marktree_get_alt(buf->b_marktree, mark, NULL);
-    push_mark(&array, ns_id, type_filter, mtpair_from(mark, end));
-next_mark:
-    if (reverse) {
-      marktree_itr_prev(buf->b_marktree, itr);
-    } else {
-      marktree_itr_next(buf->b_marktree, itr);
+    if (!mt_end(mark)) {
+      MTKey end = marktree_get_alt(buf->b_marktree, mark, NULL);
+      push_mark(&array, ns_id, type_filter, mtpair_from(mark, end));
     }
+    marktree_itr_next(buf->b_marktree, itr);
   }
   return array;
 }

test/functional/api/extmark_spec.lua

@@ -331,30 +331,30 @@ describe('API/extmarks', function()
     rv = get_extmarks(ns, lower, upper)
     eq({ { marks[3], positions[3][1], positions[3][2] } }, rv)
 
-    -- prev with mark id
+    -- reverse with mark id
     rv = get_extmarks(ns, marks[3], { 0, 0 }, { limit = 1 })
     eq({ { marks[3], positions[3][1], positions[3][2] } }, rv)
     rv = get_extmarks(ns, marks[2], { 0, 0 }, { limit = 1 })
     eq({ { marks[2], positions[2][1], positions[2][2] } }, rv)
-    -- prev with positional when mark exists at position
+    -- reverse with positional when mark exists at position
     rv = get_extmarks(ns, positions[3], { 0, 0 }, { limit = 1 })
     eq({ { marks[3], positions[3][1], positions[3][2] } }, rv)
-    -- prev with positional index (no mark at position)
+    -- reverse with positional index (no mark at position)
     rv = get_extmarks(ns, { positions[1][1], positions[1][2] + 1 }, { 0, 0 }, { limit = 1 })
     eq({ { marks[1], positions[1][1], positions[1][2] } }, rv)
-    -- prev with Extremity index
+    -- reverse with Extremity index
     rv = get_extmarks(ns, { -1, -1 }, { 0, 0 }, { limit = 1 })
     eq({ { marks[3], positions[3][1], positions[3][2] } }, rv)
 
-    -- prevrange with mark id
+    -- reverse with mark id
     rv = get_extmarks(ns, marks[3], marks[1])
     eq({ marks[3], positions[3][1], positions[3][2] }, rv[1])
     eq({ marks[2], positions[2][1], positions[2][2] }, rv[2])
     eq({ marks[1], positions[1][1], positions[1][2] }, rv[3])
-    -- prevrange with limit
+    -- reverse with limit
     rv = get_extmarks(ns, marks[3], marks[1], { limit = 2 })
     eq(2, #rv)
-    -- prevrange with positional when mark exists at position
+    -- reverse with positional when mark exists at position
     rv = get_extmarks(ns, positions[3], positions[1])
     eq({
       { marks[3], positions[3][1], positions[3][2] },
@@ -363,7 +363,7 @@ describe('API/extmarks', function()
     }, rv)
     rv = get_extmarks(ns, positions[2], positions[1])
     eq(2, #rv)
-    -- prevrange with positional index (no mark at position)
+    -- reverse with positional index (no mark at position)
     lower = { positions[2][1], positions[2][2] + 1 }
     upper = { positions[3][1], positions[3][2] + 1 }
     rv = get_extmarks(ns, upper, lower)
@@ -372,7 +372,7 @@ describe('API/extmarks', function()
     upper = { positions[3][1], positions[3][2] + 2 }
     rv = get_extmarks(ns, upper, lower)
     eq({}, rv)
-    -- prevrange with extremity index
+    -- reverse with extremity index
     lower = { 0, 0 }
     upper = { positions[2][1], positions[2][2] - 1 }
     rv = get_extmarks(ns, upper, lower)
@@ -428,6 +428,14 @@ describe('API/extmarks', function()
     set_extmark(ns, marks[3], 0, 2) -- check is inclusive
     local rv = get_extmarks(ns, { 2, 3 }, { 0, 2 })
     eq({ { marks[1], 2, 1 }, { marks[2], 1, 4 }, { marks[3], 0, 2 } }, rv)
+    -- doesn't include paired marks whose start pos > lower bound twice
+    -- and returns mark overlapping start pos but not end pos
+    local m1 = set_extmark(ns, nil, 0, 0, { end_row = 1, end_col = 4 })
+    local m2 = set_extmark(ns, nil, 0, 0, { end_row = 1, end_col = 2 })
+    local m3 = set_extmark(ns, nil, 1, 0, { end_row = 1, end_col = 4 })
+    local m4 = set_extmark(ns, nil, 1, 2, { end_row = 1, end_col = 4 })
+    rv = get_extmarks(ns, { 1, 3 }, { 1, 2 }, { overlap = true })
+    eq({ { m4, 1, 2 }, { m3, 1, 0 }, { m2, 0, 0 }, { m1, 0, 0 } }, rv)
   end)
 
   it('get_marks limit=0 returns nothing', function()
@@ -973,7 +981,7 @@ describe('API/extmarks', function()
     eq(1, #rv)
     rv = get_extmarks(ns2, { 0, 0 }, positions[2], { limit = 1 })
     eq(1, #rv)
-    -- get_prev (limit set)
+    -- reverse (limit set)
     rv = get_extmarks(ns, positions[1], { 0, 0 }, { limit = 1 })
     eq(1, #rv)
     rv = get_extmarks(ns2, positions[1], { 0, 0 }, { limit = 1 })
@@ -984,7 +992,7 @@ describe('API/extmarks', function()
     eq(2, #rv)
     rv = get_extmarks(ns2, positions[1], positions[2])
     eq(2, #rv)
-    -- get_prev (no limit)
+    -- reverse (no limit)
     rv = get_extmarks(ns, positions[2], positions[1])
     eq(2, #rv)
     rv = get_extmarks(ns2, positions[2], positions[1])