types/rtti: read a tuple schema by length, not by enumerable entries - #1712
Conversation
`parse` and `validate` walked a container schema with `Object.entries`, which skips a sparse array's holes, while the data form's `containerUnion` walks it with `for…of`, which yields `undefined` for a hole and preserves length. The three readers therefore disagreed about what a sparse tuple schema declares. Length wins, per the todo's decision: a hole is a declared position whose schema is `undefined`. `tupleSchemaEntries`/`structSchemaEntries` in `rtti/common` are now the per-kind entry readers, passed to all four container factories beside the `getItem` knob they already take. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 07eccbb | Commit Preview URL Branch Preview URL |
Aug 26 2026, 08:07 AM |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eb0142fc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
We plan to add optional types at the end of tuples. For example, |
|
Short answer: it doesn't affect that plan, and it removes an obstacle to it. Your example is untouched. This PR only changes how a sparse schema is enumerated; on a dense array
Identical, and Where it helps: arity becomes well-defined. const s = close([number, , string]) // three positions
validate(s)([1, undefined, 'x'])
// base: error (declared.length was 2, so fits required length <= 2)
// this PR: ok (agreeing with the data form, which said ok all along)An optional-suffix rule stated over The type level already reads by length, which is the part I'd most want settled before the plan lands. TypeScript types One thing to decide when the plan lands, not now: a hole is the degenerate optional — its set is exactly Generated by Claude Code |
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. All four rows of the disagreement table reproduce on 6228f0e04 exactly as written and agree three-ways on head. The fix is complete rather than proof-shaped: I swept every schema walk under fjs/types/rtti/** and no site still reads a tuple schema by enumerable entries — subset inherits toData's reading (verified, not assumed), close walks nothing. Openness survives on both dense and sparse schemas, and no reader disagrees on any row I probed. Seven mutations, including each of the four factory call sites separately: all killed, no survivors — and c2/c4 are killed only by the new close(new Array(1)) rows, so those earn their place. npm test 3378 → 3383, no pre-existing assertion weakened; npm run cov confirms the 100% claim.
Two things, neither blocking:
A second acceptance change rides along, unnamed. The length reading also stops a tuple schema's non-index enumerable own properties from being declared. Object.assign([number], { foo: string }) against [1]: base is error from validate/parse and ok from the data form — a disagreement the todo never mentions — and head is ok from all three. Strict improvement, restores exactly the agreement this PR exists for, and "read by length" covers it in spirit. But it is an acceptance change beyond holes with no changelog sentence, no README row and no proof row. One line of any of the three would do.
The new rows pin agreement, never a verdict. The 8 sparse rows assert only that the three readers answer the same, so nothing in the tree states that validate([, number])([9, 5]) is an error — the changelog's headline. It is pinned transitively and every mutation died anyway, so this is not a gap in practice, but assertError(validate([, number])([9, 5])) would state it directly.
…e verdicts
Review follow-up. Two gaps, both in what the change says rather than what it
does.
The length reading also stops a tuple schema's non-index enumerable own
properties from being declared, which the todo never mentioned. It is the same
rule and a strict improvement — `Object.assign([number], { foo: string })`
against `[1]` was an error from `parse`/`validate` and ok from the data form —
and sharper than a missing declaration: a tuple is read by index, so `foo` was
declared and then matched against `value[NaN]`. It now has a changelog line, a
README paragraph, two acceptance-table rows and a proof.
The sparse rows assert only that the three readers agree, never what they agree
on. `sparseTuple` states the verdicts directly — the hole reading, the closed
arity, and the non-index property — and all three die when `tupleSchemaEntries`
is mutated back to `Object.entries`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
|
Thanks — both taken, in The unnamed acceptance change. Confirmed exactly as you wrote it, and it is sharper than "declared but unsatisfiable": a tuple's const s = Object.assign([number], { foo: string })
validate(s)(Object.assign([1], { foo: 'x' })) // error — foo was never read
validate(s)(Object.assign([1], { NaN: 'x' })) // okSo the base behaviour was not just stricter than the data form, it was checking the wrong property. It now has all three of the things you said one of would do: a second changelog line, a README paragraph under "A hole is a declared position", two acceptance-table rows ( The verdicts. Added
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3dfaef9d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review follow-up, documentation only. `Array.from` walks the iterator, so "read by length" is imprecise for a schema carrying an overridden `Symbol.iterator`. Choosing it is deliberate: it is the same walk `containerUnion` makes, so the two agree by construction rather than by two rules that coincide on ordinary arrays. Reading indices here instead would put the schema-form readers back at odds with the data form on exactly that schema — measured: read as `number` by an entry/index reading and as `string` by `containerUnion`. Reading both by index is defensible but changes the canonical, content-addressed data form, so it belongs with that decision. FunctionalScript has neither symbols nor mutation, so such a schema is reachable only from plain JavaScript, which is why no proof can pin the case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6117eaf7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The review bot raised the same thing twice on this PR from two directions — an overridden `Symbol.iterator`, then an inherited numeric index — and both reduce to one question: should a container schema be walked by iteration or by own indices? Neither is a disagreement between the readers, because they make the same walk; both were disagreements before this PR, and closing that split is what it does. Answering it means changing `containerUnion` as well, which moves the canonical content-addressed data form, so it is its own pull request. Filed with both probes and the constraint that the two have to move together. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Still approving — the approval isn't reopened. Three commits landed since ab2cab7; comment-stripping common/module.f.mjs across all of them gives 91 byte-identical executable lines, so the +5/-1 there was JSDoc only. a6117ea is doc-only and 4330c75b8 adds a todo file with no code.
Both of my observations are closed properly. The headline now has a direct assert (assertError(validate([, number])([9, 5])), line 212) and so does the non-index change; all nine new assertions die individually under mutation, each naming its own subtest.
The non-index claim checks out on mechanism, not just outcome. The decisive row is a value carrying a property literally named NaN: at the merge-base it passes while foo fails, which is exactly the value[NaN] path the README describes. ab2cab7 and head agree on all 14 cells.
And the attribution is clean — a surgical mutant that keeps the length reading but re-appends non-index properties fails nonIndexPropertyIsNotDeclared and sameAcceptanceInTheDataForm while the hole tests survive. Worth noting sameAcceptanceAsParse does not fire there, since validate and parse share the reader; only the data-form comparison catches it, which is what those two new table rows are for.
Gates: npm test 3386/0 (+3 leaves), tsc exit 0, npm run cov still 100 across all six rtti modules, validate/proof.f.mjs 132 → 141 assert sites with nothing dropped.
One thing to fix, in the body only:
The Changelog: section is stale — one item where the file has two. The new **BREAKING CHANGES:** line about the non-index enumerable own property was never mirrored into the body, across three pushes. Placement is otherwise right. Two riders while you're in there: "gains eight sparse rows" is now ten, and fjs t 3383/3383 is now 3386.
Nit: the new todo says the readers agreed only "before a6117ea's parent" — but ab2cab7 already agrees on both of its cases, so the split closed at the code commit, several commits earlier. Its own probe outputs reproduce exactly.
Review nit. The todo said the readers disagreed until `a6117ea`'s parent; the split actually closed at `4eb0142`, this pull request's code commit, and the two after it are documentation and a changelog entry. Verified by re-running both probes there. Changelog: none Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfPCE6b3fK6cDMtYxhnLGL
|
All four corrections taken; three were in the body, one in The stale The riders. Both confirmed rather than taken on trust — The todo nit. Correct, and the fix is Thanks for the mutation attribution on
Generated by Claude Code |
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. The re-dating is right, and sharper than my nit asked for — I said the split closed at ab2cab7, which was as far back as I had measured; it actually closed at 4eb0142, two commits earlier, and that is what the file now names. Bisected both of the todo's own probes through all three readers at every commit from the merge-base: 4eb0142fc is the first where they agree, and the added clause "every commit from it onward agrees" holds across all six later commits.
Rest of the file still accurate — both probes reproduce as commented, the mechanism claims match the code at head (tupleSchemaEntries via Array.from, containerUnion still for…of), all three tasks genuinely still open, all links and the README anchor resolve.
Changelog: is fixed: two list items, byte-identical to the file after unwrapping, last section with no trailer after it. Both riders fixed too — "ten rows" and 3386/3386.
Doc-only delta confirmed: comment-stripped common, parse and validate modules are identical across it. npm test 3386/0, tsc exit 0.
One conflict, in `fjs/types/rtti/validate/proof.f.mjs`: both sides added a proof entry at the same insertion point after the acceptance tables — `optionalPositions` here, `sparseTuple` from #1712 on main. They are independent, so both are kept, this branch's first so that each comment's "the two tables/proofs above" still names the two table-driven proofs. #1712 makes the readers walk a tuple schema by length, which is the *schema* side of "a hole and a declared `undefined` are one thing". This branch's sparse cases are sparse *values* read by indexing, so they are untouched by it — re-ran them to confirm rather than assume. It also deleted `sparse-tuple-schema-entries.md`, which `parse-omits-undefined-members.md` linked to. Repoint that reference at what shipped and at `schema-walk-own-indices.md`, the part of it still open; the schema side is now settled in favour of the reading the issue already assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJuibHn4HfRBwpZgZVjobR
…G label Three findings from the fifth review round, all verified and all valid. Emptiness has to be detected semantically, not as the exported `never`. Measured: `close([number], never)`, `close([number], or())` and `close([number], close([never]))` all accept `[1, ,]` in `validate` and `parse` while their canonical data equals `close([number])` and rejects it — so an implementation keying on the singleton would pass a `never`-only proof with the disagreement intact. The task now says so and asks for one independently constructed empty rest in the proof. A row in the shared acceptance table is not enough for either task that adds one: it pins that the three readers agree, so it passes whenever all three move together. Both bullets now ask for an outright verdict beside the row, as `optionalPositions` does. And the changelog label was wrong. Narrowing `close(c, never)` on a trailing hole is observable for callers using the explicit-rest spelling, whatever the canonical semantics already said, so it is **BREAKING** — as #1712 labelled its analogous reader-alignment change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xp3Axb2CF3pSjedM9sqmM
Implements
fjs/types/rtti/todo/sparse-tuple-schema-entries.md, and deletes it.The disagreement
A
Tupleschema isreadonly Type[], and the three readers disagreed about what a sparse one declares.../data/module.f.mjs'scontainerUnionwalks the schema withfor (const item of c), which yieldsundefinedfor a hole and visits every index;parse'sconstContainerParseandvalidate'sconstContainerValidatewalked it withObject.entries(rtti), which skips holes entirely. So a hole was a declaredundefinedposition to one reader and no position at all to the other — which breaks the agreementvalidate/proof.f.mjspins as a table.Reproduced on
6228f0e, before this change:validateparsenew Array(1)[1, 2, 3][, number][9, 5]close(new Array(1))new Array(1)close(new Array(1))[undefined]closereaches it from the other side —declared.lengthwas0while the value's length was1— but it is the same bug, and predatesclose.The same entry reading also declared a tuple schema's non-index enumerable own properties, which is the second acceptance change here.
Object.assign([number], { foo: string })was a disagreement too — error fromparse/validate, ok from the data form — and it was not merely stricter: a tuple is read by index, sofoobecameNumber('foo')→NaNand was matched against the property literally namedNaN. A value carryingfoo: 'x'failed; one carryingNaN: 'x'passed.The decision
Length wins, as the issue proposed: a hole is a declared position whose schema is
undefined. Reading index0ofnew Array(1)yieldsundefined, andundefinedis aConstschema in its own right, so the length reading is what follows fromTuplebeingreadonly Type[]. It is also whattoDataalready does, which keeps the canonical — content-addressed — data form fixed.The alternative (entries win,
containerUnionswitches toObject.entries) is rejected in the issue and incommon/module.f.mjs's new doc comment: it would makenew Array(1)and[]the same schema while[undefined]stayed different from both.The change
rtti/commongains the two per-kind entry readers and theSchemaEntriestype that names them:They are passed to all four container factories —
constContainerParse,constContainerValidate, and the twocloseones — beside thegetItemknob those already take, so each factory is instantiated per kind with the entry reading that kind wants.Array.fromagrees withcontainerUnion'sfor…ofexactly, and is identical toObject.entrieson a plain dense array, which is the only shape any schema in this repository actually has: nothing about a dense schema changes.closeParse/closeValidatenow dispatch with an explicit ternary rather than selecting a function first, because the two arms no longer share one signature.Proof
validate/proof.f.mjs's acceptance table — the one table run through all three readers, and where this should have shown up — gains ten rows: five open sparse, three closed sparse, and two for the non-index property. AsparseTuplegroup then states the verdicts outright rather than only that the readers agree, includingassertError(validate([, number])([9, 5])), the closed arity, and the non-index case.common/proof.f.mjspins the entry-reading contract directly.npx tscclean,fjs t3386/3386, andfjs/types/rtti/**/module.f.mjsat 100% line/branch/function coverage.Left open, not decided here
Both readers walk a schema by iteration, so an overridden
Symbol.iteratoror an inherited numeric index changes what it declares. Neither is a reader disagreement —containerUnionmakes the same walk, and both were disagreements before this PR — but whether the walk should read own indices instead is a real question, and answering it moves the content-addressed data form. Filed asfjs/types/rtti/todo/schema-walk-own-indices.mdwith both probes.Changelog:
types/rtti:parseandvalidateread a tuple schema by length, so a hole in one is a declared position whose schema isundefined—validate([, number])([9, 5])is now an error, agreeing with the data form. Dense schemas are unaffectedtypes/rtti: a tuple schema's non-index enumerable own property is no longer a declared member — a tuple is read by index, so such a key was matched againstvalue[NaN]. The data form always ignored it