commit 65170e8dad908b15d14b5d68be3ee1a26642ad8b
parent 28e31f5d3d16fac349d5e2b55837afddb822b0f3
Author: luukvbaal <luukvbaal@gmail.com>
Date: Tue, 22 Apr 2025 01:18:03 +0200
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.
Diffstat:
5 files changed, 62 insertions(+), 43 deletions(-)
diff --git a/runtime/doc/api.txt b/runtime/doc/api.txt
@@ -2782,8 +2782,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
diff --git a/runtime/lua/vim/_meta/api.lua b/runtime/lua/vim/_meta/api.lua
@@ -375,8 +375,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
diff --git a/src/nvim/api/extmark.c b/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);
diff --git a/src/nvim/extmark.c b/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;
}
diff --git a/test/functional/api/extmark_spec.lua b/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])
