Implement the inline @type cast cleanup: 273 of 357 removed or converted - #1589
Conversation
Step 1 of todo/inline-type-casts.md. 176 of the 181 casts the audit measured as
redundant are gone; `npx tsc` and `fjs t` (2797 tests) pass unchanged.
172 came out in one sweep. The 9 in the two rtti visitors are each removable on
their own but compete for the same instantiation-depth budget, so they were
taken one at a time against a full type-check: 4 more came out, 5 stay. Their
rows in the audit table already predicted this.
Where the cast's parentheses existed only to carry it, they go too — except
after an arrow, where `x => ({ … })` needs them to stay an object literal
rather than a block.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Step 2 of todo/inline-type-casts.md. Eight `/** @type {Dir} */` casts sat directly after a guard that looked like it should already have narrowed. `Array.isArray` narrows to `any[]`, which `readonly Vec[]` is not assignable to, so its negative branch never removes a `readonly` array from a union — `instanceof Array` does. Swapping the eight guards that the casts depended on makes the surrounding `assert` and `if` narrow on their own, and all eight casts delete. Only those guards are changed. The other `Array.isArray` calls in these files narrow positively, where both spellings work, and `Array.isArray` is the cross-realm-correct one — so they stay as they are, with a comment at each swapped site explaining why it differs. `npx tsc` and `fjs t` (2797 tests) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Step 3 of todo/inline-type-casts.md.
23 casts whose expression is genuinely assignable to the target type — so the
cast was pinning a type, never overriding one. Six sit at a `const`
initializer and become the declaration form AGENTS.md prefers; `fjs/effects/node`
already spelled the identical `do_('stat')` case that way two lines above the
inline one. The other 17 become `@satisfies`, which checks assignability
without discarding the inferred type.
Four `@satisfies`-capable sites were deliberately left as `@type {any}`
(`types/function/compare`, `types/rtti/parse`): `@satisfies {any}` asserts
nothing, so an `any` cast is either load-bearing or it should go — it is never
a "check".
`npx tsc` and `fjs t` (2797 tests) pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Re-measuring after steps 1–3 found 8 more casts that `npx tsc` now passes without. They were not redundant before: each existed only to feed a cast that has since gone — `types/nominal/proof` is the clearest case, where removing the outer `_SymbolIntersectionBranded` cast left an inner `any` cast with nothing to widen for. The two in the rtti visitors again needed one at a time against a full type-check, for the same instantiation-depth reason as step 1. 218 of the original 357 casts are now gone; 142 remain. `npx tsc` and `fjs t` (2797 tests) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Step 4 of todo/inline-type-casts.md, first half: the 17 casts outside the `unknown`-in-proofs group. Each was a claim the compiler took on faith; each is now checked where it is made. - 6 nullish lookups (`bnf/ll1` dispatch map, `sul/id` point-at-infinity, `sul/level/hash` root and `storage.at(-1)`, `types/rtti/proof` entries) become `assertNotNullish`. - 3 in `media/json/parser` were narrowing `state.top` to one arm of a discriminated union. `assert(top !== null && top.kind === 'object')` narrows the same way and checks the tag; `pushKey` grows a block body to hold it. - 3 literal-range casts (`asn.1` class/PC bits, `Index<3>`, `Index<5>`) become an `assert` over the alternatives. Narrowing a number to a literal union is exactly what a compiler cannot verify and a check can. - 2 in `emergent_testing/proof` become `assert(result instanceof Array)`, which also replaces the `Array.isArray` assertion that was not narrowing. - 2 rtti visitors dispatch on a `'const'` tag the compiler cannot correlate with the payload. `Type` is `Const | Thunk` and a `Thunk` is a function, so `assert(typeof c !== 'function')` *is* the definition of `Const`. `npx tsc` and `fjs t` (2797 tests) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Step 4 of todo/inline-type-casts.md, second half, for `fjs/protocol/mcp/proof.f.mjs`: 33 casts down to none. A step's response is `unknown`, and its shape is exactly what these proofs exist to check — so each cast was assuming the thing being proved. Seven accessors (`hasError`, `errorCode`, `errorId`, `resultOf`, `protocolVersion`, `toolNames`, `nextCursor`, `firstText`) check their way in with `assert` and `in`, so a malformed response now fails at the read with the offending value attached, instead of surfacing as a confusing mismatch further along — or not at all. Six `unknown`-to-`Unknown` casts on the test messages went a different way: the local `step1`/`step2`/`step3` helpers declared `msg: unknown` when every caller passes a JSON-RPC object literal. Annotating the parameter as `Unknown` is what the casts were compensating for. `npx tsc` and `fjs t` (2797 tests) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
Continues step 4 of todo/inline-type-casts.md in `fjs/cas/proof.f.mjs` (8 casts
to none) and `fjs/mcp/proof.f.mjs` (11 to one).
`cas/proof` asserted a write failed by casting the driver's `unknown` result to
`IoResult<Vec>` and reading `[0]` and `[1]` — the shape being the very claim
each of those four proofs makes. One `errorMessage` accessor checks the pair.
`mcp/proof` gains `has`, `errorCode`, `idOf` and `toolsOf`, and `item0` now
returns a `ContentItem` so `textOf` discriminates on `type` rather than casting
to `{ text: string }`.
One cast remains, in `resultOf`, and it is now the only place a response
crosses from `unknown` to a typed `ToolsCallResult`. Everything checkable is
checked around it; the final step is on trust because rtti's `parse` reads
`Unknown`, and reaching `Unknown` from `unknown` is the same problem one level
down. Concentrating it in a single documented site is the point.
`npx tsc` and `fjs t` (2797 tests) pass. 274 of the original 357 casts are gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
`isNotFound` and its proof read `code` off an unknown rejection; `'code' in e` narrows without a cast and is the check the comparison already implied. `fjs run` cast a module's `main` straight to `NodeProgram`. A module named on the command line can export anything, so it now asserts `main` is callable and fails with the file name rather than as `main is not a function` from inside the effect runner. The narrowing from `Function` to `NodeProgram` still needs a cast — that is the boundary `assert` cannot cross — but it is now reached only after a check. `npx tsc` and `fjs t` (2797 tests) pass. 283 of the original 357 casts are gone; 74 remain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
283 of the 357 casts are gone. Replaces the audit's per-site table with the 74 that remain and the reason each stays, re-measured against the current tree — none can be deleted, and none can become a meaningful `@satisfies`, so this is a fixed point rather than a stopping point. The largest remaining group, 36 TS2322/TS2345 sites, is deliberately untouched: those casts override a real mismatch, so each needs an issue against the API it papers over rather than a different cast syntax. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | c5bd2b4 | Commit Preview URL Branch Preview URL |
Aug 15 2026, 07:52 PM |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
The code is sound — both findings below are bookkeeping.
Checked the parts tsc doesn't cover:
instanceof Array(8Dirguards) — safe.Diris the test-only virtual FS; every importer is aproof.f.mjsorfjs/dev, and every value is an in-process literal. Nonode:vm,worker_threads,new Worker,structuredCloneorcreateContextanywhere underfjs/, and the real node effects never construct aDir. No cross-realm path, so theArray.isArraysemantics aren't load-bearing here.- The 17 runtime checks — none can fire on input that previously worked. Traced each:
media/json/parser(topis set for every status reachingpushKey/endArray/endObject),sul/level/hash(nullish only for an empty stack, already guarded),asn.1(x & 0b111_00000nis always one of the 8 asserted values),bnf/ll1,index3/index5. - Proof accessors still assert; several are strictly stronger than the casts they replaced.
npx tsc exit 0, fjs t → 2838 pass / 0 fail. No suppressions added; the 7 added any lines are pre-existing double casts collapsed to one (net −6).
Two things to fix:
- No
changelog/unreleased/1589.md— AGENTS.md §5 requires one for a code change. - The remaining count is 85, not 74.
todo/inline-type-casts.md's table omitsfjs/mcp/cas/proof.f.mjs(10 casts: 33, 38, 65, 85, 93×2, 101, 121, 132, 149) andfjs/types/patricia_trie/module.f.mjs:49— both arrived from main via #1582/#1583 after the sweep. The 283-removed figure is exact (368 on main → 85 here); only the baseline and allowlist are 11 short.
Minor: whitespace-only lines left at fjs/mcp/evo/module.f.mjs:124,147 where a JSDoc was deleted.
…dant
`github-code-quality` flagged two unused variables in `types/nominal/proof`.
Following that up found the removals there had deleted the proof itself, and a
declaration-emit diff against `main` found four more removals that had changed
the published API. All ten type-checked; none were safe.
The API regressions, found by emitting `.d.mts` before and after:
- `effects/node` `createServer`: `<O extends Operation>(listener:
RequestListener<O>) => Effect<O | CreateServer, Server>` became
`(...payload: never) => Effect<Operation, never>`
- `effects/node` `log`/`error`: `Console` became the structural type
- `cas/evo` `emptyCache`: `Cache` became `{ bySubject: {} }`
- `types/range_map`: `RangeMapArray<T>` became `[T, number][]` — a purely
functional library publishing a mutable array type
Three are now annotated declarations; `createServer` goes back to an inline
cast, which is what do-generic-operation-signatures.md is about.
`types/nominal/proof` demonstrates, per branding strategy, whether `<` compiles
between two branded values. The casts *are* the demonstration, and a brand is
unconstructible by design, so the declaration form rejects what the inline cast
accepted — `const a = {}` compared against `const b = {}` proves nothing. In a
proof about types the annotation is the test, and "does it still compile"
cannot see it being deleted.
Also clears the 36 `@import` entries the cleanup orphaned, so
`--noUnusedLocals` reports exactly what it did on `main`, and records both new
checks in the audit's method.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
* todo: open per-API issues for the casts the cleanup deliberately left `todo/inline-type-casts.md` ends with 36 sites where the cast overrides a real type mismatch, and says each needs an issue against the API it papers over rather than a different cast syntax. This opens those issues for the 20 that share a cause, and records that the remaining one-offs do not. - `do_` has nowhere to put a type parameter, so the three generic effect constructors are cast while their non-generic neighbours in the same file are annotated declarations. - Six `step`/`okStep` continuations cast their way to the operation union the caller wants, hiding any operation the runner cannot interpret. - `memoryOperationMap()` is not assignable to `ToAsyncOperationMap<O>`, and that cast is the exact hazard AGENTS.md describes: it stops each handler being checked against `O`. - Keyword and operator collections typed over `string` mean `has` cannot narrow to a `JsToken` kind, so four token constructions are cast. - `btree/find` casts every tuple it builds or indexes. - `mockRun`'s operation map reaches for `Parameters<typeof mockRun<…>>[0]`, which is what a call that cannot infer its type arguments looks like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih * Address review: restore ten casts that `tsc --noEmit` said were redundant `github-code-quality` flagged two unused variables in `types/nominal/proof`. Following that up found the removals there had deleted the proof itself, and a declaration-emit diff against `main` found four more removals that had changed the published API. All ten type-checked; none were safe. The API regressions, found by emitting `.d.mts` before and after: - `effects/node` `createServer`: `<O extends Operation>(listener: RequestListener<O>) => Effect<O | CreateServer, Server>` became `(...payload: never) => Effect<Operation, never>` - `effects/node` `log`/`error`: `Console` became the structural type - `cas/evo` `emptyCache`: `Cache` became `{ bySubject: {} }` - `types/range_map`: `RangeMapArray<T>` became `[T, number][]` — a purely functional library publishing a mutable array type Three are now annotated declarations; `createServer` goes back to an inline cast, which is what do-generic-operation-signatures.md is about. `types/nominal/proof` demonstrates, per branding strategy, whether `<` compiles between two branded values. The casts *are* the demonstration, and a brand is unconstructible by design, so the declaration form rejects what the inline cast accepted — `const a = {}` compared against `const b = {}` proves nothing. In a proof about types the annotation is the test, and "does it still compile" cannot see it being deleted. Also clears the 36 `@import` entries the cleanup orphaned, so `--noUnusedLocals` reports exactly what it did on `main`, and records both new checks in the audit's method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih --------- Co-authored-by: Claude <noreply@anthropic.com>
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
The ten restorations are correct, and worth stating why since tsc --noEmit disagrees: package.json ships **/*.d.mts and prepack runs tsc --noEmit false --emitDeclarationOnly, so declaration emit is a check the repo runs that plain npx tsc never sees. Emitting declarations at 2aa5299 and at this head and diffing: emptyCache was { bySubject: {} }, now Cache; createServer was (...payload: never) => Effect<Operation, never>, now the generic signature; log/error were structural, now Console; fromRange returned [T, number][], now RangeMapArray<T>. Real published-API regressions, not cargo-cult. Across the whole emitted fjs/ tree vs main: 149 differing lines, all JSDoc, zero type differences. The types/nominal/proof restorations are right too — those casts are the branding demonstration.
Count is now accurate: 273 removed / 84 audited-remaining, all 95 sites listed with the 11 from main separated. My independent recount is 95 — exact match.
Still open (the last three now include #1590's content, folded in here):
- Changelog — no
changelog/unreleased/1589.md, and noChangelog:line in the description. AGENTS.md §5 requires one or the other. - PR title and body are stale — still "283 of 357 removed" / "74 remain" against the doc's corrected 273/84.
- The doc's "Reason | Count" summary sums to 90, not 95 —
anybridge is 24 not 23, brand 8 not 6, overrides 37 not 35. - Whitespace-only lines at
fjs/mcp/evo/module.f.mjs:123and:146(8 spaces each), both introduced here. fjs/js/todo/token-kind-narrowing.mdstill overreaches — carried over unchanged. Deleting the cast atfjs/js/tokenizer/module.f.mjs:262givesTS2322showingkindis already the 49-member literal union, notstring; it fails to distribute over_KeywordToken | _TrueToken | _FalseToken | _NullToken | _UndefinedToken. Retyping the collection won't remove that one, so "the four casts go" holds for three of the four.- Minor: the doc cites
fjs/djs/tokenizer/module.f.mjsat 294/392/405; the casts are at 295/393/406.
npx tsc exit 0; fjs t → 2838 pass / 0 fail.
Follows @o2alexanderfedin's review on #1589. All three counting points were right; the summary was computed against a pre-restoration measurement and had gone stale. - Adds `changelog/unreleased/1589.md` (AGENTS.md §5). - Recounts the remaining sites against the current tree, and totals the summary so it cannot drift again: 95 sites, of which 37 TS2322/TS2345, 24 `any` bridges and 8 unconstructible brands — not the 35/23/6 the doc claimed, which summed to 90 rather than 95. The 273-removed and 84-audited-remaining figures were already right. - Regenerates the per-site table from a fresh scan, fixing line numbers that had shifted by one under the `djs/tokenizer` casts. - Drops two whitespace-only lines left in `fjs/mcp/evo/module.f.mjs` where a JSDoc comment was deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih
# Conflicts: # todo/inline-type-casts.md
sergey-shandar
changed the title
@type cast cleanup: 283 of 357 removed or converted@type cast cleanup: 273 of 357 removed or converted
|
@o2alexanderfedin — thanks, all six addressed. Every counting point was right; the summary was computed against a pre-restoration measurement and had gone stale. 1. Changelog — added 2. PR title and body — updated to 273 removed / 84 audited-remaining, with the declaration-emit finding written up rather than left in a comment thread. 3. The summary summed to 90, not 95 — confirmed, and your three corrections are exactly right. Recounted against the current tree: 37 TS2322/TS2345 (not 35), 24 4. Whitespace-only lines — 5. 6. Line numbers — the per-site table is regenerated from a fresh scan; On your point about the 11 casts from #1582/#1583: agreed, and they're now listed in the table under "arrived on
Generated by Claude Code |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Five of the six items are resolved:
changelog/unreleased/1589.mdpresent and matchingchangelog/README.md; body ends with theChangelog:line.- Title and body now say 273 of 357 / 84 remaining / 95 rows.
- Reason table sums correctly: 37+24+8+6+4+2+3+11 = 95, matching the 95 per-site rows, with a
**Total**row added. - Whitespace-only lines gone;
git diff --checkclean. fjs/djs/tokenizercitations now 295/393/406.
Independent recount at this head: 320 inline @type casts under fjs/, 225 {const}, 95 non-const — matches the doc. I also diffed all 95 file:line citations against the actual sites: exact set match, zero discrepancies either direction. #1591's merge left the accounting intact. npx tsc exit 0; fjs t → 2844 pass / 0 fail (body still says 2838 — harmless drift from the merges).
Item 5 is still open but not worth holding this up, since it describes future work rather than anything in this diff: fjs/js/todo/token-kind-narrowing.md is unchanged, and fjs/js/keywords exports keywords = /** @type {const} */ ([...]), so that collection is not typed over string. Deleting the cast at fjs/js/tokenizer/module.f.mjs:262 still gives TS2322 — kind is already the 49-literal union and fails to distribute over _FalseToken | _KeywordToken | _NullToken | _TrueToken | _UndefinedToken. Worth fixing whenever that todo is picked up; three of its four sites are correctly characterized.
3 participants
Implements
todo/inline-type-casts.md, merged in #1586. That PR was the audit; this one is the cleanup.273 of the 357 casts are gone. 84 of the audited set remain, each with a recorded reason. The per-site table also lists 11 casts that arrived on
mainafter the audit (mcp/cas/proof,types/patricia_trievia #1582/#1583) and are untouched here — 95 rows in total.npx tscexit 0,fjs t→ 2838 pass / 0 fail, and zero type differences in the emittedfjs/declarations againstmain.What landed
fjs/js/tokenizeralone lost 37Array.isArray→instanceof ArrayDirguards@satisfies+ annotated declarations@satisfiesassertNotNullish, discriminant and literal-range assertsprotocol/mcp/proof33→0,mcp/proof11→1,cas/proof8→0tsc --noEmitis not a sufficient checkReview found ten removals that were wrong, and
npx tscwas green for every one.Four changed the published API, found by emitting
.d.mtsbefore and after:effects/nodecreateServer<O extends Operation>(listener: RequestListener<O>) => Effect<O | CreateServer, Server>(...payload: never) => Effect<Operation, never>effects/nodelog,errorConsolecas/evoemptyCacheCache{ bySubject: {} }types/range_mapfromRangeRangeMapArray<T>[T, number][]—readonlylostpackage.jsonships**/*.d.mtsandprepackrunstsc --emitDeclarationOnly, so declaration emit is a check the repo already performs and plainnpx tscnever sees.Six deleted the assertion itself.
types/nominal/proof.f.mjsdemonstrates, per branding strategy, whether<compiles between two branded values — the casts are the demonstration. A brand is unconstructible by design, so the declaration form rejects what the inline cast accepted; the removal leftconst a = {}compared againstconst b = {}, proving nothing. In a proof about types the annotation is the test, and "does it still compile" cannot see it being deleted.Both checks are now recorded in the audit's method, along with watching
--noUnusedLocals— an orphaned@typedefis how the nominal case surfaced. The 36@importentries the cleanup orphaned are cleared, so--noUnusedLocalsreports exactly what it does onmain.Other notes
Two changes are not cast removals but were the actual defect:
protocol/mcp/proof'sstep1/step2/step3declaredmsg: unknownwhen every caller passes a JSON-RPC object literal, andfjs runnow asserts a module'smainis callable before invoking it.The 37 remaining
TS2322/TS2345sites are deliberately untouched — each overrides a real mismatch and needs an issue against the API it papers over, not a different cast syntax. Those issues are in #1590.Changelog:
changelog/unreleased/1589.md🤖 Generated with Claude Code
https://claude.ai/code/session_01TQcZKuWSt2rEZrCj1jVZih