Add defensive branch coverage for endObject non-object guard - #1525
Conversation
djs/parser's endObject mirrors endArray's already-tested unreachable guard (state.top is never anything but an object when endObject runs through parseFromTokens), so cover it directly the same way pushKey and endArray already are, closing an uncovered branch in module.f.mjs.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | cad1b47 | Commit Preview URL Branch Preview URL |
Aug 13 2026, 11:55 AM |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Reviewed at cad1b47 (draft — reviewing it anyway, per the usual arrangement). The
case does what it says: it is a real mutant-killer, the branch really is
unreachable through parseFromTokens, and the coverage delta is measurable.
Approving, with one observation about assertion strength that applies equally
to the endArray sibling already on main.
Mutation test
Removing the guard entirely — const obj = fromMap(state.top[1]) — is the
mutation that matters, since with top: null the true arm cannot be taken.
(tsc rejects that edit, correctly, so I ran the runner directly with
node ./fjs/module.mjs t rather than through npm test.)
| tree | result |
|---|---|
this PR (cad1b47) |
pass: 2531, fail: 1 — the one failure is proof.endObject.nonObjectTop, nothing else |
origin/main (28a2e99) |
pass: 2531, fail: 0 — kills nothing |
So the new case is the only thing in the suite that constrains that branch, and
it did not previously exist.
The branch is genuinely unreachable through parse
Checked both ways rather than trusting the comment.
Statically: endObject has exactly three call sites — parseObjectStartOp,
parseObjectNextOp, parseObjectCommaOp — reached from foldOp's dispatch only
at value-states '{', '{v' and '{,'. '{' is written by startObject,
which sets top = ['object', null, '']; '{v' is written by pushValue's last
branch, which is reached only when top is non-null and non-array (the null
case returns earlier in both state cases); '{,' is written by
parseObjectNextOp's ',', which carries top through unchanged. top is an
object at all three.
Empirically: the guard-removal mutation on origin/main breaks nothing across
all 2531 tests, including the parse-level DJS ones — nothing in the existing
suite reaches it. Calling the internal directly is the right and established
answer here, as with pushKey.nonObjectTop and endArray.nonArrayTop in this
same file.
Coverage delta (npm run cov, both trees)
| main | PR | |
|---|---|---|
fjs/djs/parser/module.f.mjs branch |
93.61 | 94.05 |
| all files, branch | 98.17 | 98.20 |
Lines and functions stay at 100.00 for that file on both sides. npm test:
2531 → 2532.
One observation: the assertion pins the branch but not its result
Unlike the case on #1518, this one does distinguish the guarded path — the
mutation above kills it. But it asserts only result.state, and that is coarser
than the branch it covers. Changing the fallback from : null to : undefined
type-checks cleanly and the case still passes (pass: 2532, fail: 0), even
though the value pushed into module.consts changed. The branch's whole
observable output is that value.
I want to be clear this is a suggestion, not a defect: the pre-existing
endArray.nonArrayTop has exactly the same shape, so the new case is faithful to
the established pattern rather than weakening it, and the value is unobservable
through the public API by construction. If you want the case to pin behaviour and
not just reachability, asserting the pushed const (e.g. that
toArray(result.module.consts) is [null]) would close it — and the same edit
would apply to endArray.
Nit
The comment lists '{'/'{k'/'{:'/'{v'/'{,', but only three of those five
dispatch to endObject; '{k' and '{:' go to parseObjectKeyOp /
parseObjectColonOp, neither of which calls it. The claim stays true (all five
are object states), but the endArray comment right above lists exactly the
three states that reach it, so the two read inconsistently.
Rest of the battery
npx tsc --noEmit 0; npm run prepack from a clean tree 0; public surface
(extract.mjs + consts.mjs, after prepack in both trees, 46 KB / 65 KB of
output) byte-identical to main; broken-link sets identical (140 on both);
@module header survives into module.f.d.mts. CHANGELOG entry is at the top of
Unreleased, links only /pull/1525, and is within the length guidance.
createExclusiveNestedMissing, writeBytesNestedMissing, and writeBytesMissingFile all started from emptyState and only checked the error tag, so a mutant returning the right tag alongside a wiped root would pass unnoticed (the #1525/#1528 shape, previously found in this same file). Start each from a non-empty root and assert it survives untouched. writeBytesNegativeOffset only checked the error tag too, so weakening the offset guard from `!Number.isInteger(offset) || offset < 0` to `!Number.isInteger(offset)` would still pass (a different, later guard catches -1 as a size mismatch instead). Pin the exact message.
3 participants
Summary
Added a new test case to the proof object that covers a defensive, unreachable code branch in the
endObjectfunction. This improves test coverage for edge cases that cannot be reached through normal parsing flow.Key Changes
endObject.nonObjectToptest case to the proof objectstate.toptop: nullto trigger the unreachable branch'result'stateImplementation Details
endObjectis only invoked whenstate.topis an object during normal parsing (guaranteed by value-states like'{','{k','{:','{v','{,')parseFromTokenshttps://claude.ai/code/session_01SEkx5h5qGhKB7C6wAmsu2j