fix(command,protocol): cap a column width while it is still usize
improvise-yae4. grid_body narrowed every measured width to u16 and only then added the one-character gap and clamped to MAX_COL_WIDTH, so the arithmetic ran at full width before the cap that makes it safe. A stored 65535-column text value panicked at `w + 1` in debug; in release the sum wrapped to 0 and the column was drawn at MIN_COL_WIDTH — the narrowest column, for the widest value. A 65536-column value never reached the addition: `as u16` truncated it to 0 first, so it wrapped in debug builds too, silently. Widths are now capped while still usize, in the one place every measurement enters (`widen`), which is why header_level_seed folds through it rather than keeping a second `as u16`. The answer is unchanged for every input that did not overflow: (min(w,32) + 1).clamp(5,32) and (w + 1).clamp(5,32) agree everywhere, both saturating at 32 for any w >= 31. improvise_protocol::header_level_widths carried the identical defect on the row-header axis — a 65535-column item label — and is fixed the same way. It is the one home for that rule (gotcha 44), so the row header and the column widths cannot end up with two answers. wide_cell_clamps_without_overflow is un-ignored and extended to the 65536 arm and to an ordinary 20-column value. It asserts `widths == vec![expect]` rather than `all(<= 32)`: the length pins that the fixture really produces ONE column, and the ordinary arm pins that the single column is the one carrying the value — a fixture in which no column was wide would read MIN_COL_WIDTH there and pass a `<= 32` check. Both injected faults are recorded in context/test-evidence/core.md. Not fixed, and out of this ticket's scope: compute_visible_cols accumulates `acc + w` in u16, reachable only from a terminal ~65535 columns wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -612,6 +612,96 @@ Fault injected by deleting either emission: the corresponding test fails with
|
||||
`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and
|
||||
`the_two_front_ends_draw_the_same_column_header` fails with it.
|
||||
|
||||
### a_collapsed_groups_header_survives_pruning
|
||||
|
||||
### a_header_whose_items_all_prune_away_is_dropped
|
||||
|
||||
### an_ungrouped_item_after_a_pruned_group_belongs_to_no_group
|
||||
|
||||
improvise-lhi4 and improvise-u6gc: two defects in one `match` block, and they
|
||||
pull in opposite directions, which is why they were fixed together. The naive
|
||||
repair of the first — stop pruning headers — deepens the second.
|
||||
|
||||
`prune_axis_items` kept a header only if a data item under it survived the
|
||||
predicate, and dropped a `GroupEnd` whenever the header it closed was still
|
||||
pending. Both readings are wrong about a case the entry stream can distinguish.
|
||||
|
||||
**lhi4**: a collapsed group contributes **zero** `DataItem`s — `expand_category`
|
||||
emits the `GroupHeader` and then `continue`s past every item — so its header
|
||||
was always pruned and the group vanished from the grid entirely. `z`
|
||||
(toggle-group-under-cursor) resolves the group from `row_group_for(cursor)`,
|
||||
so with no header and no items no row named it: collapse became one-way from
|
||||
the grid, recoverable only through `:toggle-group <cat> <group>`.
|
||||
|
||||
**u6gc**: dropping a pruned header's `GroupEnd` **re-opens an earlier run**.
|
||||
With `Jan[Q1], Feb[Q1], Mar[Q2], Sep` and `Mar` empty, the kept list became
|
||||
`[H(Q1), Jan, Feb, Sep]` and the nearest-preceding walk answered `Q1` for
|
||||
`Sep` — exactly the inheritance improvise-gd83 added `GroupEnd` to prevent, so
|
||||
the regression was live whenever `prune_empty` was on.
|
||||
|
||||
The fix distinguishes the two emptinesses **without consulting the `View`**,
|
||||
which `GridLayout` has already dropped: a run that contained no `DataItem` at
|
||||
all was never expanded, i.e. collapsed, and its header survives; a run that
|
||||
contained items the predicate rejected is genuinely empty and goes. A
|
||||
`GroupEnd` is kept whenever a run is open in the output, so a re-opened earlier
|
||||
header is closed even though the header between them was pruned. That keeps
|
||||
gotcha 1's shape — still a predicate over the data index, no `&View`
|
||||
parameter, no flag vector.
|
||||
|
||||
The three tests are separate because each is satisfied by a fix that misses
|
||||
another. `a_header_whose_items_all_prune_away_is_dropped` **passes before the
|
||||
change** and is the discriminator: it is what stops "keep every header" from
|
||||
counting as a fix for lhi4, and it is the acceptance criterion lhi4 names.
|
||||
|
||||
Faults injected, each against the fixed code, and each fails a **different**
|
||||
pair — which is the evidence that no one of the three tests stands in for the
|
||||
others:
|
||||
- stop keeping the `GroupEnd` (drop the `run_open` push): both membership
|
||||
tests fail with `Some(("Month", "Q1"))` against `None`;
|
||||
`a_header_whose_items_all_prune_away_is_dropped` still passes.
|
||||
- drop the `!had_item` condition in `close_pending_run`, keeping every pending
|
||||
header: only `a_header_whose_items_all_prune_away_is_dropped` fails.
|
||||
- invert it to `had_item`, keeping only headers whose items all pruned:
|
||||
`a_collapsed_groups_header_survives_pruning` and
|
||||
`a_header_whose_items_all_prune_away_is_dropped` fail, and the `GroupEnd`
|
||||
test still passes.
|
||||
|
||||
Both fixtures fund their cells explicitly, so `prune_empty` really prunes: on
|
||||
an unfunded fixture every row prunes and the tests would assert over an empty
|
||||
grid. `row_count()` is asserted first in each for that reason.
|
||||
|
||||
### wide_cell_clamps_without_overflow
|
||||
|
||||
improvise-yae4. `grid_body` narrowed each measured width to `u16` and only then
|
||||
added the one-character gap and clamped to `MAX_COL_WIDTH`, so the arithmetic
|
||||
ran at full width before the cap that makes it safe. A stored 65535-column text
|
||||
value panicked at `w + 1` in debug; in release the sum wrapped to `0` and the
|
||||
column was drawn at `MIN_COL_WIDTH` — the *narrowest* column, for the widest
|
||||
value. A 65536-column value never reached the addition at all: `as u16`
|
||||
truncated it to `0` first, so it wrapped in debug builds too, silently.
|
||||
|
||||
Widths are now capped while still `usize`, in the one place every measurement
|
||||
enters (`widen`), which is also why `header_level_seed` folds through it rather
|
||||
than keeping a second `as u16`. The answer is unchanged for every input that
|
||||
did not overflow: `(min(w, 32) + 1).clamp(5, 32)` and `(w + 1).clamp(5, 32)`
|
||||
agree everywhere, because both saturate at 32 for any `w >= 31`.
|
||||
|
||||
`improvise_protocol::header_level_widths` carried the identical defect on the
|
||||
row-header axis — a 65535-column *item label* — and is fixed the same way. It
|
||||
is the one home for that rule (gotcha 44), so the row header and the column
|
||||
widths do not get two answers.
|
||||
|
||||
The test asserts `widths == vec![expect]`, not `all(<= 32)`: the length pins
|
||||
that the fixture really produces **one** column, and the ordinary 20-column arm
|
||||
pins that the single column is the one carrying the value — a fixture in which
|
||||
no column was wide would read `MIN_COL_WIDTH` there and pass a `<= 32` check.
|
||||
|
||||
Faults injected, each against the fixed code:
|
||||
- `width as u16` in `widen`, debug: the 65535 arm panics with
|
||||
`attempt to add with overflow`.
|
||||
- the same, release: the 65536 arm fails with `left: [5]`, `right: [32]` —
|
||||
the wrap reported as the minimum width.
|
||||
|
||||
### a_trailing_conversion_letter_does_not_erase_the_precision
|
||||
|
||||
improvise-711n. `parse_number_format` read the precision as
|
||||
|
||||
@@ -62,7 +62,7 @@ pub fn grid_body(
|
||||
true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(),
|
||||
false => crate::format::format_value(value.as_ref(), fmt_comma, fmt_decimals),
|
||||
};
|
||||
widen(&mut widths, ci, text.width() as u16);
|
||||
widen(&mut widths, ci, text.width());
|
||||
if let Some(sum) = totals.get_mut(ci) {
|
||||
*sum += value.as_ref().and_then(|v| v.as_f64()).unwrap_or(0.0);
|
||||
}
|
||||
@@ -75,7 +75,7 @@ pub fn grid_body(
|
||||
}
|
||||
}
|
||||
for (ci, total) in totals.iter().enumerate() {
|
||||
let width = format_f64(*total, fmt_comma, fmt_decimals).width() as u16;
|
||||
let width = format_f64(*total, fmt_comma, fmt_decimals).width();
|
||||
widen(&mut widths, ci, width);
|
||||
}
|
||||
|
||||
@@ -114,14 +114,18 @@ fn header_level_seed(layout: &GridLayout, n: usize) -> Vec<u16> {
|
||||
AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None,
|
||||
});
|
||||
for (ci, levels) in items.enumerate().take(n) {
|
||||
widths[ci] = levels.iter().map(|s| s.width() as u16).max().unwrap_or(0);
|
||||
for level in levels {
|
||||
widen(&mut widths, ci, level.width());
|
||||
}
|
||||
}
|
||||
widths
|
||||
}
|
||||
|
||||
fn widen(widths: &mut [u16], col: usize, width: u16) {
|
||||
/// Every measured width enters through here and is capped **while still
|
||||
/// `usize`**, so the `+ 1` gap below cannot overflow (improvise-yae4).
|
||||
fn widen(widths: &mut [u16], col: usize, width: usize) {
|
||||
if let Some(slot) = widths.get_mut(col) {
|
||||
*slot = (*slot).max(width);
|
||||
*slot = (*slot).max(width.min(MAX_COL_WIDTH as usize) as u16);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -396,7 +396,8 @@ pub fn header_level_widths<'a>(
|
||||
for tuple in items {
|
||||
for (level, w) in widest.iter_mut().enumerate() {
|
||||
if let Some(label) = tuple.get(level) {
|
||||
*w = (*w).max(UnicodeWidthStr::width(label.as_str()) as u16);
|
||||
let measured = UnicodeWidthStr::width(label.as_str());
|
||||
*w = (*w).max(measured.min(MAX_ROW_HEADER_W as usize) as u16);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -363,32 +363,39 @@ fn circular_formula_not_a_plausible_cached_number() {
|
||||
);
|
||||
}
|
||||
#[test]
|
||||
#[ignore = "improvise-yae4: known bug reproduction"]
|
||||
fn wide_cell_clamps_without_overflow() {
|
||||
use improvise::{
|
||||
model::cell::{CellKey, CellValue},
|
||||
ui::app::App,
|
||||
};
|
||||
let mut w = revenue_book();
|
||||
w.model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Region".into(), "East".into()),
|
||||
("_Measure".into(), "Revenue".into()),
|
||||
]),
|
||||
CellValue::Text("x".repeat(65535)),
|
||||
);
|
||||
let app = App::new(w, None);
|
||||
let widths = improvise::ui::viewport::compute_col_widths(
|
||||
&app.model_state.workbook.model,
|
||||
&app.layout,
|
||||
false,
|
||||
0,
|
||||
);
|
||||
assert_eq!(
|
||||
widths,
|
||||
vec![improvise::ui::viewport::MAX_COL_WIDTH],
|
||||
"the Revenue column must reach the width cap"
|
||||
);
|
||||
// 65535 overflows the `+ 1` gap; 65536 truncates the `as u16` cast to 0.
|
||||
// 20 is the ordinary case the cap must leave alone, gap included.
|
||||
for (len, expect) in [
|
||||
(20usize, 21u16),
|
||||
(65535, improvise::ui::viewport::MAX_COL_WIDTH),
|
||||
(65536, improvise::ui::viewport::MAX_COL_WIDTH),
|
||||
] {
|
||||
let mut w = revenue_book();
|
||||
w.model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Region".into(), "East".into()),
|
||||
("_Measure".into(), "Revenue".into()),
|
||||
]),
|
||||
CellValue::Text("x".repeat(len)),
|
||||
);
|
||||
let app = App::new(w, None);
|
||||
let widths = improvise::ui::viewport::compute_col_widths(
|
||||
&app.model_state.workbook.model,
|
||||
&app.layout,
|
||||
false,
|
||||
0,
|
||||
);
|
||||
assert_eq!(
|
||||
widths,
|
||||
vec![expect],
|
||||
"a {len}-column value must yield exactly one column, of width {expect}"
|
||||
);
|
||||
}
|
||||
}
|
||||
#[test]
|
||||
fn ordinary_controls() {
|
||||
|
||||
Reference in New Issue
Block a user