From ef1713e6f6fe0a13304c2248bfe6e23e7988185e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 12:22:12 -0700 Subject: [PATCH] docs(core): the recorded reason for leaving record_cell_key's Category arm untested was wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-round review finding, and I verified it against the code before accepting it. Commit 7d25762's message declined that test on the grounds that a formula targeting a regular category should project Derived anyway, so the injected behaviour "is not clearly a fault". That is a different key entirely. RecordCol::Category(name) carries a CATEGORY name, and RecordRow::cell_key(arg) puts its argument under _Measure (layout.rs:68-70). So answering Some there yields `coords + _Measure=`, and formula_for matches it whenever a _Measure item happens to share a name with a regular category — projecting Derived for a COORDINATE column, which is never a formula target and which the pivot path does not have at all. The two front ends would disagree, which is the §5 defect this ticket exists to close. RecordCol's own doc names the collision: the enum resolves a column from its index "so a category and a measure sharing a name stay distinguishable". The decision not to write the test is unchanged, and the reviewer's recommendation too. What changes is the argument: the REACHABLE version of that fault is a new RecordCol variant answered carelessly, and the exhaustive match makes that unrepresentable rather than unlikely. A test would pin what the type system already pins. Recorded in context/code/core.md because a wrong argument in context/ outlives the code it describes, and that one would have told a future reader a coordinate column projecting Derived is defensible. The commit message cannot be corrected without rewriting history while a review is in flight, so the durable record says so explicitly. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/core.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/context/code/core.md b/context/code/core.md index 32c2917a..8a8aa4c3 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -434,6 +434,36 @@ Registration is guaranteed for every model category: `Workbook::new`, `cycle_axis` does, and no-ops on an unknown category to match `set_axis` (improvise-t8s). +### `record_cell_key` is a match, and the match is the guard (improvise-ir7l) + +`GridLayout::record_cell_key(row, col)` is the one mapping from a records +position to a model cell, and it answers `Some` for `RecordCol::Measure` alone. +The other three arms are spelled out rather than swept into a `_`, and that +exhaustive match is the whole guard: a new `RecordCol` variant fails the build +until someone decides whether it names a stored cell. + +**There is no test for the `Category` arm, and the reason is not that the wrong +answer would be harmless.** It would be a real fault, and a subtle one worth +naming so nobody re-derives it wrongly. `RecordCol::Category(name)` carries a +**category** name, while `RecordRow::cell_key(arg)` puts its argument under +`_Measure`. So answering `Some` there yields `coords + _Measure=`, and `Model::formula_for` would match it whenever a `_Measure` item +happens to share a name with a regular category — projecting `CellId::Derived` +for a **coordinate column**, which is never a formula target and which the +pivot path does not have at all. The two front ends would then disagree, which +is the design-principles §5 defect this ticket exists to close. `RecordCol` +exists for exactly that collision: its doc says the enum resolves a column from +its index "so a category and a measure sharing a name stay distinguishable". + +What earns the decision to leave it untested is narrower: the **reachable** +version of that fault is a new variant answered carelessly, and the exhaustive +match makes that unrepresentable rather than merely unlikely +(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`). A test would +pin behaviour the type system already pins, and would not survive the arm being +renamed. Found in review, and the first recorded reason for declining it was +**wrong** — it claimed the injected behaviour was defensible by analogy with a +formula targeting a regular category, which is a different key entirely. + ## `PageAxis` — what constrains, and what the user may turn (improvise-p1ym) `GridLayout::page_coords` answers two questions that gotcha 27 says must not be