fmt: decode multi-byte code points in the identifier formatter - #30993
Conversation
|
✅ Review addressed in 038103e — the
CI (#55937): every individual lane is green — all 73 Buildkite contexts pass in |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
WalkthroughImplements WTF-8 decoding in ChangesUnicode String Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the WTF-8 decode is textbook, bounds-checked, and only reachable from FormatValidIdentifier — but ~30 lines of hand-rolled multi-byte decode in bun_core/lib.rs (when a full CodepointIterator already exists in string/immutable) is worth a quick human sanity check.
Extended reasoning...
Overview
Completes the TODO(port) stub in strings_impl::CodepointIterator::next (src/bun_core/lib.rs:2431) so it actually decodes multi-byte WTF-8 sequences instead of stepping one byte at a time. The only consumer of this crate-root stub is FormatValidIdentifier (src/bun_core/fmt.rs:1243) — all other CodepointIterator callers (lexer, printer, glob, shell, etc.) resolve through bun_core::strings:: / bun_core::immutable:: to the full NewCodePointIterator impl in string/immutable/unicode.rs, which is untouched. A regression test in bundler_naming.test.ts pins café-utils.js → require_caf_utils (single _).
Security risks
None. Input is a filename basename already held in memory; output is a generated symbol name. The slice self.bytes[i + 1..i + seq] is guarded by short-circuit && on i + seq <= self.bytes.len(), and the _ match arm is only reachable with seq == 4 (gated by (2..=4).contains(&seq)), so all indexing is in-bounds. Invalid/truncated sequences fall back to a 1-byte step, so the iterator can't get stuck or panic.
Level of scrutiny
Medium. The bit masks/shifts are the canonical UTF-8 decode (0x1F<<6, 0x0F<<12, 0x07<<18 + 0x3F continuations), the consumer mostly only cares about width > 1 vs width == 1 (so even a wrong codepoint value would just affect underscore count, not bundle correctness), and the test's run: { stdout: "1" } confirms the bundle still executes. That said, this is hand-written decode logic in a foundational crate, and there's already a full WTF-8 iterator in string/immutable/unicode.rs — a maintainer may prefer wiring fmt.rs to that rather than maintaining a second decoder here.
Other factors
No CODEOWNER for these paths. No prior review comments. The decode doesn't reject overlongs/surrogates, but WTF-8 permits surrogates and the consumer collapses any width > 1 to _ regardless, so it's immaterial for this use. One pre-existing quirk (not introduced here): fmt.rs:1255 checks is_identifier_start on the first codepoint without the width > 1 guard, so a leading multi-byte ID_Start char could pass through verbatim — but that's consumer logic unchanged by this PR and presumably matches the Zig original.
|
@robobun fix review comments |
Replaces the inline multi-byte WTF-8 decode with a call to the canonical decode_wtf8_rune_t used by strings::CodepointIterator::next, so there is a single decode path to maintain.
There was a problem hiding this comment.
LGTM — completes the TODO stub by delegating to the canonical decode_wtf8_rune_t, with a regression test.
Extended reasoning...
Overview
This PR fixes strings_impl::CodepointIterator::next in src/bun_core/lib.rs (≈20 lines), which previously had an explicit TODO(port) stub that returned width=1 for every byte regardless of UTF-8 sequence length. The fix delegates multi-byte decoding to the existing canonical crate::string::immutable::decode_wtf8_rune_t (the same decoder backing the real strings::CodepointIterator). A regression test in test/bundler/bundler_naming.test.ts asserts that café-utils.js produces require_caf_utils rather than require_caf__utils.
Security risks
None. The only consumer is FormatValidIdentifier (src/bun_core/fmt.rs:1239), which sanitizes source basenames into generated bundler symbol names. The decode is bounds-safe: take = min(len, tail.len()) clamps the slice, the 4-byte buffer is zero-padded, and decode_wtf8_rune_t_multibyte validates each continuation byte ((b & 0xC0) == 0x80) — zero padding for truncated input fails validation and falls back to width=1 + U+FFFD. Invalid lead bytes (0x80–0xBF, 0xF8+) get len=1 from wtf8_byte_sequence_length and step one byte. No untrusted-data parsing, no allocation, no panics reachable.
Level of scrutiny
Low–moderate. This is a port-completion bug fix that replaces a placeholder with a call to already-tested infrastructure rather than introducing a new decoder. The behavioral surface is narrow (cosmetic symbol naming in bundler output) and the regressed behavior was strictly worse (one _ per UTF-8 byte). The follow-up commit 038103e addressed earlier review feedback by removing the inline decode in favor of the shared helper, reducing duplication risk.
Other factors
No CODEOWNERS cover the touched paths, the bug-hunting system found no issues, and the new itBundled test both inspects the generated symbol and runs the bundle to verify execution. The change is self-contained with no design decisions warranting human deliberation.
There was a problem hiding this comment.
LGTM — finishes the TODO stub by delegating to the canonical decode_wtf8_rune_t, with a regression test pinning the symbol name.
Extended reasoning...
Overview
Replaces the placeholder strings_impl::CodepointIterator::next (which returned width=1 for every byte, with an explicit TODO(port)) with a real WTF-8 decode that delegates to the existing canonical decode_wtf8_rune_t decoder. Adds a bundler regression test asserting café-utils.js produces require_caf_utils (single underscore) and that the bundle runs.
Security risks
None. This is pure string decoding for generated identifier names in bundler output. No user-controlled data flows into auth, filesystem, or network paths; invalid/truncated sequences fall back to width=1 + replacement char, so iteration always advances and cannot loop or read out of bounds (input is bounds-clamped via .min(tail.len()) and copied into a zero-padded 4-byte buffer before decode).
Level of scrutiny
Low. The change is ~20 lines, replaces a stub that was already known-broken (explicit TODO), and routes through the same decoder the canonical strings::CodepointIterator uses rather than reimplementing WTF-8 logic. The only consumer (FormatValidIdentifier::fmt at fmt.rs:1239) keys off cursor.width > 1 / == 1, which this change now sets correctly. Behavior for ASCII and for invalid lead bytes is unchanged.
Other factors
wtf8_byte_sequence_lengthnever returns 0 (returns 1 for invalid leads), solen >= 1and the decoder'slen > 1debug-assert in the multibyte path holds.- Truncated tails are handled: zero-padded continuation bytes fail the
(b & 0xC0) != 0x80check insidedecode_wtf8_rune_t_multibyte, yielding-1→ width=1 fallback. - New test exercises both the generated symbol name and runtime execution of the bundle.
- No CODEOWNERS for the touched paths; bug-hunting system found nothing.
The libuv-derived uv__tty_make_raw() calls assert() but the header was only pulled in transitively by other files in the same unified source group; when the grouping shifts the build fails with 'use of undeclared identifier assert'.
There was a problem hiding this comment.
LGTM — the stub now mirrors the canonical strings::CodepointIterator::next decode line-for-line, with a regression test covering the café case.
Extended reasoning...
Overview
This PR touches three files: src/bun_core/lib.rs replaces the TODO(port) stub in strings_impl::CodepointIterator::next (which previously returned (byte, width=1) for every byte) with a real WTF-8 decode that defers to decode_wtf8_rune_t; test/bundler/bundler_naming.test.ts adds a regression test asserting café-utils.js produces require_caf_utils (single underscore) and runs the bundle; src/jsc/bindings/wtf-bindings.cpp adds the missing <assert.h> include for uv__tty_make_raw's assert() call.
Security risks
None. The affected path generates internal CommonJS wrapper symbol names from source basenames during bundling — purely cosmetic output naming, no auth/crypto/permissions/untrusted-input parsing implications. The <assert.h> include is a build-hygiene fix.
Level of scrutiny
Low-to-moderate. The new next() body is a near-verbatim copy of the already-shipping canonical iterator at src/bun_core/string/immutable.rs:320-346 (same wtf8_byte_sequence_length → clamp-to-tail → zero-padded [u8;4] → decode_wtf8_rune_t with -1 sentinel → width=1 fallback on invalid). I traced the edge cases: truncated tail (missing continuation bytes become 0x00 in the buffer and fail the (b & 0xC0) != 0x80 check → -1 → width=1), invalid lead bytes 0x80-0xBF/0xF8-0xFF (wtf8_byte_sequence_length returns 1 → single-byte step), and valid multi-byte (width=len). No infinite-loop or panic risk; len is always ≤4 so buf[..take] is in-bounds.
Other factors
No CODEOWNERS cover these paths. The bug-hunting system found nothing. A focused regression test was added that asserts both the positive (require_caf_utils) and negative (not.toContain("require_caf__utils")) and executes the bundle. The FormatValidIdentifier consumer at fmt.rs:1259/1277 only cares about width > 1 vs width == 1, which the fix gets right for all valid multi-byte sequences.
|
Updated 1:38 AM PT - May 20th, 2026
✅ @Jarred-Sumner, your commit fd49d432d84d8ef15b3f13258e50bb848d01d106 passed in 🧪 To try this PR locally: bunx bun-pr 30993That installs a local version of the PR into your bun-30993 --bun |
There was a problem hiding this comment.
LGTM — the stub now mirrors the canonical strings::CodepointIterator::next decode path verbatim, and the wtf-bindings change is just duplicate-include cleanup.
Extended reasoning...
Overview
Three files: (1) src/bun_core/lib.rs finishes the strings_impl::CodepointIterator::next stub by replacing its TODO(port) byte-at-a-time fallback with a real WTF-8 decode; (2) src/jsc/bindings/wtf-bindings.cpp drops one redundant #include <cassert> inside the #if !OS(WINDOWS) block (a top-level <cassert> and a second in-block one remain); (3) a new naming/NonAsciiSourceFilenameSymbol regression test in bundler_naming.test.ts.
Security risks
None. This is pure string-decoding logic feeding the bundler's identifier-name sanitizer (which already replaces every non-identifier codepoint with _). No auth, crypto, FS, or network surface. The decode is bounds-safe: take = min(len, tail.len()) clamps the copy into a stack [u8; 4], and wtf8_byte_sequence_length never returns 0, so the iterator always advances ≥1 byte — no infinite-loop risk on malformed input.
Level of scrutiny
Low. The new multi-byte branch in lib.rs:2445-2458 is a line-for-line copy of the canonical implementation at src/bun_core/string/immutable.rs:332-344 (same wtf8_byte_sequence_length → clamp → 4-byte buf → decode_wtf8_rune_t → -1 sentinel → replacement-char fallback). There is no novel decode logic to audit; it just routes the stub through the already-tested decoder. The wtf-bindings.cpp hunk is a no-op include dedup. The test is additive.
Other factors
The bug-hunting system found nothing, CodeRabbit had no actionable comments, and CI lanes are reported green. The only behavioral effect is that non-ASCII basenames now collapse to one _ per codepoint instead of one per UTF-8 byte — exactly what the regression test pins down and what the PR description states matches released Bun's output.
3 participants
Bundling a module whose source basename contains non-ASCII characters generated a different CommonJS wrapper symbol name than released Bun:
café-utils.js→require_caf__utils(double_) instead ofrequire_caf_utils, because each UTF-8 byte oféwas walked individually.FormatValidIdentifierresolvedCodepointIteratorto a crate-root stub whosenext()returned(byte, width=1)for every byte and never decoded multi-byte sequences (it had an explicitTODO(port): full UTF-8 decode). The sanitizer'swidth > 1multi-byte-collapse branch was therefore dead.Finish the stub's decode using the in-module
wtf8_byte_sequence_lengthplus an inline WTF-8 decode (bounds- and continuation-byte-validated; invalid/truncated → 1-byte step). Generated symbol names are now byte-identical to released Bun forcafé/résumé/日本語. Regression test inbundler_naming.test.ts.