diff --git a/context/adr/002-naming-a-cell.md b/context/adr/002-naming-a-cell.md index f777136c..80626411 100644 --- a/context/adr/002-naming-a-cell.md +++ b/context/adr/002-naming-a-cell.md @@ -226,3 +226,9 @@ learns no coordinate space, and an invalid operation is unconstructible rather than rejected. What the extension adds is that the enum is now **four** answers to "what may be done here", so `None` finally means only what this ADR always said it meant — no projected cell at all. + +**A fifth kind (2026-09-22, improvise-exdg).** The records-mode coordinate cell +was the last `None` that named a visible cell. It is now `CellId::Coordinate`. +The variant carries no operand, because no client composes a coordinate rename. +The previous paragraph's "four answers" is therefore five. `None` now means +only a padded or blanked slot. Reasoning: `context/code/protocol.md` §3. diff --git a/context/code/cli.md b/context/code/cli.md index 372597e6..330b95c8 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -161,7 +161,7 @@ limitation that is not new: `ExecuteCommand` returns one effect batch, so the segments of `:a . b` all see the pre-batch cursor, exactly as a `Binding::Sequence` does (gotchas 21 and 23). -## 8 — the interactive arm is `parse`, then context; it is never a second parser +## 8 — the interactive arm is `parse`, then the cursor; it is never a second parser #151 fixed three instances of *a registry with two construction arms demotes whichever caller reaches the weaker one*, one patch each. A post-merge review @@ -175,31 +175,59 @@ and the key from the cursor. So a script that wrote 5 to `Region/East` The shape rather than the instances is what changed. `CmdRegistry::register` used to take two free functions, `parse(args)` and `interactive(args, ctx)`, and nothing related them; each command re-derived its arguments twice and every -disagreement was silent. It now takes three pieces, and the interactive arm is -**built from `parse`**, not beside it: +disagreement was silent. The interactive arm is now **built from `parse`**, not +beside it: - `parse(args) -> C` — the only function that reads arguments a caller supplied. -- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and - never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is - handed the built command and **not the args**, so it cannot discard them. - `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused. This is where "no coordinates means the cursor" lives: `clear-cell` bare, and `commit-cell-edit ` bare, which is what the keymap binds. +- For `register_at_cursor` only, the registry itself then sets the command's + **cursor** — its `CursorState`, reached through `AtCursor::cursor_mut` — to the + live one. That is the whole of what the interactive arm adds to a parsed + command, and no registration supplies it. -Two consequences hold by construction rather than by test. **Whatever `parse` -accepts, the interactive arm accepts** — `contextualize` is infallible — so the -§7 gate cannot pass a segment the loop then refuses. And **an accepted argument -list cannot be overridden by the cursor**, because the only function that sees -both args and context never runs on an accepted list. `register_pure` and -`register_nullary` already had one arm; they are the `contextualize = identity, -fallback = None` case. +Three consequences hold by construction rather than by test. **Whatever `parse` +accepts, the interactive arm accepts** — nothing after `parse` can fail — so the +§7 gate cannot pass a segment the loop then refuses. **An accepted argument list +cannot be overridden by the cursor**, because the only function that sees both +args and context never runs on an accepted list. And **no field `parse` set can +be overwritten from context**, because nothing a registration writes can name +one: the cursor slot is typed `&mut CursorState`, and no command parses a +`CursorState` out of its arguments. `register_pure` and `register_nullary` +already had one arm; they are `register` with no fallback. + +That last one is improvise-yf7p, and it is why the registry lost a parameter. +#171 took a third function, `contextualize(C, &CmdContext) -> C`, handed the +built command and not the args, and stated honestly that it could still +**overwrite** a parsed field — naming the field was "a visible line in review". +Review then injected five such overwrites on commands other than the one with a +test (yank, paste and drill-into-cell taking their key from the cursor over the +parsed one; toggle-panel-visibility ignoring its parsed panel; move-panel-cursor +clamping its parsed delta to ±1) and the whole workspace stayed green. A hole +guarded by review discipline plus one command's test is not guarded. Every +contextualizer in the tree fell into one of two shapes, and each had an answer +that removes the function rather than checking it: + +- **The cursor** (`Move`, `EnterAdvance`, `CommitAndAdvance`). Always one + `CursorState` field, always `CursorState::from_ctx`. So the registry does it, + through a slot whose *type* is the guard: an `AtCursor` impl pointing at a + `CellKey` or a delta is a compile error, which was tried. +- **A read of context that belonged in `execute`** (`toggle-panel-visibility`'s + `currently_open`, `cycle-panel-focus`'s three flags, `move-panel-cursor`'s + cursor and bound). These copied context into the command only to read it back; + `execute` now reads `ctx` directly, the same answer `toggle-panel-and-focus` + got below. Those three are `register`/`register_nullary` now, so their + interactive arm is exactly `parse`. + +What `execute` does with a parsed field is still the command's own logic and is +tested as such — `toggle_panel_visibility_reads_the_panel_it_names` and +`move_panel_cursor_moves_from_the_named_panels_own_cursor` fail if the panel or +the delta is ignored there. The difference is that the registry is no longer a +second place such a bug can be written. What is still representable, stated rather than hidden: -- `contextualize` can overwrite a field `parse` set. It would have to name the - field, which is a visible line in review, where the old `|_args, ctx|` was an - invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor` - fails if `commit-cell-edit`'s does. - A `fallback` sees the args `parse` refused and could ignore some of them. `commit-cell-edit`'s refuses anything but a lone mode, so the malformed `commit-cell-edit editing 5` is an error on the `:` line — which has no pure @@ -207,9 +235,12 @@ What is still representable, stated rather than hidden: - The gate is **stricter** than the loop wherever a fallback exists: a bare `clear-cell` is refused in a script. That is the safe direction — the whole line is refused — and it is unchanged from before #151. +- A new command could declare a *second* `CursorState` field and parse it from + args; `cursor_mut` could then point at the wrong one. No command has one, and + a parsed cursor would be a new idea deserving its own argument. -`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm -defaulted the omitted flags to `true` and a contextualizer cannot tell an +`toggle-panel-and-focus` did not fit #171's `contextualize` either, because its +pure arm defaulted the omitted flags to `true` and a contextualizer cannot tell an omitted flag from an explicit one. So the omission is now in the type — `target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is resolved in `execute` against the same context. That is the honest home: the diff --git a/context/code/clients.md b/context/code/clients.md index ff0eb935..4934919a 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1706,6 +1706,31 @@ watchdog must be DISARMED and not merely pre-empted**: it fires later and would overwrite the better message with "did not answer" (PR #13 review). The watchdog is for silence only, and `setShellFailure` clears the timer. +**Success is claimed by the first frame, and only if nothing failed first** +(improvise-i0v9). The worker transport used to write `in-page worker · ` +the moment it spawned the worker — 13ms after navigation, before anything had +answered — so a worker that could not start showed a success-looking status +over a blank page. It now writes `starting the worker · …`, and +`createShell().arm(message, { answer })` holds the claim until `sawFrame`. The +rule the claim obeys is **the watchdog's own state**: it is written only if the +watchdog is still armed when the first frame lands. That is not a convenience. +`worker.js` posts a `ReadOnly` (or `SeedMissing`) notice straight after +`start()` and **before** the Snapshot that answers the handshake, so an +unconditional "first frame ⇒ say we are up" would overwrite the precise reason +the notice just wrote. `fail` already disarms the watchdog, so "armed" answers +"has anything been said since arming" without a second flag to keep in step. +`fail` disarms **directly** rather than through `sawFrame`, or it would +briefly flash the success text on its way to the failure. The starting text +deliberately does not contain `in-page worker`, so the older specs asserting +that substring now need the first frame to pass. The socket path passes no +`answer`: `open` is already a real fact, and its status was never premature. + +One consequence changes what the watchdog guards. In the read-only case a +Snapshot always follows the notice, and `sawFrame` disarms the watchdog either +way, so a notice that forgot to disarm would **not** be overwritten eight +seconds later. It is overwritten **at once**, by the claim. The eight-second +overwrite survives only where no frame follows: `BootFailed`. + **Did this socket ever open?** The page has to record that itself, because nothing on the close event carries it: `readyState` is already CLOSED by the time a close listener runs, whichever happened, and a refused connection and a diff --git a/context/code/command.md b/context/code/command.md index 3c7a0412..3dd16fd3 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -553,6 +553,78 @@ status line. one, and the limit is checked in exactly one place — `Model::add_category` — so nothing in the effect restates it. +### A model effect that cannot do what it names refuses: `App::refuse`, `on_view` and `refuse_and_stop` (improvise-bok4, cz8d) + +`AddCategory` was the only wire-reachable model effect that aborted when it +failed. The others did one of two things: + +- **Narrated.** `AddItem`, `AddItemInGroup`, `AddFormula`, `LoadModel` and + `ImportJsonHeadless` wrote a sentence to the *host's* `status_msg` and + carried on. +- **Swallowed.** Every view-addressed effect (`SetAxis`, `SetPageSelection`, + `ToggleGroup`, `HideItem`, `ShowItem`, `TransposeAxes`, `CycleAxis`, + `SetNumberFormat`, `TogglePruneEmpty`) did nothing when `view_mut_of` found + no such view. `DeleteView` discarded "cannot delete the last view" with + `let _`, and answered `Ok` for a view the document did not have + (improvise-40j4). `AddFormula` stored a formula for a category that was gone, + through `let _ = add_item(…)` (improvise-40j4). + +Either way the batch ran to its end, and `Host::mutating_frame` reported the +frame **accepted**. That is clause 4's partial-and-silent shape, one layer below +the record tier that `broken_chain` guards. The reviewer measured two cases. +`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]` was accepted with +half of it a no-op. `[RemoveCategory("Region"), AddItem { category: "Region" }]` +was accepted too, and its reason went only to the host's own status line, so +the client was told nothing at all. + +**One writer, and the abort is separate (improvise-cz8d).** improvise-c5ip +(PR #185) had meanwhile added `App::refuse`, which writes the reason, sets +`batch_refused` and does **not** abort. The combination is now: + +- **`App::refuse` is the only writer** of a refusal. bok4's own `refused()` + helper, which aborted, is deleted. +- **`on_view(app, target, write)`** writes the named view, or calls + `App::refuse` naming the missing one. +- **`Host::mutating_frame` rejects on `last_batch_refused() || + last_batch_aborted()`.** That is sound because each `ModelCommand` maps to a + one-effect batch, so either flag belongs to the mutation that raised it. +- **Aborting is an explicit extra step**, taken only where a later effect in a + key-path batch assumes the failed one happened. `refuse_and_stop` does it for + `LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and + `import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its + own inline abort (gotcha 21). The item, formula and view effects only refuse: + the frame is rolled back by the refusal alone. On the key path, a failed + `AddItem`, `AddFormula` or view edit no longer stops the rest of its batch, + which is what it did before bok4. + +In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame +is undone whole, and the client is told, which is the channel the narrated +reason never had. + +**This takes the ticket's first option, scoped to the effects the wire can +reach.** Failure is *reported* by the effect that knows it failed. The second +option, growing `broken_chain` to ask whether every view, category and item +target still resolves, would have been a second description of which targets +each effect needs. That second copy would be free to disagree with the effect +itself, which is gotcha 24's reason for observing rather than predicting. + +Consequences: + +- **A failed load is refused**, where it used to be accepted with a host status + line. So `a_load_that_fails_is_not_a_rebase` now witnesses the failure by the + refusal, not by `status_msg`. The frame's rollback also rolls the host's + status line back. +- **Every refusal site has a test that fails when it is reverted** to a + status-only write (improvise-9fpl). Most are one table, + `every_failing_model_command_refuses_its_whole_frame`. That matters because + these are exactly the lines a merge conflict could silently drop. +- **Not covered:** a no-op *inside* a view that does resolve. Examples are + `SetAxis` on a category the view does not have, `HideItem` of an absent + item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already + gone. The removals are idempotent by intent. The others are the remaining + instances of this class, and each needs its own `View` method to say whether + it changed anything. + ### `AddFormula` and non-`_Measure` targets For non-`_Measure` targets the item is added to the category so it appears in the @@ -2494,12 +2566,12 @@ available for the rare command that needs to enumerate all views, such as the vi panel. The interactive arm (keymap dispatch, scripts, the `:` line) is built **from** -`parse`: `register` takes `parse`, a `contextualize` that fills context-only -fields of the command `parse` built without seeing the args, and an optional -`fallback` reached only for argument lists `parse` refused. So an accepted -argument list is never overridden by context (improvise-2bac; the argument is -`context/code/cli.md` §8). `register_pure` is the case with nothing to -contextualize and no fallback: both arms are `parse`. +`parse`: `register` takes `parse` and an optional `fallback` reached only for +argument lists `parse` refused, and `register_at_cursor` additionally has the +registry set the command's `CursorState` to the live cursor. No registration +writes any other field, so an accepted argument list is never overridden by +context (improvise-2bac, improvise-yf7p; the argument is `context/code/cli.md` +§8). `register_pure` is the case with no fallback: both arms are `parse`. `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers diff --git a/context/code/protocol.md b/context/code/protocol.md index 67ad3aea..105f1059 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -230,11 +230,33 @@ guard below (improvise-c7mu). It removes three things a scan must not see. composers, so no verdict was wrong yet. It also had a mirror: a comment explaining that a `NOT_COMPOSED` variant is held back would have raised a false "stale entry" alarm. -- **Test items inside a file.** A line scan that relies on rustfmt putting an - item's closing brace at the attribute's own indentation. It treats - `cfg(any(test, …))` as test code and `cfg(not(test))` as production. - Over-stripping **fails** the test rather than passing it, which is the safe - direction. +- **Test items inside a file.** Each test-only attribute's item is bounded by + **balanced brackets on the blanked code** (improvise-59q8). It ends at a `;` + at depth zero, or at the `}` closing its body unless `;`, `.`, `?` or `else` + follows. The first version was a line scan that relied on rustfmt putting an + item's closing brace at the attribute's own indentation. The #172 review + found three shapes it never bounded: + - an item ending in `];` or `);`; + - a `#[cfg(test)]` statement spanning lines; + - a one-line `#[cfg(test)] use x;`. + + In each case the scan read on to the end of the file and **hid every line of + production code after it**, which is the silent direction for the forward + guard. An item the scan cannot bound is now a **panic**, never a scan to EOF. + + **Which `cfg` is test-only is evaluated, not substring-matched.** The + predicate is read off the file's *text*, because a `feature` value is a + literal and blanked in the code. It is evaluated in three-valued logic for a + build without `test`: `test` is false, and so is `feature = "test-support"`, + which this repo never ships (improvise-26dm). Everything else a scan cannot + know, such as a target or an ordinary feature, is unknown. An item is + test-only when its predicate is **false**. So + `cfg(all(test, not(target_arch = "wasm32")))` is test code, which the old + "any line containing `not(`" rule got wrong. `cfg(any(test, feature = + "serde"))` and `cfg(not(test))` are production. An inner `#![cfg(test)]` + heading a file makes the whole file test code. One appearing anywhere else + is a panic, because this scan cannot bound an inline module's inner + attribute. - **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under `x/` test code, even though nothing inside the file says so. Before improvise-c7mu, this checker's own file and `improvise-io`'s @@ -592,14 +614,37 @@ broader than its reason — improvise-gcvb, which also records the two things that make it more than a one-liner: an inserted `+cat` column means the TUI does **not** advance, and `AddCategory` can abort. +### `CellId::Coordinate` — a drawn coordinate is not "no cell" (improvise-exdg) + +A records-mode coordinate cell (the `Region` column holding `North`) used to +project `None`. That was a **third** meaning of `None`, next to the padded slot +and the blanked position: a visible cell that holds text and whose write +belongs to the host. So absence was encoding a permission, the same conflation +`Create` removed for the creating columns (improvise-poia). The pinning test, +`a_coordinate_column_is_projected_with_no_identity`, asserted the bug and is +gone. + +`Coordinate` carries **nothing**. Editing a coordinate renames it across every +measure cell in the row (gotcha 1), and no client composes that operation: it +needs `record_col`, and the browser declines every records-mode edit before +reaching a cell. Giving the variant a `RecordHash` would hand a client a +surrogate with nothing to spend it on. The operand belongs with the command +that consumes it, whenever that command exists. Nothing a client can do has +changed. `render.rs::editable` is now an exhaustive `match` rather than a +`matches!`, so the next variant has to be classified. `typed_cell_command`, +`record_at_cursor` and `record_cell_at_cursor` still answer `None` for it. +So paste onto a coordinate is still a silent `Upstream` in the browser. The +TUI has declined it out loud since PR #184. That divergence is +improvise-kkcy, and a named kind is what lets the browser decline deliberately +now. + ### `CellDisplay::id` — `None` is not "aggregate" That is `Some(CellId::Aggregate(..))`, and the distinction is the whole point of the enum. `None` means the slot holds **no projected cell**: a `GridCache` -position padded by `fit_to_labels`, blanked by a reframing, or a -records-mode column that is not a cell to write into at all (a category column -renames a coordinate across the whole row, gotcha 1, which is a different -intent from `SetCell`). Conflating the two either hides a cell or offers a +position padded by `fit_to_labels`, or one blanked by a reframing. Every +records-mode column is some kind of cell (`Record`, `Derived`, `Create` or +`Coordinate`). Conflating the two either hides a cell or offers a write the type system exists to make unconstructible. ### `CellDisplay::edit_text` — the difference corrupts data @@ -615,8 +660,8 @@ and an editor's seed is exactly a feedback path. It is the projection of `App::display_value_at_cursor`, which is what `EditOrDrill` seeds the TUI's buffer from, so both front ends start from one string rather than each deriving one. `None` where there is nothing to edit: -the same set of positions `id` is `None` for, plus cells whose value is -absent. An error cell carries none, because its text is a diagnostic and not +positions whose `id` is `None`, plus pivot cells with no key. A records-mode +cell always carries one, even an empty one. An error cell carries none, because its text is a diagnostic and not a value. A browser found this: `tests/lockstep.rs` drove `i` through the host and the @@ -812,9 +857,16 @@ one**. `AxisFrame::flat` leaves `cats` empty on purpose, so it is not a way to smuggle a multi-level axis in flattened. `FrameGroup::collapsed` is on the wire rather than left to the client because -it is a fact about the *view*, which only the server holds. `FrameGroup::cat` -travels so a client can echo it back in `ToggleGroup` without ever working out -which category a label came from — which slash-joined labels made impossible. +it is a fact about the *view*, which only the server holds. There is no +`FrameGroup::cat` any more (improvise-7xyx). Its doc comment promised that a +client echoes it back in `ToggleGroup`, but no client ever did. `resolve.rs` +has no `toggle-group-under-cursor` arm (`z` is on the still-needs-the-host +list), and neither renderer read the field. A promise about an interaction no +code performs is the stale-sibling defect, so the field went with it. It is +not a loss for `z`. The TUI's `ToggleGroupAtCursor` also places the cursor on +the layout the toggle *will produce*, which a thin client cannot compute. So +composing `z` in a browser needs its own design, and the operand should come +back with that design rather than wait for it. ### `AxisFrame`: four accessors that are **not for a draw loop** @@ -1321,13 +1373,13 @@ frozen behaviour reads as decided afterwards and nothing says it was only frozen for the duration of the move. **The ticket's own proposed fix does not work, and that is worth more than the -fix.** h0iv suggested comparing the `(cat, name)` pair, `FrameGroup` carrying -`cat` precisely so a client need not work out which category a label came from. +fix.** h0iv suggested comparing the `(cat, name)` pair; `FrameGroup` carried +`cat` then (improvise-7xyx has since removed it). But its own reachability case is *one* category: `Month` grouped into `Q1`, nested under `Region`, giving `North/Q1` and `South/Q1` adjacent — the two headers are `(Month, Q1)` and `(Month, Q1)`, **equal as values**, so the pair -separates nothing. `the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them` -pins that, so the next reader does not re-propose it. What separates them is +separates nothing. `the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them` +pins that as whole-value equality, so the next reader does not re-propose it. What separates them is that they are two different `FrameEntry::Group` entries, and `groups()` already borrows each from `self.entries` — so the comparison is `std::ptr::eq` on the header, which is the literal statement of "the same header opened both". diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index d978d2c9..1d656a33 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -298,8 +298,9 @@ The unit-level form of the headless test, against `CmdRegistry::interactive` directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key and value. Rival (a) above — a `contextualize` that overwrites the parsed -fields — fails it; this is the one hole the new registry shape leaves -representable, so it is the one that needs a test. +fields — failed it. improvise-yf7p then removed `contextualize` altogether +(`context/code/cli.md` §8), so rival (a) no longer compiles; the test stays as +the behavioural statement of what the signature now guarantees. ### every_argument_list_the_gate_accepts_the_run_accepts_too diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 950132a3..79d9ed79 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3054,6 +3054,23 @@ The third is the gotcha 48 property, and it needs `timesArmed() > 0` first or it denies an unreachable event: the precise reason must still be on screen after the 8s window, not replaced by the watchdog. `page.clock` walks past it. +**Strengthened by improvise-i0v9's second pass.** Every assertion above read the +*final* `#status`, so two failures could still pass it. A watchdog that fired +first and was then replaced by a late notice passed. So did a success claim +shown before the notice. The test now records every status with `logStatus` and +asserts, **before** the page clock is walked, that the log ends in `read-only` +and never contained `did not answer` or the exact `in-page worker · `. A +reason that arrived after the watchdog would sit in that log behind "did not +answer"; that is the "well before 8s" property, stated without a wall-clock +bound. Measured failing against `gh/main` at `da2e5ab`, whose log was +`["in-page worker · two-tab-….improv", "read-only — …"]`: the premature claim +the ticket measured at 13ms is still there once 1wft's notice exists. The +brief's rival, `onNotice` calling `setShell` instead of `fail` (the watchdog not +disarmed), fails it at the first `toContainText("read-only")`. The Snapshot's +claim replaced the reason at once, which is why the eight-second form of this +rival is now exercised by the invalid-`?doc=` spec instead (see +`context/code/clients.md`, "The watchdog"). + It runs on Chromium and Firefox — the two engines with OPFS — rather than Chromium alone, because the exclusivity semantics are the engine's and the two word the refusal completely differently. @@ -4446,6 +4463,38 @@ registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on `+`, which is why that filter was removed rather than kept. The evidence entries for the host-side tests are in `command.md`. +### the worker is not called up until its first frame has arrived + +improvise-i0v9. The worker transport wrote `in-page worker · ` when it +spawned the worker, before anything had answered. A worker that then failed +therefore showed a success-looking status over a blank page. The spec records +every `#status` with `logStatus` and requires the whole log to be exactly +`["starting the worker · …", "in-page worker · "]`. It pins the order +as well as the final text, since a final read cannot see a claim made too early. +Measured failing against `gh/main` at `da2e5ab`. The rival, restoring the +immediate `setShell(\`in-page worker · ${DOC}\`)` in `workerTransport`, fails it +on the log's first entry. The final read is `toHaveText`, not `toContainText`. +The older specs' `toContainText("in-page worker")` could not tell the starting +text from the claim if the one contained the other, and the starting text +deliberately does not. + +### only the first frame claims the peer answered, and not before it + +improvise-i0v9, the unit half of the spec above. `arm(message, { answer })` +must write nothing until `sawFrame`, then write `answer` once, however many +frames follow. Fails against `gh/main`'s `createShell`, which has no `answer` +and so never writes the claim at all. + +### a reason given before the first frame is not replaced by the frame's claim + +The order `worker.js` really produces on a locked document: the `ReadOnly` +notice is posted after `start()` and **before** the Snapshot. So `fail` then +`sawFrame` must leave the notice's text as the only thing ever shown. The rival +is the obvious fix, `sawFrame` writing `answer` unconditionally. It fails both +this test and the one above: the first on its exact log, which is then written +twice. This test passes against `gh/main` by construction, since no claim exists +there. It is not evidence of the bug; it guards against the naive fix. + ## Numbers across the serialized wire (improvise-6oy4) No manifest enabled serde_json's `float_roundtrip`, so its default f64 parser diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 3f9d7133..22f763f1 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1978,12 +1978,27 @@ Two measure columns of one row are two cells of one record, and the next row is different record — the same shape the pivot case has, checked here because records mode reaches it by a different path. -### a_coordinate_column_is_projected_with_no_identity +### a_coordinate_cell_is_named_rather_than_left_as_no_cell -A coordinate column is not a cell to write into: editing it renames that coordinate -across *every* measure cell in the row (gotcha 1), which is a different intent from -`SetCell`. So it is projected with no id at all, and a client cannot compose a -write against it. +improvise-exdg. This entry replaces `a_coordinate_column_is_projected_with_no_identity`, +a test that **pinned the bug**. It asserted that a records-mode coordinate cell +projects `id: None`. ADR 002 and gotcha 42 define `None` as "no projected cell", +yet the Region column is drawn and holds text. So a client could not tell it +from a padded slot. Absence was being used to encode a permission ("the host's"), +which is the conflation ADR 002's rejected `Option` draft was about. +`improvise-poia` removed the same conflation for the creating columns. + +The cell now projects `CellId::Coordinate`. That keeps the old test's real +property: it is still not a `SetCell` target, because `typed_cell_command`, +`record_cell_at_cursor` and `render.rs::editable` all match `Record` alone. +The test seats the cursor on the **second** row, so the first-row and +first-item cases cannot coincide. It then enumerates every cell of the records +grid and requires each to carry an id, which is the ticket's claim with no +case picked by hand. Before the fix it failed at `a drawn coordinate projected +as no cell`. Rival injected afterwards: `RecordCol::Category(_) => return None` +restored. This test failed at the `Some(CellId::Coordinate)` assertion, and +`a_creating_column_is_named_rather_than_left_as_no_cell` (whose `None`-count +now expects zero) failed too. ### a_write_moves_the_version_under_the_clients_own_surrogate @@ -3140,10 +3155,13 @@ Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails. ### toggle_panel_visibility_reads_the_panel_it_names -The behavioural half, and the one that reaches the registry. Its interactive arm -passes `currently_open` straight through, so with exactly one panel open the command -built for each of the three names carries its own panel's flag. Both injections above -fail here too. +The behavioural half, and the one that reaches the registry. With exactly one panel +open, the command built for each of the three names and executed must emit +`SetPanelOpen` for **its own** panel, flipping **its own** flag. Both injections above +fail here too. Since improvise-yf7p the flag is read in `execute` rather than copied +in by a registry contextualizer, so the assertion moved from the built command's +`Debug` to its effects; it also names the panel, so an `execute` that ignores the +parsed panel (`ctx.panel_open(Panel::Formula)`) fails it — injected and run. ### a_focused_open_panel_closes_and_a_closed_one_opens @@ -3295,11 +3313,12 @@ aggregates, arriving a third time. The test walks **every** column of a records grid and counts, rather than asserting the two creating columns alone. That matters in the direction a -weaker test fails: a producer that answered `Create` for everything, or that -made the `Region` coordinate column nameable, would pass a two-position check -and fails this one. The fault injected — returning `None` for both creating -columns, which is the pre-fix code — reports three nameless columns where one -is allowed. +weaker test fails: a producer that answered `Create` for everything would pass +a two-position check and fails this one. The fault injected — returning `None` +for both creating columns, which is the pre-fix code — reported three nameless +columns where one was allowed. Since improvise-exdg the coordinate column is +named too (`CellId::Coordinate`), so the count of nameless columns is now zero. +See `a_coordinate_cell_is_named_rather_than_left_as_no_cell`. ### a_records_coordinate_is_text_however_it_is_spelled @@ -4728,19 +4747,52 @@ Its mirror, and the one that pins the wrap: from the first item `[` must land on `SetPanelCursor` carries a panel *and* a cursor and neither was checked. Both are now named, so moving the right cursor on the wrong panel fails. +### move_panel_cursor_moves_from_the_named_panels_own_cursor + +improvise-yf7p. `move-panel-cursor formula 2` through the registry's interactive +arm, with the formula cursor at **1** of five formulas and the category and view +cursors at 0, must land on **3**. Before yf7p the cursor and bound were copied in +by a registry `contextualize`, and review showed one that clamped the parsed delta +to `delta.signum()` left the whole workspace green: every test moved by ±1, where +a clamp is invisible. They are read in `execute` now, and this test is what sees a +bug there. Rival `self.delta.signum()` in `execute` — injected, run, fails (lands +on 2). Reading another panel's cursor fails too, because the fixture seats them at +different values — the index-0 fixture could not separate them. + ### delete_formula_at_cursor_with_formulas -Asserts the `RemoveFormula` names `Profit` in `Type` — the formula the cursor is -on — rather than merely that a formula was removed. +Asserts the `RemoveFormula` names the formula the cursor is on, rather than merely +that a formula was removed. PR #148 named the field, but its fixture was **one** +formula with the cursor at **0**, where "the formula under the cursor" and "the +first formula" are the same formula (improvise-y1av). The rival it could not see — +`DeleteFormulaAtCursor` reading index `0` instead of `ctx.formula_cursor` — passed +the **whole workspace**, so `d` in the formula panel deleting the wrong formula had +no guard anywhere. Now two formulas, cursor at **1**: it must remove `Tax` and must +not name `Profit`. Rival `let cursor = 0usize.min(..)` injected, full suite run: +this is the only test that fails. ### switch_view_at_cursor_with_valid_cursor -Names `Default`, the view under the cursor. +Names the view under the cursor. Same fixture defect as above (improvise-y1av): +`two_cat_model` has one view, so the cursor at 0 named `Default` whether or not +the command read it. Now `Default, Other` with the cursor at **1**, asserting +`Other`. Rival `get(cursor).and(first())` — in bounds only, so the out-of-bounds +arm is unchanged and `switch_view_at_cursor_out_of_bounds_returns_empty` cannot be +what catches it — injected and run: fails here, in +`delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor`, and in the two +`wasm-client/tests/lockstep.rs` tests that were previously the only guard. + +### delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor + +improvise-y1av. `Default, Other, Third` with the cursor at **1**: deletes `Other` +and steps the panel cursor back to 0. The same rival as above fails it. ### delete_view_at_cursor_zero_does_not_adjust_cursor Names `Default` too. Its existing negative half — no `SetPanelCursor` at -position 0 — is unchanged. +position 0 — is unchanged. Position 0 is this test's subject, so it cannot +separate "the view under the cursor" from "the first view" and does not try to; +the test above does. ### commit_category_add_with_name_produces_add_effect @@ -5006,6 +5058,31 @@ run alone with the ones before it removed, and each fails: the diff returns test passes. See `render::tests::a_cell_equals_another_exactly_when_the_wire_cannot_tell_them_apart`. +### ui::host::tests::a_frame_writing_a_view_its_own_first_command_deleted_is_refused + +improvise-bok4. This is the reviewer's first measured case: +`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]`. Before the fix +the asker's reply was `[Commands([ForgetView("Scratch")]), Commands([Project…])]` +and then `Accepted`. The second command had done nothing, because +`view_mut_of` found no view and every view-addressed effect swallowed the +`None`. Now `on_view` refuses, the abort becomes +`Rejected(MutationRefused)`, and the assertion that `Scratch` still exists +proves the deletion was undone with the rest of the frame. + +Its sibling, `a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused`, +is the reviewer's second case: `[RemoveCategory("Region"), AddItem { category: +"Region" }]`. It was accepted as well, with `AddItem`'s error written only to +the host's `status_msg`. Both tests failed first against the unfixed effects. + +`deleting_the_documents_last_view_is_refused_rather_than_accepted` covers the +one-command case, which `DeleteView`'s `let _` swallowed. The fault injected +was to put the `let _` back. The test then failed with a projection batch +where the refusal should have been, and it was reverted. + +The control is `a_frame_whose_commands_all_land_is_accepted`, the same two +commands in an order where both land. Without it, a host that refused every +two-command frame would pass all three tests. + ### ui::reduce::vocabulary_is_complete::every_effect_is_on_the_wire_or_has_a_verdict improvise-h3y7, the forward direction of the vocabulary derivation. It is the @@ -5139,6 +5216,53 @@ the first message does not mention drilling. At `0ed2e41` it failed with the old text. The rival is restoring that text. Real drill paste, which means staging a typed `CellValue`, is still open as improvise-48jd. +### ui::host::tests::every_failing_model_command_refuses_its_whole_frame + +improvise-9fpl, found by PR #181's review. Eight of bok4's refusal sites had no +test: reverting any one of them to a status-only write failed nothing. They +were `AddItemInGroup`, `AddFormula`'s parse error, and all six +`ImportJsonHeadless` arms. Two improvise-40j4 cases were added to the same +table: `DeleteView` of an absent view, and `AddFormula` into an absent +category. + +Each case sends a frame of two commands. The first, `AddItem { Region, +"Landed" }`, lands. The second fails. The test asserts that the asker hears +exactly `[Rejected(MutationRefused)]` and that `Landed` is gone. The fixture +has a second view, `Scratch`, because with one view `DeleteView("Ghost")` is +refused as "the last view" and the absent-name case is never reached. The +first run found exactly that: the case passed before the fix. + +Faults injected by a script (`/tmp/wire2_inject2.py`, not committed): each of +the 14 refusal sites was reverted **alone** to a status-only write, after the +merge with #185. Every one failed at least one test. The eight that were +unwitnessed fail this test, `DeleteView`'s last-view arm fails +`deleting_the_documents_last_view_is_refused_rather_than_accepted`, `on_view` +fails `a_frame_writing_a_view_its_own_first_command_deleted_is_refused`, and +`LoadModel` fails both `a_load_that_fails_is_not_a_rebase` and +`a_failed_load_or_import_does_not_run_the_quit_after_it`. The file was +restored and compared. + +### ui::host::tests::a_frame_adding_a_formula_to_a_category_its_first_command_removed_is_refused + +improvise-40j4. `[RemoveCategory("Region"), AddFormula { "Total = 1", Region }]` +was accepted, and the document held a formula for a category that did not +exist; `format_md` wrote it and `parse_md` reloaded it. `AddFormula` ran +`let _ = add_item(…)` and then `add_formula` anyway. The test failed first with +the frame accepted. `AddFormula` now refuses when `add_item` errs, before +storing anything. + +### ui::app::tests::a_failed_load_or_import_does_not_run_the_quit_after_it + +improvise-cz8d. After #185, `App::refuse` does not abort. So on gh/main a failed +`load` refused, and the `force-quit` after it in the same batch still ran: +`load absent.improv . q` quit having loaded nothing. bok4's aborting helper had +prevented that, and the merge would have silently lost it. The test binds +`[load|import-json , force-quit]` as one sequence and asserts that the +refusal is recorded and the mode is not `Quit`. `import-json` failed first, +before `refuse_and_stop` was applied to its arms. For `load`, the injected fault +is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test +then fails with "`load absent.improv . q` quit having loaded nothing". + ### `an_untouched_pivot_edit_of_a_misread_value_writes_nothing` ### `an_untouched_records_edit_of_a_misread_value_writes_nothing` diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index fa9f896d..e1dc17a6 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -557,7 +557,7 @@ the evidence that nothing was relying on the permissive parse. ### render::tests::two_runs_under_two_headers_of_one_name_are_two_runs -### render::tests::the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them +### render::tests::the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them improvise-h0iv. `group_runs` coalesced adjacent positions by group **name**, so two runs under two different headers sharing a name read as one and both front @@ -571,10 +571,12 @@ and nothing ungrouped between the runs. Before the fix it returns at the precondition rather than pass with a single run. The second test exists because the ticket proposed the wrong fix and a reader -will propose it again. h0iv says `FrameGroup` carries `cat` and "comparing the -pair would fix it" — but its own reachability case is a single category nested -under an outer one, so both headers are `(Month, Q1)`, equal as values. The test -asserts that equality directly. What actually separates them is that they are +will propose it again. h0iv said `FrameGroup` carried `cat` and that "comparing +the pair would fix it". But its own reachability case is a single category +nested under an outer one, so both headers are `(Month, Q1)`, equal as values. +The test asserts that equality directly. Since improvise-7xyx removed +`FrameGroup::cat`, it asserts equality of the whole header, which is stronger: +no field-wise comparison can separate two equal values. What actually separates them is that they are two distinct entries in `self.entries`, which `std::ptr::eq` on the borrowed header states exactly. @@ -1050,3 +1052,51 @@ Faults injected, one per half, each compiling only `improvise-protocol`: A field added to an existing variant is caught too, but not in isolation, because every constructor of that variant also fails. Reverted with `git restore`. + +### source_scan::tests::a_test_only_item_hides_nothing_after_it + +improvise-59q8, found by the PR #172 review. The scanner's line-based +`strip_test_items` looked for `}` at the attribute's indentation, or for an item +line ending in `;`. So three shapes were never bounded, and it dropped +**everything to the end of the file**: +- a test-only `const … = &[\n … \n];`; +- a `#[cfg(test)] let x = foo(\n 1,\n);` statement; +- a one-line `#[cfg(test)] use foo::bar;`, where the attribute's own line is + also the item line. + +In `vocabulary_is_complete` that silently hides every effect after such an +item. Each shape is a case here, asserting that the production code after it +(`Hidden1`, `Visible`) survives. Against the line scan all three failed; the +first printed `"impl X {\n"` and nothing else. They now pass because items are +bounded by balanced brackets on the blanked code. +`an_unbounded_test_only_item_is_refused_not_swallowed` pins the other half of +the acceptance criteria: an item that cannot be bounded panics instead of +consuming the rest of the file. Against the line scan it failed, because +nothing panicked. + +### source_scan::tests::a_test_only_cfg_is_recognised_whatever_its_spelling + +improvise-59q8, the other direction: test code counted as production, which +makes `vocabulary_is_composed` weaker. `cfg(all(test, not(target_arch = +"wasm32")))` was kept because the old rule rejected any line containing +`not(`. A leading `#![cfg(test)]` was kept because only `#[cfg(` was +recognised. An attribute split across lines was not seen at all. All three +failed against the old scanner. The predicate is now parsed and evaluated in +three-valued logic (`context/code/protocol.md` §2). The controls +(`cfg(not(test))`, `cfg(any(test, feature = "serde"))` and `cfg(target_arch = +…)`) must stay production, so a scanner that dropped every `cfg` item could +not pass. + +A consequence to know about: `cfg(any(test, feature = "x"))` for an ordinary +feature is now production. Only `feature = "test-support"`, the one test +feature this repo has, counts as a test build. +`test_only_items_and_modules_are_not_production` was updated to that spelling. + +### source_scan::tests::a_c_string_literal_is_blanked + +improvise-59q8's minor case. In `cr#"x" Command::InRawC(2) "y"#` the `r` has a +word character before it, so the literal was lexed as two ordinary strings, +and the text between the inner quotes leaked. It failed as +`InRawC survived`. `c"…"` and `cr#"…"#` are now literals. A first version of +this test used a C string with no inner quotes and passed against the bug, +because the ordinary lexing happened to blank it too. diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 3d79bb7b..26448153 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -3,6 +3,8 @@ use std::fmt::Debug; use crate::command::key::KeyCode; +use super::navigation::CursorState; + use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; use crate::ui::app::AppMode; @@ -141,13 +143,15 @@ pub trait Cmd: Debug + Send + Sync { /// Factory that constructs a Cmd from text arguments (headless/script). pub type ParseFn = fn(&[String]) -> Result, String>; -/// Fills the context-only fields of a command `parse` already built. It never -/// sees the args, so it cannot discard them — `context/code/cli.md` §8. -pub type ContextualizeFn = fn(C, &CmdContext) -> C; - /// Builds a command from context, reached ONLY for args `parse` refused. pub type FallbackFn = fn(&[String], &CmdContext) -> Result; +/// A command whose one context-only field is the cursor it moves from. The +/// registry fills it, and no registration can — `context/code/cli.md` §8. +pub trait AtCursor: Cmd { + fn cursor_mut(&mut self) -> &mut CursorState; +} + type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; @@ -188,25 +192,48 @@ impl CmdRegistry { name } - /// Register a command whose interactive arm is `parse` then `contextualize`, so - /// args `parse` accepts are honoured whole — `context/code/cli.md` §8. + /// Register a command whose interactive arm is `parse`, reaching `fallback` + /// only where `parse` refused — `context/code/cli.md` §8. pub fn register( &mut self, prototype: &C, parse: fn(&[String]) -> Result, - contextualize: ContextualizeFn, fallback: Option>, + ) { + self.push(prototype.name(), parse, fallback, |_, _| {}); + } + + /// As [`register`](Self::register), then the interactive arm moves the + /// command's cursor to the live one. The cursor is the only field it writes. + pub fn register_at_cursor( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + fallback: Option>, + ) { + self.push(prototype.name(), parse, fallback, |cmd, ctx| { + *cmd.cursor_mut() = CursorState::from_ctx(ctx); + }); + } + + fn push( + &mut self, + name: &'static str, + parse: fn(&[String]) -> Result, + fallback: Option>, + at_cursor: fn(&mut C, &CmdContext), ) { self.entries.push(CmdEntry { - name: prototype.name(), + name, parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), interactive: Box::new(move |args, ctx| { - let built = match (parse(args), fallback) { - (Ok(cmd), _) => Ok(contextualize(cmd, ctx)), - (Err(_), Some(fallback)) => fallback(args, ctx), - (Err(err), None) => Err(err), + let mut built = match (parse(args), fallback) { + (Ok(cmd), _) => cmd, + (Err(_), Some(fallback)) => fallback(args, ctx)?, + (Err(err), None) => return Err(err), }; - built.map(|c| Box::new(c) as Box) + at_cursor(&mut built, ctx); + Ok(Box::new(built) as Box) }), }); } diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index a69469f8..457fad6c 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -118,6 +118,22 @@ pub(super) mod test_helpers { wb } + /// `two_cat_model` with one `_Measure` formula per target, in order. + pub fn with_formulas(targets: &[&str]) -> Workbook { + let mut wb = two_cat_model(); + for target in targets { + wb.model.add_formula(crate::formula::ast::Formula { + raw: format!("{target} = 0"), + target: target.to_string(), + target_category: "_Measure".to_string(), + expr: crate::formula::ast::Expr::Number(0.0), + filter: None, + format: None, + }); + } + wb + } + pub fn effects_debug(effects: &[Box]) -> String { format!("{:?}", effects) } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 1d27a42f..6793c910 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -192,11 +192,7 @@ mod tests { let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); - let cmd = CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); } @@ -208,11 +204,7 @@ mod tests { let reg = make_registry(); let mut ctx = make_ctx(&m, &layout, ®); ctx.formula_panel_open = true; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: false, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = format!("{:?}", effects[0]); @@ -231,11 +223,7 @@ mod tests { let mut ctx = make_ctx(&m, &layout, ®); ctx.formula_panel_open = true; ctx.category_panel_open = true; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -256,11 +244,7 @@ mod tests { ctx.formula_panel_open = true; ctx.category_panel_open = true; ctx.mode = &AppMode::FormulaPanel; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -281,11 +265,7 @@ mod tests { ctx.category_panel_open = true; ctx.view_panel_open = true; ctx.mode = &AppMode::CategoryPanel; - let cmd = CyclePanelFocus { - formula_open: false, - category_open: true, - view_open: true, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -305,11 +285,7 @@ mod tests { ctx.formula_panel_open = true; ctx.view_panel_open = true; ctx.mode = &AppMode::ViewPanel; - let cmd = CyclePanelFocus { - formula_open: true, - category_open: false, - view_open: true, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let dbg = effects_debug(&effects); @@ -329,11 +305,7 @@ mod tests { let mut ctx = make_ctx(&m, &layout, ®); ctx.category_panel_open = true; ctx.mode = &AppMode::CategoryPanel; - let cmd = CyclePanelFocus { - formula_open: false, - category_open: true, - view_open: false, - }; + let cmd = CyclePanelFocus; let effects = cmd.execute(&ctx); let dbg = effects_debug(&effects); assert!( @@ -344,15 +316,13 @@ mod tests { #[test] fn move_panel_cursor_down_from_zero() { - let m = two_cat_model(); + let m = with_formulas(&["A", "B"]); let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: 1, - current: 0, - max: 5, }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); @@ -363,17 +333,35 @@ mod tests { ); } + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn move_panel_cursor_moves_from_the_named_panels_own_cursor() { + let m = with_formulas(&["A", "B", "C", "D", "E"]); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.formula_cursor = 1; + let args = ["formula".to_string(), "2".to_string()]; + let effects = reg + .interactive("move-panel-cursor", &args, &ctx) + .expect("move-panel-cursor resolves") + .execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + dbg.contains("SetPanelCursor { panel: Formula, cursor: 3 }"), + "must move from the formula cursor (1) by the parsed delta (2), got: {dbg}" + ); + } + #[test] fn move_panel_cursor_clamps_at_zero() { - let m = two_cat_model(); + let m = with_formulas(&["A", "B"]); let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: -1, - current: 0, - max: 5, }; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); @@ -388,46 +376,55 @@ mod tests { let cmd = MovePanelCursor { panel: effect::Panel::Formula, delta: 1, - current: 0, - max: 0, }; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); } + /// Evidence: `context/test-evidence/command.md`. #[test] fn delete_formula_at_cursor_with_formulas() { let mut m = two_cat_model(); - m.model.add_formula(crate::formula::ast::Formula { - raw: "Profit = Revenue - Cost".to_string(), - target: "Profit".to_string(), - target_category: "Type".to_string(), - expr: crate::formula::ast::Expr::Number(0.0), - filter: None, - format: None, - }); + for (target, raw) in [("Profit", "Profit = 1"), ("Tax", "Tax = 2")] { + m.model.add_formula(crate::formula::ast::Formula { + raw: raw.to_string(), + target: target.to_string(), + target_category: "Type".to_string(), + expr: crate::formula::ast::Expr::Number(0.0), + filter: None, + format: None, + }); + } let layout = make_layout(&m); let reg = make_registry(); - let ctx = make_ctx(&m, &layout, ®); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.formula_cursor = 1; let effects = DeleteFormulaAtCursor.execute(&ctx); let dbg = effects_debug(&effects); assert!( - dbg.contains("RemoveFormula { target: \"Profit\", target_category: \"Type\" }"), - "must remove the formula AT THE CURSOR, got: {dbg}" + dbg.contains("RemoveFormula { target: \"Tax\", target_category: \"Type\" }"), + "must remove the formula AT THE CURSOR (Tax, index 1), got: {dbg}" + ); + assert!( + !dbg.contains("\"Profit\""), + "removed the first formula, not the one under the cursor: {dbg}" ); } + /// Evidence: `context/test-evidence/command.md`. #[test] fn switch_view_at_cursor_with_valid_cursor() { - let m = two_cat_model(); + let mut m = two_cat_model(); + m.create_view("Other"); let layout = make_layout(&m); let reg = make_registry(); - let ctx = make_ctx(&m, &layout, ®); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.view_panel_cursor = 1; let effects = SwitchViewAtCursor.execute(&ctx); let dbg = effects_debug(&effects); assert!( - dbg.contains("SwitchView(\"Default\")"), - "must switch to the view UNDER THE CURSOR, got: {dbg}" + dbg.contains("SwitchView(\"Other\")"), + "must switch to the view UNDER THE CURSOR (Other, index 1), got: {dbg}" ); assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); } @@ -504,6 +501,27 @@ mod tests { ); } + /// Evidence: `context/test-evidence/command.md`. + #[test] + fn delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor() { + let mut m = two_cat_model(); + m.create_view("Other"); + m.create_view("Third"); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.view_panel_cursor = 1; + let dbg = effects_debug(&DeleteViewAtCursor.execute(&ctx)); + assert!( + dbg.contains("DeleteView(\"Other\")"), + "must delete the view UNDER THE CURSOR (Other, index 1), got: {dbg}" + ); + assert!( + dbg.contains("SetPanelCursor { panel: View, cursor: 0 }"), + "a delete off position 0 steps the cursor back, got: {dbg}" + ); + } + #[test] fn delete_view_at_cursor_zero_does_not_adjust_cursor() { let m = two_cat_model(); @@ -559,16 +577,15 @@ impl Cmd for TogglePanelAndFocus { #[derive(Debug)] pub struct TogglePanelVisibility { pub panel: Panel, - pub currently_open: bool, } impl Cmd for TogglePanelVisibility { fn name(&self) -> &'static str { "toggle-panel-visibility" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { vec![Box::new(effect::SetPanelOpen { panel: self.panel, - open: !self.currently_open, + open: !ctx.panel_open(self.panel), })] } } @@ -576,20 +593,16 @@ impl Cmd for TogglePanelVisibility { /// Tab through open panels Formula → Category → View, wrapping, starting from the /// focused one. One open panel self-transitions; none open is a no-op. #[derive(Debug)] -pub struct CyclePanelFocus { - pub formula_open: bool, - pub category_open: bool, - pub view_open: bool, -} +pub struct CyclePanelFocus; impl Cmd for CyclePanelFocus { fn name(&self) -> &'static str { "cycle-panel-focus" } fn execute(&self, ctx: &CmdContext) -> Vec> { let order = [ - (self.formula_open, AppMode::FormulaPanel), - (self.category_open, AppMode::CategoryPanel), - (self.view_open, AppMode::ViewPanel), + (ctx.formula_panel_open, AppMode::FormulaPanel), + (ctx.category_panel_open, AppMode::CategoryPanel), + (ctx.view_panel_open, AppMode::ViewPanel), ]; // Index of the first candidate: the panel after the focused one, // or the first panel when invoked from a non-panel mode. @@ -614,16 +627,17 @@ impl Cmd for CyclePanelFocus { pub struct MovePanelCursor { pub panel: Panel, pub delta: i32, - pub current: usize, - pub max: usize, } impl Cmd for MovePanelCursor { fn name(&self) -> &'static str { "move-panel-cursor" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { - let cursor = self.current; - let max = self.max; + fn execute(&self, ctx: &CmdContext) -> Vec> { + let (cursor, max) = match self.panel { + Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), + Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), + Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), + }; if max == 0 { return vec![]; } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 136b22ed..10fad1b6 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -27,19 +27,21 @@ fn cursor_key(ctx: &CmdContext) -> Result { ctx.cell_key().clone().ok_or("no cell at cursor".into()) } -/// A `Move` built from args takes the live cursor as its origin. -fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move { - Move { - cursor: CursorState::from_ctx(ctx), - ..cmd +impl AtCursor for Move { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor } } -/// A commit built from args still advances from the live cursor. -fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance { - CommitAndAdvance { - cursor: CursorState::from_ctx(ctx), - ..cmd +impl AtCursor for EnterAdvance { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor + } +} + +impl AtCursor for CommitAndAdvance { + fn cursor_mut(&mut self) -> &mut CursorState { + &mut self.cursor } } @@ -62,11 +64,6 @@ fn commit_at_cursor( }) } -/// For a command with no context-only fields: `parse`'s result is used as built. -fn as_parsed(cmd: C, _ctx: &CmdContext) -> C { - cmd -} - /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -91,7 +88,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(ClearCellCommand { key: cursor_key(ctx)?, @@ -122,7 +118,7 @@ pub fn default_registry() -> CmdRegistry { r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); // ── Navigation (unified Move) ────────────────────────────────────── - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Relative(0, 0), cursor: CursorState::default(), @@ -138,13 +134,12 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "move-selection", }) }, - move_from_cursor, None, ); // Jump-to-edge commands: first/last row/col macro_rules! reg_jump { ($r:expr, $is_row:expr, $to_end:expr, $name:expr) => { - $r.register( + $r.register_at_cursor( &Move { kind: if $to_end { MoveKind::ToEnd($is_row) @@ -165,7 +160,6 @@ pub fn default_registry() -> CmdRegistry { cmd_name: $name, }) }, - move_from_cursor, None, ); }; @@ -174,7 +168,7 @@ pub fn default_registry() -> CmdRegistry { reg_jump!(r, true, true, "jump-last-row"); reg_jump!(r, false, false, "jump-first-col"); reg_jump!(r, false, true, "jump-last-col"); - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Relative(0, 0), cursor: CursorState::default(), @@ -189,10 +183,9 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "scroll-rows", }) }, - move_from_cursor, None, ); - r.register( + r.register_at_cursor( &Move { kind: MoveKind::Page(0), cursor: CursorState::default(), @@ -207,10 +200,9 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "page-scroll", }) }, - move_from_cursor, None, ); - r.register( + r.register_at_cursor( &EnterAdvance { cursor: CursorState::default(), }, @@ -228,9 +220,6 @@ pub fn default_registry() -> CmdRegistry { }, }) }, - |_, ctx| EnterAdvance { - cursor: CursorState::from_ctx(ctx), - }, None, ); @@ -247,7 +236,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(YankCell { key: cursor_key(ctx)?, @@ -266,7 +254,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(PasteCell { key: cursor_key(ctx)?, @@ -319,7 +306,6 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), }) }, - as_parsed, Some(|_, ctx| { Ok(DrillIntoCell { key: cursor_key(ctx)?, @@ -357,77 +343,30 @@ pub fn default_registry() -> CmdRegistry { }); Ok(TogglePanelAndFocus { panel, target }) }, - as_parsed, None, ); r.register( &TogglePanelVisibility { panel: Panel::Formula, - currently_open: false, }, |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; - Ok(TogglePanelVisibility { - panel, - currently_open: false, - }) - }, - |cmd, ctx| TogglePanelVisibility { - currently_open: ctx.panel_open(cmd.panel), - ..cmd - }, - None, - ); - r.register( - &CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }, - |_| { - Ok(CyclePanelFocus { - formula_open: false, - category_open: false, - view_open: false, - }) - }, - |_, ctx| CyclePanelFocus { - formula_open: ctx.formula_panel_open, - category_open: ctx.category_panel_open, - view_open: ctx.view_panel_open, + Ok(TogglePanelVisibility { panel }) }, None, ); + r.register_nullary(|| Box::new(CyclePanelFocus)); r.register( &MovePanelCursor { panel: Panel::Formula, delta: 0, - current: 0, - max: 0, }, |args| { require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; - Ok(MovePanelCursor { - panel, - delta, - current: 0, - max: 0, - }) - }, - |cmd, ctx| { - let (current, max) = match cmd.panel { - Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), - Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), - Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), - }; - MovePanelCursor { - current, - max, - ..cmd - } + Ok(MovePanelCursor { panel, delta }) }, None, ); @@ -494,7 +433,7 @@ pub fn default_registry() -> CmdRegistry { // ── Commit ─────────────────────────────────────────────────────────── // These take a mode-name arg as args[0]; the command never inspects ctx.mode. - r.register( + r.register_at_cursor( &CommitAndAdvance { key: CellKey::new(vec![]), value: String::new(), @@ -517,10 +456,9 @@ pub fn default_registry() -> CmdRegistry { opened: None, }) }, - commit_from_cursor, Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)), ); - r.register( + r.register_at_cursor( &CommitAndAdvance { key: CellKey::new(vec![]), value: String::new(), @@ -530,7 +468,6 @@ pub fn default_registry() -> CmdRegistry { opened: None, }, |_| Err("commit-and-advance-right requires context".into()), - commit_from_cursor, Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); r.register( @@ -542,7 +479,6 @@ pub fn default_registry() -> CmdRegistry { let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; Ok(CommitBuffer { kind }) }, - as_parsed, Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; @@ -867,12 +803,16 @@ mod panel_open_tests { let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); for asked in Panel::ALL { let args = [asked.name().to_string()]; - let cmd = ctx + let effects = ctx .registry .interactive("toggle-panel-visibility", &args, &ctx) - .expect("toggle-panel-visibility resolves"); - let dbg = format!("{cmd:?}"); - let expected = format!("currently_open: {}", asked == opened); + .expect("toggle-panel-visibility resolves") + .execute(&ctx); + let dbg = format!("{effects:?}"); + let expected = format!( + "SetPanelOpen {{ panel: {asked:?}, open: {} }}", + asked != opened + ); assert!( dbg.contains(&expected), "with only {opened:?} open, toggling {asked:?} read another \ diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 2a4127b2..c98fe66a 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -2991,6 +2991,43 @@ mod tests { ); } + /// improvise-cz8d: a refusal need not abort, so a failed document swap must + /// ask for the abort itself. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_failed_load_or_import_does_not_run_the_quit_after_it() { + use improvise_protocol::keymap::Keymap; + + for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] { + let mut app = App::new(Workbook::new("T"), None); + app.storage = Box::new(crate::storage::MemoryFs::new()); + let key = KeyCode::Char('x'); + let mods = KeyModifiers::NONE; + let mut km = Keymap::new(); + km.bind_seq( + key, + mods, + vec![(command, vec![path.to_string()]), ("force-quit", vec![])], + ); + let effects = { + let ctx = app.cmd_context(key, mods); + app.keymap_set.dispatch_transient(&km, &ctx, key, mods) + } + .expect("both steps resolve in the registry"); + app.apply_effects(effects); + + assert!( + app.last_batch_refused(), + "setup: `{command}` must actually have failed, got {:?}", + app.view_state.status_msg + ); + assert_ne!( + app.view_state.mode, + AppMode::Quit, + "`{command} {path} . q` quit having loaded nothing" + ); + } + } + /// A prefix is consumed whether or not the next key matches, so a stray key /// after `g` does nothing rather than acting as a fresh Normal binding. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index e1493995..3c6bde18 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -44,6 +44,31 @@ pub enum ViewTarget { Named(String), } +/// Refuse AND stop the batch, for a failed document swap: what follows it in +/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d). +fn refuse_and_stop(app: &mut App, why: impl Into) { + app.refuse(why); + app.abort_effects = true; +} + +/// Write the view a geometry effect names, or refuse: a view the document no +/// longer has is a failure to report, not a no-op to swallow (improvise-bok4). +fn on_view(app: &mut App, target: &ViewTarget, write: impl FnOnce(&mut crate::view::View)) { + let missing = match app.view_mut_of(target) { + Some(view) => { + write(view); + None + } + None => Some(match target { + ViewTarget::Named(name) => format!("No view named \"{name}\""), + ViewTarget::Active => "No active view".to_string(), + }), + }; + if let Some(why) = missing { + app.refuse(why); + } +} + /// A discrete state change produced by a command. /// Effects know how to apply themselves to the App. pub trait Effect: Debug { @@ -183,11 +208,13 @@ impl Effect for AddFormula { // grid; _Measure targets arrive via `measure_item_names()`. if !crate::model::category::CategoryKind::of_name(&formula.target_category) .is_measure() - { - let _ = app + && let Err(e) = app .model_state .workbook - .add_item(&formula.target_category, &formula.target); + .add_item(&formula.target_category, &formula.target) + { + // Its category may be gone: store no formula for it (40j4). + return app.refuse(e.to_string()); } app.model_state.workbook.model.add_formula(formula); } @@ -255,9 +282,9 @@ impl Effect for EnterEditAtCursor { pub struct TogglePruneEmpty(pub ViewTarget); impl Effect for TogglePruneEmpty { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.prune_empty = !v.prune_empty; - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -321,7 +348,14 @@ impl Effect for CreateView { pub struct DeleteView(pub String); impl Effect for DeleteView { fn apply(&self, app: &mut App) { - let _ = app.model_state.workbook.delete_view(&self.0); + // `delete_view` answers Ok for a name it lacks, which is a failure to + // report, not a deletion (improvise-40j4). + if !app.model_state.workbook.views.contains_key(&self.0) { + return app.refuse(format!("No view named \"{}\"", self.0)); + } + if let Err(e) = app.model_state.workbook.delete_view(&self.0) { + return app.refuse(e.to_string()); + } // This client may have been the one on it. Idempotent for every other // view; the mirrors owe the same move (`Sessions::adopt_deleted_views`). app.view_state.active_view = app.view_name().to_string(); @@ -405,9 +439,9 @@ pub struct SetAxis { } impl Effect for SetAxis { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.set_axis(&self.category, self.axis); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -422,9 +456,9 @@ pub struct SetPageSelection { } impl Effect for SetPageSelection { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.set_page_selection(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -439,9 +473,9 @@ pub struct ToggleGroup { } impl Effect for ToggleGroup { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.toggle_group_collapse(&self.category, &self.group); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -456,9 +490,9 @@ pub struct HideItem { } impl Effect for HideItem { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.hide_item(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -473,9 +507,9 @@ pub struct ShowItem { } impl Effect for ShowItem { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.view) { + on_view(app, &self.view, |v| { v.show_item(&self.category, &self.item); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -486,9 +520,9 @@ impl Effect for ShowItem { pub struct TransposeAxes(pub ViewTarget); impl Effect for TransposeAxes { fn apply(&self, app: &mut App) { - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.transpose_axes(); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -500,9 +534,9 @@ pub struct CycleAxis(pub ViewTarget, pub String); impl Effect for CycleAxis { fn apply(&self, app: &mut App) { let category = self.1.clone(); - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.cycle_axis(&category); - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -514,9 +548,9 @@ pub struct SetNumberFormat(pub ViewTarget, pub String); impl Effect for SetNumberFormat { fn apply(&self, app: &mut App) { let format = self.1.clone(); - if let Some(v) = app.view_mut_of(&self.0) { + on_view(app, &self.0, |v| { v.number_format = format; - } + }); } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -939,9 +973,7 @@ impl Effect for LoadModel { app.install_workbook(loaded); app.view_state.status_msg = format!("Loaded from {}", self.0.display()); } - Err(e) => { - app.refuse(format!("Load error: {e}")); - } + Err(e) => refuse_and_stop(app, format!("Load error: {e}")), } } fn kind(&self) -> EffectKind { @@ -998,7 +1030,7 @@ impl Effect for ImportJsonHeadless { let bytes = match app.storage.read(&self.path) { Ok(b) => b, Err(e) => { - app.refuse(format!("Cannot read '{}': {e}", self.path.display())); + refuse_and_stop(app, format!("Cannot read '{}': {e}", self.path.display())); return; } }; @@ -1009,7 +1041,7 @@ impl Effect for ImportJsonHeadless { (recs, raw) } Err(e) => { - app.refuse(format!("CSV error: {e}")); + refuse_and_stop(app, format!("CSV error: {e}")); return; } } @@ -1017,21 +1049,21 @@ impl Effect for ImportJsonHeadless { let content = match app.storage.read(&self.path) { Ok(c) => c, Err(e) => { - app.refuse(format!("Cannot read '{}': {e}", self.path.display())); + refuse_and_stop(app, format!("Cannot read '{}': {e}", self.path.display())); return; } }; let value: serde_json::Value = match serde_json::from_slice(&content) { Ok(v) => v, Err(e) => { - app.refuse(format!("JSON parse error: {e}")); + refuse_and_stop(app, format!("JSON parse error: {e}")); return; } }; match json_import_records(&value, self.array_path.as_deref()) { Ok(recs) => (recs, value), Err(msg) => { - app.refuse(msg); + refuse_and_stop(app, msg); return; } } @@ -1070,7 +1102,7 @@ impl Effect for ImportJsonHeadless { }; } Err(e) => { - app.refuse(format!("Import error: {e}")); + refuse_and_stop(app, format!("Import error: {e}")); } } } diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 166f16e0..f31b1d08 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -166,11 +166,12 @@ impl Host { if let Some(reason) = reduce::broken_chain(&self.app, cmd.command()) { return self.undo_frame(id, undo, mirrors, reason); } - // Only a mutation runs `apply_effects`, and only `apply_effects` - // resets the flag — so after anything else it is a STALE answer. + // Only a mutation runs `apply_effects`, which resets both flags — so + // after anything else they are STALE. A refusal need not abort (cz8d). let mutation = matches!(cmd.command(), Command::Model { .. }); out.extend(self.command(id, cmd)); - if mutation && self.app.last_batch_aborted() { + let failed = self.app.last_batch_refused() || self.app.last_batch_aborted(); + if mutation && failed { return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); } } @@ -1541,10 +1542,12 @@ mod tests { .any(|f| matches!(f.msg, ServerFrame::Snapshot(_))), "a failed load cost every client its cursors: {frames:?}" ); - assert!( - host.app().view_state.status_msg.starts_with("Load error"), - "the fixture did not actually fail the load: {:?}", - host.app().view_state.status_msg + // Since improvise-bok4 a failed load is REFUSED, so the refusal is the + // witness that it failed; the host's status line is rolled back with it. + assert_eq!( + to(&frames, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "the fixture did not actually fail the load" ); } @@ -1778,4 +1781,283 @@ mod tests { let json = serde_json::to_string(&inbound).unwrap(); assert_eq!(serde_json::from_str::(&json).unwrap(), inbound); } + + /// Two model commands, each caught up against the document as the client + /// saw it, so no basis tier can refuse the frame first (improvise-bok4). + fn two_command_frame(host: &Host, first: ModelCommand, then: ModelCommand) -> ClientFrame { + ClientFrame::Commands(vec![ + crate::ui::reduce::caught_up(host.app(), first), + crate::ui::reduce::caught_up(host.app(), then), + ]) + } + + /// improvise-bok4, the view-addressed case: the second command's view is + /// gone. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_frame_writing_a_view_its_own_first_command_deleted_is_refused() { + let mut host = host(); + host.app_mut().model_state.workbook.create_view("Scratch"); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::DeleteView("Scratch".into()), + ModelCommand::SetNumberFormat { + view: "Scratch".into(), + format: ",.2f".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "half the frame did nothing and was reported accepted: {answer:?}" + ); + assert!( + host.app() + .model_state + .workbook + .views + .contains_key("Scratch"), + "the refused frame's deletion must be undone" + ); + } + + /// improvise-bok4, the category-addressed case: it narrated its failure into + /// the HOST's status line and told the client nothing. + #[test] + fn a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::RemoveCategory("Region".into()), + ModelCommand::AddItem { + category: "Region".into(), + item: "East".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "half the frame did nothing and was reported accepted: {answer:?}" + ); + assert!( + host.app() + .model_state + .workbook + .model + .category("Region") + .is_some(), + "the refused frame's removal must be undone" + ); + } + + /// The one-command case of the same class: deleting the last view was + /// swallowed with `let _`, so the frame was accepted having done nothing. + #[test] + fn deleting_the_documents_last_view_is_refused_rather_than_accepted() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + let only = host.app().view_name().to_string(); + + let answer = host.model(asker, ModelCommand::DeleteView(only.clone())); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "a deletion the workbook refused was reported accepted: {answer:?}" + ); + assert!(host.app().model_state.workbook.views.contains_key(&only)); + } + + /// The control: two commands that both land are accepted, so the refusals + /// above are about a failure and not about the frame's shape. + #[test] + fn a_frame_whose_commands_all_land_is_accepted() { + let mut host = host(); + host.app_mut().model_state.workbook.create_view("Scratch"); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::SetNumberFormat { + view: "Scratch".into(), + format: ",.2f".into(), + }, + ModelCommand::AddItem { + category: "Region".into(), + item: "East".into(), + }, + ), + ); + + assert!( + to(&answer, asker).contains(&ServerFrame::Accepted), + "a frame with nothing to refuse was refused: {answer:?}" + ); + } + + /// improvise-40j4: the formula was stored for a category the frame's own + /// first command removed, and it survived save and reload. + #[test] + fn a_frame_adding_a_formula_to_a_category_its_first_command_removed_is_refused() { + let mut host = host(); + let (asker, _) = host.connect(10, 10); + + let answer = host.handle( + asker, + two_command_frame( + &host, + ModelCommand::RemoveCategory("Region".into()), + ModelCommand::AddFormula { + raw: "Total = 1".into(), + target_category: "Region".into(), + }, + ), + ); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "a formula for a vanished category was accepted: {answer:?}" + ); + let model = &host.app().model_state.workbook.model; + assert!( + model.category("Region").is_some(), + "the removal must be undone" + ); + assert!( + model.formulas().iter().all(|f| f.target != "Total"), + "and the formula must not be stored" + ); + } + + /// A host whose storage holds one file per way `ImportJsonHeadless` can + /// fail, plus nothing at the two paths that must fail to read. + fn host_with_import_fixtures() -> Host { + let storage = MemoryFs::new(); + let files: [(&str, &[u8]); 4] = [ + // Every field numeric, so no category is accepted and the build fails. + ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), + ("bad.json", b"{not json"), + ("nested.json", br#"{"data": [{"Region": "North"}]}"#), + // A record that is not UTF-8 is a CSV read error, not a warning. + ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), + ]; + for (name, bytes) in files { + crate::storage::Storage::write(&storage, Path::new(name), bytes) + .expect("fixture writes"); + } + // A second view, so deleting an absent one is not refused as "the last". + let mut wb = workbook(); + wb.create_view("Scratch"); + let mut app = App::new(wb, None); + app.storage = Box::new(storage); + app.rebuild_layout(); + Host::new(app) + } + + fn import(path: &str, array_path: Option<&str>) -> ModelCommand { + ModelCommand::ImportJsonHeadless { + path: PathBuf::from(path), + model_name: None, + array_path: array_path.map(String::from), + } + } + + /// improvise-9fpl: one case per refusal #181 converted that no other test + /// witnesses. Evidence: `context/test-evidence/command.md`. + #[test] + fn every_failing_model_command_refuses_its_whole_frame() { + let cases: Vec<(&str, ModelCommand)> = vec![ + ( + "AddItemInGroup into an absent category", + ModelCommand::AddItemInGroup { + category: "Ghost".into(), + item: "East".into(), + group: "Coastal".into(), + }, + ), + ( + "AddFormula that does not parse", + ModelCommand::AddFormula { + raw: "Total = (".into(), + target_category: "_Measure".into(), + }, + ), + ( + "ImportJsonHeadless of an absent JSON", + import("absent.json", None), + ), + ( + "ImportJsonHeadless of an absent CSV", + import("absent.csv", None), + ), + ( + "ImportJsonHeadless of a CSV that is not text", + import("bad.csv", None), + ), + ( + "ImportJsonHeadless of JSON that does not parse", + import("bad.json", None), + ), + ( + "ImportJsonHeadless at a path with no array", + import("nested.json", Some("nope")), + ), + ( + "ImportJsonHeadless that builds no model", + import("measures.json", None), + ), + // improvise-40j4: `delete_view` answers Ok for a name it lacks. + ( + "DeleteView of a view the document lacks", + ModelCommand::DeleteView("Ghost".into()), + ), + ( + "AddFormula into a category the document lacks", + ModelCommand::AddFormula { + raw: "Total = 1".into(), + target_category: "Ghost".into(), + }, + ), + ]; + for (what, failing) in cases { + let mut host = host_with_import_fixtures(); + let (asker, _) = host.connect(10, 10); + let landed = ModelCommand::AddItem { + category: "Region".into(), + item: "Landed".into(), + }; + + let answer = host.handle(asker, two_command_frame(&host, landed, failing)); + + assert_eq!( + to(&answer, asker), + vec![ServerFrame::Rejected(Rejection::MutationRefused)], + "{what}: the frame was not refused: {answer:?}" + ); + assert!( + !host + .app() + .model_state + .workbook + .model + .category("Region") + .is_some_and(|c| c.ordered_item_names().contains(&"Landed")), + "{what}: the half of the frame that landed was not undone" + ); + } + } } diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 2a433d94..ff23ca77 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -222,7 +222,6 @@ fn axis_frame( pending.clear(); } pending.push(FrameEntry::Group(FrameGroup { - cat: cat_name.clone(), name: group_name.clone(), collapsed: view.is_group_collapsed(cat_name, group_name), })); @@ -324,7 +323,7 @@ fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option return Some(CellId::Create(CreateKind::Category)), // Renaming a coordinate moves every measure cell in the row, which is a // different intent from `SetCell` and still the host's (gotcha 1). - RecordCol::Category(_) => return None, + RecordCol::Category(_) => return Some(CellId::Coordinate), }; return Some(CellId::Record(RecordCell { record: RecordHash::of(record), @@ -493,7 +492,10 @@ pub fn published_basis(app: &App, grid: &GridCache) -> Basis { .flatten() .filter_map(|cell| match cell.id.as_ref()? { CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, }) .collect(); basis::published(app.document_generation(), app.layout_version(), records) @@ -4853,20 +4855,6 @@ mod cell_identity { ); } - /// A coordinate column is not a cell to write into (gotcha 1), so it is projected - /// with no id and a client cannot compose a write against it. - #[test] - fn a_coordinate_column_is_projected_with_no_identity() { - let app = records_app(); - let region = column_named(&app, "Region"); - assert_eq!( - grid_of(&app, WHOLE) - .cell(0, region) - .and_then(|c| c.id.clone()), - None - ); - } - /// A document where `Profit` is BOTH an explicit `_Measure` item and a /// formula target — which the `+` column lets a user build. fn shadowed_measure_records_app() -> App { @@ -5138,10 +5126,9 @@ mod cell_identity { let ids: Vec> = (0..app.layout.col_count()) .map(|c| grid.cell(0, c).and_then(|cell| cell.id.clone())) .collect(); - assert_eq!( - ids.iter().filter(|id| id.is_none()).count(), - 1, - "only the one Region coordinate column may be nameless: {ids:?}" + assert!( + ids.iter().all(Option::is_some), + "no records column is nameless since improvise-exdg: {ids:?}" ); assert_eq!( ids.iter() @@ -5162,6 +5149,30 @@ mod cell_identity { ); } + /// improvise-exdg: a coordinate cell is drawn and holds text, so it is not + /// "no cell". `context/test-evidence/command.md`. + #[test] + fn a_coordinate_cell_is_named_rather_than_left_as_no_cell() { + let app = records_app(); + let grid = grid_of(&app, WHOLE); + let region = column_named(&app, "Region"); + let north = grid.cell(1, region).expect("the second row's coordinate"); + assert_eq!(north.text, "North", "setup: a visible coordinate cell"); + assert_eq!( + north.id, + Some(CellId::Coordinate), + "a drawn coordinate is named" + ); + let nameless: Vec<(usize, usize)> = (0..app.layout.row_count()) + .flat_map(|r| (0..app.layout.col_count()).map(move |c| (r, c))) + .filter(|&(r, c)| grid.cell(r, c).is_some_and(|cell| cell.id.is_none())) + .collect(); + assert!( + nameless.is_empty(), + "every records cell is something: {nameless:?}" + ); + } + /// The version counter is keyed by the surrogate the client holds, so one /// identity answers ADR 002's question and ADR 001's. #[test] diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 647e9bb1..6397136e 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -55,6 +55,9 @@ pub enum CellId { /// A records-mode creating column: typing a **name** into it adds that /// measure or category and writes no cell at all (gotcha 1, improvise-poia). Create(CreateKind), + /// A records-mode coordinate. Renaming it moves every measure cell in the + /// row (gotcha 1), which no client composes: `protocol.md` §3 (improvise-exdg). + Coordinate, } /// What a creating column creates. The name typed into it is the whole @@ -194,9 +197,6 @@ pub const MAX_ROW_HEADER_W: u16 = 24; /// because it is a fact about the *view*, which only the server holds. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct FrameGroup { - /// The category the group belongs to, echoed back in `ToggleGroup` so a - /// client never works out which category a label came from. - pub cat: String, pub name: String, pub collapsed: bool, } @@ -1137,7 +1137,6 @@ mod tests { FrameEntry::Item(vec!["A".into(), "x".into()]), FrameEntry::Item(vec!["A".into(), "y".into()]), FrameEntry::Group(FrameGroup { - cat: "Type".into(), name: "Late".into(), collapsed: true, }), @@ -1150,7 +1149,6 @@ mod tests { /// ungrouped between them — improvise-h0iv's reachable shape. fn repeated_group_frame() -> AxisFrame { let q1 = || FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }; @@ -1176,7 +1174,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }), @@ -1214,7 +1211,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: true, }), @@ -1266,7 +1262,7 @@ mod tests { /// The pair `FrameGroup` carries cannot separate them — h0iv's own proposed /// fix. See `context/test-evidence/protocol.md`. #[test] - fn the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them() { + fn the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them() { let frame = repeated_group_frame(); let headers: Vec<&FrameGroup> = frame .entries @@ -1277,10 +1273,9 @@ mod tests { }) .collect(); assert_eq!( - (&headers[0].cat, &headers[0].name), - (&headers[1].cat, &headers[1].name), - "both headers name the same category AND the same group, so the \ - ticket's (cat, name) comparison would still merge them" + headers[0], headers[1], + "both headers are the same group in the same state, so no \ + comparison of their fields can split them" ); } @@ -1344,7 +1339,7 @@ mod tests { assert_eq!(frame.item(2), Some(&vec!["B".to_string(), "z".to_string()])); assert!(frame.group_of(0).is_none(), "[A,x] is in no group"); let group = frame.group_of(2).expect("[B,z] sits under Late"); - assert_eq!((group.cat.as_str(), group.name.as_str()), ("Type", "Late")); + assert_eq!(group.name, "Late"); assert!(group.collapsed, "the collapse state travels with the group"); } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 1948ef14..7ed73ee8 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -755,6 +755,7 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { // Both creating columns are records-only, so the guard above takes // them; declining rather than `None` keeps that a door, not a hole. CellId::Create(_) => Some(vec![Command::SetStatus(not_yet("creating a column"))]), + CellId::Coordinate => Some(vec![Command::SetStatus(not_yet("renaming a coordinate"))]), } } @@ -966,7 +967,9 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option fn record_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.record), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } @@ -1046,7 +1049,9 @@ fn model(state: &ClientState, cmd: ModelCommand) -> Option { fn record_cell_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.clone()), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 419705e8..f002c958 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -11,28 +11,28 @@ pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> { for root in roots { collect_rust(root, &mut files); } - let code: Vec<(PathBuf, String)> = files + let read: Vec<(PathBuf, Source)> = files .into_iter() .map(|file| { let text = std::fs::read_to_string(&file).expect("a listed source reads"); - let code = code_only(&text); - (file, code) + let source = Source::of(&text, &file); + (file, source) }) .collect(); - let test_modules: BTreeSet = code + let test_modules: BTreeSet = read .iter() - .flat_map(|(file, code)| test_module_paths(file, code)) + .flat_map(|(file, source)| test_module_paths(file, source)) .collect(); - code.into_iter() + read.into_iter() .filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file))) - .map(|(file, code)| (file, strip_test_items(&code))) + .map(|(file, source)| (file, source.production())) .collect() } /// One file's text as [`production_code`] cleans it. Which *files* are test /// modules needs their declaring file too, so only `production_code` knows. pub fn production_text(text: &str) -> String { - strip_test_items(&code_only(text)) + Source::of(text, Path::new("")).production() } fn collect_rust(dir: &Path, into: &mut Vec) { @@ -55,83 +55,319 @@ fn is_module_file(m: &Path, file: &Path) -> bool { file == m.with_extension("rs") || file.starts_with(m) } +/// One file, read once. Its `cfg` predicates are read off the TEXT, whose +/// `feature` values are literals; every bracket off the code (improvise-59q8). +struct Source { + code: Vec, + /// Test-only items, as `(attribute start, item start, item end)`. + items: Vec<(usize, usize, usize)>, + /// A leading `#![cfg(test)]`: the whole file is test code. + whole_file: bool, +} + +impl Source { + fn of(text: &str, file: &Path) -> Self { + let code: Vec = code_only(text).chars().collect(); + let text: Vec = text.chars().collect(); + let (items, whole_file) = test_only_items(&text, &code, file); + Self { + code, + items, + whole_file, + } + } + + /// The code with every test-only item removed. + fn production(&self) -> String { + if self.whole_file { + return String::new(); + } + let mut kept = String::with_capacity(self.code.len()); + let mut at = 0; + for &(start, _, end) in &self.items { + kept.extend(&self.code[at..start]); + at = end; + } + kept.extend(&self.code[at..]); + kept + } + + fn item_code(&self, start: usize, end: usize) -> String { + self.code[start..end].iter().collect() + } +} + /// The out-of-line modules `file` declares under a test-only `cfg`, as paths /// without extension (`#[cfg(test)] mod x;` is `/x`). -fn test_module_paths(file: &Path, code: &str) -> Vec { +fn test_module_paths(file: &Path, source: &Source) -> Vec { let dir = match file.file_stem().and_then(|s| s.to_str()) { Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf), Some(stem) => file.parent().map(|p| p.join(stem)), None => None, } .expect("a source file has a directory and a stem"); - let lines: Vec<&str> = code.lines().map(str::trim).collect(); - let mut found = Vec::new(); - for (i, line) in lines.iter().enumerate() { - if !is_test_cfg(line) { - continue; - } - let item = lines[i + 1..] - .iter() - .find(|l| !l.starts_with("#[") && !l.is_empty()); - if let Some(name) = item.and_then(|l| out_of_line_mod(l)) { - found.push(dir.join(name)); - } - } - found + source + .items + .iter() + .filter_map(|&(_, start, end)| out_of_line_mod(source.item_code(start, end).trim())) + .map(|name| dir.join(name)) + .collect() } /// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`. -fn out_of_line_mod(line: &str) -> Option<&str> { - let rest = line.split_once("mod ")?.1.strip_suffix(';')?; - let prefix = line.split_once("mod ")?.0.trim(); - (prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim()) +fn out_of_line_mod(item: &str) -> Option { + let (prefix, rest) = item.split_once("mod ")?; + let name = rest.strip_suffix(';')?.trim(); + let prefix = prefix.trim(); + (prefix.is_empty() || prefix.starts_with("pub")).then(|| name.to_string()) } -/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))` -/// is production, and `"test-support"` is a literal, already blanked. -fn is_test_cfg(line: &str) -> bool { - let line = line.trim(); - line.starts_with("#[cfg(") - && !line.contains("not(") - && line - .split(|c: char| !c.is_alphanumeric() && c != '_') - .any(|word| word == "test") +/// Every test-only item in a file, bounded by balanced brackets on the code +/// rather than by indentation. An item it cannot bound is a panic, never EOF. +fn test_only_items( + text: &[char], + code: &[char], + file: &Path, +) -> (Vec<(usize, usize, usize)>, bool) { + let mut items = Vec::new(); + let mut k = 0; + while k < code.len() { + let Some((inner, end)) = attribute_at(code, k) else { + k += 1; + continue; + }; + let predicate: String = text[k..end].iter().collect(); + if !is_test_only(&predicate) { + k = end; + continue; + } + if inner { + // An inner attribute heads the file or it heads an inline module; + // only the first is a shape this scan can honour. + let leading = code[..k].iter().all(|c| c.is_whitespace()); + assert!( + leading, + "{}: an inner test-only cfg could not be bounded", + file.display() + ); + return (Vec::new(), true); + } + let start = past_attributes(code, end); + let item_end = item_end(code, start).unwrap_or_else(|| { + panic!( + "{}: a test-only item at char {k} could not be bounded", + file.display() + ) + }); + items.push((k, start, item_end)); + k = item_end; + } + (items, false) } -/// Drop every test-only item. rustfmt puts an item's closing brace at the -/// attribute's own indentation, which is what makes this a line scan (§2). -fn strip_test_items(text: &str) -> String { - let lines: Vec<&str> = text.lines().collect(); - let mut kept = String::new(); - let mut i = 0; - while i < lines.len() { - let line = lines[i]; - match is_test_cfg(line) { - false => { - kept.push_str(line); - kept.push('\n'); - i += 1; - } - true => { - let indent = line.len() - line.trim_start().len(); - let close = format!("{}}}", " ".repeat(indent)); - i += 1; - while i < lines.len() && lines[i].trim_start().starts_with("#[") { - i += 1; - } - match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) { - true => i += 1, - false => { - while i < lines.len() && lines[i].trim_end() != close { - i += 1; - } - i += 1; - } +/// An attribute starting at `k`: whether it is inner (`#!`) and where it ends. +fn attribute_at(code: &[char], k: usize) -> Option<(bool, usize)> { + if code[k] != '#' || (k > 0 && (code[k - 1].is_alphanumeric() || code[k - 1] == '_')) { + return None; + } + let inner = code.get(k + 1) == Some(&'!'); + let open = k + 1 + usize::from(inner); + (code.get(open) == Some(&'[')).then(|| matching(code, open).map(|close| (inner, close + 1)))? +} + +/// The index of the bracket closing the one at `open`, counting all three kinds. +fn matching(code: &[char], open: usize) -> Option { + let mut depth = 0usize; + for (k, &c) in code.iter().enumerate().skip(open) { + match c { + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => { + depth -= 1; + if depth == 0 { + return Some(k); } } + _ => {} } } - kept + None +} + +/// Skip whitespace and any further outer attributes on the same item. +fn past_attributes(code: &[char], mut k: usize) -> usize { + loop { + while code.get(k).is_some_and(|c| c.is_whitespace()) { + k += 1; + } + let next = (k < code.len()).then(|| attribute_at(code, k)).flatten(); + match next { + Some((false, end)) => k = end, + _ => return k, + } + } +} + +/// Where the item starting at `start` ends: a `;` at depth zero, or the `}` +/// closing its body. `None` at the end of the file, which is not an end. +fn item_end(code: &[char], start: usize) -> Option { + let mut depth = 0usize; + let mut k = start; + while k < code.len() { + match code[k] { + '(' | '[' | '{' => depth += 1, + ';' if depth == 0 => return Some(k + 1), + // The enclosing block closed first: the item was its last expression. + ')' | ']' | '}' if depth == 0 => return Some(k), + ')' | ']' => depth -= 1, + '}' => { + depth -= 1; + if depth == 0 && !continues(code, k + 1) { + return Some(k + 1); + } + } + _ => {} + } + k += 1; + } + None +} + +/// Whether the item goes on after a `}` at depth zero: `S { .. };`, a method +/// call on a block, or an `if` with an `else`. +fn continues(code: &[char], after: usize) -> bool { + let rest: String = code[after..] + .iter() + .skip_while(|c| c.is_whitespace()) + .take(5) + .collect(); + rest.starts_with([';', '.', '?']) || rest.starts_with("else") +} + +/// A `cfg` attribute whose predicate no shipped build satisfies. Evaluated, +/// not substring-matched (improvise-59q8): `context/code/protocol.md` §2. +fn is_test_only(attribute: &str) -> bool { + let tokens = cfg_tokens(attribute); + match tokens.as_slice() { + [Token::Word(cfg), Token::Open, rest @ ..] if cfg == "cfg" => { + let mut at = 0; + predicate(rest, &mut at) == Truth::No + } + _ => false, + } +} + +/// What a `cfg` predicate is in a build without `test`. `Unknown` for what a +/// source scan cannot know, such as a target or an ordinary feature. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Truth { + Yes, + No, + Unknown, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum Token { + Word(String), + Str(String), + Open, + Close, + Comma, + Equals, +} + +/// The tokens of an attribute's text, from its first word: `#`, `!` and the +/// brackets around it are dropped. +fn cfg_tokens(attribute: &str) -> Vec { + let inner = attribute + .trim_start_matches('#') + .trim_start_matches('!') + .trim() + .trim_start_matches('[') + .trim_end() + .trim_end_matches(']'); + let chars: Vec = inner.chars().collect(); + let mut tokens = Vec::new(); + let mut k = 0; + while k < chars.len() { + let c = chars[k]; + match c { + '(' => tokens.push(Token::Open), + ')' => tokens.push(Token::Close), + ',' => tokens.push(Token::Comma), + '=' => tokens.push(Token::Equals), + '"' => { + let end = quoted_end(&chars, k + 1, '"'); + tokens.push(Token::Str(chars[k + 1..end - 1].iter().collect())); + k = end; + continue; + } + c if c.is_alphanumeric() || c == '_' => { + let word: String = chars[k..] + .iter() + .take_while(|c| c.is_alphanumeric() || **c == '_') + .collect(); + k += word.chars().count(); + tokens.push(Token::Word(word)); + continue; + } + _ => {} + } + k += 1; + } + tokens +} + +/// One predicate, advancing `at` past it. `test` is false; so is the +/// `test-support` feature, which this repo never ships (improvise-26dm). +fn predicate(tokens: &[Token], at: &mut usize) -> Truth { + let Some(Token::Word(word)) = tokens.get(*at) else { + return Truth::Unknown; + }; + *at += 1; + match (word.as_str(), tokens.get(*at)) { + (op @ ("all" | "any" | "not"), Some(Token::Open)) => { + *at += 1; + let mut args = Vec::new(); + while !matches!(tokens.get(*at), Some(Token::Close) | None) { + args.push(predicate(tokens, at)); + if tokens.get(*at) == Some(&Token::Comma) { + *at += 1; + } + } + *at += 1; + combine(op, &args) + } + (key, Some(Token::Equals)) => { + *at += 1; + let value = match tokens.get(*at) { + Some(Token::Str(v)) => v.clone(), + _ => String::new(), + }; + *at += 1; + match (key, value.as_str()) { + ("feature", "test-support") => Truth::No, + _ => Truth::Unknown, + } + } + ("test", _) => Truth::No, + _ => Truth::Unknown, + } +} + +/// Kleene's three-valued `all`, `any` and `not`. +fn combine(op: &str, args: &[Truth]) -> Truth { + match op { + "all" if args.contains(&Truth::No) => Truth::No, + "all" if args.iter().all(|a| *a == Truth::Yes) => Truth::Yes, + "any" if args.contains(&Truth::Yes) => Truth::Yes, + "any" if args.iter().all(|a| *a == Truth::No) => Truth::No, + "not" => match args.first() { + Some(Truth::Yes) => Truth::No, + Some(Truth::No) => Truth::Yes, + _ => Truth::Unknown, + }, + _ => Truth::Unknown, + } } /// `text` with comments, string, byte-string, raw-string and char literals @@ -166,8 +402,8 @@ fn literal_or_comment_end(c: &[char], i: usize) -> Option { (Some('/'), Some('/')) => Some(line_end(c, i)), (Some('/'), Some('*')) => Some(block_end(c, i)), (Some('"'), _) => Some(quoted_end(c, i + 1, '"')), - (Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')), - (Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i), + (Some('b' | 'c'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')), + (Some('r' | 'b' | 'c'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i), (Some('\''), _) => char_end(c, i), _ => None, } @@ -211,9 +447,10 @@ fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize { c.len() } -/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier. +/// `r"…"`, `r#"…"#`, `br#"…"#`, `cr#"…"#`. `None` when the `r` is only an +/// identifier. fn raw_end(c: &[char], i: usize) -> Option { - let mut k = i + usize::from(c[i] == 'b'); + let mut k = i + usize::from(matches!(c[i], 'b' | 'c')); if c.get(k) != Some(&'r') { return None; } @@ -283,21 +520,20 @@ mod tests { "}\n", "#[cfg(test)]\n", "mod out_of_line;\n", - "#[cfg(any(test, feature = \"x\"))]\n", + "#[cfg(any(test, feature = \"test-support\"))]\n", "pub mod gated;\n", "#[cfg(not(test))]\n", "fn production_only() {}\n", "fn also_kept() {}\n", ); - let code = code_only(text); - let stripped = strip_test_items(&code); + let stripped = production_text(text); for kept in ["fn kept", "fn production_only", "fn also_kept"] { assert!(stripped.contains(kept), "{kept} was dropped: {stripped}"); } assert!(!stripped.contains("gone_inline"), "{stripped}"); let lib = Path::new("/c/src/lib.rs"); assert_eq!( - test_module_paths(lib, &code), + test_module_paths(lib, &Source::of(text, lib)), vec![ PathBuf::from("/c/src/out_of_line"), PathBuf::from("/c/src/gated") @@ -305,7 +541,7 @@ mod tests { ); let nested = Path::new("/c/src/ui/reduce.rs"); assert_eq!( - test_module_paths(nested, &code)[0], + test_module_paths(nested, &Source::of(text, nested))[0], PathBuf::from("/c/src/ui/reduce/out_of_line") ); assert!(is_module_file( @@ -322,6 +558,112 @@ mod tests { )); } + /// improvise-59q8, the shapes that HID production code: a test-only item + /// whose end the line scan could not find. Evidence: test-evidence/protocol.md. + #[test] + fn a_test_only_item_hides_nothing_after_it() { + let cases = [ + ( + "a multi-line item ending in `];`", + concat!( + "impl X {\n", + " #[cfg(test)]\n", + " const NAMES: &[&str] = &[\n", + " \"a\",\n", + " ];\n", + " fn prod() { let c = Command::Hidden1(1); }\n", + "}\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ( + "a multi-line statement ending in `);`", + concat!( + "fn f() {\n", + " #[cfg(test)]\n", + " let x = foo(\n", + " 1,\n", + " );\n", + " let c = Command::Hidden1(1);\n", + "}\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ( + "a one-line `#[cfg(test)] use`", + concat!( + "#[cfg(test)] use foo::bar;\n", + "fn prod() { Command::Hidden1(1) }\n", + "fn after() { Command::Visible(2) }\n", + ), + ), + ]; + for (what, text) in cases { + let kept = production_text(text); + for name in ["Hidden1", "Visible"] { + assert!(kept.contains(name), "{what}: {name} was hidden: {kept:?}"); + } + } + } + + /// improvise-59q8, the shapes that COUNTED test code as production. + #[test] + fn a_test_only_cfg_is_recognised_whatever_its_spelling() { + let cases = [ + ( + "`all(test, not(..))`", + "#[cfg(all(test, not(target_arch = \"wasm32\")))]\nfn t() { Command::TestOnly(1) }\n", + ), + ( + "an inner `#![cfg(test)]` heading the file", + "#![cfg(test)]\nfn t() { Command::TestOnly(1) }\n", + ), + ( + "an attribute split across lines", + "#[cfg(\n test\n)]\nfn t() { Command::TestOnly(1) }\n", + ), + ]; + for (what, text) in cases { + let kept = production_text(text); + assert!( + !kept.contains("TestOnly"), + "{what} was read as production: {kept:?}" + ); + } + // The controls: a predicate a shipped build can satisfy is production. + for text in [ + "#[cfg(not(test))]\nfn p() { Command::Shipped(1) }\n", + "#[cfg(any(test, feature = \"serde\"))]\nfn p() { Command::Shipped(1) }\n", + "#[cfg(target_arch = \"wasm32\")]\nfn p() { Command::Shipped(1) }\n", + ] { + assert!( + production_text(text).contains("Shipped"), + "{text:?} was dropped" + ); + } + } + + /// improvise-59q8's minor case: a C string literal leaked its contents. + #[test] + fn a_c_string_literal_is_blanked() { + // The raw one quotes inside itself, so lexing it as ordinary strings + // leaves the text BETWEEN the inner quotes in the open. + let kept = code_only( + "let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n", + ); + for gone in ["InC", "InRawC"] { + assert!(!kept.contains(gone), "{gone} survived: {kept}"); + } + } + + /// A test-only item the scan cannot bound is REFUSED, never read to the + /// end of the file, which is what silently hid every shape above. + #[test] + #[should_panic(expected = "could not be bounded")] + fn an_unbounded_test_only_item_is_refused_not_swallowed() { + let _ = production_text("#[cfg(test)]\nfn broken() {\n Command::X(1)\n"); + } + #[test] fn comments_and_literals_are_blanked_and_code_is_kept() { let text = concat!( diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index 5630cc5a..77073043 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -173,7 +173,10 @@ impl Client { pub fn record_cell_at(&self, row: usize, col: usize) -> Option { match self.state.grid.cell(row, col)?.id.clone()? { CellId::Record(cell) => Some(cell), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index fa48a6bd..8aa220b4 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -144,7 +144,16 @@ impl RenderCell { text: cell.text.clone(), numeric: cell.numeric, error: cell.error, - editable: matches!(cell.id, Some(CellId::Record(_))), + editable: match cell.id { + Some(CellId::Record(_)) => true, + Some( + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate, + ) + | None => false, + }, selected, edit_text: cell.edit_text.clone().unwrap_or_default(), } @@ -563,9 +572,8 @@ mod tests { /// A 2x3 grid whose cells cover the four id shapes plus a padded slot, so /// one fixture answers every editability question. - fn group(cat: &str, name: &str, collapsed: bool) -> FrameEntry { + fn group(name: &str, collapsed: bool) -> FrameEntry { FrameEntry::Group(improvise_protocol::FrameGroup { - cat: cat.into(), name: name.into(), collapsed, }) @@ -583,11 +591,11 @@ mod tests { first: 0, cats: vec!["Month".into()], entries: vec![ - group("Month", "Q1", false), + group("Q1", false), item("Jan"), - group("Fiscal", "Q1", true), + group("Q1", true), item("Feb"), - group("Month", "Q2", false), + group("Q2", false), item("Apr"), ], }; @@ -628,6 +636,10 @@ mod tests { }), ), ((1, 0), CellDisplay::text("pending")), + ( + (1, 2), + CellDisplay::text("North").with_id(CellId::Coordinate), + ), ])), ], }); @@ -659,6 +671,10 @@ mod tests { !model.rows[1].cells()[1].editable, "and neither is a slot fit_to_labels padded" ); + assert!( + !model.rows[1].cells()[2].editable, + "renaming a coordinate moves the whole row, which is not a cell write" + ); } /// The numeric/error flags are the server's answers, echoed — so the test diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9e393226..73feb294 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -187,6 +187,21 @@ test.describe("the standalone deployment", () => { await expect(page.locator("#grid td.editable").first()).toBeVisible(); }); + // improvise-i0v9: "in-page worker · " was written at 13ms, before the + // worker had answered anything, and read as success over a blank page. + test("the worker is not called up until its first frame has arrived", async ({ page }) => { + const doc = `first-frame-${Date.now()}.improv`; + await page.addInitScript(logStatus); + await page.goto(`/app/index.html?doc=${doc}`); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(status(page)).toHaveText(`in-page worker · ${doc}`); + const log = await page.evaluate(() => window.__statusLog); + expect(log, `#status said: ${json(log)}`).toEqual([ + `starting the worker · ${doc}…`, + `in-page worker · ${doc}`, + ]); + }); + // improvise-9cma: a `?doc=` this backend cannot hold used to reach the // generic watchdog 8s later. The REASON now crosses the port (improvise-1wft). test("an invalid ?doc= is named on screen, and not blamed on the browser", async ({ @@ -235,6 +250,7 @@ test.describe("the standalone deployment", () => { const second = await context.newPage(); await second.clock.install(); + await second.addInitScript(logStatus); await second.goto(`/app/index.html?doc=${doc}`); // It PAINTS. That is the whole point: a locked document is read-only, not @@ -242,6 +258,15 @@ test.describe("the standalone deployment", () => { await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).toContainText("another tab"); + // The page's clock has not been walked, so a reason that arrived AFTER the + // watchdog would have been logged behind "did not answer". Nor was success + // ever claimed: the Snapshot that follows the notice must not overwrite it. + const early = await second.evaluate(() => window.__statusLog); + expect(early.at(-1), `#status said: ${json(early)}`).toContain("read-only"); + expect( + early.filter((t) => t.includes("did not answer") || t === `in-page worker · ${doc}`), + `#status said: ${json(early)}`, + ).toEqual([]); // And the reason STANDS: it is not replaced 8s later by a watchdog // blaming the browser for something the code knew precisely. @@ -252,6 +277,10 @@ test.describe("the standalone deployment", () => { await second.clock.runFor(HANDSHAKE_TIMEOUT_MS + 2_000); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).not.toContainText("did not answer"); + const late = await second.evaluate(() => window.__statusLog); + expect(late.some((t) => t.includes("did not answer")), `#status said: ${json(late)}`).toBe( + false, + ); await second.close(); await first.close(); diff --git a/web/app.js b/web/app.js index 617725c0..7c547bd9 100644 --- a/web/app.js +++ b/web/app.js @@ -181,7 +181,8 @@ function workerTransport() { shell.fail(`worker failed to start: ${event.message ?? "see console"}`); console.error("improvise: worker error", event); }); - shell.setShell(`in-page worker · ${DOC}`); + // Not yet a claim that it works: the first frame makes that (improvise-i0v9). + shell.setShell(`starting the worker · ${DOC}…`); // Frames posted before the worker has finished `start()` are held by // worker.js in arrival order, so the handshake can go out immediately. return { send: (text) => worker.postMessage(text), ready: Promise.resolve() }; @@ -798,6 +799,7 @@ shell.arm( WS_URL ? `${WS_URL} never answered — is improvise-ws-server running, and did it send a document?` : "the in-page worker did not answer (see console)", + { answer: WS_URL ? null : `in-page worker · ${DOC}` }, ); await transport.ready; sendViewport(); // the handshake: the snapshot is built at this extent diff --git a/web/shell.js b/web/shell.js index c2393afc..169373a7 100644 --- a/web/shell.js +++ b/web/shell.js @@ -339,7 +339,12 @@ export function createShell({ clearTimer = clearTimeout, }) { let watchdog = null; + let answered = null; let arms = 0; + const disarm = () => { + clearTimer(watchdog); + watchdog = null; + }; return { setShell(text) { @@ -350,24 +355,25 @@ export function createShell({ }, /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ - arm(message) { + arm(message, { answer = null } = {}) { arms += 1; - clearTimer(watchdog); + answered = answer; + disarm(); watchdog = setTimer(() => { watchdog = null; this.setShell(message); }, timeoutMs); }, - /** A frame arrived, so the peer answered: the watchdog has no question - * left to ask. */ + /** A frame arrived, so the peer answered. Only the FIRST, and only if no + * failure spoke first, may say so (improvise-i0v9, clients.md). */ sawFrame() { - clearTimer(watchdog); - watchdog = null; + if (watchdog !== null && answered !== null) this.setShell(answered); + disarm(); }, /** A peer that failed *loudly* has already said something more precise, so * the watchdog must be DISARMED and not merely pre-empted (PR #13). */ fail(text) { - this.sawFrame(); + disarm(); this.setShell(text); }, armed: () => watchdog !== null, diff --git a/web/tests/shell.test.js b/web/tests/shell.test.js index b2d02999..e8a9570d 100644 --- a/web/tests/shell.test.js +++ b/web/tests/shell.test.js @@ -351,6 +351,43 @@ test("a notice DISARMS the watchdog rather than being replaced by it", () => { ); }); +// improvise-i0v9: "in-page worker · " was written before the worker had +// answered anything, and read as success over a blank page for eight seconds. +test("only the first frame claims the peer answered, and not before it", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + assert.deepEqual(seen, [], "the claim was made before any frame arrived"); + shell.sawFrame(); + shell.sawFrame(); + assert.deepEqual(seen, ["in-page worker · a.improv"]); +}); + +// The order worker.js really produces on a locked document: the ReadOnly +// notice is posted after `start()` and BEFORE the Snapshot answers the handshake. +test("a reason given before the first frame is not replaced by the frame's claim", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + shell.fail(noticeText({ kind: "ReadOnly", document: "a.improv", detail: "" })); + shell.sawFrame(); + clock.fire(); + assert.equal(seen.length, 1, `#status said: ${JSON.stringify(seen)}`); + assert.match(seen[0], /read-only/); +}); + // The notice's remembered state. The value crosses `localStorage`, which is // allowed to forget and allowed to throw, so the interesting inputs are the // ones that are NOT either spelling.