test: add a forceUTF16 harness helper; the slice-off-a-wide-char idiom yields 8-bit strings - #39478
test: add a forceUTF16 harness helper; the slice-off-a-wide-char idiom yields 8-bit strings#39478robobun wants to merge 1 commit into
Conversation
…yields 8-bit strings Several tests forced 16-bit string storage with (s + "\u0100").slice(0, -1) or the same with an emoji. JSC serves that slice straight from the rope's first fiber, which is the original 8-bit string, so these "16-bit path" tests were running the 8-bit path a second time. forceUTF16 builds the string through a utf16le round trip and asserts it really is 16-bit; the five call sites now use it.
|
Status: ready for review. Test-only change (no Verified on a debug build that the old idiom returns an 8-bit string at every length used by these tests and that |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 1 remains after this review. WalkthroughChangesThe test harness adds UTF-16 Test Inputs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This test-only change makes the UTF-16 coverage helper explicit and verifies the affected tests with real 16-bit inputs; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Nothing to address from the automated review. The Buildkite build is still running; the failing claude-find-issues job is the action failing to start (it fails the same way on unrelated PRs, e.g. #39453) and is not related to this change. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only change replacing a broken 16-bit-string idiom with a verified harness helper.
Checked that the utf16le round-trip preserves content exactly and that the length < 2 early return is harmless at every call site (headers loop len 0/1 is below SIMD stride; buffer sweep starts at 30 chars; sliceAnsi inputs are ≥4). The isUTF16() postcondition means future changes to Buffer's string materialization would fail loudly rather than silently drop coverage again.
Extended reasoning...
Overview
Adds forceUTF16(s) to test/harness.ts and replaces the (s + "\u0100").slice(0, -1) idiom (and its emoji variant) at five call sites across buffer.test.js, headers.test.ts, sliceAnsi.test.ts, and sliceAnsi-fuzz.test.ts. The old idiom returned the original 8-bit fiber of an unresolved rope, so the "16-bit path" tests were re-running the 8-bit path. The new helper round-trips through Buffer.from(s, "utf16le").toString("utf16le") and asserts isUTF16() on the result.
Security risks
None. Test-only; no production code, no external I/O, no new dependencies.
Level of scrutiny
Low. This is a mechanical test-quality fix: a shared helper replacing local lambdas that were provably not doing what their comments claimed. The helper has a self-checking postcondition, so if the utf16le materialization behavior ever changes the tests fail rather than silently losing coverage.
Other factors
- Verified the round-trip is content-preserving for arbitrary JS strings (per-code-unit LE encode/decode), so
out === ssemantically at every call site. - The
length < 2escape hatch is correct (JSC interns empty/single-Latin-1 as 8-bit) and no call site depends on len 0/1 being 16-bit — the headers sweep at len 0/1 is below any vector width, and all other inputs are ≥4 chars. - The PR description states all four affected test files pass on the debug build with the real 16-bit inputs, and the change is already on
mainas816f8930. - No prior reviews or comments on the timeline.
|
Nothing to change from the review. One correction to it: 816f893 is this branch's head, not a commit on main; the PR is not merged yet. Buildkite build #100253 is still running (24 of 179 jobs done so far). |
1 participant
Test-only follow-up to #39460.
Problem
(s + "\u0100").slice(0, -1)(or the same with an emoji) so they can exercise an API's 16-bit code path on Latin-1 content:test/js/node/buffer.test.js(the hex length sweep, the invalid-character sweep and thebuf.write"16-bit string path" case),test/js/web/fetch/headers.test.ts(lowercaseHeaderNameSIMD"16-bit: ... across lengths and alignments"),test/js/bun/util/sliceAnsi.test.ts("UTF-16 ASCII fast path") and both "encoding equivalence" tests intest/js/bun/util/sliceAnsi-fuzz.test.ts.s + "\u0100"is an unresolved rope, and slicing a range that lies entirely inside its first fiber returns that fiber, which is the original 8-bits. Checked withjscInternals.isUTF16Stringon a debug build: false at every length in the buffer sweep (15 to 1024 pairs) and for 0 to 160 characters of the headers alphabet (table below).DecodeHex16Implwere the new ones that contain genuine wide units; the length sweep never did.Fix
test/harness.ts:forceUTF16(s)builds the string through autf16leround trip (Buffer.from(s, "utf16le").toString("utf16le"), which Bun materializes as a 16-bit string) and throws if the result is not 16-bit, so a future change to how such strings are materialized fails the test instead of silently dropping the coverage again. Strings shorter than 2 code units are returned as-is: JSC interns the empty string and single Latin-1 characters as 8-bit, so they cannot be forced.toUTF16/to16lambdas and the comments describing the old trick are removed.test/js/node/buffer.test.js(645 pass),headers.test.ts,sliceAnsi.test.ts,sliceAnsi-fuzz.test.ts(313 pass across the three) all pass with real 16-bit input, so the 16-bit kernels they now reach agree with the 8-bit ones on this content.escapeHTML,stringWidth,stripANSI,yaml, theucs2fill case inbuffer.test.js) keep the wide character in the string, so they are genuinely 16-bit and are unchanged.Background
sliceAnsi) have a separate implementation per storage width, and an all-ASCII string can legitimately arrive in 16-bit storage (for example a substring of a string that also contained non-Latin-1 text), so tests want to run both.a + bon strings produces a rope (a lazily concatenated string withaandbas fibers). Substring of a rope that falls inside a single fiber returns that fiber directly rather than flattening the rope, which is why slicing the appended character back off hands back the original 8-bit string.bun:internal-for-testing'sjscInternals.isUTF16String(exposed by the harness asString.prototype.isUTF16and thetoBeUTF16Stringmatcher) reports the actual storage width; the helper uses it as its postcondition.Storage width of the old idiom vs the helper (debug build)
charsis the length of the ASCII input; each column isisUTF16String(...)of the result.(s + "\u0100").slice(0, -1)Buffer.from(s, "utf16le").toString("utf16le")("hello world" + "\u{1F600}").slice(0, 11)and(s + "\u{1F600}").slice(0, -2)(the sliceAnsi variants) are also false. Slicing a rope that has been resolved first (for example after acharCodeAt) does stay 16-bit, which is presumably how the idiom was originally observed to work.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.