Remove unreachable fallback branch in UTF-16 decode step - #1540

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fefab25d23

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe050dde98

Reviewed at b98491f3 against origin/main e7e1881a.

The code change is correct, and I proved the unreachability rather than taking it on trust. One
thing to fix: the CHANGELOG entry describes the wrong half of the change.

The branch is dead — but only because this PR made it dead

I instrumented the removed arm with a throw and drove toCodePointList over its full domain:
all 65,536 code units in every state position (fresh, pending high surrogate at both ends of the
high range, after a BMP word, after a lone low surrogate, after a completed pair), each with the
empty suffix and with [0xDC00] / [0x0041] / [0xD800] appended, plus empty input, truncated
pairs, valid-pair-then-EOF, and out-of-domain words (-1, 0x10000, NaN, ±Infinity, 1e21,
and fractional values straddling each range boundary). 1,835,456 sequences.

So on the merge base the arm is not dead. It fires on non-integer words in a pending-surrogate
state — e.g. toCodePointList([0xD800, 55295.5]), [0xD800, 56319.5], [0xD800, 57343.5]. The
three predicates partition only the integers in 0x0000–0xFFFF; contains is b <= i && i <= e,
so 56319.5 slips past u16, past isLowSurrogate, past isBmpCodePoint and past
isHighSurrogate, landing in the fall-through. Nothing to do with #1537's shared EOF flush — that
change did not touch this classifier's reachability.

What makes the arm dead is the other edit in this PR: u16 = i => Number.isInteger(i) && isInU16Range(i).
That is the right fix, the reasoning in the new JSDoc is exactly correct, and the two new proof
cases pin it.

Deletion changes nothing on the integer domain

Same sweep, old toCodePointList vs new, restricted to integer words: 1,835,036 sequences, 0
differences. Over the full sweep including non-integers: 196 of 1,835,456 differ, all of them
non-integer inputs, and all of them improvements — main returns toCodePointList([0.5]) === [0.5],
i.e. it emits the fractional value as a code point; this PR returns [0xFFFFFFFF]. Likewise
[55295.5] was [-2147428353] (a flagged-but-fractional code point) and is now the invalid
sentinel.

§8.3 — the entry describes the removal, not the fix

text/utf16: utf16ByteToCodePointOp drops its trailing fallback arm — after ruling out a low
surrogate, a non-BMP word is always a high surrogate, so the isHighSurrogate recheck and its
errorMask fallback were dead code

They were not dead code on main; they became dead code in this commit. And the branch removal is
the part with no observable effect, while the u16 tightening — the part the entry doesn't
mention — is the only user-visible change here (196 input sequences change output). As written the
entry tells a reader "nothing changed for you", which is wrong for anyone feeding non-integer
U16 values.

Suggested shape:

text/utf16: u16 now rejects non-integers, so a fractional word is reported invalid
(0xFFFFFFFF) instead of being misclassified by the surrogate/BMP range checks — which only
partition the integers in 0x0000–0xFFFF. That closes the only path into
utf16ByteToCodePointOp's trailing fallback arm, which is removed along with its
isHighSurrogate recheck.

The rest of the verification battery, on top of the sweeps above:

• npx tsc --noEmit — exit 0.
• npm run prepack from a cleaned tree — exit 0 (both passes).
• npm test — 2553 pass / 0 fail, vs 2551 on origin/main; +2 is exactly the two new proof cases.
• Public surface, bin/extract.mjs + bin/consts.mjs after prepack in both trees — byte-identical
  to main. No new exports, no widening to any.
• bin/linkcheck.mjs — broken-link sets identical to main (137 each). Deleting
  todo/unreachable-pending-state-fallthrough.md strands nothing; nothing in the tree references it.

Not blocking the code — just the changelog wording.

Re-reviewed at 4e2b6738, baseline origin/main = e7e1881a (unchanged since the last round). The open finding from b98491f3 is resolved — approving.

The previous finding is fixed

Last round the entry read "…the isHighSurrogate recheck and its errorMask fallback were dead code", which documented the no-op half of the change and said nothing about the u16 tightening that is the actual user-visible half. The only diff between b98491f3 and 4e2b6738 is CHANGELOG.md, and it now leads with the real change:

u16 now rejects non-integers, so a fractional word is reported invalid (0xFFFFFFFF) instead of being misclassified by the surrogate/BMP range checks … That closes the only path into utf16ByteToCodePointOp's trailing fallback arm, which is removed along with its isHighSurrogate recheck

That is the right causal order: the u16 change is the change, the deletion is its consequence. Nothing left to say here.

§8.4: no **BREAKING CHANGES:** prefix, and "fix" is the right characterisation

Verified rather than assumed, since the assessment turns on it:

• Public surface is untouched. npm run prepack in both trees, then extract.mjs (46 326 bytes on main) and consts.mjs (64 315 bytes on main) — both diffs empty. No export added, removed, retyped, or widened to any; no new unprefixed public export type. u16 is module-private and stays that way.
• No valid input changes behaviour. 400 000 driven sequences (length 1–5) over integer boundary words (0, 0x41, 0xD7FF, 0xD800, 0xDBFF, 0xDC00, 0xDFFF, 0xE000, 0xFFFF, plus out-of-range 0x10000/-1), main vs this head: 0 diffs.
• The inputs that do change were already broken. 200 000 mixed sequences seeded with fractional words: 146 884 diffs — every one of them main producing something that is not a code point. toCodePointList([0.5]) returned [0.5] on main and returns [4294967295] here; toCodePointList([55296, 56319.5]) returned [-2147428352, -2147427329] and now returns [4294967295, -2147428352].

So the only observable difference is on inputs the declared U16 contract already excludes, and on those the function moves from violating its own output contract (emitting 0.5 as a code point) to emitting the documented invalid sentinel. SemVer's break signal is about the API contract, not about every observable value, and no contract-conforming caller can be broken by this.

On the repo's own precedent this is the #1524 case, not the #1520 case. #1520's prefix records a specifier-level break — files that consumers import by path stopped existing. #1524 (decToBin no longer returning a 54-bit mantissa) changed what comes out of a function, kept the API shape, and correctly carried no prefix. #1540 is the same shape as #1524, and arguably a weaker claim on the prefix: #1524 changed the representation of results for in-contract inputs, whereas #1540 leaves in-contract inputs bit-identical and only corrects out-of-contract ones. "Reported invalid … instead of being misclassified" reads as a fix because it is one. No prefix.

Re-verified at this head

The code is byte-identical to b98491f3 (diff is CHANGELOG.md only), but the reachability argument is the whole basis of the change, so I re-derived it rather than carrying it forward — and this time exhaustively rather than by sampling:

• The removed arm is unreachable, by exhaustion. With u16 now admitting only integers in 0x0000–0xFFFF, the count of integers in that range classified by none of isLowSurrogate/isBmpCodePoint/isHighSurrogate is 0 — over all 65 536 of them, not a sample. Negative control: re-running the same sweep against a deliberately holed predicate (isBmpCodePoint minus 0x41) reports 1, so the check can see a gap when one exists. Since isLowSurrogate is taken above, a non-BMP word at that point is always a high surrogate; the deleted return had no preimage.
• npx tsc --noEmit — exit 0.
• npm run prepack from a clean tree — exit 0, both trees.
• npm test — 2553 pass / 0 fail, vs 2551 on e7e1881a. The +2 are exactly the two new toCodePointList proof cases.
• npm run cov — fjs/text/utf16/module.f.mjs at 100.00 / 100.00 / 100.00 (lines / branches / functions), which is the acceptance criterion the deleted unreachable-pending-state-fallthrough.md set for itself. Whole tree 99.94 / 98.37 / 99.78.
• linkcheck.mjs — 137 broken relative links, byte-identical sets on both trees; deleting the todo stranded nothing (no remaining reference to unreachable-pending-state-fallthrough anywhere in the tree).
• Rust gates not run: nanvm-lib/ untouched.

The two new proof cases are the right ones — they pin both the bare fractional word and the fractional-word-during-pending-surrogate interaction, which is the case where the old code produced its worst output.