diff --git a/.kiro/specs/calculator/design.md b/.kiro/specs/calculator/design.md index c7663cb..9691b0b 100644 --- a/.kiro/specs/calculator/design.md +++ b/.kiro/specs/calculator/design.md @@ -107,39 +107,57 @@ pub const MultiBaseResult = struct { ### 2.3 AST Nodes +The shape as implemented (`engine/src/ast.zig`); the earlier sketch here omitted +the `text` field, which the whole exact tier depends on. + ```zig pub const Expr = union(enum) { - number_literal: NumberLiteral, - unary_op: UnaryOp, - binary_op: BinaryOp, - function_call: FunctionCall, + number: Number, + string_literal: []const u8, // ASCII bytes to pack, quotes stripped + unary: Unary, + binary: Binary, + call: Call, variable: []const u8, assignment: Assignment, }; -pub const NumberLiteral = struct { - value: f64, - /// For programmer mode: preserve original base and integer value +pub const Number = struct { + float_value: f64, + /// For programmer mode: the exact integer when the literal fits u64. int_value: ?u64, base: Base, + /// The literal exactly as written, pointing into the source. Decimal literals + /// are re-parsed from this rather than from `float_value`, because + /// `float_value` has already rounded and `0.1` cannot be recovered from it. + text: []const u8, }; pub const Base = enum { decimal, hex, octal, binary }; -pub const BinaryOp = struct { - op: Operator, +pub const Binary = struct { + op: BinaryOp, left: *Expr, right: *Expr, }; -pub const Operator = enum { +pub const BinaryOp = enum { add, sub, mul, div, mod, pow, bit_and, bit_or, bit_xor, shift_left, - shift_right_logical, shift_right_arithmetic, + shift_right, shift_right_logical, rotate_left, rotate_right, }; ``` +Ownership: nodes are allocated individually and the caller releases the tree with +`parser.freeExpr`. Passing an arena and skipping the walk is allowed, but nothing +assumes it (see `ast.zig` for why that distinction matters). + +**Size limits.** `Parser.max_nodes` (1000) and `Parser.max_nest_depth` (128) bound +the tree, because every walk over it recurses. A flat chain like `1+1+1+...` builds +a left-deep tree whose depth is the operator count, and 4000 terms segfaulted +`evalExact` before the limit existed. Exceeding either reports +`InvalidExpression`. + ### 2.4 Parser Design **Approach: Pratt parser (top-down operator precedence)** @@ -175,18 +193,23 @@ The evaluator maintains an `Environment`: ```zig pub const Environment = struct { + allocator: Allocator, mode: Mode, - variables: std.StringHashMap(Value), - history: std.ArrayList(HistoryEntry), + /// Variables hold `Number`, so an assignment keeps whatever exactness its + /// expression had: `X = 0.1` stores exactly one tenth. + variables: std.StringHashMap(Number), + ans: ?Number, + /// A counter only. Displayed history lives in the frontend (`src/tui.zig`), + /// because the engine has no I/O and no display concerns. + history_len: usize, programmer_config: ProgrammerConfig, +}; - pub const Mode = enum { standard, programmer, financial }; - - pub const ProgrammerConfig = struct { - bit_width: BitWidth = .bits64, - signedness: Signedness = .signed, - display_endian: Endianness = .little, - }; +pub const ProgrammerConfig = struct { + bit_width: BitWidth = .bits64, + signedness: Signedness = .signed, + /// Big-endian by default so the HEX row reads as the number itself; see FR-2.8. + display_endian: Endianness = .big, }; ``` @@ -1095,16 +1118,17 @@ $ tally "2^32 - 1" 4,294,967,295 ``` -Programmer: +Programmer (actual output, verified against the binary): ``` $ tally -p "0xFF & 0x0F" dec(signed): 15 dec(unsigned): 15 - hex: 0x0000_0000_0000_000F - oct: 0o17 + hex: 00 00 00 00 00 00 00 0F + oct: 0 000 000 000 000 000 000 017 bin: 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 1111 - bits: [64] ``` +Separators are spaces, not underscores, and the base prefix appears only in the +clipboard form. There is no `bits:` line. Struct: ``` @@ -1128,17 +1152,18 @@ ABI: x86-64 System V | Endian: little 0C: tttt tttt tttt tttt tttt tttt tttt tttt] ``` -Unit conversion: +Unit conversion (actual output): ``` -$ tally convert 100 km miles -62.1371 miles +$ tally convert 100 km mi +100 km = 62.13711922373339696174 mi $ tally "72 F to C" -22.2222 °C - -$ tally "5 kg + 3 lb" -6.36078 kg +72 F = 22.22222222222222222222 C ``` +Both operands are echoed, canonical short names are used rather than long or +symbol forms, and the full exact expansion is shown (20 fractional digits for a +non-terminating one). `tally "5 kg + 3 lb"` is FR-4.2 and is NOT implemented: it +reports `error: unexpected token`. --- @@ -1152,10 +1177,19 @@ Most of it does not. Two harnesses cover nearly all of it: - **Rendered frames.** `src/tui/test_render.zig` draws a real frame through the real widget draw path into a real surface, then reads the cells back as rows of text. Assertions are made against what a user would see (`"= PMT -1,199.10"`, - `"rows 1-7 of 360"`), plus a structural check that every row is exactly the - requested width, since the drawing helpers clip silently rather than erroring - when a column is miscomputed. Views are exercised at 20x6 through 200x60 to - catch layouts that only work at one size. + `"rows 1-7 of 360"`), plus `furniture(rows).intact()`, which checks that the + separator, prompt and status rows are still where they belong. That last check is + the load-bearing one: content which overflows its region lands on those rows, so + checking them is how overflow is detected. + + This originally offered `wellFormed(rows, width)` instead, described here as a + check that no column had been miscomputed. It compared each row's length against + the width it had just been allocated with, so it could never fail. Two layout + bugs (programmer rows drawn over the prompt at 100x16, financial chips drawn over + it at 20x5) lived under a green suite because of it. Style is still not + observable: `flatten` keeps graphemes and drops attributes, so focus highlighting + and the cursor cannot be asserted, and tests that care assert a neighbouring + glyph and say so. - **Real event handlers.** `vxfw.EventContext` is constructible in a test, so key presses and mouse events go through `handleKey` and `handleMouse` rather than through a reimplementation of them. This is what catches a binding that was @@ -1198,6 +1232,13 @@ Key properties: draw function, and each view calls `addRegion` right where it draws the thing. Hit targets therefore cannot drift out of sync with what is on screen, which is the usual failure mode for hand-maintained click maps. + + With one caveat that was found by review: registration is co-located with + drawing, but drawing itself had no bounds check, so a row drawn off the bottom + registered a region anyway and a click on the separator focused an invisible + field. Co-location keeps the mapping honest only as far as the drawing is; the + height guards added to the programmer and convert views are what make the + property hold. - **Newest region wins.** `at()` searches backwards, so a view can register a broad row-wide fallback first (click anywhere on the HEX row to focus it) and then finer targets on top (click a specific nibble to put the cursor there). @@ -1239,9 +1280,13 @@ clicking places the cursor for typing instead. │ │ │ = 10.42477796... │ │ │ -└───────────────────────────── [d]ec [h]ex [o]ct [b]in ── q:quit ── ?:help┘ +└─ ?:help | Tab:mode | Enter:eval | Ctrl-L:clear | Ctrl-C:quit ───────────┘ ``` +The footer is the real status line. The `[d]ec [h]ex [o]ct [b]in` quick-switch it +used to show was FR-7.8, which has been removed: those letters are hex and binary +digits once the value fields became editable. + ### 8.2 Programmer Mode ``` ┌─ Tally ─────────────────────── [Standard] [Programmer] [Financial] [Convert] ─┐ diff --git a/.kiro/specs/calculator/requirements.md b/.kiro/specs/calculator/requirements.md index 0d9bb52..6e234e4 100644 --- a/.kiro/specs/calculator/requirements.md +++ b/.kiro/specs/calculator/requirements.md @@ -15,9 +15,9 @@ A calculator application with three frontends (CLI, TUI, Android) sharing a comm - **FR-1.3**: Support parentheses for grouping. - **FR-1.4**: Support built-in functions: `sin`, `cos`, `tan`, `asin`, `acos`, `atan`, `log` (base-10), `ln` (natural), `sqrt`, `cbrt`, `abs`, `ceil`, `floor`, `round`, `factorial`. - **FR-1.5**: Support constants: `pi`, `e`, `tau`. -- **FR-1.6**: Support variable storage (Ans for last result, named variables A-F, X, Y, Z). +- **FR-1.6**: Support variable storage: `Ans` for the last result, plus any identifier as a named variable. (The original wording restricted this to `A-F, X, Y, Z`; the implementation accepts any name, which is a superset and the better behaviour, so the requirement follows the code.) Assignment to a constant name (`pi`, `e`, `tau`, `Ans`) is currently accepted and then ignored, which is a known defect rather than intended behaviour. - **FR-1.7**: Maintain calculation history with replay capability. -- **FR-1.8**: Commas accepted as digit separators in input (e.g., `1,000 * 2` = 2000). Spaces in hex literals treated as byte separators (e.g., `0xFF FF` = 0xFFFF). +- **FR-1.8**: Commas accepted as digit separators in input, but only in thousands groups: a comma must be followed by exactly three digits (`1,000 * 2` is 2000, `1,234,567` is one number). A comma followed by any other number of digits is an argument separator, which is what makes `log(100,10)` two arguments rather than the number 10010. The ambiguous case `max(1,234)` resolves in favour of the grouping and reads as `max(1234)`; write a space to mean two arguments. A malformed group such as `1,00` is an error rather than a silently merged number. Spaces and underscores group hex/octal/binary literals (`0xFF FF`, `0xFF_FF`); commas do not. - **FR-1.9**: When a standard-mode expression contains any non-decimal literal (hex `0x`, octal `0o`, or binary `0b`) and the result is a non-negative integer, enrich the result display with hex/octal/binary representations inline (without leaving standard mode). Uses the smallest standard bit width (8/16/32/64/128) that holds the value. This does not change the evaluation semantics (still f64 arithmetic, `^` is still power); it only augments the display. Fractional or negative results show decimal only. ### FR-2: Programmer Mode @@ -30,7 +30,7 @@ A calculator application with three frontends (CLI, TUI, Android) sharing a comm - **FR-2.5**: Display both signed (two's complement) and unsigned interpretations of the current value. - **FR-2.6**: Visualize the bit pattern as a grid (integer.exposed style) - bits individually addressable/toggleable in TUI and Android. - **FR-2.7**: Quick toggle between base display formats via dedicated keyboard shortcuts (TUI). -- **FR-2.8**: Configurable endianness display (little-endian default, big-endian available). Affects byte order in HEX display and byte-level visualizations. Does not change the underlying value - purely a display/interpretation toggle. +- **FR-2.8**: Configurable endianness display (big-endian default, little-endian available). Affects byte order in HEX and ASCII rows and byte-level visualizations. Does not change the underlying value - purely a display/interpretation toggle. Big-endian is the default so the HEX row reads as the number itself, matching the DEC, OCT and BIN rows; the little-endian view (x86 memory order) is one keystroke away. (This requirement previously specified a little-endian default, which the implementation deliberately did not follow.) - **FR-2.9**: ASCII/text literal input via single-quoted strings: `'hello'` packs ASCII bytes into the integer value. First character occupies the most significant used byte (big-endian packing when display is BE) or least significant byte (when display is LE). Useful for examining magic numbers, file signatures, protocol headers. - **FR-2.10**: ASCII interpretation display - when the current value contains printable ASCII bytes, show the text representation alongside the numeric bases (e.g., `ASCII: "ELF."` or `ASCII: ..lf` with dots for non-printable bytes). - **FR-2.11**: IEEE 754 floating-point interpretation (float.exposed style): @@ -121,7 +121,8 @@ A calculator application with three frontends (CLI, TUI, Android) sharing a comm - **FR-7.1**: Full-screen terminal UI with mode tabs (Standard, Programmer, Financial, Convert). Tab moves to the next mode and Shift-Tab to the previous one, so a mis-hit costs one keystroke rather than a full lap. - **FR-7.2**: Standard mode: expression input line, result display, scrollable history. -- **FR-7.3**: Programmer mode: bit grid (navigable with arrow keys, toggle with Space/Enter), simultaneous base displays, expression input. +- **FR-7.3**: Programmer mode: bit grid (navigable with arrow keys, toggled with Space), simultaneous base displays, expression input. NOTE: Enter does not toggle a bit. It is consumed by the input line, and the value-zone status hint that advertised `Enter:set` is a known defect. Space is the only toggle. +- **FR-7.3.1**: When the terminal is too short for the bit grid plus all six base rows, the view says so and how many rows it needs, rather than drawing rows over the separator and the input line. - **FR-7.4**: Programmer mode struct sub-view: field list editor, live-updating memory map visualization. - **FR-7.5**: Financial mode: form-style input for parameters, result display with formula breakdown, and a scrollable amortization schedule. Four calculations behind one selector: CAGR, compound interest, TVM, and amortization. TVM and compound interest solve for whichever field is left blank; an unanswerable form reports what it is waiting for rather than doing nothing. Results are live, so financial mode records nothing in history. - **FR-7.5.1**: Form fields group digits as they are typed: entering `200` then `0` displays `2,000`. The stored text stays ungrouped so it still parses, and a typed comma is accepted and discarded rather than corrupting the value. @@ -131,9 +132,9 @@ A calculator application with three frontends (CLI, TUI, Android) sharing a comm - **FR-7.6**: Convert mode: select category, input value, select from/to units, live result. - **FR-7.6.1**: The help overlay scrolls (Up/Down, PgUp/PgDn, or the mouse wheel) and shows its position when content is off screen. Any other key returns. Sections are a single line list rather than individually height-gated draw calls, which is what previously caused later sections to be dropped silently on a normal-sized terminal. - **FR-7.7**: Every action must be reachable from the keyboard alone; the TUI is fully usable without a mouse. -- **FR-7.8**: Quick-switch keys for base display in programmer mode (e.g., `d`=dec, `h`=hex, `o`=oct, `b`=bin to highlight primary). +- **FR-7.8**: REMOVED. Quick-switch keys (`d`=dec, `h`=hex, `o`=oct, `b`=bin) to highlight the primary base cannot coexist with editable value fields: in the value zone those letters are hex and binary digits. The requirement predates editable fields, and the fields are the more useful feature. Base selection stays on the arrow keys and the mouse. - **FR-7.9**: Support terminal resize gracefully. -- **FR-7.10**: Vi-style and Emacs-style keybinding options for expression input. +- **FR-7.10**: Vi-style and Emacs-style keybinding options for expression input. NOT IMPLEMENTED: the prompt is a plain text field. Deferred, not dropped. #### FR-7.11: Mouse Support diff --git a/.kiro/specs/calculator/tasks.md b/.kiro/specs/calculator/tasks.md index 60b9d1d..33fa3fc 100644 --- a/.kiro/specs/calculator/tasks.md +++ b/.kiro/specs/calculator/tasks.md @@ -756,6 +756,109 @@ Remaining subcommands deferred until their engine modules exist. - NOT DONE: mouse wheel scrolling for history - Verify: help overlay works, mouse interactions work, looks reasonable in 80x24 terminal +### Task 5.10: Act on the full-codebase review [PARTIAL] + +A four-part review of the whole codebase (numeric core, language layer, domain +modules, frontends plus build) produced 29 wrong-answer or crash findings, 10 +groups of tests giving false confidence, and a set of documentation mismatches. +This task records what was fixed, what was deliberately not, and what remains. + +FIXED: +- **Comma digit separator** (`tokenizer.zig`): a comma now groups digits only when + exactly three follow. The old rule was "a digit follows", so `log(100,10)` lexed + as `log(10010)` and returned 4.0004, and `max(1,2)` became `max(12)`. Every FR-5.7 + financial call was unreachable without a space after each comma. Base literals no + longer accept commas at all (they group with spaces and underscores, FR-1.8). + `max(1,234)` is inherently ambiguous and resolves to `max(1234)`; FR-1.8 now says + so. +- **Exact rendering carry** (`rational.zig`): `roundUpDecimal` dropped the carry + when the integer part was all nines, so `10 - 1/(3*10^20)` printed as + `0.00000000000000000000` in the display AND the clipboard. +- **Exact rendering of small magnitudes** (`rational.zig`, `formatter.zig`): values + below the 20-digit fractional budget rendered as zero. New + `Rational.toScientificString` renders from the rational rather than from + already-rendered text, so `2^-70` shows as `8.4703294725430034e-22`. +- **Unchecked float-to-integer conversions** (`evaluator.zig`, `programmer.zig`): + `2^64 and 1`, `~1e30` and `tally -p '1e40'` aborted the process. Now `Overflow` + or `DomainError` through `toFixedWidthBits`/`shiftAmount`. +- **`minInt(i128)` negation** (`formatter.zig`): panicked when formatting a 128-bit + value with only the sign bit set, reachable from the TUI. Now negates in the + unsigned domain. +- **Unbounded recursion** (`parser.zig`): `Parser.max_nodes` (1000) and + `max_nest_depth` (128). 4000 terms of `1+1+1+...` segfaulted `evalExact`. +- **The tautological render assertion** (`test_render.zig`): `wellFormed` compared + each row's length to the width it was allocated with and could never fail. + Replaced by `furniture(rows).intact()`, which checks the separator, prompt and + status rows. That immediately caught three layout bugs, now also fixed: the + programmer view drawing over the prompt on a short terminal (it now reports how + many rows it needs), the convert view's overflow notice erasing the unit row + above it and undercounting, and the TUI's convert value defaulting to + `Number.fromFloat(1)` so the opening screen read `999,999,999.9999999 nm`. +- Strengthened the tests the review named as coverage theater: float-view + classifications are asserted by name, the convert overflow count is asserted + exactly, programmer base rows are asserted by row index, and the financial "draws + without panicking" test now asserts every field label. + +DELIBERATELY NOT FIXED, documentation corrected instead: +- `roundToCents` is half-even on the binary value, which differs from decimal + half-even on 573 of 10000 decimal ties. Fixing it means a decimal type in the + money path; the no-bias property FR-5.8 exists for still holds. Documented on + `RoundingMode`. +- The `psi` f64 factor is 1 ulp from the correctly rounded ratio because the + comptime `p/q` division rounds twice. The exact path is unaffected. The + "cannot drift apart" claim in `units.zig` has been qualified. +- The rate solver picks whichever of several roots its seed order reaches first, + and its scale-relative tolerance does not cover noise proportional to + `(1+r)^n`, so some well-posed inputs report `ConvergenceFailure`. Both are now + documented on `solveRate` rather than half-fixed. +- The unit round-trip tests are tautological (`fromBase` is the algebraic inverse + of `toBase`); they are kept because they do catch asymmetric edits to the affine + temperature conversions, and the comment claiming a stronger guarantee is gone. +- Shift-by-width-or-more clamps rather than yielding zero. It is a policy choice + (C, Zig, x86 and Java all differ); the decision belongs with the operator + unification below. +- FR-7.8 (base quick-switch keys) is removed rather than implemented: `d`, `h`, + `o` and `b` are digits in the editable value fields. +- FR-1.6 (variables limited to `A-F, X, Y, Z`) and FR-2.8 (little-endian default) + now follow the implementation, which is right in both cases. +- Documentation corrected where it described behaviour the code does not have: + `ast.zig`'s ownership header (it claimed arena allocation with no per-node + deallocation, the opposite of the truth, which is how the AST leaks happened), + the `caret` token comment, design 2.3 and 2.5 (stale AST and Environment + shapes), design 7's programmer and conversion output, design 8.1's footer, and + design 8.0/8.0.0's overclaims about hit regions and the render harness. + +STILL OPEN, in the order I would take them: +1. `>>` is logical in standard mode and arithmetic in programmer mode, shift + amounts wrap in one and clamp in the other, and `evaluator.zig` ignores the + configured bit width entirely. The two implementations of these nine operators + need to become one, parameterised by width (FR-2.12 promises they agree). +2. Signed division and modulo in programmer mode use unsigned semantics: + `-10 / 2` gives 9223372036854775803. +3. Multi-base detail lines are computed through an f64 round trip, so + `0xDEADBEEFDEADBEEF` shows `hex: DE AD BE EF DE AD C0 00`. The exact value is + right there; the display path should use it. +4. Cross-tier `max`/`min` compare through f64 and return the wrong operand when + the values differ below f64 resolution. +5. `Rational.toFloat` double-rounds (no sticky bit), so results can be 1 ulp off. +6. Convert mode's selection zone falls through to the programmer key handler, so + Enter never fires there and printable keys mutate `prog_value`. +7. Amortization rejects valid loans because the derived payment rounds down to + cents before the "covers the interest" check; the principal column is not + rounded at all. +8. History display caps at 512 flattened lines built oldest-first, so results stop + appearing after roughly 102 detailed entries. History memory is never reclaimed + (Ctrl-L frees into an arena). +9. Money formatting degrades to `?` and still exits 0 at large magnitudes. +10. Assignment parses in prefix position (`1 + x = 2` mutates `x`), and assignment + to a constant name is silently discarded. +11. Literals longer than 128 characters are rejected by a fixed tokenizer buffer, + and non-decimal literals are capped at 64 bits, both below what the exact tier + supports. +12. `engine/src/c_api.zig` has no tests and appears in no coverage report, because + no test target builds the shared library. Deferred until Phase 6 gives it a + caller; recorded here so it is a known gap rather than an oversight. + --- ## Phase 6: Android App diff --git a/engine/src/ast.zig b/engine/src/ast.zig index 8ec4941..c34c828 100644 --- a/engine/src/ast.zig +++ b/engine/src/ast.zig @@ -1,7 +1,15 @@ //! AST node definitions for Tally expressions. //! -//! The AST is arena-allocated: all nodes live in a single arena and are freed -//! together when the expression is no longer needed. No per-node deallocation. +//! Ownership: the parser allocates each node individually with the allocator it +//! was given, hands the root to the caller, and the caller releases the tree with +//! `parser.freeExpr`, which walks it and destroys every node. Callers are free to +//! pass an arena and skip the walk (the CLI does), but the tree does not assume +//! one: `evalStringInfo` and `evalProgrammerString` both free explicitly, because +//! the TUI's allocator lives as long as the process. +//! +//! This header used to say the opposite ("arena-allocated ... no per-node +//! deallocation"), which is how both eval entry points ended up leaking the whole +//! tree on every call. const types = @import("types.zig"); const Base = types.Base; diff --git a/engine/src/evaluator.zig b/engine/src/evaluator.zig index 42c95e9..006b897 100644 --- a/engine/src/evaluator.zig +++ b/engine/src/evaluator.zig @@ -169,10 +169,9 @@ fn evalExact(env: *Environment, scratch: Allocator, expr: *const Expr) CalcError // Bitwise NOT is a fixed-width integer operation, not rational // arithmetic, so it drops to the float/integer path. .bitwise_not => blk: { - const f = operand.toFloat(scratch); - const int_val: u64 = @bitCast(@as(i64, @intFromFloat(f))); + const bits = try toFixedWidthBits(operand.toFloat(scratch)); const mask_val: u64 = @truncate(env.programmer_config.bit_width.mask()); - const result = ~int_val & mask_val; + const result = ~bits & mask_val; break :blk Number.fromFloat(@floatFromInt(@as(i64, @bitCast(result)))); }, }; @@ -231,15 +230,15 @@ fn evalBinaryOp(scratch: Allocator, op: BinaryOp, left: Number, right: Number) C .pow => Number.pow(scratch, left, right) catch |err| mapError(err), // The remaining operators are fixed-width integer operations rather than // rational arithmetic, so they work on the float/integer projection. - .bit_and => Number.fromFloat(floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseAnd)), - .bit_or => Number.fromFloat(floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseOr)), - .bit_xor => Number.fromFloat(floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseXor)), - .shift_left => Number.fromFloat(floatShift(left.toFloat(scratch), right.toFloat(scratch), true)), - .shift_right, .shift_right_logical => Number.fromFloat(floatShift(left.toFloat(scratch), right.toFloat(scratch), false)), + .bit_and => Number.fromFloat(try floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseAnd)), + .bit_or => Number.fromFloat(try floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseOr)), + .bit_xor => Number.fromFloat(try floatBitwise(left.toFloat(scratch), right.toFloat(scratch), bitwiseXor)), + .shift_left => Number.fromFloat(try floatShift(left.toFloat(scratch), right.toFloat(scratch), true)), + .shift_right, .shift_right_logical => Number.fromFloat(try floatShift(left.toFloat(scratch), right.toFloat(scratch), false)), .rotate_left, .rotate_right => blk: { // Rotations need bit width context; in standard mode, use 64-bit - const l: u64 = @bitCast(@as(i64, @intFromFloat(left.toFloat(scratch)))); - const r: u6 = @intFromFloat(@mod(right.toFloat(scratch), 64.0)); + const l = try toFixedWidthBits(left.toFloat(scratch)); + const r = try shiftAmount(right.toFloat(scratch)); const result = if (op == .rotate_left) math.rotl(u64, l, r) else @@ -249,6 +248,35 @@ fn evalBinaryOp(scratch: Allocator, op: BinaryOp, left: Number, right: Number) C }; } +/// Project a float onto the 64-bit integer domain the bitwise operators work in. +/// +/// Every one of these operators used to do `@intFromFloat` straight onto the +/// unchecked value, which is illegal behaviour out of range and aborted the +/// process: `2^64 and 1` and `~1e30` both killed it, and a NaN operand produced a +/// garbage answer instead. An operand that does not fit a machine word is a +/// reportable error, not a crash. +fn toFixedWidthBits(value: f64) CalcError!u64 { + if (!math.isFinite(value)) return CalcError.DomainError; + // i64 covers [-2^63, 2^63); 2^63 itself is the first excluded value and is + // exactly representable, so these bounds are exact. + if (value >= 9223372036854775808.0 or value < -9223372036854775808.0) { + return CalcError.Overflow; + } + return @bitCast(@as(i64, @intFromFloat(value))); +} + +/// The shift or rotate distance, reduced into 0..63. +/// +/// NOTE: reducing modulo the width is the behaviour standard mode has always had, +/// and it disagrees with programmer mode, which saturates. That divergence is a +/// separate open issue (the two implementations of these operators need to become +/// one); this only stops the conversion from being undefined. +fn shiftAmount(value: f64) CalcError!u6 { + const bits = try toFixedWidthBits(value); + const signed: i64 = @bitCast(bits); + return @intCast(@mod(signed, 64)); +} + fn bitwiseAnd(a: u64, b: u64) u64 { return a & b; } @@ -259,16 +287,16 @@ fn bitwiseXor(a: u64, b: u64) u64 { return a ^ b; } -fn floatBitwise(left: f64, right: f64, op: *const fn (u64, u64) u64) f64 { - const l: u64 = @bitCast(@as(i64, @intFromFloat(left))); - const r: u64 = @bitCast(@as(i64, @intFromFloat(right))); +fn floatBitwise(left: f64, right: f64, op: *const fn (u64, u64) u64) CalcError!f64 { + const l = try toFixedWidthBits(left); + const r = try toFixedWidthBits(right); const result = op(l, r); return @floatFromInt(@as(i64, @bitCast(result))); } -fn floatShift(left: f64, right: f64, is_left: bool) f64 { - const l: u64 = @bitCast(@as(i64, @intFromFloat(left))); - const shift_amt: u6 = @intFromFloat(@mod(right, 64.0)); +fn floatShift(left: f64, right: f64, is_left: bool) CalcError!f64 { + const l = try toFixedWidthBits(left); + const shift_amt = try shiftAmount(right); const result = if (is_left) l << shift_amt else l >> shift_amt; return @floatFromInt(@as(i64, @bitCast(result))); } @@ -1418,3 +1446,102 @@ test "no leak: repeated evaluation does not accumulate" { value.deinit(); } } + +// -- Function arguments versus grouped digits -- +// +// The tokenizer used to treat any comma followed by a digit as a thousands +// separator, so a call written without spaces silently became a call with one +// merged argument. These are the end-to-end cases: what a user types, and what +// they get. + +test "function arguments survive a comma with no space after it" { + // log(100, 10) is 2. The old rule lexed this as log(10010) and returned + // 4.0004, which is a wrong answer rather than an error. + try testing.expectApproxEqAbs(@as(f64, 2.0), try testEval("log(100,10)"), 1e-12); + try testing.expectEqual(@as(f64, 2.0), try testEval("max(1,2)")); + try testing.expectEqual(@as(f64, 1.0), try testEval("min(1,2)")); + try testing.expectApproxEqAbs(@as(f64, 0.2011244), try testEval("cagr(10000,25000,5)"), 1e-7); + try testing.expectApproxEqAbs( + @as(f64, -1199.10105), + try testEval("tvm_pmt(360,0.5,200000,0)"), + 1e-5, + ); + // Whitespace must not change the meaning. + try testing.expectEqual(try testEval("max(1, 2)"), try testEval("max(1,2)")); + try testing.expectEqual(try testEval("log(100, 10)"), try testEval("log(100,10)")); +} + +test "grouped digits still work, inside and outside a call" { + try testing.expectEqual(@as(f64, 2000.0), try testEval("1,000 * 2")); + try testing.expectEqual(@as(f64, 1234568.0), try testEval("1,234,567 + 1")); + // A grouped argument is one argument. + try testing.expectEqual(@as(f64, 1000.0), try testEval("max(1,000, 500)")); + try testing.expectEqual(@as(f64, 1500.0), try testEval("min(1,500, 2,000)")); +} + +test "a malformed group is an error, not a silently merged number" { + // Two digits after the comma is neither a group nor a valid argument list + // here, so it fails loudly instead of evaluating as 100. + try testing.expectError(CalcError.UnexpectedToken, testEval("1,00")); + try testing.expectError(CalcError.UnexpectedToken, testEval("1,0000")); + try testing.expectError(CalcError.UnexpectedToken, testEval("2+3,4")); +} + +test "a grouped literal past 2^53 is still exact" { + // The separators have to reach the exact re-parse, not just the f64 channel. + var env = Environment.init(testing.allocator, .standard); + defer env.deinit(); + var value = try evalString(&env, testing.allocator, "9,007,199,254,740,993"); + defer value.deinit(); + try testing.expect(value.isExact()); + const shown = try @import("formatter.zig").formatNumber(testing.allocator, value); + defer shown.deinit(testing.allocator); + try testing.expectEqualStrings("9007199254740993", shown.raw); +} + +// -- Operands that do not fit a machine word -- +// +// The bitwise operators project through f64 and then convert to an integer. That +// conversion used to be unchecked, so ordinary input aborted the process: +// `2^64 and 1` and `~1e30` both died with "integer part of floating point value +// out of bounds", and a NaN operand produced a garbage number instead. + +test "bitwise operands outside i64 report overflow instead of aborting" { + const cases = [_][]const u8{ + "2^64 and 1", + "1e30 and 1", + "1e30 or 1", + "1e30 xor 1", + "~1e30", + "~(2^1000)", + "1e30 rol 1", + "1e30 ror 1", + "1e30 << 1", + "1e30 >> 1", + "1 << 1e30", + "1 rol 1e30", + "0 - 1e30 and 1", + }; + for (cases) |source| { + const result = testEval(source); + try testing.expectError(CalcError.Overflow, result); + } +} + +test "a non-finite bitwise operand is a domain error" { + // ln(-1) is NaN, and 1/0 raises before it can reach here, so NaN arrives via + // the transcendental fallback. + try testing.expectError(CalcError.DomainError, testEval("~ln(-1)")); + try testing.expectError(CalcError.DomainError, testEval("ln(-1) and 1")); + try testing.expectError(CalcError.DomainError, testEval("1 << ln(-1)")); +} + +test "bitwise operators still work at the edges of the range" { + // Just inside i64: 2^63 - 1 as a float is 2^63, so use 2^62 for a clean case. + try testing.expectEqual(@as(f64, 0.0), try testEval("2^62 and 1")); + try testing.expectEqual(@as(f64, 15.0), try testEval("0xFF and 0x0F")); + try testing.expectEqual(@as(f64, 8.0), try testEval("1 << 3")); + try testing.expectEqual(@as(f64, -1.0), try testEval("~0")); + // Negative operands are fine; they are two's complement bit patterns. + try testing.expectEqual(@as(f64, -2.0), try testEval("~1")); +} diff --git a/engine/src/financial.zig b/engine/src/financial.zig index 7d9b6eb..cc67eca 100644 --- a/engine/src/financial.zig +++ b/engine/src/financial.zig @@ -332,6 +332,18 @@ fn solvePeriods(p: TvmParams) CalcError!f64 { /// Several starting points are tried because the residual can be flat or have a /// bad slope near a poor initial guess, and a single seed makes the solver fail /// on otherwise well-posed inputs. +/// +/// Two known limits, both left as they are for now: +/// +/// 1. When a root exists at more than one rate, the seed order decides which one +/// is returned, with no indication that another exists. `tvm_rate(2, -1, 5, -11)` +/// has roots at 100% and 200% and reports the first the sequence reaches. +/// 2. The scale-relative tolerance covers noise proportional to the cash flows but +/// not noise proportional to `(1+r)^n`, so a large growth factor can put the +/// residual permanently above the limit and report `ConvergenceFailure` for an +/// input that does have an answer (n=360 with r near 6% and a matching payment +/// is one). Fixing that means scaling by the computed terms rather than by the +/// inputs, which changes acceptance for every case and needs its own testing. fn solveRate(p: TvmParams) CalcError!TvmSolution { const n = p.periods.?; const pv = p.present_value.?; @@ -391,8 +403,18 @@ fn solveRate(p: TvmParams) CalcError!TvmSolution { // -- Money rounding -- pub const RoundingMode = enum { - /// Halves go to the nearest even digit. The default for money because it - /// does not bias totals upward the way half-up does across many roundings. + /// Halves go to the nearest even digit. The default for money because it does + /// not bias totals upward the way half-up does across many roundings. + /// + /// It is half-even on the BINARY value, which is not always half-even on the + /// decimal one. `roundToScale` multiplies by a power of ten and rounds the + /// result, and most decimal halves are not exactly representable: 0.545 + /// becomes 54.499999999999996 once scaled and rounds down, where exact decimal + /// half-even would round up. Sweeping every value `k/1000` whose last digit is + /// 5 shows 573 of 10000 going the other way, in both directions, so the + /// no-bias property this exists for still holds. Matching decimal half-even + /// exactly would mean a decimal type in the money path, which is a larger + /// change than the discrepancy warrants. half_even, /// Halves go away from zero. What most people mean by "round". half_up, @@ -922,8 +944,11 @@ test "tvm: periods with zero rate and zero payment is unsolvable" { } test "roundToScale: half-even avoids the upward bias of half-up" { - // The classic pair: both are exactly halfway, and half-even splits them. + // 0.025 scales to exactly 2.5, a true tie, and half-even takes it down. try testing.expectEqual(@as(f64, 0.02), roundToScale(0.025, 2, .half_even)); + // 0.035 is NOT a tie once scaled: 0.035*100 is 3.5000000000000004, so this + // rounds up on magnitude rather than by the tie rule. It is here as the + // companion figure people expect, not as a second demonstration of half-even. try testing.expectEqual(@as(f64, 0.04), roundToScale(0.035, 2, .half_even)); // Half-up sends both the same direction. try testing.expectEqual(@as(f64, 0.03), roundToScale(0.025, 2, .half_up)); diff --git a/engine/src/formatter.zig b/engine/src/formatter.zig index ab0e5b0..54cb775 100644 --- a/engine/src/formatter.zig +++ b/engine/src/formatter.zig @@ -8,10 +8,13 @@ //! - Decimal: comma-separated groups of 3 (e.g. "4,294,967,295"). The integer //! part is grouped whether or not there is a fractional part, so "231,677.04" //! and "231,677" read consistently. A fractional part is never grouped. -//! - Hex value view: underscore per 16-bit word (e.g. "0xFFFF_FFFF") +//! - Hex value view: space per byte (e.g. "FF FF FF FF"); the prefix appears only +//! in the clipboard form ("0xFFFFFFFF") //! - Binary: space per nibble (e.g. "1111 1111") -//! - Octal: underscore per 3-digit group (e.g. "0o37_777_777_777") -//! - Scientific notation only when |value| > 10^15 or < 10^-15 or > 15 sig digits +//! - Octal: space per 3-digit group, zero-padded to the bit width +//! - Scientific notation when |value| is above 10^15 or below 10^-15, and for an +//! exact value whose integer part exceeds `max_display_integer_digits` or whose +//! magnitude is below the fractional budget const std = @import("std"); const types = @import("types.zig"); @@ -28,10 +31,16 @@ pub const FormattedValue = struct { /// Format a floating-point value for display. /// Uses comma grouping for integers, avoids scientific notation unless necessary. /// -/// The 2^53 bound below is NOT a display preference: past it an f64 no longer -/// distinguishes consecutive integers, so printing one as an exact-looking -/// integer would assert precision the value does not have. Exact values are not -/// subject to this and go through `formatNumber`, which prints them in full. +/// The 2^53 bound is NOT a display preference: past it an f64 no longer +/// distinguishes consecutive integers, so printing one as an exact-looking integer +/// would assert precision the value does not have. Exact values are not subject to +/// this and go through `formatNumber`, which prints them in full. +/// +/// In practice the `< 1e15` test on the next line is the stricter of the two, so +/// the 2^53 bound never decides an outcome on its own. It is kept because the two +/// bounds mean different things: one is about representability, the other about how +/// many digits are worth showing, and a change to the display threshold should not +/// silently remove the representability check. pub fn formatFloat(buf: []u8, value: f64) FormattedValue { const is_integer = value == @trunc(value) and @abs(value) < 9007199254740992.0; // 2^53 @@ -128,13 +137,41 @@ pub fn formatNumber(allocator: std.mem.Allocator, value: Number) !NumberDisplay return .{ .display = display, .raw = rendered.text, .exact = false }; } - // Group the integer part for readability; the raw form stays plain. - const display = try groupDecimalText(allocator, rendered.text); - return .{ .display = display, .raw = rendered.text, .exact = rendered.exact }; + // The other end of the same problem: a value smaller than the + // fractional budget renders as all zeros, which destroys it in the + // clipboard as well as on screen (2^-70 printed as 0.00000...). + // Scientific notation is the only honest rendering, and it has to come + // from the rational rather than from this text, which has no digits + // left in it. + if (!isZeroText(rendered.text)) { + // Group the integer part for readability; the raw form stays plain. + const display = try groupDecimalText(allocator, rendered.text); + return .{ .display = display, .raw = rendered.text, .exact = rendered.exact }; + } + if (r.isZero()) { + const display = try allocator.dupe(u8, rendered.text); + return .{ .display = display, .raw = rendered.text, .exact = true }; + } + allocator.free(rendered.text); + const scientific = try r.toScientificString(allocator, scientific_significant_digits); + errdefer allocator.free(scientific); + const raw = try allocator.dupe(u8, scientific); + // Rounded to 17 significant digits, so not exact even though the value + // is: the tag describes the text, not the value behind it. + return .{ .display = scientific, .raw = raw, .exact = false }; }, } } +/// True when decimal text carries no significant digit, i.e. it is some spelling +/// of zero ("0", "0.00", "-0.000"). +fn isZeroText(text: []const u8) bool { + for (text) |ch| { + if (ch >= '1' and ch <= '9') return false; + } + return true; +} + /// Fractional digits produced for an exact value whose decimal expansion does /// not terminate (1/3, 1/7). Exact arithmetic can justify more digits than f64, /// so this is above f64's ~17 significant digits. @@ -376,8 +413,6 @@ pub fn formatBinary(buf: []u8, value: u128, bit_width: BitWidth) FormattedValue return .{ .display = display, .raw = raw }; } -/// Format an integer for programmer mode octal display. -/// Display: "0o37_777_777_777" (underscore per 3-digit group) /// Format an integer for programmer mode octal display. /// Display: "0 000 000 000 777" (space per 3-digit group, zero-padded to full width) /// Raw: "0o0000000000777" (no separators, with prefix) @@ -519,12 +554,21 @@ fn writeUnsignedInt(buf: []u8, value: u128) usize { fn writeSignedInt(buf: []u8, value: i128) usize { if (value < 0) { buf[0] = '-'; - const abs_val: u128 = @intCast(-value); - return 1 + writeUnsignedInt(buf[1..], abs_val); + return 1 + writeUnsignedInt(buf[1..], absoluteValue(value)); } return writeUnsignedInt(buf, @intCast(value)); } +/// Magnitude of a signed 128-bit value as an unsigned one. +/// +/// `-value` overflows for `minInt(i128)`, which panics in Debug and ReleaseSafe. +/// That value is reachable: a 128-bit programmer-mode word with only the sign bit +/// set formats through here. Negating in the unsigned domain has no such edge. +fn absoluteValue(value: i128) u128 { + const bits: u128 = @bitCast(value); + return if (value < 0) ~bits +% 1 else bits; +} + fn writeUnsignedWithCommas(buf: []u8, value: u128) usize { if (value == 0) { buf[0] = '0'; @@ -557,8 +601,7 @@ fn writeUnsignedWithCommas(buf: []u8, value: u128) usize { fn writeDecimalWithCommas(buf: []u8, value: i128) usize { if (value < 0) { buf[0] = '-'; - const abs_val: u128 = @intCast(-value); - return 1 + writeUnsignedWithCommas(buf[1..], abs_val); + return 1 + writeUnsignedWithCommas(buf[1..], absoluteValue(value)); } return writeUnsignedWithCommas(buf, @intCast(value)); } @@ -1205,3 +1248,106 @@ test "grouped display re-parses to the same value" { try testing.expectEqualStrings("1,234,567.891", result.display); try testing.expectApproxEqAbs(@as(f64, 1234567.891), reparsed.toFloat(testing.allocator), 1e-9); } + +// -- Exact values too small for the fractional budget -- +// +// The exact path renders 20 fractional digits, so anything below 1e-20 came out +// as "0.00000000000000000000" in `display` AND in `raw`. That destroyed the value +// at the last step, in the one tier whose entire purpose is not doing that, and it +// made the exact tier display strictly worse than the inexact one. + +test "formatNumber: an exact value below the fractional budget uses scientific notation" { + // 2^-70, exactly representable, equal to 8.470329472543003e-22. + var value = try Number.parse(testing.allocator, "1"); + defer value.deinit(); + var divisor = try Number.parse(testing.allocator, "1180591620717411303424"); + defer divisor.deinit(); + var tiny = try Number.div(testing.allocator, value, divisor); + defer tiny.deinit(); + + const shown = try formatNumber(testing.allocator, tiny); + defer shown.deinit(testing.allocator); + try testing.expectEqualStrings("8.4703294725430034e-22", shown.display); + // The clipboard form must not be zero either. + try testing.expectEqualStrings("8.4703294725430034e-22", shown.raw); + // Rounded to 17 significant digits, so the text is not exact even though the + // value is. + try testing.expect(!shown.exact); +} + +test "formatNumber: a small exact value keeps its sign" { + var numerator = try Number.parse(testing.allocator, "-1"); + defer numerator.deinit(); + var divisor = try Number.parse(testing.allocator, "1180591620717411303424"); + defer divisor.deinit(); + var tiny = try Number.div(testing.allocator, numerator, divisor); + defer tiny.deinit(); + + const shown = try formatNumber(testing.allocator, tiny); + defer shown.deinit(testing.allocator); + try testing.expect(shown.display[0] == '-'); + try testing.expectEqualStrings("-8.4703294725430034e-22", shown.display); +} + +test "formatNumber: exact zero is still zero, not scientific" { + var zero = try Number.parse(testing.allocator, "0"); + defer zero.deinit(); + const shown = try formatNumber(testing.allocator, zero); + defer shown.deinit(testing.allocator); + try testing.expectEqualStrings("0", shown.display); + try testing.expectEqualStrings("0", shown.raw); + try testing.expect(shown.exact); +} + +test "formatNumber: values just inside the budget still print fixed point" { + // 1e-20 is the last magnitude the 20-digit budget can show. + var value = try Number.parse(testing.allocator, "0.00000000000000000001"); + defer value.deinit(); + const shown = try formatNumber(testing.allocator, value); + defer shown.deinit(testing.allocator); + try testing.expectEqualStrings("0.00000000000000000001", shown.display); + try testing.expect(shown.exact); +} + +test "isZeroText: recognises every spelling of zero" { + try testing.expect(isZeroText("0")); + try testing.expect(isZeroText("0.00")); + try testing.expect(isZeroText("-0.00000000000000000000")); + try testing.expect(!isZeroText("0.00000000000000000001")); + try testing.expect(!isZeroText("10.00")); + try testing.expect(!isZeroText("-0.5")); +} + +// -- The most negative 128-bit value -- +// +// Formatting used to negate the value to get its magnitude, which overflows for +// minInt(i128) and panics. It is reachable: a 128-bit programmer word with only +// the sign bit set formats through here, so cycling the TUI to 128 bits and +// entering 2^127 killed the process. + +test "absoluteValue: the most negative value has a magnitude" { + try testing.expectEqual(@as(u128, 1 << 127), absoluteValue(std.math.minInt(i128))); + try testing.expectEqual(@as(u128, 5), absoluteValue(-5)); + try testing.expectEqual(@as(u128, 5), absoluteValue(5)); + try testing.expectEqual(@as(u128, 0), absoluteValue(0)); + try testing.expectEqual(@as(u128, std.math.maxInt(i128)), absoluteValue(std.math.maxInt(i128))); +} + +test "formatDecimalSigned: minInt(i128) formats instead of panicking" { + var buf: [256]u8 = undefined; + const result = formatDecimalSigned(&buf, std.math.minInt(i128)); + // -2^127 + try testing.expectEqualStrings("-170141183460469231731687303715884105728", result.raw); + try testing.expectEqualStrings("-170,141,183,460,469,231,731,687,303,715,884,105,728", result.display); +} + +test "formatDecimalSigned: the rest of the signed range still formats" { + var buf: [256]u8 = undefined; + try testing.expectEqualStrings("-1", formatDecimalSigned(&buf, -1).raw); + try testing.expectEqualStrings("-1,234", formatDecimalSigned(&buf, -1234).display); + try testing.expectEqualStrings("0", formatDecimalSigned(&buf, 0).raw); + try testing.expectEqualStrings( + "170,141,183,460,469,231,731,687,303,715,884,105,727", + formatDecimalSigned(&buf, std.math.maxInt(i128)).display, + ); +} diff --git a/engine/src/parser.zig b/engine/src/parser.zig index 00b52c6..d5c6b2f 100644 --- a/engine/src/parser.zig +++ b/engine/src/parser.zig @@ -48,6 +48,24 @@ pub const Parser = struct { allocator: Allocator, had_error: bool, error_pos: ?usize, + /// Nodes built so far, checked against `max_nodes`. + node_count: usize, + /// Current parseExpr/parsePrefix nesting, checked against `max_nest_depth`. + nest_depth: usize, + + /// Ceiling on tree size. + /// + /// This is a stack-safety limit, not a style preference. Everything that walks + /// a tree recurses on it (`evalExact`, `freeExpr`, `hasNonDecimalLiteral`), and + /// a flat chain like `1+1+1+...` builds a left-deep tree whose depth equals the + /// number of operators. Measured on this build: 3000 terms evaluated fine and + /// 4000 terms segfaulted in `evalExact`, from about 8 KB of input. A thousand + /// nodes is far more than any hand-written expression and leaves a wide margin. + pub const max_nodes: usize = 1000; + + /// Ceiling on nesting, which bounds recursion inside the parser itself. + /// Reached long before `max_nodes` by input like `((((...1...))))`. + pub const max_nest_depth: usize = 128; pub fn init(allocator: Allocator, source: []const u8, mode: Mode) Parser { var tok = Tokenizer.init(source, mode); @@ -61,6 +79,8 @@ pub const Parser = struct { .allocator = allocator, .had_error = false, .error_pos = null, + .node_count = 0, + .nest_depth = 0, }; } @@ -85,6 +105,10 @@ pub const Parser = struct { /// Parse an expression with the given minimum precedence. fn parseExpr(self: *Parser, min_prec: Prec) CalcError!*Expr { + if (self.nest_depth >= max_nest_depth) return CalcError.InvalidExpression; + self.nest_depth += 1; + defer self.nest_depth -= 1; + var left = try self.parsePrefix(); // Each successful parseInfix returns a node that has adopted `left`, so // this errdefer always covers the whole tree built so far. @@ -341,6 +365,13 @@ pub const Parser = struct { } fn makeNode(self: *Parser, expr: Expr) CalcError!*Expr { + // Budget checked here so every construction site is covered by one test. + if (self.node_count >= max_nodes) { + self.had_error = true; + self.error_pos = self.current.start; + return CalcError.InvalidExpression; + } + self.node_count += 1; const node = self.allocator.create(Expr) catch return CalcError.OutOfMemory; node.* = expr; return node; @@ -715,3 +746,92 @@ test "an allocation failure mid-parse frees whatever was built" { } } } + +// -- Size and depth limits -- +// +// Every tree walk in the engine recurses (evalExact, freeExpr, +// hasNonDecimalLiteral), and a flat chain builds a left-deep tree whose depth is +// the operator count. Before these limits, 4000 terms of "1+1+1+..." segfaulted +// the process from roughly 8 KB of input, and 6000 nested parens crashed inside +// the parser. + +test "a tree larger than the node budget is rejected, not built" { + // One node per literal plus one per operator, so 1000 nodes is about 500 terms. + var over = std.ArrayList(u8).empty; + defer over.deinit(testing.allocator); + for (0..900) |i| { + if (i != 0) try over.appendSlice(testing.allocator, "+"); + try over.appendSlice(testing.allocator, "1"); + } + + var parser = Parser.init(testing.allocator, over.items, .standard); + try testing.expectError(CalcError.InvalidExpression, parser.parse()); + // Nothing is left allocated: testing.allocator would report a leak otherwise. +} + +test "an expression within the node budget still parses" { + var ok = std.ArrayList(u8).empty; + defer ok.deinit(testing.allocator); + for (0..400) |i| { + if (i != 0) try ok.appendSlice(testing.allocator, "+"); + try ok.appendSlice(testing.allocator, "1"); + } + + var parser = Parser.init(testing.allocator, ok.items, .standard); + const expr = try parser.parse(); + defer freeExpr(testing.allocator, expr); + try testing.expect(parser.node_count <= Parser.max_nodes); +} + +test "nesting deeper than the depth limit is rejected" { + var deep = std.ArrayList(u8).empty; + defer deep.deinit(testing.allocator); + for (0..Parser.max_nest_depth + 10) |_| try deep.append(testing.allocator, '('); + try deep.append(testing.allocator, '1'); + for (0..Parser.max_nest_depth + 10) |_| try deep.append(testing.allocator, ')'); + + var parser = Parser.init(testing.allocator, deep.items, .standard); + try testing.expectError(CalcError.InvalidExpression, parser.parse()); +} + +test "nesting within the depth limit parses" { + var deep = std.ArrayList(u8).empty; + defer deep.deinit(testing.allocator); + for (0..64) |_| try deep.append(testing.allocator, '('); + try deep.append(testing.allocator, '7'); + for (0..64) |_| try deep.append(testing.allocator, ')'); + + var parser = Parser.init(testing.allocator, deep.items, .standard); + const expr = try parser.parse(); + defer freeExpr(testing.allocator, expr); + try testing.expectEqual(@as(f64, 7.0), expr.number.float_value); +} + +test "unbalanced deep nesting is rejected without leaking the partial tree" { + var deep = std.ArrayList(u8).empty; + defer deep.deinit(testing.allocator); + for (0..8000) |_| try deep.append(testing.allocator, '('); + try deep.append(testing.allocator, '1'); + + var parser = Parser.init(testing.allocator, deep.items, .standard); + try testing.expect(if (parser.parse()) |_| false else |_| true); +} + +test "the depth limit also covers nested calls and unary operators" { + var deep = std.ArrayList(u8).empty; + defer deep.deinit(testing.allocator); + for (0..Parser.max_nest_depth + 10) |_| try deep.appendSlice(testing.allocator, "sqrt("); + try deep.append(testing.allocator, '4'); + for (0..Parser.max_nest_depth + 10) |_| try deep.append(testing.allocator, ')'); + + var parser = Parser.init(testing.allocator, deep.items, .standard); + try testing.expectError(CalcError.InvalidExpression, parser.parse()); + + var unary = std.ArrayList(u8).empty; + defer unary.deinit(testing.allocator); + for (0..Parser.max_nest_depth + 10) |_| try unary.append(testing.allocator, '-'); + try unary.append(testing.allocator, '1'); + + var unary_parser = Parser.init(testing.allocator, unary.items, .standard); + try testing.expectError(CalcError.InvalidExpression, unary_parser.parse()); +} diff --git a/engine/src/programmer.zig b/engine/src/programmer.zig index ac2b744..24d01c0 100644 --- a/engine/src/programmer.zig +++ b/engine/src/programmer.zig @@ -36,7 +36,14 @@ fn evalExpr(config: ProgrammerConfig, expr: *const Expr) CalcError!u128 { // Float literal in programmer mode: truncate to integer. // (Number literals are always non-negative; unary minus is a // separate operator handled below.) - const val: u128 = @intFromFloat(n.float_value); + // + // The range check is not optional: `@intFromFloat` on an out-of-range + // value is illegal behaviour, and `tally -p '1e40'` aborted the process + // before this guard existed. + const value = n.float_value; + if (!std.math.isFinite(value) or value < 0) return CalcError.DomainError; + if (value >= 340282366920938463463374607431768211456.0) return CalcError.Overflow; + const val: u128 = @intFromFloat(value); return val & config.bit_width.mask(); }, .string_literal => |text| { @@ -466,3 +473,31 @@ test "no leak: evalProgrammerString releases the parsed tree" { return error.TestUnexpectedResult; } } + +test "programmer mode: a float literal out of range errors instead of aborting" { + const config: ProgrammerConfig = .{}; + // `tally -p '1e40'` used to abort the process here: @intFromFloat on a value + // past u128 is illegal behaviour, and only 3.14 was ever tested. + try std.testing.expectError( + CalcError.Overflow, + evalProgrammerString(std.testing.allocator, "1e40", config), + ); + try std.testing.expectError( + CalcError.Overflow, + evalProgrammerString(std.testing.allocator, "1e100", config), + ); + // Still truncates the values that do fit. + const small = try evalProgrammerString(std.testing.allocator, "3.99", config); + try std.testing.expectEqual(@as(u128, 3), small.unsignedValue()); + const large = try evalProgrammerString(std.testing.allocator, "1e30", config); + try std.testing.expect(large.unsignedValue() != 0); +} + +test "programmer mode: an infinite or NaN literal is a domain error" { + const config: ProgrammerConfig = .{}; + // 10^400 overflows the exact tier's float projection to infinity. + try std.testing.expectError( + CalcError.DomainError, + evalProgrammerString(std.testing.allocator, "1e400", config), + ); +} diff --git a/engine/src/rational.zig b/engine/src/rational.zig index ca84605..8fcb375 100644 --- a/engine/src/rational.zig +++ b/engine/src/rational.zig @@ -655,7 +655,13 @@ pub const Rational = struct { defer two.deinit(); try doubled.mul(&rem, &two); if (doubled.order(self.den) != .lt) { - roundUpDecimal(out.items); + if (roundUpDecimal(out.items)) { + // The carry ran off the front, so the integer part was all + // nines and gained a digit: 9.99... becomes 10.00..., and + // 9.99... just under 10 must not print as 0.00... + const insert_at: usize = if (out.items.len > 0 and out.items[0] == '-') 1 else 0; + try out.insert(allocator, insert_at, '1'); + } } } @@ -669,8 +675,157 @@ pub const Rational = struct { return .{ .text = try out.toOwnedSlice(allocator), .exact = exact }; } + + /// Render in scientific notation with `significant_digits` mantissa digits, + /// rounded half-up. + /// + /// This exists for values the fixed-point renderer cannot show at all. An + /// exact `2^-70` is 8.47e-22, and `toDecimalString` with a 20-digit budget + /// renders it as `0.000...0`, destroying the value in the display and in the + /// clipboard alike. Working from the rational rather than from already + /// rendered text means the exponent can be arbitrarily negative without + /// materialising the leading zeros. + pub fn toScientificString( + self: Rational, + allocator: Allocator, + significant_digits: usize, + ) Error![]u8 { + std.debug.assert(significant_digits >= 1); + if (self.isZero()) return allocator.dupe(u8, "0"); + + var magnitude = try self.num.clone(); + defer magnitude.deinit(); + magnitude.abs(); + + // First guess from the bit lengths: log10(x) is about bits(x) * log10(2). + // It can be off by one either way, which the loop below corrects. + const num_bits: i64 = @intCast(magnitude.bitCountAbs()); + const den_bits: i64 = @intCast(self.den.bitCountAbs()); + const log10_of_2 = 0.30102999566398120; + var exponent: i64 = @intFromFloat( + @floor(@as(f64, @floatFromInt(num_bits - den_bits)) * log10_of_2), + ); + + const wanted: i64 = @intCast(significant_digits); + // The exponent has to be settled on the TRUNCATED scaling, not the rounded + // one: for 2/3 with one significant digit, rounding 0.667 gives "1", which + // looks like a correct one-digit mantissa while actually meaning 1e0 + // instead of 7e-1. Floor first, fix the exponent, then round. + var attempt: usize = 0; + while (attempt < 4) : (attempt += 1) { + var scaled = try scaleFloor(allocator, magnitude, self.den, wanted - 1 - exponent); + defer scaled.quotient.deinit(); + + var digits = try decimalDigits(scaled.quotient, allocator); + defer allocator.free(digits); + + // A zero quotient means the exponent guess was too high. Its digit + // string is "0", one character, so the length check below would accept + // it for a one-digit mantissa and then round it to "1". + if (scaled.quotient.eqlZero()) { + exponent -= 1; + continue; + } + + if (digits.len != significant_digits) { + exponent += @as(i64, @intCast(digits.len)) - wanted; + continue; + } + + // Half-up on the digit string, which is where a carry can add a digit: + // 9.99 with three digits becomes 1e1, not 10.0e0. + if (scaled.round_up) { + var i = digits.len; + var carried = true; + while (i > 0 and carried) { + i -= 1; + if (digits[i] == '9') { + digits[i] = '0'; + } else { + digits[i] += 1; + carried = false; + } + } + if (carried) { + // Every digit was a nine: the mantissa is 1 and the exponent + // moves up. + digits[0] = '1'; + exponent += 1; + } + } + + var out = std.ArrayList(u8).empty; + errdefer out.deinit(allocator); + if (self.isNegative()) try out.append(allocator, '-'); + try out.append(allocator, digits[0]); + + // Trailing zeros in the mantissa carry no information here. + var end = digits.len; + while (end > 1 and digits[end - 1] == '0') end -= 1; + if (end > 1) { + try out.append(allocator, '.'); + try out.appendSlice(allocator, digits[1..end]); + } + var exponent_buf: [24]u8 = undefined; + // 24 bytes holds "e-9223372036854775808", the widest an i64 exponent + // can be, so the only way this fails is a bug in this buffer size. + const exponent_text = std.fmt.bufPrint(&exponent_buf, "e{d}", .{exponent}) catch + @panic("exponent buffer too small"); + try out.appendSlice(allocator, exponent_text); + return out.toOwnedSlice(allocator); + } + // The estimate failed to settle, which should not happen. Rather than spin, + // report it instead of returning something wrong. + return Error.ExponentTooLarge; + } }; +/// `floor(magnitude * 10^shift / denominator)` plus whether a half-up rounding +/// step would increment it. A negative `shift` scales the denominator instead. +/// +/// The caller needs these separately: the digit count of the floor decides the +/// exponent, and only then can the mantissa be rounded. +fn scaleFloor( + allocator: Allocator, + magnitude: Managed, + denominator: Managed, + shift: i64, +) Error!struct { quotient: Managed, round_up: bool } { + var ten = try Managed.initSet(allocator, 10); + defer ten.deinit(); + + var numerator = try magnitude.clone(); + defer numerator.deinit(); + var divisor = try denominator.clone(); + defer divisor.deinit(); + + const shift_magnitude = @abs(shift); + if (shift_magnitude > std.math.maxInt(u32)) return Error.ExponentTooLarge; + const power: u32 = @intCast(shift_magnitude); + if (power != 0) { + try ten.pow(&ten, power); + if (shift > 0) { + try numerator.mul(&numerator, &ten); + } else { + try divisor.mul(&divisor, &ten); + } + } + + var quotient = try Managed.init(allocator); + errdefer quotient.deinit(); + var remainder = try Managed.init(allocator); + defer remainder.deinit(); + try quotient.divFloor(&remainder, &numerator, &divisor); + + // Half-up: 2*remainder >= divisor rounds away from zero. + var doubled = try Managed.init(allocator); + defer doubled.deinit(); + var two = try Managed.initSet(allocator, 2); + defer two.deinit(); + try doubled.mul(&remainder, &two); + return .{ .quotient = quotient, .round_up = doubled.order(divisor) != .lt }; +} + /// Base-10 digits of a big integer. Base 10 is always valid, so the only real /// failure mode is allocation. fn decimalDigits(m: Managed, allocator: Allocator) Error![]u8 { @@ -681,9 +836,15 @@ fn decimalDigits(m: Managed, allocator: Allocator) Error![]u8 { } /// Propagate a half-up rounding carry through a decimal string in place. -/// Only called when the digits produced can absorb the carry, which is -/// guaranteed here because the integer part is present. -fn roundUpDecimal(text: []u8) void { +/// +/// Returns true when the carry ran off the front, which happens whenever every +/// digit was a nine: `9.99...` rounds to `10.00...` and needs a digit the string +/// does not have room for. The caller prepends the `1`. +/// +/// The previous version returned void and claimed the integer part could always +/// absorb the carry. It cannot: `10 - 1/(3*10^20)` printed as `0.000...`, and the +/// same wrong text went to the clipboard. +fn roundUpDecimal(text: []u8) bool { var i = text.len; while (i > 0) { i -= 1; @@ -691,10 +852,11 @@ fn roundUpDecimal(text: []u8) void { if (c == '.' or c == '-') continue; if (c != '9') { text[i] = c + 1; - return; + return false; } text[i] = '0'; } + return true; } // -- Tests -- @@ -1662,3 +1824,185 @@ test "parse: malformed fractions error" { try testing.expectError(Error.InvalidNumber, Rational.parse(alloc, "/2")); try testing.expectError(Error.InvalidNumber, Rational.parse(alloc, "a/b")); } + +// -- Rounding carry out of the integer part -- +// +// roundUpDecimal used to return void and assumed the integer part could always +// absorb the carry. When every digit is a nine it cannot, and the value printed +// as zero: `10 - 1/(3*10^20)` came out as 0.00000000000000000000, in the clipboard +// text as well as on screen. + +test "toDecimalString: a carry out of an all-nines integer part adds a digit" { + // The file-level `alloc` is testing.allocator. + + // 10 - 1/(3*10^20) is 9.999...967, which rounds to 10 at 20 digits. + var tiny = try Rational.initRatio(alloc, 1, 3); + defer tiny.deinit(); + var scale = try Rational.initInt(alloc, 100000000000000000000); // 10^20 + defer scale.deinit(); + var epsilon = try Rational.div(alloc, tiny, scale); + defer epsilon.deinit(); + var ten = try Rational.initInt(alloc, 10); + defer ten.deinit(); + var value = try Rational.sub(alloc, ten, epsilon); + defer value.deinit(); + + const rendered = try value.toDecimalString(alloc, 20); + defer alloc.free(rendered.text); + try testing.expectEqualStrings("10.00000000000000000000", rendered.text); + try testing.expect(!rendered.exact); +} + +test "toDecimalString: the carry works at every power of ten and keeps the sign" { + // The file-level `alloc` is testing.allocator. + // 100 - epsilon, 1000 - epsilon, and the negative case. + const cases = [_]struct { whole: i64, expected: []const u8 }{ + .{ .whole = 10, .expected = "10.00000000000000000000" }, + .{ .whole = 100, .expected = "100.00000000000000000000" }, + .{ .whole = 1000, .expected = "1000.00000000000000000000" }, + .{ .whole = 1000000, .expected = "1000000.00000000000000000000" }, + }; + for (cases) |case| { + var third = try Rational.initRatio(alloc, 1, 3); + defer third.deinit(); + var scale = try Rational.initInt(alloc, 100000000000000000000); + defer scale.deinit(); + var epsilon = try Rational.div(alloc, third, scale); + defer epsilon.deinit(); + var whole = try Rational.initInt(alloc, case.whole); + defer whole.deinit(); + + var value = try Rational.sub(alloc, whole, epsilon); + defer value.deinit(); + const rendered = try value.toDecimalString(alloc, 20); + defer alloc.free(rendered.text); + try testing.expectEqualStrings(case.expected, rendered.text); + + var negated = try Rational.negate(alloc, value); + defer negated.deinit(); + const negative = try negated.toDecimalString(alloc, 20); + defer alloc.free(negative.text); + try testing.expect(negative.text[0] == '-'); + try testing.expectEqualStrings(case.expected, negative.text[1..]); + } +} + +test "toDecimalString: a carry that does not escape still works" { + // The file-level `alloc` is testing.allocator. + // 2/3 rounds up in its last digit only. + var value = try Rational.initRatio(alloc, 2, 3); + defer value.deinit(); + const rendered = try value.toDecimalString(alloc, 20); + defer alloc.free(rendered.text); + try testing.expectEqualStrings("0.66666666666666666667", rendered.text); +} + +// -- Scientific rendering -- + +test "toScientificString: values the fixed-point renderer cannot show" { + // The file-level `alloc` is testing.allocator. + + // 2^-70 = 8.470329472543003e-22, independently: 1/1180591620717411303424. + var value = try Rational.initRatio(alloc, 1, 1180591620717411303424); + defer value.deinit(); + const text = try value.toScientificString(alloc, 17); + defer alloc.free(text); + try testing.expectEqualStrings("8.4703294725430034e-22", text); +} + +test "toScientificString: sign, exponent boundaries and trailing zeros" { + // The file-level `alloc` is testing.allocator. + + const cases = [_]struct { num: i64, den: i64, expected: []const u8 }{ + .{ .num = 1, .den = 1, .expected = "1e0" }, + .{ .num = -1, .den = 1, .expected = "-1e0" }, + .{ .num = 1, .den = 2, .expected = "5e-1" }, + .{ .num = 1, .den = 3, .expected = "3.3333333333333333e-1" }, + .{ .num = -1, .den = 3, .expected = "-3.3333333333333333e-1" }, + .{ .num = 1, .den = 1000, .expected = "1e-3" }, + .{ .num = 999, .den = 1000, .expected = "9.99e-1" }, + .{ .num = 123456, .den = 1, .expected = "1.23456e5" }, + .{ .num = 1000000, .den = 1, .expected = "1e6" }, + }; + for (cases) |case| { + var value = try Rational.initRatio(alloc, case.num, case.den); + defer value.deinit(); + const text = try value.toScientificString(alloc, 17); + defer alloc.free(text); + try testing.expectEqualStrings(case.expected, text); + } +} + +test "toScientificString: rounding that carries into a new exponent" { + // The file-level `alloc` is testing.allocator. + // 0.99999999999999999999 (twenty nines) with 3 significant digits is 1.00e0, + // so the exponent has to move. + var value = try Rational.initRatio(alloc, 99999999999999999, 100000000000000000); + defer value.deinit(); + const text = try value.toScientificString(alloc, 3); + defer alloc.free(text); + try testing.expectEqualStrings("1e0", text); +} + +test "toScientificString: zero" { + // The file-level `alloc` is testing.allocator. + var value = try Rational.initInt(alloc, 0); + defer value.deinit(); + const text = try value.toScientificString(alloc, 17); + defer alloc.free(text); + try testing.expectEqualStrings("0", text); +} + +test "toScientificString: a single significant digit rounds correctly" { + // The file-level `alloc` is testing.allocator. + var value = try Rational.initRatio(alloc, 2, 3); // 0.666... + defer value.deinit(); + const text = try value.toScientificString(alloc, 1); + defer alloc.free(text); + try testing.expectEqualStrings("7e-1", text); +} + +test "toScientificString: agrees with f64 for values f64 can hold" { + // The file-level `alloc` is testing.allocator. + // Cross-check against the float formatter's own rendering of the same value, + // so the mantissa and exponent are not just self-consistent. + const values = [_]struct { num: i64, den: i64 }{ + .{ .num = 1, .den = 8 }, + .{ .num = 3, .den = 4 }, + .{ .num = 5, .den = 16 }, + .{ .num = 1, .den = 1024 }, + }; + for (values) |case| { + var value = try Rational.initRatio(alloc, case.num, case.den); + defer value.deinit(); + const text = try value.toScientificString(alloc, 17); + defer alloc.free(text); + + const as_float = value.toFloat(alloc); + const reparsed = try std.fmt.parseFloat(f64, text); + try testing.expectEqual(as_float, reparsed); + } +} + +fn bodyScientificRendering(a: Allocator) anyerror!void { + // Covers the allocation paths inside toScientificString and scaleFloor, + // including the negative-exponent case that scales the denominator. + var tiny = try Rational.initRatio(a, 1, 1180591620717411303424); + defer tiny.deinit(); + const small = try tiny.toScientificString(a, 17); + a.free(small); + + var large = try Rational.initInt(a, 123456789); + defer large.deinit(); + const big = try large.toScientificString(a, 5); + a.free(big); + + var negative = try Rational.initRatio(a, -2, 3); + defer negative.deinit(); + const signed = try negative.toScientificString(a, 3); + a.free(signed); +} + +test "OOM safety: scientific rendering" { + try oomSweep(bodyScientificRendering); +} diff --git a/engine/src/tokenizer.zig b/engine/src/tokenizer.zig index 4ee8b53..adc12ac 100644 --- a/engine/src/tokenizer.zig +++ b/engine/src/tokenizer.zig @@ -22,7 +22,7 @@ pub const TokenKind = enum { star, slash, percent, - caret, // ^ (power in standard, XOR in programmer) + caret, // ^ (always exponentiation, in both modes: see FR-2.12) star_star, // ** (power in programmer) ampersand, // & pipe, // | @@ -302,9 +302,7 @@ pub const Tokenizer = struct { // Digit separator self.pos += 1; } else if (ch == ',') { - // Comma as digit separator, but only if followed by a digit - // (to avoid eating commas in function args like max(1, 2)) - if (self.pos + 1 < self.source.len and predicate(self.source[self.pos + 1])) { + if (self.commaIsGrouping(predicate)) { self.pos += 1; } else { break; @@ -315,6 +313,35 @@ pub const Tokenizer = struct { } } + /// Decide whether the comma at `self.pos` groups digits or separates + /// arguments. + /// + /// It groups only when followed by EXACTLY three digits, which is what a + /// thousands group is. Checking merely for "a digit follows" is not enough: + /// it made `log(100,10)` lex as `log(10010)` and return 4.0004 instead of 2, + /// and `max(1,2)` lex as `max(12)`, which then failed as an unknown function. + /// Every such call worked only if the author happened to put a space after + /// the comma, which is why the tests and the help examples all passed. + /// + /// `max(1,234)` remains ambiguous by construction: three digits follow, so it + /// reads as `max(1234)`. FR-1.8 resolves that in favour of the grouping, and a + /// space is the way to ask for two arguments. + fn commaIsGrouping(self: *Tokenizer, predicate: *const fn (u8) bool) bool { + var digits: usize = 0; + var i = self.pos + 1; + while (i < self.source.len and predicate(self.source[i])) : (i += 1) { + digits += 1; + // More than a group: not grouping, whatever follows. + if (digits > 3) return false; + } + if (digits != 3) return false; + // A fourth digit cannot appear (the loop above would have counted it), so + // the group is well formed if what follows is not another digit. What may + // follow is another group, a decimal point, an exponent, an operator, or + // the end of input. + return true; + } + /// Like consumeDigits but also treats spaces as separators (only when the /// space is followed by a run of valid digits, so "0xFF + 1" stops at the /// space and "0xFF and 1" is not swallowed - "and" has a non-hex letter). @@ -332,12 +359,6 @@ pub const Tokenizer = struct { } else { break; } - } else if (ch == ',') { - if (self.pos + 1 < self.source.len and predicate(self.source[self.pos + 1])) { - self.pos += 1; - } else { - break; - } } else { break; } @@ -612,14 +633,28 @@ test "tokenize unrecognized character is invalid" { try testing.expectEqual(@as(usize, 1), t.len); } -test "tokenize base literal with comma separator" { +test "tokenize base literal does not take a comma as a separator" { + // Base literals group with spaces and underscores (FR-1.8); commas are the + // decimal grouping character. Accepting them here only reintroduced the + // argument-separator ambiguity in another place. var tok = Tokenizer.init("0xFF,FF", .programmer); const t = tok.next(); try testing.expectEqual(TokenKind.number, t.kind); - try testing.expectEqualStrings("0xFF,FF", t.text("0xFF,FF")); + try testing.expectEqualStrings("0xFF", t.text("0xFF,FF")); + try testing.expectEqual(TokenKind.comma, tok.next().kind); + // The trailing "FF" is a bare identifier now, not a continuation of the + // literal, which is exactly the point: it is not silently absorbed. + try testing.expectEqual(TokenKind.identifier, tok.next().kind); try testing.expectEqual(TokenKind.eof, tok.next().kind); } +test "tokenize base literal still groups with spaces and underscores" { + var tok = Tokenizer.init("0xFF FF", .programmer); + try testing.expectEqualStrings("0xFF FF", tok.next().text("0xFF FF")); + var underscored = Tokenizer.init("0xFF_FF", .programmer); + try testing.expectEqualStrings("0xFF_FF", underscored.next().text("0xFF_FF")); +} + test "parseNumber huge decimal exceeds u64 and falls back to float here" { // The tokenizer's own integer channel is a u64, so a value this large has no // `int_value` at this layer. That is NOT a precision limit of the engine: @@ -698,3 +733,61 @@ test "no implicit mul: spaces are just whitespace" { try testing.expectEqual(TokenKind.number, tok.next().kind); try testing.expectEqual(TokenKind.eof, tok.next().kind); } + +// -- The comma grouping rule -- +// +// A comma groups digits only when exactly three digits follow. The old rule was +// "a digit follows", which silently merged function arguments: `log(100,10)` +// became `log(10010)` and returned 4.0004 instead of 2. Every affected call +// worked if the author put a space after the comma, which is why the old tests +// and every documented example passed. + +test "comma groups digits only in threes" { + // Grouped: consumed as one number. + for ([_][]const u8{ "1,000", "1,234,567", "12,345", "123,456,789" }) |source| { + var tok = Tokenizer.init(source, .standard); + const t = tok.next(); + try testing.expectEqual(TokenKind.number, t.kind); + try testing.expectEqualStrings(source, t.text(source)); + try testing.expectEqual(TokenKind.eof, tok.next().kind); + } +} + +test "comma with the wrong number of digits is a separate token" { + // One, two or four digits are not a thousands group, so the comma stays a + // comma. This is what makes `log(100,10)` and `max(1,2)` parse as two + // arguments. + for ([_][]const u8{ "100,10", "1,2", "1,00", "1,0000" }) |source| { + var tok = Tokenizer.init(source, .standard); + const first = tok.next(); + try testing.expectEqual(TokenKind.number, first.kind); + try testing.expectEqual(TokenKind.comma, tok.next().kind); + try testing.expectEqual(TokenKind.number, tok.next().kind); + try testing.expectEqual(TokenKind.eof, tok.next().kind); + } +} + +test "comma at the end of input is a separate token" { + var tok = Tokenizer.init("1,", .standard); + try testing.expectEqual(TokenKind.number, tok.next().kind); + try testing.expectEqual(TokenKind.comma, tok.next().kind); + try testing.expectEqual(TokenKind.eof, tok.next().kind); +} + +test "comma followed by a non-digit is a separate token" { + var tok = Tokenizer.init("max(1, 2)", .standard); + try testing.expectEqual(TokenKind.identifier, tok.next().kind); + try testing.expectEqual(TokenKind.left_paren, tok.next().kind); + try testing.expectEqual(TokenKind.number, tok.next().kind); + try testing.expectEqual(TokenKind.comma, tok.next().kind); + try testing.expectEqual(TokenKind.number, tok.next().kind); + try testing.expectEqual(TokenKind.right_paren, tok.next().kind); +} + +test "a grouped literal is still exact and keeps its full text" { + // The exact tier re-parses the literal text, so the separators have to remain + // in the token for it to see them. + var tok = Tokenizer.init("9,007,199,254,740,993", .standard); + const t = tok.next(); + try testing.expectEqualStrings("9,007,199,254,740,993", t.text("9,007,199,254,740,993")); +} diff --git a/engine/src/units.zig b/engine/src/units.zig index b133d39..cfc2caf 100644 --- a/engine/src/units.zig +++ b/engine/src/units.zig @@ -118,8 +118,14 @@ pub const UnitDef = struct { }; /// A table entry. Factors and offsets are written as exact text; the f64 forms -/// are computed from them at comptime by `define`, so the two representations -/// cannot drift apart the way two hand-written fields would. +/// are computed from them at comptime by `define`, so there is one source of truth +/// per unit rather than two hand-written fields that can disagree. +/// +/// One caveat: for a `p/q` factor whose parts are not both exactly representable, +/// the comptime division rounds twice (once per operand, once for the quotient), so +/// the derived f64 can be 1 ulp away from the correctly rounded value of the true +/// ratio. `psi` is the one entry where this happens. The exact text remains the +/// source of truth and the exact conversion path is unaffected. const UnitSpec = struct { name: []const u8, aliases: []const []const u8 = &.{}, @@ -187,8 +193,10 @@ pub const ConvertResult = struct { // // Factors and offsets are written as EXACT text (a decimal, or `p/q` where the // value has no terminating decimal form). The f64 fields are derived from that -// text at comptime by `define`, so there is one source of truth per unit and the -// exact and approximate forms cannot drift apart. +// text at comptime by `define`, so there is one source of truth per unit. The +// derived f64 can sit 1 ulp from the correctly rounded ratio when neither part of +// a `p/q` factor is exactly representable (psi is the only such entry); the exact +// path does not go through it. // // Each table's base unit has factor "1" and no offset. @@ -404,9 +412,16 @@ fn matchesIgnoreCase(unit: UnitDef, name: []const u8) bool { /// Look up a unit by canonical name or alias. /// /// An exact (case-sensitive) match is tried first so that case-distinguished -/// units keep their meaning (e.g. "mB" does not silently become "MB", and "K" -/// stays Kelvin). Only if nothing matches exactly does a case-insensitive pass -/// run, which is what makes forgiving input like "KM" or "Celsius" work. +/// units keep their meaning: "K" stays Kelvin, "B" stays byte, and "kB" does not +/// become "KiB". Only if nothing matches exactly does a case-insensitive pass run, +/// which is what makes forgiving input like "KM" or "Celsius" work. +/// +/// That fallback is deliberately permissive, so a spelling with no exact entry +/// resolves to whatever differs only in case: "mB" and "Mb" both reach "MB". A +/// user who means megabits has to write "Mbit" or "Mb" ... which is the same +/// string, so they have to write "Mbit". Tightening this would mean rejecting +/// case-variant input outright, which is a bigger behavioural decision than a +/// comment can carry. pub fn findUnit(name: []const u8) ?UnitDef { if (name.len == 0) return null; for (categories) |table| { @@ -1377,9 +1392,12 @@ test "exact: non-terminating conversions are exact values with rounded display" } test "exact: every unit pair within a category round-trips EXACTLY" { - // The f64 version of this invariant could only assert a tolerance. With - // exact factors the round trip is bit-for-bit, which is a far stronger - // guarantee against a mistyped table entry. + // NOTE what this does and does not prove. `fromBase` is the algebraic inverse + // of `toBase`, so a round trip is exact even if the factor itself is mistyped; + // this cannot catch a wrong table entry. What it does catch is an asymmetric + // edit to the two directions, which matters for the affine (temperature) + // conversions where the inverse is written out by hand. The guarantee against + // wrong factors comes from the known-value tests, not from here. const alloc = testing.allocator; for (std.enums.values(UnitCategory)) |category| { for (unitsIn(category)) |a| { diff --git a/src/tui.zig b/src/tui.zig index 5470971..ec7d9c4 100644 --- a/src/tui.zig +++ b/src/tui.zig @@ -237,7 +237,11 @@ pub const App = struct { .conv_category = .length, .conv_from_idx = default_from, .conv_to_idx = default_to, - .conv_value = engine.Number.fromFloat(1), + // The value being converted starts at exactly one. It used to be + // Number.fromFloat(1), which made the opening screen read + // "1 m = 999,999,999.9999999 nm" while the CLI printed the exact + // 1,000,000,000 for the same conversion. + .conv_value = engine.Number.parse(allocator, "1") catch engine.Number.fromFloat(1), .conv_zone = .from, .fin = .{}, .last_height = 24, @@ -1885,7 +1889,7 @@ test "render: the tab bar shows every mode and marks the active one" { for ([_]Mode{ .standard, .programmer, .financial, .convert }) |mode| { app.setMode(mode); const rows = try renderApp(arena, &app, 100, 24); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "Tally")); try testing.expect(test_render.contains(rows, "Standard")); try testing.expect(test_render.contains(rows, "Programmer")); @@ -1914,7 +1918,7 @@ test "render: standard mode shows history, results and details" { try press(&app, &ctx, .{ .codepoint = vaxis.Key.enter }); const rows = try renderApp(arena, &app, 80, 24); - try testing.expect(test_render.wellFormed(rows, 80)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "2 + 3 * 4")); try testing.expect(test_render.contains(rows, "= 14")); try testing.expect(test_render.contains(rows, "= 256")); @@ -1960,7 +1964,7 @@ test "render: programmer mode draws the bit grid and all base rows" { app.prog_value = 0xDEADBEEF; const rows = try renderApp(arena, &app, 100, 30); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "Bits: 64")); try testing.expect(test_render.contains(rows, "Signed: yes")); try testing.expect(test_render.contains(rows, "DEC(s):")); @@ -1969,9 +1973,9 @@ test "render: programmer mode draws the bit grid and all base rows" { try testing.expect(test_render.contains(rows, "OCT:")); try testing.expect(test_render.contains(rows, "BIN:")); try testing.expect(test_render.contains(rows, "[float: Ctrl-F]")); - // 0xDEADBEEF in the decimal row and the hex row. - try testing.expect(test_render.contains(rows, "3,735,928,559") or - test_render.contains(rows, "3735928559")); + // 0xDEADBEEF in the decimal row and the hex row. Pinned exactly: an `or` of + // two spellings would not have caught a formatting regression in either. + try testing.expect(test_render.contains(rows, "3,735,928,559")); try testing.expect(test_render.contains(rows, "DE AD BE EF")); } @@ -2007,7 +2011,7 @@ test "render: programmer mode is well formed at every width and setting" { for ([_]engine.types.Signedness{ .signed, .unsigned }) |signedness| { app.prog_config.signedness = signedness; const rows = try renderApp(arena, &app, 100, 40); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); var buf: [24]u8 = undefined; const expected = try std.fmt.bufPrint(&buf, "Bits: {d}", .{width.bits()}); try testing.expect(test_render.contains(rows, expected)); @@ -2030,7 +2034,7 @@ test "render: the float view decodes a known bit pattern" { app.prog_value = @as(u32, @bitCast(@as(f32, 1.0))); const rows = try renderApp(arena, &app, 100, 30); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "sign")); try testing.expect(test_render.contains(rows, "exponent")); try testing.expect(test_render.contains(rows, "significand")); @@ -2042,7 +2046,7 @@ test "render: the float view decodes a known bit pattern" { try testing.expect(test_render.contains(rows, "normal")); } -test "render: the float view handles every classification" { +test "render: the float view decodes every classification by name" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); @@ -2052,25 +2056,33 @@ test "render: the float view handles every classification" { app.setMode(.programmer); app.float_view_active = true; - const cases = [_]struct { format: engine.FloatFormat, bits: u128 }{ - .{ .format = .f32, .bits = 0 }, // zero - .{ .format = .f32, .bits = 1 }, // denormal (smallest) - .{ .format = .f32, .bits = 0x7F800000 }, // infinity - .{ .format = .f32, .bits = 0x7FC00000 }, // NaN - .{ .format = .f64, .bits = @as(u64, @bitCast(@as(f64, -3.14))) }, - .{ .format = .f64, .bits = @as(u64, @bitCast(@as(f64, 0))) }, + // Asserting the classification text, not merely that the word "Class:" is on + // screen. The previous version of this test could not tell a NaN from a normal + // number. + const cases = [_]struct { format: engine.FloatFormat, bits: u128, class: []const u8 }{ + .{ .format = .f32, .bits = 0, .class = "zero" }, + .{ .format = .f32, .bits = 1, .class = "denormal" }, + .{ .format = .f32, .bits = 0x3F800000, .class = "normal" }, + .{ .format = .f32, .bits = 0x7F800000, .class = "infinity" }, + .{ .format = .f32, .bits = 0x7FC00000, .class = "NaN" }, + .{ .format = .f64, .bits = @as(u64, @bitCast(@as(f64, -3.14))), .class = "normal" }, + .{ .format = .f64, .bits = 0, .class = "zero" }, + .{ .format = .f64, .bits = 0x7FF0000000000000, .class = "infinity" }, }; for (cases) |case| { app.float_format = case.format; app.prog_config.bit_width = if (case.format == .f32) .bits32 else .bits64; app.prog_value = case.bits; const rows = try renderApp(arena, &app, 100, 34); - try testing.expect(test_render.wellFormed(rows, 100)); - try testing.expect(test_render.contains(rows, "Class:")); + try testing.expect(test_render.furniture(rows).intact()); + if (!test_render.contains(rows, case.class)) { + std.debug.print("float view did not report \"{s}\" for bits 0x{X}\n", .{ case.class, case.bits }); + return error.TestUnexpectedResult; + } } } -test "render: convert mode draws chips, both unit columns and a factor" { +test "render: convert mode draws chips, both unit columns and the exact result" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); @@ -2080,13 +2092,16 @@ test "render: convert mode draws chips, both unit columns and a factor" { app.setMode(.convert); const rows = try renderApp(arena, &app, 100, 34); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "Category:")); try testing.expect(test_render.contains(rows, "Length")); try testing.expect(test_render.contains(rows, "From")); try testing.expect(test_render.contains(rows, "To")); - // Default pair is the base unit against a distinct second unit. - try testing.expect(test_render.contains(rows, "1 m =")); + // The full result line, not just a prefix: asserting "1 m =" passed equally for + // the correct 1,000,000,000 and for the 999,999,999.9999999 the inexact default + // value used to produce. + try testing.expect(test_render.contains(rows, "1 m")); + try testing.expect(test_render.contains(rows, "= 1,000,000,000 nm")); try testing.expect(test_render.contains(rows, "Ctrl-S:swap")); } @@ -2119,12 +2134,12 @@ test "render: convert mode is well formed for every category" { var ctx = testCtxNoCmds(); try app.applyAction(&ctx, .{ .conv_category = category }); const rows = try renderApp(arena, &app, 100, 34); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, category.label())); } } -test "render: convert mode reports a unit list too long for the terminal" { +test "render: convert mode reports exactly how many units are hidden" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); @@ -2133,9 +2148,52 @@ test "render: convert mode reports a unit list too long for the terminal" { defer app.deinit(); app.setMode(.convert); - // Length has 14 units, which cannot fit in a 16-row terminal. - const rows = try renderApp(arena, &app, 100, 16); - try testing.expect(test_render.contains(rows, "more (resize to see all)")); + // Length has 14 units. At 100x19 only a few fit, and the notice must account + // for every one it does not show. The old code wrote the notice over the last + // unit it had just drawn and undercounted by one; the old test only checked + // that the words "more (resize to see all)" appeared somewhere. + const table_len = engine.units.unitsIn(.length).len; + const rows = try renderApp(arena, &app, 100, 19); + + const header_row = test_render.rowOf(rows, "From") orelse return error.NoHeader; + const notice_row = test_render.rowOf(rows, "more (resize to see all)") orelse + return error.NoOverflowNotice; + try testing.expect(notice_row > header_row); + + // Unit rows are the ones between the header and the notice. + const shown = notice_row - header_row - 1; + try testing.expect(shown > 0); + + var expected_buf: [48]u8 = undefined; + const expected = try std.fmt.bufPrint(&expected_buf, "... {d} more", .{table_len - shown}); + if (!test_render.contains(rows, expected)) { + std.debug.print("expected \"{s}\", got \"{s}\"\n", .{ expected, rows[notice_row] }); + return error.TestUnexpectedResult; + } + + // The row above the notice still holds a unit: the notice has its own row and + // does not erase the last one drawn. + const last_unit_row = rows[notice_row - 1]; + try testing.expect(std.mem.trim(u8, last_unit_row, " ").len > 0); +} + +test "render: a unit list that fits shows no notice and every unit" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var app = testApp(); + defer app.deinit(); + app.setMode(.convert); + + const rows = try renderApp(arena, &app, 100, 40); + try testing.expect(!test_render.contains(rows, "more (resize to see all)")); + for (engine.units.unitsIn(.length)) |unit| { + if (!test_render.contains(rows, unit.name)) { + std.debug.print("unit \"{s}\" was not drawn\n", .{unit.name}); + return error.TestUnexpectedResult; + } + } } test "render: the help overlay lists every mode's bindings across its pages" { @@ -2167,7 +2225,10 @@ test "render: the help overlay lists every mode's bindings across its pages" { var page: usize = 0; while (page < 12) : (page += 1) { const rows = try renderApp(arena, &app, 80, 24); - try testing.expect(test_render.wellFormed(rows, 80)); + // The help overlay has no prompt, so its structural invariant is its own: + // the title on row 1 and a footer on the last row. + try testing.expectEqual(@as(?usize, 1), test_render.rowOf(rows, "Tally - Help")); + try testing.expect(test_render.rowOf(rows, "scroll") == rows.len - 1); for (wanted, 0..) |needle, i| { if (test_render.contains(rows, needle)) found[i] = true; } @@ -2211,11 +2272,11 @@ test "render: the help overlay survives a terminal too short for any content" { app.show_help = true; const rows = try renderApp(arena, &app, 80, 4); - try testing.expect(test_render.wellFormed(rows, 80)); + try testing.expectEqual(@as(?usize, 1), test_render.rowOf(rows, "Tally - Help")); try testing.expect(test_render.contains(rows, "Tally - Help")); } -test "render: every mode survives a tiny terminal" { +test "render: every mode keeps its input line at every terminal size" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); @@ -2223,15 +2284,52 @@ test "render: every mode survives a tiny terminal" { var app = testApp(); defer app.deinit(); + // The point of this test is the furniture check: content that overflows its + // region lands on the separator or the prompt, which is exactly what happened + // in programmer mode at 100x16 and financial mode at 20x5. The old assertion + // here could not fail, so both went unnoticed. for ([_]Mode{ .standard, .programmer, .financial, .convert }) |mode| { app.setMode(mode); - for ([_][2]u16{ .{ 20, 6 }, .{ 40, 10 }, .{ 60, 15 }, .{ 200, 60 } }) |size| { + for ([_][2]u16{ .{ 20, 6 }, .{ 40, 10 }, .{ 60, 15 }, .{ 100, 16 }, .{ 200, 60 } }) |size| { const rows = try renderApp(arena, &app, size[0], size[1]); - try testing.expect(test_render.wellFormed(rows, size[0])); + const bottom = test_render.furniture(rows); + if (!bottom.intact()) { + std.debug.print( + "{s} at {d}x{d}: separator={} prompt={} status={}\n", + .{ @tagName(mode), size[0], size[1], bottom.separator, bottom.prompt, bottom.status }, + ); + return error.TestUnexpectedResult; + } } } } +test "render: 128-bit programmer mode keeps its input line on a short terminal" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var app = testApp(); + defer app.deinit(); + app.setMode(.programmer); + app.prog_config.bit_width = .bits128; + + // Four grid rows plus six base rows do not fit in 16 rows. The view used to + // draw them anyway, putting the BIN row on the prompt. + const rows = try renderApp(arena, &app, 100, 16); + try testing.expect(test_render.furniture(rows).intact()); + try testing.expect(test_render.contains(rows, "terminal too short")); + // And it says what it needs, rather than showing a half-drawn layout. + try testing.expect(test_render.contains(rows, "128-bit")); + try testing.expect(!test_render.contains(rows, "BIN:")); + + // With room, the full view is back. + const tall = try renderApp(arena, &app, 100, 30); + try testing.expect(test_render.furniture(tall).intact()); + try testing.expect(test_render.contains(tall, "BIN:")); + try testing.expect(!test_render.contains(tall, "terminal too short")); +} + /// An EventContext for calls that cannot issue a command, so nothing needs to be /// freed afterwards. fn testCtxNoCmds() vxfw.EventContext { @@ -2900,7 +2998,12 @@ test "render: programmer mode draws the cursor in whichever field is focused" { app.prog_config.bit_width = width; app.bit_cursor = @intCast(@min(5, width.bits() - 1)); const rows = try renderApp(arena, &app, 110, 40); - try testing.expect(test_render.wellFormed(rows, 110)); + try testing.expect(test_render.furniture(rows).intact()); + // The bit grid has one row per 32 bits, and the base rows follow it, so + // this pins the layout rather than just the presence of a string. + const grid_rows = (@as(usize, width.bits()) + 31) / 32; + try testing.expectEqual(@as(?usize, 4 + grid_rows + 1), test_render.rowOf(rows, "DEC(s):")); + try testing.expectEqual(@as(?usize, 4 + grid_rows + 6), test_render.rowOf(rows, "BIN:")); // The config line is always present, whichever field has focus. try testing.expect(test_render.contains(rows, "Bits:")); } @@ -2920,7 +3023,7 @@ test "render: convert mode draws the focused column and category" { for ([_]ConvZone{ .category, .from, .to }) |zone| { app.conv_zone = zone; const rows = try renderApp(arena, &app, 100, 34); - try testing.expect(test_render.wellFormed(rows, 100)); + try testing.expect(test_render.furniture(rows).intact()); try testing.expect(test_render.contains(rows, "Category:")); } } diff --git a/src/tui/convert.zig b/src/tui/convert.zig index a4f1ddf..0e42524 100644 --- a/src/tui/convert.zig +++ b/src/tui/convert.zig @@ -36,6 +36,9 @@ pub fn drawConvertMode(app: *tui.App, surface: *vxfw.Surface, width: u16, height row += 1; col = 4; } + // And stop before the furniture rows, so a narrow terminal does not draw + // chips over the separator and the prompt. + if (row >= height -| 3) break; const selected = category == app.conv_category; const focused = selected and zone_active and app.conv_zone == .category; const style: vaxis.Style = if (focused) @@ -106,8 +109,14 @@ pub fn drawConvertMode(app: *tui.App, surface: *vxfw.Surface, width: u16, height row += 1; const list_start = row; + // One row is reserved for the overflow notice, so a hidden unit gets reported + // rather than the notice being written over the last unit drawn. It used to + // land on `list_end - 1`, erasing the unit already there and undercounting the + // remainder by one. const list_end = height -| 4; - const visible: usize = if (list_end > list_start) list_end - list_start else 0; + const room: usize = if (list_end > list_start) list_end - list_start else 0; + const overflows = table.len > room; + const visible: usize = if (overflows) (if (room > 0) room - 1 else 0) else room; for (table, 0..) |unit, i| { if (i >= visible) break; @@ -117,12 +126,16 @@ pub fn drawConvertMode(app: *tui.App, surface: *vxfw.Surface, width: u16, height drawUnitCell(app, surface, list_row, to_col, unit, i == app.conv_to_idx, to_focused, .{ .conv_to = i }); } - // If the terminal is too short for the whole list, say so rather than - // silently truncating. - if (table.len > visible and visible > 0) { + // If the terminal is too short for the whole list, say so rather than silently + // truncating. Drawn even when nothing fits at all, which is the case that used + // to show two empty columns and no explanation. + if (overflows) { var more_buf: [64]u8 = undefined; const more = std.fmt.bufPrint(&more_buf, "... {d} more (resize to see all)", .{table.len - visible}) catch "..."; - draw.writeStr(surface, list_end -| 1, from_col, more, .{ .fg = C.dim }); + const notice_row = list_start + @as(u16, @intCast(visible)); + if (notice_row < list_end) { + draw.writeStr(surface, notice_row, from_col, more, .{ .fg = C.dim }); + } } // -- Separator, input, status -- diff --git a/src/tui/financial.zig b/src/tui/financial.zig index 6ec0241..8fd17e5 100644 --- a/src/tui/financial.zig +++ b/src/tui/financial.zig @@ -519,6 +519,7 @@ pub fn drawFinancialMode(app: *tui.App, surface: *vxfw.Surface, width: u16, heig draw.writeStr(surface, 2, 2, "Calculation:", .{ .fg = C.muted }); var row: u16 = 3; var col: u16 = label_col; + const content_end = height -| 3; for (std.enums.values(Form)) |form| { const label = form.label(); const chip_len: u16 = @intCast(label.len + 2); @@ -526,6 +527,10 @@ pub fn drawFinancialMode(app: *tui.App, surface: *vxfw.Surface, width: u16, heig row += 1; col = label_col; } + // Wrapping can push the chips into the separator and the input line on a + // short terminal, which is what happened at 20x5: a chip was drawn on the + // prompt row. + if (row >= content_end) break; const selected = form == state.form; const style: vaxis.Style = if (selected) .{ .fg = C.bg, .bg = C.yellow, .bold = true } @@ -1527,7 +1532,7 @@ test "render: the status line reflects which zone has focus" { try testing.expect(frameContains(form_zone, "Up/Dn:field")); } -test "render: a short terminal drops rows instead of writing out of bounds" { +test "render: a short terminal keeps the input line and drops content" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); @@ -1535,9 +1540,15 @@ test "render: a short terminal drops rows instead of writing out of bounds" { // Ten rows cannot hold the chips, six TVM fields, a result and an input line. const state = stateWith(.tvm, &.{ "360", "0.5", "200000", "", "0" }); const rows = try renderFrame(testing.allocator, arena, state, 80, 10, false); - try testing.expectEqual(@as(usize, 10), rows.len); - // The input line is anchored to the bottom, so it survives the squeeze. - try testing.expect(frameContains(rows, ">")); + // Checking the furniture rather than "a > appears somewhere": at 20x5 a chip + // was being drawn onto the prompt row, and a bare `contains(">")` passed. + try testing.expect(test_render.furniture(rows).intact()); + // The fields that did fit are real ones, not half-drawn rows over the prompt. + try testing.expect(frameContains(rows, "N (periods)")); + + // The narrowest, shortest case the reviewers found broken. + const tiny = try renderFrame(testing.allocator, arena, state, 20, 5, true); + try testing.expect(test_render.furniture(tiny).intact()); } test "render: a narrow terminal wraps the calculation chips" { @@ -1551,15 +1562,25 @@ test "render: a narrow terminal wraps the calculation chips" { try testing.expect(frameContains(rows, "Amortization")); } -test "render: every form draws without panicking, empty or filled" { +test "render: every form draws its own fields, empty or filled" { var arena_state = std.heap.ArenaAllocator.init(testing.allocator); defer arena_state.deinit(); const arena = arena_state.allocator(); + // This used to assert nothing at all (`_ = try renderFrame(...)`), which made + // it a smoke test wearing a render test's name. Now every field label of every + // form has to appear, and the input line has to survive. for (std.enums.values(Form)) |form| { var empty: State = .{}; empty.setForm(form); - _ = try renderFrame(testing.allocator, arena, empty, 80, 24, true); + const empty_rows = try renderFrame(testing.allocator, arena, empty, 80, 24, true); + try testing.expect(test_render.furniture(empty_rows).intact()); + for (form.fields()) |spec| { + if (!frameContains(empty_rows, spec.label)) { + std.debug.print("{s}: field \"{s}\" was not drawn\n", .{ form.label(), spec.label }); + return error.TestUnexpectedResult; + } + } // Every field filled with the same plausible number, which is nonsense // for some forms: the point is that no combination crashes or writes @@ -1567,7 +1588,10 @@ test "render: every form draws without panicking, empty or filled" { var filled: State = .{}; filled.setForm(form); for (0..filled.fieldCount()) |i| filled.fieldAt(i).set("12"); - _ = try renderFrame(testing.allocator, arena, filled, 80, 24, true); + const filled_rows = try renderFrame(testing.allocator, arena, filled, 80, 24, true); + try testing.expect(test_render.furniture(filled_rows).intact()); + // Whatever the form computes, the entered value is on screen. + try testing.expect(frameContains(filled_rows, "12")); } } diff --git a/src/tui/programmer.zig b/src/tui/programmer.zig index fda0716..cf5f6a7 100644 --- a/src/tui/programmer.zig +++ b/src/tui/programmer.zig @@ -23,11 +23,14 @@ pub fn drawProgrammerMode(app: *tui.App, surface: *vxfw.Surface, width: u16, hei draw.writeStr(surface, 2, 2, config_str, .{ .fg = C.muted }); registerConfigRegions(app, 2, 2, config_str); - // "float view" affordance on the same line, at the right. + // "float view" affordance on the same line, at the right. Only when there is + // room for it: at narrow widths it collided with the config text. const float_hint = "[float: Ctrl-F]"; - const float_col = width -| @as(u16, @intCast(float_hint.len + 2)); - draw.writeStr(surface, 2, float_col, float_hint, .{ .fg = C.dim }); - app.addRegion(2, float_col, @intCast(float_hint.len), .toggle_float); + if (width > config_str.len + float_hint.len + 6) { + const float_col = width -| @as(u16, @intCast(float_hint.len + 2)); + draw.writeStr(surface, 2, float_col, float_hint, .{ .fg = C.dim }); + app.addRegion(2, float_col, @intCast(float_hint.len), .toggle_float); + } // Truncation warning: the stored value has bits beyond the current width, // so the display below shows only the low bits. Widening restores them. @@ -39,11 +42,29 @@ pub fn drawProgrammerMode(app: *tui.App, surface: *vxfw.Surface, width: u16, hei // Bit grid const grid_start: u16 = 4; - drawBitGrid(app, surface, grid_start, val, bw); - - // Multi-base display below bit grid const grid_rows: u16 = (@as(u16, bw.bits()) + 31) / 32; const base_start: u16 = grid_start + grid_rows + 1; + + // The value view needs the grid plus six base rows. Without this check the + // rows were drawn unconditionally and landed on the separator and the input + // line: at 100x16 in 128-bit mode the BIN row overwrote the prompt, and at + // 20x6 every base row was clipped away with no indication. Saying so is better + // than drawing a layout that lies. + const content_end = height -| 3; + if (base_start + 6 > content_end) { + var need_buf: [96]u8 = undefined; + const need = std.fmt.bufPrint( + &need_buf, + "terminal too short for the {d}-bit view: needs {d} rows, has {d}", + .{ bw.bits(), base_start + 6 + 3, height }, + ) catch "terminal too short for the bit view"; + draw.writeStr(surface, 4, 2, need, .{ .fg = C.pink }); + drawFurniture(app, surface, width, height, focused); + return; + } + + drawBitGrid(app, surface, grid_start, val, bw); + const int = engine.types.Integer{ .raw = val, .bit_width = bw, .signedness = .signed }; // DEC(s) @@ -133,6 +154,19 @@ pub fn drawProgrammerMode(app: *tui.App, surface: *vxfw.Surface, width: u16, hei } // Separator + input + status + drawFurniture(app, surface, width, height, focused); +} + +/// The three rows every input mode reserves at the bottom: separator, prompt, +/// status. Factored out so the short-terminal path above draws them too, rather +/// than returning early and leaving no prompt at all. +fn drawFurniture( + app: *tui.App, + surface: *vxfw.Surface, + width: u16, + height: u16, + focused: tui.App.ProgField, +) void { draw.fillRow(surface, height -| 3, '-', .{ .fg = C.dim }); app.drawInput(surface, height -| 2); app.addRegion(height -| 2, 0, width, .focus_input); diff --git a/src/tui/test_render.zig b/src/tui/test_render.zig index 4aa00a7..069b363 100644 --- a/src/tui/test_render.zig +++ b/src/tui/test_render.zig @@ -1,12 +1,31 @@ //! Test-only frame rendering for the TUI. //! //! Draws a real frame into a real surface and reads the cells back as text, which -//! is the only way to test a terminal view without a terminal. It catches the -//! failures that matter in drawing code: a layout that writes nothing, one that -//! overlaps itself, and one that runs off the bottom or the right edge. +//! is the only way to test a terminal view without a terminal. //! -//! Nothing here is used outside tests. It lives in its own file so both the view -//! modules and tui.zig can share one harness rather than each growing a copy. +//! ## What can and cannot be asserted here +//! +//! This file used to offer `wellFormed(rows, width)`, which checked that every row +//! was exactly `width` bytes long. `flatten` allocates each row as exactly `width` +//! bytes, so that check could never fail: it was a tautology dressed up as a +//! structural guarantee, and the design document claimed it caught miscomputed +//! columns. It caught nothing. Every layout bug the reviewers found (content drawn +//! over the input line at small heights, a notice that erased the row above it) was +//! green under it. +//! +//! What is actually detectable, and what the helpers below provide: +//! +//! - **Furniture position.** Every mode draws a separator on `height-3`, the prompt +//! on `height-2`, and a status line on `height-1`. Content that overflows its +//! region lands on one of those rows, so checking them detects the overflow. +//! - **Right-edge clipping.** `draw.writeStr` clips instead of erroring, so text +//! that reaches the final column has almost certainly lost its tail. +//! - **Cell content**, via `contains` and `rowOf`. +//! +//! Style is NOT observable: `flatten` keeps graphemes and discards attributes, so +//! focus highlighting, the selection fill and the block cursor cannot be asserted +//! from here. Tests that care about those assert on a neighbouring glyph instead +//! and say so. const std = @import("std"); const vaxis = @import("vaxis"); @@ -65,12 +84,197 @@ pub fn rowOf(rows: [][]u8, needle: []const u8) ?usize { return null; } -/// True when every row is exactly `width` wide and nothing was written past it. -/// A view that miscomputes a column would otherwise fail silently, since the -/// drawing helpers clip rather than error. -pub fn wellFormed(rows: [][]u8, width: u16) bool { - for (rows) |row| { - if (row.len != width) return false; +/// The state of the three rows every input mode reserves at the bottom. +/// +/// A view whose content is taller than the space it was given writes over these, +/// which is precisely the failure that needs detecting: at 100x16 the programmer +/// view's binary row landed on the prompt, and at 20x5 the financial chips did. +pub const Furniture = struct { + /// `height-3` is filled with '-' across the full width. + separator: bool, + /// `height-2` carries the prompt marker at column 1. + prompt: bool, + /// `height-1` has key hints on it. + status: bool, + + pub fn intact(self: Furniture) bool { + return self.separator and self.prompt and self.status; } - return true; +}; + +/// Inspect the bottom three rows. Only meaningful for the modes that have an input +/// line (standard, programmer, convert, financial), not for the help overlay. +pub fn furniture(rows: [][]u8) Furniture { + if (rows.len < 3) return .{ .separator = false, .prompt = false, .status = false }; + const separator_row = rows[rows.len - 3]; + const prompt_row = rows[rows.len - 2]; + const status_row = rows[rows.len - 1]; + + var separator = separator_row.len > 0; + for (separator_row) |ch| { + if (ch != '-') { + separator = false; + break; + } + } + + const prompt = prompt_row.len > 2 and prompt_row[1] == '>' and prompt_row[0] == ' '; + + var status = false; + for (status_row) |ch| { + if (ch != ' ') { + status = true; + break; + } + } + + return .{ .separator = separator, .prompt = prompt, .status = status }; +} + +/// Rows whose final column holds something other than a space, i.e. rows whose +/// text ran to the edge and was probably clipped. +/// +/// The separator rows legitimately fill the width, so callers pass how many of +/// those to expect rather than getting a bare boolean. +pub fn rowsReachingRightEdge(rows: [][]u8) usize { + var count: usize = 0; + for (rows) |row| { + if (row.len == 0) continue; + if (row[row.len - 1] != ' ') count += 1; + } + return count; +} + +/// True when no row other than the separator reaches the right edge. +pub fn noTextClipped(rows: [][]u8) bool { + // One row may reach the edge: the separator, which is drawn full width on + // purpose. Anything more means a line of text hit the boundary. + return rowsReachingRightEdge(rows) <= 1; +} + +// -- Tests for the harness itself -- +// +// The assertion this file replaced could not fail, so these prove the replacement +// can. Rows are built by hand rather than drawn, so each check is exercised in +// both directions. + +const testing = std.testing; + +fn syntheticFrame(arena: std.mem.Allocator, rows_text: []const []const u8, width: usize) ![][]u8 { + const rows = try arena.alloc([]u8, rows_text.len); + for (rows_text, 0..) |text, i| { + const line = try arena.alloc(u8, width); + @memset(line, ' '); + const n = @min(text.len, width); + @memcpy(line[0..n], text[0..n]); + rows[i] = line; + } + return rows; +} + +test "furniture: intact when the bottom three rows are where they belong" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + const rows = try syntheticFrame(arena_state.allocator(), &.{ + " some content", + "----------", + " > 2 + 2", + " ?:help | Tab:mode", + }, 10); + const bottom = furniture(rows); + try testing.expect(bottom.separator); + try testing.expect(bottom.prompt); + try testing.expect(bottom.status); + try testing.expect(bottom.intact()); +} + +test "furniture: a separator with content drawn over it is detected" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + // This is the programmer-mode failure: a base row landed on the separator. + const rows = try syntheticFrame(arena_state.allocator(), &.{ + " content", + "--BIN:----", + " > ", + " status", + }, 10); + const bottom = furniture(rows); + try testing.expect(!bottom.separator); + try testing.expect(!bottom.intact()); +} + +test "furniture: a prompt overwritten by content is detected" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + // And this is the financial failure: a chip drawn on the prompt row. + const rows = try syntheticFrame(arena_state.allocator(), &.{ + " content", + "----------", + " CAGR", + " status", + }, 10); + const bottom = furniture(rows); + try testing.expect(bottom.separator); + try testing.expect(!bottom.prompt); + try testing.expect(!bottom.intact()); +} + +test "furniture: a missing status line is detected" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + const rows = try syntheticFrame(arena_state.allocator(), &.{ + "----------", + " > ", + "", + }, 10); + const bottom = furniture(rows); + try testing.expect(!bottom.status); + try testing.expect(!bottom.intact()); +} + +test "furniture: a frame too short to have furniture reports nothing intact" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + const rows = try syntheticFrame(arena_state.allocator(), &.{ "a", "b" }, 4); + try testing.expect(!furniture(rows).intact()); +} + +test "rowOf and contains distinguish present from absent" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + const rows = try syntheticFrame(arena_state.allocator(), &.{ + "first", + "second", + "third", + }, 10); + try testing.expectEqual(@as(?usize, 1), rowOf(rows, "second")); + try testing.expectEqual(@as(?usize, null), rowOf(rows, "fourth")); + try testing.expect(contains(rows, "third")); + try testing.expect(!contains(rows, "fourth")); +} + +test "rowsReachingRightEdge counts only rows whose text hits the boundary" { + var arena_state = std.heap.ArenaAllocator.init(testing.allocator); + defer arena_state.deinit(); + + const rows = try syntheticFrame(arena_state.allocator(), &.{ + "short", + "0123456789", + "----------", + }, 10); + try testing.expectEqual(@as(usize, 2), rowsReachingRightEdge(rows)); + // Two rows at the edge is one more than the separator, so text was clipped. + try testing.expect(!noTextClipped(rows)); + + const clean = try syntheticFrame(arena_state.allocator(), &.{ + "short", + "----------", + }, 10); + try testing.expect(noTextClipped(clean)); }