fix(transpiler): scanImports drops dynamic import() with non-ASCII specifier - #34015
fix(transpiler): scanImports drops dynamic import() with non-ASCII specifier#34015robobun wants to merge 1 commit into
Conversation
…ecifier The SCAN_ONLY branch of parse_import_expr only recorded a dynamic import when the string literal was stored as UTF-8 bytes. Any non-ASCII code point (including one produced by a \u escape in pure-ASCII source) makes the lexer store the literal as UTF-16, so is_utf8() is false and the branch fell through without calling add_import_record. EString::slice() already transcodes UTF-16 to UTF-8 (same call the full parse uses in transpose_import), so the is_utf8() guard is unnecessary.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:12 AM PT - Jul 12th, 2026
❌ @robobun, your commit b4f4095 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34015That installs a local version of the PR into your bun-34015 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line guard removal that aligns the SCAN_ONLY path with transpose_import, plus a parity test.
What was reviewed:
- Verified
EString::slice()(src/ast/e.rs:1920) transcodes UTF-16 viastring(), so droppingis_utf8()is safe. - Confirmed
transpose_import(src/js_parser/p.rs:1087) already callsslice()without the guard — this just bringsscanImportsto parity. - Test covers literal non-ASCII,
\u/\u{}escapes, CJK, emoji, and an ASCII control case; assertsscanImports==scan().importsrather than hardcoding path bytes.
Extended reasoning...
Overview
Removes the e_string.is_utf8() guard in the SCAN_ONLY branch of parse_import_expr (src/js_parser/parse/parse_import_export.rs:75), leaving only is_present(). Adds a test in test/bundler/transpiler/transpiler.test.js that iterates six dynamic-import specifiers (non-ASCII literal, \u00e9, \u{e9}, CJK, emoji, plain ASCII) and asserts scanImports() returns exactly one dynamic-import entry that matches scan().imports.
Security risks
None. This is parser bookkeeping — the only effect is that a dynamic import specifier containing non-ASCII characters is now recorded in the import-record list during scan-only mode, matching what the full parse already does. No untrusted-size arithmetic, no allocation-size changes driven by user input, no auth/crypto/permissions.
Level of scrutiny
Low. The diff is a single-token deletion whose correctness is directly verifiable against existing code: EString::slice() (src/ast/e.rs:1920-1923) calls string(), which branches on is_utf8() and transcodes UTF-16 via strings::to_utf8_alloc when false — so the removed guard was redundant. The full-parse path (transpose_import, src/js_parser/p.rs:1086-1087) already calls str_.slice(self.arena) with no is_utf8() check, so this change brings the two paths to parity rather than introducing new behavior.
Other factors
- The
is_present()guard is retained, so empty-string specifiers are still skipped (no behavior change there). - The test wisely asserts parity between
scanImports()andscan().importswithexpect.any(String)for the path, sidestepping the pre-existing Latin-1 decoding quirk innamed_imports_to_jsthat the PR description explicitly calls out as tracked separately. Including./plainas a control case guards against the loop trivially passing. - No CODEOWNERS entry matches
src/js_parser/or the transpiler test file. - No prior human review comments or outstanding requests on the PR.
|
CI build #72106: the only hard failure is The changed code path ( |
|
The |
1 participant
What
Bun.Transpiler.scanImports()silently dropped every dynamicimport()whose specifier contains a non-ASCII character, whilescan()and static imports with the same specifier were reported.Why
The
SCAN_ONLYbranch ofparse_import_expronly calledadd_import_recordwhene_string.is_utf8()was true. Any non-ASCII code point makes the lexer store the literal as UTF-16, so the guard failed and the branch fell through without recording the import.The full parse records the same import via
transpose_import, which callsEString::slice()without the guard.slice()already handles both encodings (it transcodes UTF-16 to UTF-8 viastring()).Fix
Drop the
is_utf8()guard;slice()handles both encodings.The test asserts
scanImports()matchesscan().importsfor non-ASCII dynamic imports rather than hardcoding path bytes, since the path text is currently Latin-1-decoded innamed_imports_to_js(pre-existing, affects static imports andscan()too, tracked separately).[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file