From ef99b64f95ee0d3c06458b28a0204d9413226c19 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 23:06:25 +0200 Subject: [PATCH] fix(listview): ids must also be unique and not NaN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scalar contract said "identity" and enforced only "scalar", so two ways to hold an id that is not one survived. NaN passes `type(x) == "number"` and then errors at `p.collapsed[row.id]` with "table index is NaN" — the one scalar Lua accepts as a number and refuses as a key. Bitten with the check removed, it reports exactly that, from inside listview, naming no row. DUPLICATES do not merely collide. Every lookup here — `line_of_id`, and toggle's scan for the row index — resolves an id to the FIRST row bearing it, so selecting the second such row toggles the first and re-seats the cursor onto it: a stray jump with nothing pointing at the id. Bitten with the check removed, nothing is raised at all. Both are enforced in `check_ids`, where rows already enter, so the error names the offending row (and, for a duplicate, both of them) instead of surfacing as a low-level error or a wrong jump later. The error text says why, not just what, since the reason is not guessable from the rule. Verified: fmt, clippy, diff-check, --lib 1897/0, crdt 2082/0, listview 26/26, m4 150/0, gpu 221/0, bottom_panel_stage1 47/47. Co-Authored-By: Claude Opus 5 --- builtin/runtime/listview.lua | 58 ++++++++++++++++++++++++++-------- docs/tree-primitive-framing.md | 2 +- tests/listview_acceptance.rs | 52 ++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 14 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 4d6cd00..a10cf57 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -158,26 +158,58 @@ local function hidden_by_ancestor(p, rows, i) return false end --- Ids must be scalars, and the reason is not fussiness about types. --- Selection compares them with `==`, which honours `__eq`; collapse --- state stores them as TABLE KEYS, and Lua indexes tables by raw --- identity, consulting no metamethod. A table id would therefore +-- Ids must be usable, unique table keys, and none of the three checks +-- below is fussiness about types. +-- +-- SCALAR. Selection compares ids with `==`, which honours `__eq`; +-- collapse state stores them as TABLE KEYS, and Lua indexes tables by +-- raw identity, consulting no metamethod. A table id would therefore -- satisfy one and quietly fail the other: after a refresh minted fresh -- id tables, selection would be restored and the fold would be lost. -- -- Equality-aware collapse lookup is the alternative, and it is worse -- here: `hidden_by_ancestor` runs per row and would turn a linear --- render quadratic to support a key type no consumer has wanted. So --- the contract is narrowed to the one both halves can honour, and --- enforced where rows enter rather than discovered as a lost fold. +-- render quadratic to support a key type no consumer has wanted. +-- +-- NOT NaN. `0/0` passes a `type(x) == "number"` test and then *errors* +-- at `p.collapsed[row.id]` with "table index is NaN" — the one scalar +-- Lua accepts as a number and refuses as a key. Caught here so the +-- report names the row, rather than surfacing on whichever later TAB +-- happens to reach it. +-- +-- UNIQUE. Every lookup here resolves an id to the FIRST row bearing +-- it, so duplicates do not merely collide: selecting the second such +-- row toggles the first and re-seats the cursor onto it. An id that +-- does not identify a node is not an id, and the contract's word for +-- itself is identity. +-- +-- All three are enforced where rows enter, so a bad id is a named +-- error at the call site instead of a lost fold or a stray jump later. local function check_ids(rows) + local seen = {} for i, row in ipairs(rows) do - local k = type(row.id) - if row.id ~= nil and k ~= "string" and k ~= "number" then - error(string.format( - "listview: row %d has a %s id; ids must be a string or number " - .. "(collapse state keys a table by identity, so a %s id would " - .. "lose its fold across a refresh)", i, k, k)) + local id, k = row.id, type(row.id) + if id ~= nil then + if k ~= "string" and k ~= "number" then + error(string.format( + "listview: row %d has a %s id; ids must be a string or number " + .. "(collapse state keys a table by identity, so a %s id would " + .. "lose its fold across a refresh)", i, k, k)) + end + if id ~= id then + error(string.format( + "listview: row %d has a NaN id; NaN is a number but not a " + .. "usable table key, and collapse state would raise " + .. "\"table index is NaN\" on the first fold", i)) + end + if seen[id] then + error(string.format( + "listview: rows %d and %d share the id %q; ids must be unique " + .. "(every lookup resolves to the first match, so selecting " + .. "the later row would toggle and re-seat the earlier one)", + seen[id], i, tostring(id))) + end + seen[id] = i end end return rows diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 7e82093..6e58d9b 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -21,7 +21,7 @@ toward the consumer keeping `text`. |---|---| | **Q#TR1** | **Extend `listview`.** A separate `treeview` would either duplicate ~200 lines of panel discipline (Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, quit chain, generated-buffer writes) or require *extracting* them from a shipped primitive first — the riskier change. Extending is backward-compatible by construction: absent `depth`/`id` give today's behaviour exactly, which the three flat consumers already produce. | | **Q#TR2** | **Primitive-owned collapse state**, keyed by row id, held in the panel record. Consumer-owned would make every consumer reimplement refresh survival. | -| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | +| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter — and with it that ids are **unique** and **not NaN**, since every lookup takes the first match (so a duplicate makes selecting the later row toggle the earlier) and `0/0` is the one scalar Lua counts as a number and refuses as a table key.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | | **Q#TR4** | **Consumer keeps pre-rendered `text`**; `depth` is structural only. Also sidesteps the conflict with dired's fixed-width `_layout` column contract when it adopts. | **Acceptance 5 is decided too: byte-identity coverage is written**, diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 84245bf..e845fce 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -1144,6 +1144,58 @@ fn tr_6_a_non_scalar_id_is_rejected_where_rows_enter() { ); } +/// A NaN id passes `type(x) == "number"` and then errors at +/// `p.collapsed[row.id]` with "table index is NaN" — the one scalar +/// Lua accepts as a number and refuses as a table key. It must be +/// caught where rows enter, naming the row, rather than surfacing on +/// whichever later TAB happens to reach it. +#[test] +fn tr_7_a_nan_id_is_rejected_rather_than_erroring_on_the_first_fold() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr7*", + header = "h", + rows = { { text = "a", depth = 0, id = 0 / 0 } }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("NaN id"), + "named at entry, not as a table-index error later: {err}" + ); +} + +/// Duplicate ids do not merely collide — every lookup resolves an id to +/// the FIRST row bearing it, so selecting the second toggles the first +/// and re-seats the cursor onto it. An id that does not identify a node +/// is not an id. +#[test] +fn tr_8_duplicate_ids_are_rejected_because_lookup_takes_the_first_match() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr8*", + header = "h", + rows = { + { text = "first", depth = 0, id = "same" }, + { text = "second", depth = 0, id = "same" }, + }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("share the id") && err.contains("rows 1 and 2"), + "both offending rows named: {err}" + ); +} + // Isolated bootstrap storage roots (see the module docs): an // integration test is compiled without `cfg(test)`, so a raw // `EditorState::new()` would read the developer's real `init.lua` and