Improve djs/tokenizer branch coverage: 96.83% → 98.40% - #1553
Conversation
- parseDjsMinusState: drop the `case '-'` arm. js/tokenizer always
merges two adjacent '-' characters into a single '--' token, so
minus-state (entered only after one unmerged '-') can never itself
see another '-'-kind input; such input already falls through to
`default` unchanged. Corrects a proof.f.mjs comment that had
misattributed the '--' test's error to this (dead) arm — it's
actually the default state's unknown-token fallback mapping the
whole '--' token to one error.
- metadataAfterTag: its one call site only ever passes a `tag` already
confirmed present via `flatTokens.includes(tag)`, so `indexOf` can
never return -1 there. Drop the `idx < 0` ternary and document the
invariant instead of branching on an impossible case.
- Add a `djsTokenize` proof case for '-' followed by a token that's
neither a number/bigint nor eof (`'-{'`), covering
parseDjsMinusState's `default` arm, which no existing test reached.
Two remaining low-branch-count gaps (stringDecodeScan's grammar-
guaranteed escape-char default, tagToToken's boolean-tag default) are
left as-is: eliminating them cleanly would require deeper changes
(literal-typed escape chars, or tracing whether the shared AstTag
type's `true` variant is reachable through this specific grammar)
that don't fit this coverage pass.
|
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 | e4776e4 | Commit Preview URL Branch Preview URL |
Aug 14 2026, 05:04 PM |
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. Baseline origin/main = f04c670bfe3283dc0ea579aeae37b3f17667feb9; tested head 656f0367.
The title states a number, so I measured it
npm run cov produces real per-file numbers in this environment (2675 tests on main, not the vacuous 0-test 100.00 case), so the claim is checkable:
| line % | branch % | funcs %
main djs/tokenizer | 100.00 | 96.83 | 100.00
PR djs/tokenizer | 100.00 | 98.40 | 100.00
main all files | 99.96 | 98.64 | 99.81
PR all files | 99.96 | 98.71 | 99.81
96.83% → 98.40% — exactly the title. npm test: 2675 pass / 0 fail on main, 2676 pass / 0 fail here. npx tsc --noEmit and npm run prepack from a freshly cleaned tree both exit 0.
Worth naming plainly: most of that gain comes from deleting two unreachable branches, not from covering them. That is the right move — but it only holds if they really are unreachable, so that is what I spent the review on.
parseDjsMinusState's case '-' — verified dead
The claim is that js/tokenizer always merges two adjacent - into one '--' token, so minus-state can never see a -. I checked it two ways rather than reading the merge logic.
Exhaustively, by tokenizing every string over a 29-character alphabet (- / * \ space \n a 1 " ' > = + . ? # \r \t @ ! < | & % ^ ~ ; ,) up to length 4, plus every string over a 9-character alphabet at lengths 5 and 6, and searching for two adjacent -`-kind js tokens:
strings up to len 4 producing two adjacent "-" js tokens: 0
len 5-6 restricted alphabet hits: 0
The reason is visible in the token streams: separating two - characters necessarily inserts a token between them, so minus-state sees that instead — '- -' gives [error, ws, error, eof], '---' gives [error, error, eof] ('--' then '-', the '--' landing in default state, not minus-state).
And by execution, replacing the arm on origin/main with a throw: the full 2675-test suite still passes. parseDjsMinusState is a module-private const reachable only through tokenize, so those two together are convincing.
I also confirmed the probe is not vacuous — the same throw placed in the live case 'bigint' arm fails 2 tests, so a reached arm does get caught.
Correcting the stale comment on the '--' proof case is a real improvement on its own: main's comment said "stays in minus-state waiting for what follows", which was never true. The old case '-' looked live because of that comment.
metadataAfterTag's idx < 0 guard — verified dead
One call site (line 546), and it is inside if (structuralError !== null) where structuralError was itself derived from flatTokens.includes('unterminated') ? 'unterminated' : flatTokens.includes('numError') ? 'numError' : null. The tag passed is one the array is already known to contain, so indexOf cannot be -1. The same throw substitution on origin/main also leaves all 2675 tests passing.
The new proof case is two-sided
The '-{' case targets minus-state's default arm. Mutating that arm to drop the mapped follow-on token (tail: mapDjsToken(input) → return the error alone):
- on this PR: 2675 pass, 1 fail — the new case catches it;
- the identical mutation on
origin/main: 2675 pass, 0 fail — it kills nothing there.
So this is genuinely new coverage, not a case that main already had.
On the failure modes this family has hit before: #1518 pinned an error message and let a branch-changing mutation through, and #1525/#1528 pinned a single field so a mutant that returned the right tag while clobbering state survived. Neither applies here — the assertion is stringify over the whole result array, so kinds, values, line/column/path metadata and the token count are all pinned at once. That is what let the mutation above be detected: it changes the number of tokens, and the assertion sees it. Nor is this the #1535 shape, where proof.throw legitimately asserts only that something throws; this is a full value comparison.
Conventions
- §8.3: a code change, so it needs a CHANGELOG entry and has one, in
## Unreleased, linking only/pull/1553, no issue ortodo/reference. Accurately describes the change as dead-branch removal rather than as new tests. - No new exports and no new
export type, so the §6.2_-prefix rule is not engaged; module-private consts only. - §2 file pairing untouched; the proof stays
proof.f.mjsalongsidemodule.f.mjs.
Two small notes, neither blocking:
- The new entry is followed by a blank line, which main's entries are not (
main: 1 of 30 items preceded by a blank line, the first one after the heading; here: 2 of 31). A blank line between items makes the whole## Unreleasedlist loose in Markdown, so one entry's spacing changes how every entry in the section renders. Dropping that one blank line keeps it tight. - This touches
CHANGELOG.md, which #1552 replaces withchangelog/. Whichever lands second will need its entry re-homed aschangelog/unreleased/1553.md.
Non-blocking review nit: a blank line between two entries makes the whole Unreleased list loose in Markdown, changing how every entry in the section renders, not just the one it's next to.
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Re-review at 78e1b0a8 after the previous approval at 656f0367. Baseline: origin/main at f04c670b (unchanged since the last round).
The delta is one line. git diff 656f0367 78e1b0a8 is CHANGELOG.md | 1 - — the stray blank line inside the ## Unreleased list, and nothing else. fjs/djs/tokenizer/module.f.mjs and proof.f.mjs are byte-identical to the head I already approved, so the settled findings carry forward unchanged: the two removed branches proven dead, the two-sided mutation on the new '-{' case, and the measured djs/tokenizer branch coverage 96.83% → 98.40%.
Re-ran on the new head anyway, from a cleaned tree:
npx tsc --noEmit— exit 0.npm test—pass: 2676, fail: 0, total: 2676, matching the count established at the previous head.
Note 1 (blank line in the Unreleased list) — resolved. The ## Unreleased section now carries exactly the same blank-line structure as main's (2 blank lines in the section on both, i.e. only the one after the heading and the one before the next ##), so the list stays tight and the whole ## Unreleased block renders the same way it does on main.
Note 2 (CHANGELOG re-homing) — still open, still non-blocking. This PR writes its entry into CHANGELOG.md. #1552 ("Migrate changelog from single file to directory structure") is still open and approved at 631223dd, and it replaces that file with a changelog/ directory. Whichever of the two merges second will need this entry re-homed as changelog/unreleased/1553.md, with the body only and no heading inside the file, per changelog/README.md. Purely a merge-order matter — nothing to change here unless #1552 lands first.
Approving.
…coverage-improvement # Conflicts: # CHANGELOG.md
Summary
fjs/djs/tokenizer/module.f.mjswas at 96.83% branch coverage. This closes two of the six uncovered branches by restructuring away code that's genuinely unreachable, and a third by adding a missing test:parseDjsMinusState'scase '-'—js/tokenizeralways merges two adjacent-characters into a single'--'token (the decrement operator), so minus-state (entered only after one unmerged-) can never itself see another'-'-kind input. Dropped the case; such input already falls through todefaultunchanged, which is correct since it never actually happens. Also corrected aproof.f.mjscomment that had misattributed the existing'--'test's error to this (dead) arm — it's actually produced by the default state's unknown-token fallback, mapping the whole merged'--'token to one error.metadataAfterTag'sidx < 0branch — its one call site only ever passes atagit already confirmed present viaflatTokens.includes(tag), soindexOfcan never return-1there. Dropped the ternary and documented the invariant instead.parseDjsMinusState'sdefaultarm — genuinely reachable (-followed by a non-number/bigint/eof token, e.g.-{) but had no test. Added one.Per
AGENTS.md§3.2, the first two are removed rather than covered with contrived direct-invocation tests (matching the pattern from #1540, #1541, #1544).Two remaining low-branch-count gaps are left as-is, noted in the commit message:
stringDecodeScan's grammar-guaranteed escape-chardefault(the switch is over a plainnumber, so eliminating it cleanly would need literal-typed escape-char constants — a larger change) andtagToToken's boolean-tagdefault(would need tracing whetherAstTag's sharedtruevariant is actually reachable through this specific grammar, which touches the wholebnfsubsystem). Branch coverage moves from 96.83% to 98.40%.Test plan
npx tsc --noEmitnode --test --experimental-test-coverage --test-coverage-include='fjs/djs/tokenizer/module.f.mjs' fjs/emergent_testing/all.test.mjs→ 100.00% line, 98.40% branch, 100.00% funcnode ./fjs/module.mjs t→ 2676 pass, 0 fail🤖 Generated with Claude Code
Generated by Claude Code