Remove unreachable throw in djs serializer's constSerialize - #1544
Conversation
consts only ever holds values getConstants found shared for — i.e. values that already have an entry in refs — so refs.get(entry) inside constSerialize can never be undefined through the public stringify. Replace the defensive throw with a type assertion documenting the invariant, per AGENTS.md's guidance to restructure genuinely unreachable code away rather than leave it uncovered. fjs/djs/serializer/module.f.mjs reaches 100% line/branch/function coverage.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 47417f9 | Commit Preview URL Branch Preview URL |
Aug 14 2026, 07:46 AM |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 161a548d56
ℹ️ 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".
| if (refCounter === undefined) { | ||
| throw 'unexpected behavior' | ||
| } | ||
| const refCounter = /** @type {_RefCounter} */ (refs.get(entry)) |
There was a problem hiding this comment.
If getConstants is later changed so an emitted entry lacks a corresponding refs value, this inline @type cast suppresses the compiler warning and makes refCounter[0] fail at runtime. Preserve the invariant in the types instead—for example, have getConstants carry each _RefCounter alongside its entry—rather than overriding the inferred _RefCounter | undefined; repository guidance explicitly applies the avoid-assertions rule to inline JSDoc casts.
AGENTS.md reference: AGENTS.md:L678-L685
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4ba4111 by using assertNotNullish (a runtime-checked narrowing helper already used elsewhere in the codebase) instead of an unchecked @type cast — if the invariant ever breaks, this throws immediately rather than silently reading undefined[0].
I considered the "carry the counter alongside its entry" alternative too, but it would change getConstants's return type from List<Unknown> to something pairing each entry with its _RefCounter, which is a larger, more invasive change than this coverage-focused PR warrants — assertNotNullish gets the same safety (loud failure, not silent) with a much smaller diff.
Generated by Claude Code
| if (refCounter === undefined) { | ||
| throw 'unexpected behavior' | ||
| } | ||
| const refCounter = /** @type {_RefCounter} */ (refs.get(entry)) |
There was a problem hiding this comment.
use assert instead of type casting
There was a problem hiding this comment.
Fixed in 4ba4111 — switched to assertNotNullish(refs.get(entry)), which narrows via a runtime check instead of trusting an unchecked cast.
Generated by Claude Code
Address review feedback: assertNotNullish narrows refCounter's type via a runtime check instead of trusting an unchecked cast, while still failing loudly (not silently) if the invariant it documents ever breaks.
Prompted by review discussion on this PR: an inline @type cast documents an invariant but doesn't check it, so a broken assumption fails silently instead of at the point that assumed it. assert/assertNotNullish narrow the same way while also verifying the claim at runtime.
|
Added in 47417f9: a new paragraph in Generated by Claude Code |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. Baseline for every comparison below is origin/main at 6684bef2.
What the change actually is
Worth stating plainly, because the title and the CHANGELOG both read as a deletion: the
defensive check is not removed. throw 'unexpected behavior' becomes
assertNotNullish(refs.get(entry)), which still throws on a miss — only the thrown value
changes ('unexpected behavior' → 'assertion failed'). So even in the worst case where the
invariant were wrong, this is a message change on a throw path, not a newly-silent wrong value.
That is a materially lower-risk shape than a genuine dead-arm deletion, and it matches the
new AGENTS.md guidance the same PR adds.
Is the lookup really unreachable?
Statically it is a local, one-hop invariant: getConstants pushes djs onto consts only
under shared(djs) !== undefined, and sharedRef is refs.get(v) with a rc[1] > 1 filter —
same refs binding, never reassigned between the two uses. So refs.get(entry) in
constSerialize cannot miss.
Empirically, I instrumented the branch with a distinct throw 'PROBE_HIT' and drove the public
entry point stringify over a deterministic random DJS domain (all primitives incl. NaN,
-0, ±Infinity, '', bigints incl. a 30-digit one, null/undefined, arrays and objects to
depth 4, with a reuse pool so subvalues are deliberately shared — sharing is the only thing that
produces consts at all), under both sort and identity for the sort parameter:
PR head 47417f90: runs 400000, runs that emitted >=1 `const cN` 88096, PROBE_HIT 0, other errors 0
Negative control (relaxed getConstants' checkSelf guard to !state.added.has(djs), so
non-shared values — numbers, booleans, null, which countRefs never adds to refs — reach
constSerialize): runs 240432, PROBE_HIT 159568. The harness detects a hit.
Same probe against origin/main
Per the lesson from #1540 — where the removed arm was dead at the PR head but live on main,
killed by a separate unmentioned tightening — I ran the identical probe against 6684bef2 with
main's own throw re-labelled 'PROBE_HIT':
origin/main 6684bef2: runs 400000, with-consts 88096, PROBE_HIT 0, other errors 0
Dead on main too, and the module diff is the only change. Nothing else in this PR tightens the
input domain, so #1540's pattern does not apply here.
Behaviour neutrality
Same 400k-call sweep, SHA-256 over every emitted string (NUL-separated), both trees unmodified:
PR head 47417f90: 80d2a25b05295dcecb9ba1eac1bcb91caf4637d6817648babc7ced5c854afb5a
origin/main 6684bef2: 80d2a25b05295dcecb9ba1eac1bcb91caf4637d6817648babc7ced5c854afb5a
Identical. No divergence to classify.
Battery
npx tsc --noEmit— exit 0. TheassertNotNullishreturn type narrows well enough that
refCounter[0]still type-checks with no cast.npm run prepackfrom a freshly cleaned tree — exit 0 (both passes).npm test—pass: 2646, fail: 0on the PR head;pass: 2646, fail: 0onorigin/main.
Exact match.- Dual-axis public surface (
bin/extract.mjs/bin/consts.mjsover the emitted.d.mts,
prepackrun in both trees) — byte-identical on both axes. Expected: the edit is inside a
function body. No unprefixed type additions to weigh against §6.2, nothing widened toany. bin/linkcheck.mjs— broken-link sets identical (154 = 154,diffempty). The new
AGENTS.md link to./fjs/asserts/module.f.mjsresolves.- Rust gates skipped:
nanvm-lib/untouched.
Conventions
- §8.3: code change, one entry, links the PR only. AGENTS.md is docs and correctly gets no
entry of its own. Released sections untouched. - §8.4: no
**BREAKING CHANGES:**needed. This sits with #1524 (representation-only, correctly
unprefixed) rather than #1520 (specifier-level break): the emitted surface is provably
identical, and the only observable delta is the thrown value on a path proven unreachable on
bothmainand the PR head. - §6.2 / §4: no new types, no header changes.
The AGENTS.md section reads well and the assert-over-cast rationale is the right one — a cast
is erased, an assertion fails where the invariant actually broke.
One nit (non-blocking)
The CHANGELOG entry says constSerialize "drops a defensive throw". It doesn't drop it; it
swaps throw 'unexpected behavior' for assertNotNullish, which is still a throw. Something
like "replaces a hand-rolled undefined check with assertNotNullish" would describe the
diff more exactly — and would make the entry an example of the convention the PR is
simultaneously writing into AGENTS.md, which seems like the more useful thing for it to say.
Not worth a re-push on its own.
3 participants
Summary
fjs/djs/serializer/module.f.mjswas at 99.03% line / 98.72% branch coverage:stringify's innerconstSerializethrew'unexpected behavior'whenrefs.get(entry)wasundefined, but that can never happen through the publicstringify.consts(the listconstSerializeiterates) is built bygetConstants, which only adds a value viacheckSelfwhenshared(djs) !== undefined— andshareditself only returns non-undefinedwhenrefs.get(v)already has an entry. So every value inconstsis guaranteed to already have arefsentry by construction.AGENTS.md§3.2, the branch is removed rather than covered with a direct-invocation test — replaced with a type assertion and a comment documenting the invariant.fjs/djs/serializer/module.f.mjsnow reaches 100% line/branch/function coverage.Test plan
npx tsc --noEmitnode --test --experimental-test-coverage --test-coverage-include='fjs/djs/serializer/module.f.mjs' fjs/emergent_testing/all.test.mjs→ 100.00% line/branch/funcnode ./fjs/module.mjs t→ 2646 pass, 0 fail🤖 Generated with Claude Code
Generated by Claude Code