diff --git a/changelog/unreleased/1713.md b/changelog/unreleased/1713.md new file mode 100644 index 000000000..f471257bf --- /dev/null +++ b/changelog/unreleased/1713.md @@ -0,0 +1,3 @@ +- `effects/node/memory`: new `memoryRun` — an `asyncRun` runner owning a fresh + memory store, so a caller can keep one across effects instead of building the + operation map and the runner by hand diff --git a/fjs/AGENTS.md b/fjs/AGENTS.md index f1d91e79c..6f1d7e71c 100644 --- a/fjs/AGENTS.md +++ b/fjs/AGENTS.md @@ -526,12 +526,35 @@ check its argument. A cast around a big object literal passed to a `ToAsyncOperationMap`-shaped parameter, for example, blocks TypeScript from checking each operation's implementation against `O` — the object literal is no longer contextually typed by the call site, so a drifted handler shape is -absorbed by the cast instead of flagged. Prefer no cast at all when the callee -already supplies enough context (as `asyncRun(map)` does here) so the object -literal is checked structurally on its own; reach for `@satisfies` only where a -check without adopting the target type is actually wanted, e.g. a value that -must additionally be nominal-branded — `asNominal(x) satisfies T` becomes -`/** @satisfies {T} */ (asNominal(x))`, not `@type`. +absorbed by the cast instead of flagged. Prefer no cast at all, so the object +literal is checked structurally on its own. + +`asyncRun(map)` is worth spelling out, because the callee does **not** supply +that context on its own: `ToAsyncOperationMap` is a mapped type keyed on +`O[0]`, not a homomorphic `{[K in keyof T]: …}`, so TypeScript cannot infer `O` +back out of the argument. Left to argument inference `O` falls back to its +`Operation` constraint — payloads and outputs `never` — which no real map is +assignable to, and the call site reaches for exactly the cast this section warns +about. **Annotate the result instead**: pin the runner's own type +(`/** @type {_EffectToPromise} */`, `/** @type {MemoryRun} */`) and `O` is +inferred from the return type, giving the call a real `O` to check its argument +against. Both Node runners are written that way — +`fjs/effects/node/module.mjs`'s `runNodeEffect` and +`fjs/effects/node/memory/module.mjs`'s `memoryRun`. + +What that buys differs with what the call passes, and only the first case is +the hazard this section is about. `runNodeEffect` passes an object **literal**, +so its annotation is the only thing checking any handler — without it nothing +is checked. `memoryRun` passes an already-annotated call result, whose handlers +are checked at the factory regardless; there the annotation buys agreement +between the *declared map type* and `O`, caught at the runner rather than +wherever the map is spread next. Both are worth having. Don't claim the second +is the first. + +Reach for `@satisfies` only where a check without adopting the target type is +actually wanted, e.g. a value that must additionally be nominal-branded — +`asNominal(x) satisfies T` becomes `/** @satisfies {T} */ (asNominal(x))`, not +`@type`. #### Mutually recursive constants: cross-reference with `typeof` diff --git a/fjs/effects/node/memory/module.mjs b/fjs/effects/node/memory/module.mjs index 6fba29eda..6aa3ab29e 100644 --- a/fjs/effects/node/memory/module.mjs +++ b/fjs/effects/node/memory/module.mjs @@ -56,8 +56,42 @@ export const memoryOperationMap = (uuid = randomUUID) => { } /** - * Runs a memory-only effect using a fresh memory store. - * @type {(effect: Effect) => Promise>} + * An {@link asyncRun} runner over {@link MemOp}: an effect in, its `Result` out. + * @typedef {(effect: Effect) => Promise>} MemoryRun */ -export const run = effect => - asyncRun(/** @type {ToAsyncOperationMap} */ (memoryOperationMap()))(effect) + +/** + * Creates a runner owning a fresh memory store. Every effect passed to the + * *same* runner shares that store; a new runner starts empty. + * + * **The `MemoryRun` annotation is what types the `asyncRun` call.** `asyncRun` + * takes a `ToAsyncOperationMap`, a mapped type keyed on `O[0]`, and + * TypeScript cannot infer `O` back out of one — only a homomorphic + * `{[K in keyof T]: …}` is reversed. Left to argument inference `O` falls back + * to its `Operation` constraint, whose payloads and outputs are `never`, and no + * real map is assignable to that; the call site then needs a cast. Annotating + * the result instead lets `O` be inferred from the return type, which is what + * gives the call a real `O` to check the argument against. + * + * **What that check is worth here is narrow, so it is worth stating exactly.** + * Each handler is already checked against `MemOp` by + * {@link memoryOperationMap}'s own annotation, and was before this call was + * written — a drifted handler is reported there, at the factory, either way. + * What the runner adds is agreement between the *declared map type* and + * `MemOp`: give `MemoryOperationMap` a narrower operation set and this line + * reports the missing handler, where an unchecked call site would leave it to + * whoever spreads the map next. The wider hazard — a cast stripping the + * contextual type from an object *literal*, so no handler is checked at all — + * is `runNodeEffect`'s (`../module.mjs`), which passes a literal; this call + * passes an already-annotated result. + * @type {(uuid?: Uuid) => MemoryRun} + */ +export const memoryRun = (uuid = randomUUID) => asyncRun(memoryOperationMap(uuid)) + +/** + * Runs a memory-only effect using a fresh memory store — a store per call, so + * nothing written by one `run` is visible to the next. Use {@link memoryRun} + * to keep one. + * @type {MemoryRun} + */ +export const run = effect => memoryRun()(effect) diff --git a/fjs/effects/node/memory/proof.mjs b/fjs/effects/node/memory/proof.mjs index f55da9a1d..6453027d6 100644 --- a/fjs/effects/node/memory/proof.mjs +++ b/fjs/effects/node/memory/proof.mjs @@ -3,16 +3,16 @@ * * @module * - * @import { Key, MemOp } from '../../memory/types.ts' + * @import { Key } from '../../memory/types.ts' + * @import { Uuid } from './module.mjs' */ -import { asyncRun } from '../../module.mjs' import { errorSummary } from '../module.f.mjs' import { - asNominal, + asBase, asNominal, create, read, write, } from '../../memory/module.f.mjs' -import { memoryOperationMap, run } from './module.mjs' +import { memoryRun, run } from './module.mjs' import { assert, assertEq } from '../../../asserts/module.f.mjs' import { unwrap } from "../../../types/result/module.f.mjs" import { step, unwrapStep } from '../../module.f.mjs' @@ -28,8 +28,8 @@ export const proof = { assert(r[0] === 'ok', r) assertEq(r[1], 2) }, - reusedOperationMapPersists: async () => { - const runner = asyncRun(/** @type {import('../../types.ts').ToAsyncOperationMap} */ (memoryOperationMap())) + reusedRunnerPersists: async () => { + const runner = memoryRun() // `unwrapStep` empties the channel, so what the runner hands back is // an `ok` and these unwraps are total. const key = unwrap(await runner(unwrapStep(create(1), errorSummary))) @@ -37,6 +37,36 @@ export const proof = { const result = unwrap(await runner(unwrapStep(read(key), errorSummary))) assertEq(result, 2) }, + runnersDoNotShareStore: async () => { + // Both runners mint the *same* key id, so the id cannot be what tells + // them apart — only store ownership can. A store shared between + // runners (module-level rather than per call) passes every other proof + // here; this is the one that fails it. + /** @type {Uuid} */ + const uuid = () => 'fixed' + const a = memoryRun(uuid) + const b = memoryRun(uuid) + const key = unwrap(await a(unwrapStep(create(1), errorSummary))) + const result = await b(read(key)).then( + () => undefined, + error => error, + ) + assert(result instanceof Error, result) + assertEq(result.message, 'memory key not found: fixed', result) + }, + runIsPerCall: async () => { + // `run` builds a runner per call, so the store the first call wrote to + // is gone by the second. One runner shared across every `run` — the + // other half of the store-ownership mutant — passes every other proof + // here, including `runnersDoNotShareStore`. + const key = unwrap(await run(unwrapStep(create(1), errorSummary))) + const result = await run(read(key)).then( + () => undefined, + error => error, + ) + assert(result instanceof Error, result) + assertEq(result.message, `memory key not found: ${asBase(key)}`, result) + }, missingKeyThrows: async () => { /** @type {Key} */ const key = asNominal('missing') diff --git a/fjs/effects/node/todo/async-operation-map-assignability.md b/fjs/effects/node/todo/async-operation-map-assignability.md deleted file mode 100644 index a7aebf241..000000000 --- a/fjs/effects/node/todo/async-operation-map-assignability.md +++ /dev/null @@ -1,38 +0,0 @@ -# `ToAsyncOperationMap` rejects the operation maps built for it - -**Priority:** P3 -**Status:** open - -### Problem - -`asyncRun` takes a `ToAsyncOperationMap`, and `memoryOperationMap()` exists to -supply one — but the result is not assignable, so both call sites cast: - -- `fjs/effects/node/memory/module.mjs:60` — `asyncRun(/** @type {ToAsyncOperationMap} */ (memoryOperationMap()))` -- `fjs/effects/node/memory/proof.mjs:28` — the same cast, spelled with an inline `import(…)` - -`npx tsc` reports `MemoryOperationMap` is not assignable to -`ToAsyncOperationMap`, so the mismatch is between the map type the -factory returns and the shape the runner asks for — not between `MemOp` and some -other operation set. - -This matters beyond tidiness: [`fjs/AGENTS.md`](../../../AGENTS.md) notes that a -cast around a value handed to a `ToAsyncOperationMap`-shaped parameter blocks -TypeScript from checking each operation's implementation against `O`, so a -drifted handler shape is absorbed rather than reported. These two casts are that -exact hazard. - -Two nearby casts in the same area may or may not share a cause, and are worth -checking at the same time: - -- `fjs/effects/node/module.mjs:287` — `Erl` on a request listener -- `fjs/effects/node/virtual/module.f.mjs:410` — `SandboxResult` on `f()` - -### Proposal - -Make `memoryOperationMap()` return something `asyncRun` accepts, so the object -literal is checked structurally against `O` at the call site and both casts go. - -### Related - -- [`todo/inline-type-casts.md`](../../../../todo/inline-type-casts.md) diff --git a/fjs/effects/node/todo/generic-operation-payload-erasure.md b/fjs/effects/node/todo/generic-operation-payload-erasure.md new file mode 100644 index 000000000..8e0ba9bb6 --- /dev/null +++ b/fjs/effects/node/todo/generic-operation-payload-erasure.md @@ -0,0 +1,73 @@ +# `Pr` erases a generic operation's type parameter + +**Priority:** P3 +**Status:** open + +### Problem + +An `Operation` may declare a generic signature — `MemRead` is +`(key: Key) => OpResult`, and `Sandbox` (`../types.ts:325`) is +`(f: () => T) => OpResult>`. `Pr` reads the payload +and the output off that signature with `infer P` / `infer R`, and inference +through a generic signature instantiates its type parameter at the constraint. +So the handler an operation map writes receives `T = unknown` and has to cast +its way back — `fjs/effects/node/virtual/module.f.mjs:627` is +`sandbox: f => … ok(/** @type {SandboxResult} */ (f()))`, and without +the cast `f()` is `unknown`, which `OpResult>` will not +take. + +Note where `SandboxResult` sits: on the *output* side, wrapping what the handler +must produce, not inside `f`. That is the whole reason the cast is needed. Were +the payload `f: () => SandboxResult`, erasure would hand the handler a +`SandboxResult` already and there would be nothing to cast. + +The same erasure is why `fjs/effects/memory/module.f.mjs:34,39` cast the +`do_('memCreate')` / `do_('memRead')` results back to their generic shapes. +`write`, three lines below them, is the same `do_` call written as an annotated +declaration and needs no cast — but the declaration form was tried on `create` +and does not help: `Func` is `(value: unknown) => Effect>`, and `Key` is not assignable to `Key` in the +covariant result. `write` escapes only because `T` appears in its parameters +alone, where `unknown` is the accepting side. + +### A neighbour that looks like this and is not + +`fjs/effects/node/module.mjs:438` — `answerRequest(/** @type {Erl} */ +(requestListener))` — was grouped here first, and does not belong: `CreateServer` +is **not** a generic operation. `../types.ts:235` declares it as +`['createServer', (listener: RequestListener) => OpResult]`, +with `Operation` written into the declaration, so `Pr` erases nothing — the +handler is handed the widest listener the type says it may be handed, and +narrowing it to `NodeOp` is what the cast does. The `` +signature it looks like it should have exists only on the effect constructor +(`../module.f.mjs:346`), which is itself a cast. + +That makes it a third cause, needing its own answer: whether `CreateServer` can +carry the listener's op-set instead of pinning `Operation`, and what the +constructor's cast is standing in for. Left here as the nearest home rather than +filed separately, because whoever takes `Pr` will read this file first. + +**Neither of these is the `asyncRun` cause**, which sat beside them and was +checked at the same time: inference of `O` from `ToAsyncOperationMap` at the +call, cured by annotating the runner's own result — see "Prefer `@satisfies` +over `@type` when checking, not overriding" in +[`fjs/AGENTS.md`](../../../AGENTS.md), and `memoryRun` in +[`../memory/module.mjs`](../memory/module.mjs). Deleting each cast and reading +the compiler error is what separated the three, and it is the check to repeat on +any further candidate. + +### Proposal + +No design yet. Work out whether a generic operation's type parameter can be +carried through `Pr` at all — the handler would have to be typed as a generic +function rather than an instantiation of one, which `OperationMap`'s +`(...payload: Pr[0]) => R` cannot currently express — or record that it +cannot and that a generic operation costs one cast per handler by construction. + +### Related + +- [`todo/inline-type-casts.md`](../../../../todo/inline-type-casts.md) — the + audit that measured these sites and asked for "its own issue against the + API it is papering over"; this is that issue for them. +- [`fjs/effects/types.ts`](../../types.ts) — `Pr`, `OperationMap`, + `ToAsyncOperationMap`.