RTTI TS printer: recursive schemas via the data form - #1547

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7438e44ddb

Reviewed at b1c971a5, baseline origin/main = 725129fe (#1544). Everything re-derived
locally rather than taken from the description.

Gates

Broken-link sets compared, not counts: the diff is removals only — the three refs inside the
deleted fjs/types/rtti/todo/662.md and the 66d-… → todo.md ref this PR repoints. No new
broken link, and nothing else in the tree still references 662.md.

Public surface (bin/extract.mjs / bin/consts.mjs, both trees after prepack): exactly one
added const, /fjs/types/rtti/ts::dataToTs; added types are _Ctx, _StringNamed, _T0Named,
all _-prefixed per §6.2; nothing removed, no signature widened to any. §6.2 declaration-emit
hazard: fjs/types/rtti/ts/module.f.d.mts keeps its @module header, grep -c elided = 0, and
zero type-level any (checked the matches, not the count).

npm run cov: fjs/types/rtti/ts/module.f.mjs 100.00 / 100.00 / 100.00, all-files
99.95 / 98.59 / 99.80 — real numbers, not the vacuous variant.

Scope is what the CHANGELOG says it is. The only code file touched is
fjs/types/rtti/ts/module.f.mjs (plus its proof and two todo/ docs); the surface diff confirms
no other module's exports moved. fjs/types/rtti/data is untouched, so the entry names
types/rtti/ts and nothing else, correctly.

Output changes for non-recursive schemas — representation-only, plus two latent fixes

Corpus of 1008 non-recursive schemas printed in both trees, readonly and mut: 433 change text.
I fed every differing pair to tsc --strict as [A] extends [B] ? ([B] extends [A] ? true : false)
in both directions (the harness is its own two-way control — it separates 383 from 50):

• 383 are mutually assignable — reordering (option(number): number|undefined →
  undefined|number), absorption (or(42, number) → number), unit collapse
  (or(true, false) → boolean). Pure representation.
• 50 are not, and in every one the PR's text is the more faithful one. Checked against
  validate, not asserted:
  • {} accepts {z:1} and rejects 42 / "s" / []. Main printed TypeScript's {}, which
    accepts 42 and "s"; the PR prints {readonly[k in string]?:unknown}. Latent bug fixed —
    same shape as #1542's never → {"not":{}}.
  • {a: unknown, b: string} accepts a absent. Main printed {readonly"a":unknown,…}, which
    requires a; the PR drops the unconstrained key. Latent bug fixed.
  • Uninhabited components collapse (readonly[never] → never, array(never) →
    readonly[]) — same value set either way.

All 866 emitted strings parse as valid TypeScript (only TS2322 assignability diagnostics, zero
syntax errors). Acceptance is unchanged in every case, so by the #1542 precedent this correctly
carries no **BREAKING CHANGES:** prefix.

Nit, not a defect: the entry says "output follows the data form's canonical order", but the
change is canonical form, not just order — absorption, never collapse, unconstrained-key
elision and the empty-struct fix are not reorderings.

Recursive corpora

Thunk-derived — 300 generated rule sets across 5 name pools, weighted toward mutual
recursion between differently-named rules rather than self-reference: 212 convert (88 throw
inside toData, identically on origin/main — the data module is untouched, so pre-existing),
104 carry definitions. 0 duplicate identifiers, 0 dangling references, 0 unreachable definitions,
0 idempotence failures, and the 741-line emitted .ts compiles clean under tsc --strict.

Data-level, which is dataToTs's actual public contract — 400 hand-built rule sets across
10 name pools including empty names, non-identifiers, predefined names and T0/T1 collisions:
0 throws, 0 duplicate identifiers, 0 dangling, 0 idempotence failures. Dangling is in fact
unrepresentable — nodeToTs panics (missing definition: B), which I confirmed fires.

That second corpus is where the one problem showed up.

Finding: reserved words pass isTypeName, so recursive schemas can emit invalid TypeScript

isTypeName rejects the empty string, non-identifiers, and the 14 names in predefined — but not
TypeScript's reserved words, which are equally unusable as type alias names. Of 56 keywords I put
through tsc, 36 are illegal as a type alias name (if else class return new typeof in for while do switch case break continue delete instanceof try catch finally throw var const with debugger function import export default enum extends super this infer keyof readonly unique) and
all 36 pass isTypeName, so they are kept rather than routed to the generated T<n> path.

Reachable from the public dataToTs, whose RuleSet is StringMap<UnionSet> with arbitrary
string keys — the point of a serializable form is that the data can come from outside:

dataToTs()([{ infer: { array: [{ prefix: [], rest: 'infer' }] } }, 'infer'])
// [[['infer', 'readonly(infer)[]']], 'infer']

And reachable from thunk rtti too, via exactly the object-property construction this PR's own
stringNamedHolder proof uses to build the predefined-name case:

const g = { if: () => ['array', g.if] }
g.if.name                    // 'if'
toData(g.if)                 // [{ if: { array: [{ prefix: [], rest: 'if' }] } }, 'if']
dataToTs()(toData(g.if))     // [[['if', 'readonly(if)[]']], 'if']
printer()(g.if)              // 'if'

type if = readonly(if)[] is not TypeScript:

kw2.ts(1,28): error TS2457: Type alias name cannot be 'if'.
kw2.ts(1,31): error TS1005: '(' expected.

In the 400-case Data corpus this is the only source of invalid output, and the attribution is
exact: 128 of 400 cases fail tsc, and all 128 are precisely the cases declaring a reserved
word — 0 failures among the 272 that declare none.

Not a regression: on main the printer never emitted a name at all, it just overflowed the stack.
But it is a gap against this module's own stated contract — "one that cannot name a TypeScript
type alias … gets a deterministic generated identifier". The generated-identifier path already
handles it; these names simply never reach it. Adding the reserved words alongside predefined
(they can share the "cannot name an alias" test) fixes all 128, and the collision-skipping in
identifiers already covers the extra T<n> pressure.

Only the recursive path is affected — a schema with no cycles emits no definitions and no names.

Everything else here looks right to me, and the identifier machinery — kept-skipping, the
T0-named-rule collision case, the empty-name case, mutable output, the dangling panic — is
carefully proved.

Re-reviewed at 72326a44. The delta since my comment at b1c971a5 is merge-only — one merge commit bringing in main's #1544. Comparing each head against its own merge-base with main, the PR's own diff is byte-identical apart from two lines of CHANGELOG context. fjs/types/rtti/ts/module.f.mjs is unchanged, so the one open finding is unchanged too; I re-derived it at this head rather than carrying the numbers forward.

Baseline origin/main = e2199caf.

Still open: reserved words pass isTypeName, so recursive schemas can emit invalid TypeScript

isTypeName still rejects only the empty string, non-identifiers, and the 14 predefined names. TS reserved words are not in that list, so they are kept as alias names instead of being routed to the generated T<n> path.

Re-derived at 72326a44 with a widened keyword list (80 candidates: ES reserved words, strict-mode reserved words, contextual and TS-specific type keywords):

• 66 of the 80 are kept as-is; the 14 routed to T0 are exactly the predefined list.
• Emitting type <name> = <rhs> for each and running tsc --strict on the 80 files, 37 fail — every one of them a kept name, none of the routed ones. 31 fail with TS2457 Type alias name cannot be '<x>' outright; the rest (this, super, default, readonly, unique, infer, keyof, await, …) fail as parse errors before tsc gets that far.
• The headline case is unchanged: [{ if: { array: [{ prefix: [], rest: 'if' }] } }, 'if'] still gives [[['if', 'readonly(if)[]']], 'if'], i.e. type if = readonly(if)[] — TS2457: Type alias name cannot be 'if'.

Reachable from the public dataToTs (arbitrary StringMap keys) and from thunk rtti via the same object-property trick the PR's own stringNamedHolder proof uses. Not a regression — main overflowed the stack instead of emitting anything — but a gap against the module's own stated contract.

That contract is the reason this is worth fixing rather than documenting away. The dataToTs JSDoc says a rule name "that cannot name a type alias (not an identifier, or a predefined type name) gets a deterministic generated identifier". The parenthetical is the implementation, but the clause it explains — "cannot name a type alias" — is strictly broader than what is implemented, and the 37 cases above fall in the gap. So the doc as written promises the routing that does not happen. Either extend predefined with the reserved words (the cheaper fix, and it makes the doc true as written), or narrow the doc to say what the code actually screens for and note reserved words as a known limitation.

Everything else re-checked on the merged tree

• npx tsc --noEmit — exit 0.
• npm run prepack from a cleaned tree — exit 0 (both passes).
• npm test — 2663 pass / 0 fail, against 2646 on e2199caf (+17 from the new proofs).
• Broken-link set — a strict subset of main's: the four entries under the deleted fjs/types/rtti/todo/662.md and fjs/types/todo/66d-… are gone, nothing new is stranded (132 → 128).
• No new export type/export const beyond dataToTs; _Ctx/_StringNamed/_T0Named remain _-prefixed per §6.2.

The findings I settled at b1c971a5 — no **BREAKING CHANGES:** prefix needed, recursive-corpus id/reachability/idempotence checks, toData throw parity — are unaffected by a merge and I have not redone them.

One nit still carried from last round, unchanged: the CHANGELOG entry says output "follows the data form's canonical order", but what the data form fixes here is canonical form (which schemas are named and how they are spelled), not the order the definitions come out in. Wording only.

Commenting rather than approving, on the open finding above.

Re-reviewed at d688162f. Since 72326a44 the only substantive commit is 22d18830 ("TS printer: screen reserved words from type-alias names"); the rest is a merge of main. That commit is a direct answer to the previous round's finding, and it closes most of it — but not all of it, so this stays a comment rather than an approval.

Re-derived, not accepted

I rebuilt the keyword corpus from scratch (84 candidates: ES reserved words, strict-mode future reserved words, the 14 predefined type names, TS contextual/type-operator/modifier keywords, plus 3 ordinary-identifier controls), fed each one through dataToTs()([{ [w]: { array: [{ prefix: [], rest: w }] } }, w]), rendered the result as type <id> = <expr> in a real module, and ran tsc --strict --target es2022 --module esnext.

• Routed to T0: 51 — exactly the new reserved array (14 predefined + 33 ES reserved + 4 type-operator). Up from 14.
• Kept as-is: 33, of which 3 are the controls.
• tsc --strict failures: 10, down from 37. All ten are kept names.

So the failure count did not go to zero.

Open: nine strict-mode reserved words plus intrinsic still emit invalid TypeScript

Each of these still round-trips to its own alias name and fails to compile:

yield  implements  interface  let  package
private  protected  public  static          -> TS1214
intrinsic                                    -> TS2795

e.g. dataToTs()(toData({ let: () => ['array', g.let] })) emits type let = readonly(let)[], and in a module:

let.ts: error TS1214: Identifier expected. 'let' is a reserved word in strict mode.
                     Modules are automatically in strict mode.
intrinsic.ts: error TS2795: The 'intrinsic' keyword can only be used to declare
                     compiler provided intrinsic types.

Control Foo.ts compiles clean, so the probe is not just failing everything.

These nine are ECMAScript reserved words — reserved in strict-mode code, and every module is strict-mode code. The reserved array's // ECMAScript reserved words block covers the always-reserved set but omits the strict-mode/future-reserved set. intrinsic is a separate case: TypeScript rejects it in the alias-name position outside lib.d.ts.

This matters for the same reason as last round: the JSDoc still states a condition broader than what is screened.

Rule names come from the data form; one that cannot name a type alias — not an identifier, a predefined type name, an ECMAScript reserved word, or a type-operator keyword — gets a deterministic generated identifier

let and yield are ECMAScript reserved words, so this is still not a documented limitation — the doc asserts coverage the code does not have. The CHANGELOG entry has the same shape ("a rule name that cannot name a type alias (reserved word, predefined type name, non-identifier)").

Fix is the same two options as before, now much smaller: add the nine strict-mode reserved words and intrinsic to reserved, or narrow both prose claims to the set actually screened. Given the commit already took the first option for 51 names, finishing it looks cheaper than qualifying the doc.

Closed since last round

• CHANGELOG wording nit — resolved. The entry now says the output "is the data form's canonical form", not "canonical order".

Battery at this head

• npx tsc --noEmit — exit 0.
• npm run prepack from a cleaned tree — exit 0 (both passes).
• npm test — 2671 pass, 0 fail, against 2652 on origin/main (6d26e264). The +19 are the new proof cases, including the two added by 22d18830 (reservedName, typeOperatorName).
• Scope, surface (dataToTs plus _Ctx / _StringNamed / _T0Named, all _-prefixed), the non-recursive output-text analysis, and the recursive-corpus invariants were settled in the previous round and are untouched by 22d18830; I did not redo them.

Re-reviewed at 6304513a, baseline origin/main = 6d26e264. The reserved-word finding
that survived the last three rounds is closed — verified by re-derivation, not by reading
the list.

The reserved-word finding: resolved

I rebuilt the keyword corpus from scratch rather than from reserved: the ECMAScript
ReservedWord production, the strict-mode future reserved words, arguments/eval, the ES
contextual keywords (as, async, from, get, of, set, target, meta), the
TypeScript predefined type names, the TS modifier/type-operator keywords
(abstract, accessor, assert, asserts, declare, global, infer, is, keyof,
module, namespace, out, override, readonly, require, satisfies, type,
unique, using, constructor), and six non-keyword controls — 95 names.

Each name became a recursive one-rule schema
([{ [n]: { array: [{ prefix: [], rest: n }] } }, n]), went through dataToTs(), and the
emitted definitions plus entry expression were written to a module and compiled with
tsc --strict:

total 95   routed 61   kept 34   failing 0        (tsc exit 0)

Negative control on the harness — hand-written type let = …, type intrinsic = …,
type string = … through the same compile — reports TS1214, TS2795, TS2457
respectively, so the zero is a real zero and not a harness that never fails.

For the record, the trajectory across heads on comparable corpora: 72326a44 37 failures,
d688162f 10 (yield, implements, interface, let, package, private, protected,
public, static at TS1214; intrinsic at TS2795), 6304513a 0. The nine
strict-mode reserved words and intrinsic are exactly what commit 6304513a added.

Wording now matches the screen: the CHANGELOG says "reserved word — strict-mode ones
included, predefined type name, non-identifier", and the JSDoc list carries a
// reserved in strict-mode code — and every module is strict-mode code section. The one
remaining nit is that the summary sentences on reserved and on dataToTs still fold the
strict-mode-only words into "the ECMAScript reserved words" without qualification; that is
defensible (they are reserved words in strict-mode code per the grammar) and it now
under-claims rather than over-claims, so I am not treating it as a finding.

The rest of the delta at this head

6304513a also widened the scope beyond rtti/ts into fjs/types/ts, so I checked the new
behaviour rather than carrying the earlier rounds' conclusions forward.

Optional keys and the props-with-rest index, printed and then compiled:

{x: option(string)}         → {readonly"x"?:undefined|string}
{x: string}                 → {readonly"x":string}
record(string)              → {readonly[k in string]?:string}
props {a:number} rest string→ {readonly"a":number}&{readonly[k in string]?:number|string}

All four compile under tsc --strict; {}, {x: undefined} and {x: 'q'} are all accepted
for the optional form and {} for the record, while Req = {} still errors (checked with
@ts-expect-error, which did not go unused). The rest-index widening to number|string is a
supertype of the validator's set — {a: 1, b: 2} now type-checks but would fail validation —
but the previous struct&record made valid values unrepresentable, so this is strictly the
better of the two expressible options and the JSDoc says so.

Battery:

• npx tsc --noEmit — exit 0.
• npm run prepack from a cleaned tree — exit 0 (both passes).
• npm test — 2674 pass / 0 fail, against 2652 on 6d26e264; +22 is the new proof cases.
• Link check — broken-link sets compared, not counts: the PR removes 4 broken links
  (fjs/types/rtti/todo/662.md ×3, fjs/types/todo/66d-…md ×1) and adds none; 128 remain,
  all pre-existing on main.
• Public surface — StructField added (unprefixed, but it is a genuine public type: it is a
  parameter of the exported Printer, and the CHANGELOG documents it); dataToTs added;
  _Ctx, _IfNamed, _StringNamed, _T0Named all _-prefixed. No signature widened to
  any. Note that bin/extract.mjs truncates multi-line aliases at the first line, so it
  does not show the Printer change — I checked that one by hand instead.
• CHANGELOG — both touched modules now have entries (types/rtti/ts and types/ts), each
  linking only /pull/1547.

One non-blocking note, take it or leave it: Printer.struct narrowing its parameter from
readonly (readonly [string, string])[] to readonly StructField[] is source-breaking for
an external implementer of Printer who annotates the parameter explicitly —

error TS2322: Type 'StructField' is not assignable to type 'readonly [string, string]'.
              Target allows only 2 element(s) but source may have more.

Callers of printer() are unaffected, an implementer who leaves the parameter to contextual
typing is unaffected, and tsPrinter is the only implementation in the repo — so I do not
think this earns a **BREAKING CHANGES:** prefix, but it is the one public-type signature
change in the PR and you may see it differently.

Approving.