js_parser: sanitize auto-generated default export name for digit-named modules#31403
Conversation
An anonymous `export default function () {}` is given an auto-generated
name derived from the module filename (`<name>_default`). The transpile
and `bun run` paths do not run the renamer, so the generated name is
emitted verbatim. `create_default_name` built it with the non-allocating
`fmt_identifier()` formatter, which — unlike the allocating
`nonUniqueNameString` helper the Zig reference used — does not prepend an
underscore when the name starts with a non-identifier-start character.
For a module named `1.ts` this produced `function 1_default()`, an
invalid identifier that JSC's lexer rejected with 'No identifiers allowed
directly after numeric literal'.
Use `MutableString::ensure_valid_identifier` (the #2946 fix) to sanitize
the filename-derived base before appending `_default`, matching the
bundler's NumberRenamer output (`_1_default`).
|
Warning Review limit reached
More reviews will be available in 31 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR fixes a parser bug where modules with digit-prefixed filenames (e.g., ChangesIdentifier sanitization for digit-prefixed modules
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 7:12 PM PT - May 25th, 2026
✅ @robobun, your commit 9beb90a9466af70be71e20a70634a2888f9182be passed in 🧪 To try this PR locally: bunx bun-pr 31403That installs a local version of the PR into your bun-31403 --bun |
![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 fix correctly swaps fmt_identifier() for MutableString::ensure_valid_identifier (matching both the Zig reference and the existing Source::identifier_name pattern), and the regression tests are solid. Deferring only because this is in the parser's transpile path, so it's worth a quick human glance.
Extended reasoning...
Overview
This PR fixes a regression where bun run 1.ts (or any module whose filename starts with a digit) containing export default function () {} fails with SyntaxError: No identifiers allowed directly after numeric literal. The root cause is that create_default_name in src/js_parser/p.rs used the non-allocating fmt_identifier() Display formatter to build the <name>_default symbol, but that formatter (src/bun_core/fmt.rs:1284-1288) does not prepend _ when the first character is a non-identifier-start — unlike MutableString::ensure_valid_identifier (src/bun_core/string/MutableString.rs:201-207), which carries the #2946 fix. On the no-renamer path (transpile / bun run), the unsanitized 1_default was emitted verbatim and rejected by JSC's lexer.
The fix replaces the formatter with non_unique_name_string_base() + ensure_valid_identifier(), then copies the result plus the _default suffix into the bump arena. This is the exact same pattern already used by Source::identifier_name (src/ast/lib.rs:2922-2924) and matches the Zig createDefaultName reference implementation. The arena allocation uses alloc_slice_fill_copy(sanitized.len() + SUFFIX.len(), 0u8) with correctly-bounded copy_from_slice calls. The intermediate Box<[u8]> from ensure_valid_identifier is dropped at end of block. The ? propagation of AllocError → bun_core::Error is already established by the identical pattern in identifier_name.
A new regression test (test/regression/issue/31401.test.ts) covers three paths: bun run 1.ts, importing a digit-named module from another module, and bun build --no-bundle output verification. The PR description confirms these fail under USE_SYSTEM_BUN=1 and pass with the fix, and that existing bundler regression tests (including #2946) and the transpiler suite remain green.
Security risks
None. The change only affects the auto-generated symbol name for anonymous default function exports. Input is the module's own filename base, output is an internal AST symbol name. No user-controlled data flows into anything security-sensitive; if anything, the fix makes the sanitization stricter.
Level of scrutiny
Moderate. The change itself is small (~10 LOC), mechanical, and mirrors an established in-tree pattern exactly. However, create_default_name lives in the JS parser and runs on every transpiled file containing an anonymous default function export — this is production-critical runtime code for Bun. While I'm confident the fix is correct, parser changes in a production runtime warrant a human reviewer's eyes rather than bot-only approval.
Other factors
- No CODEOWNERS entry covers
src/js_parser/. - No prior human reviews or outstanding comments on the PR.
- The bug hunting system found no issues.
- The diagnosis in the PR description is accurate (I verified the divergence between
fmt_identifierandensure_valid_identifierat the relevant lines).
Normalize CRLF in spawned-process stdout assertions and drop the over-strict empty-stderr check (replaced with the meaningful 'no numeric-literal SyntaxError' invariant) so the test is robust on the Windows baseline lane.
There was a problem hiding this comment.
The fix looks correct and well-tested, but this touches the JS parser hot path and CI hasn't reported green on the latest commit yet (the Windows transpiler.test.js failure was on 0f04b583, and 6b87f027 only touched the new regression test) — worth a human look before merging.
Extended reasoning...
Overview
This PR fixes #31401 by changing create_default_name in src/js_parser/p.rs to sanitize the filename-derived base via MutableString::ensure_valid_identifier (which prepends _ for non-identifier-start leading chars per #2946) instead of the non-allocating fmt_identifier() formatter, which lacks that prefix logic. A new regression test covers bun run, import, and build --no-bundle paths for digit-named modules with anonymous default function exports.
Security risks
None. This is purely identifier-name generation for emitted JS; no untrusted input handling, auth, crypto, or filesystem boundaries are touched.
Level of scrutiny
Medium-high. The diff is small (~15 lines) and the logic is straightforward, but create_default_name runs in the JS parser for every module with an anonymous default export, on the no-renamer path used by bun run. There is also a subtle behavioral side-effect: ensure_valid_identifier additionally applies strict_mode_reserved_word_remap, so e.g. let.ts will now yield _let_default instead of let_default. That is harmless (both are valid identifiers) and actually matches the Zig reference more closely, but it is a change in emitted output.
Other factors
- Several callers wrap
create_default_name(...).expect("unreachable"); the newly-introduced?only propagatesAllocError(OOM), which is already treated as unreachable elsewhere, so no new panic surface. - robobun reported
test/bundler/transpiler/transpiler.test.jsfailing on two Windows lanes for commit0f04b583. The follow-up commit6b87f027only hardened the new31401.test.ts(CRLF normalization, looser stderr check) and did not touchtranspiler.test.js, so it is unclear whether that failure is flaky or real; CI for the latest commit has not yet been reported in the thread. - No CODEOWNERS entry for
src/js_parser/.
Given the parser-hot-path location and the not-yet-confirmed-green Windows CI, deferring to a human reviewer rather than auto-approving.
Launch the digit-named modules via `bun <absolute-path>` instead of `bun run <relative>` + cwd. The direct-file form (AutoCommand) is the path already exercised on Windows CI by test/regression/issue/08965, and avoids the run-command's script-resolution step for a bare relative filename. The fix under test lives in the parser and runs identically regardless of how the entry file is launched.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/31401.test.ts`:
- Around line 1-3: Replace raw stdout assertions (e.g.,
expect(stdout).toBe(...), expect(stdout).toContain(...)) with normalized
snapshot assertions using normalizeBunSnapshot: call
expect(normalizeBunSnapshot(stdout)).toMatchSnapshot() (or
toMatchInlineSnapshot() if desired). Update the three occurrences referenced
(the assertions around lines noted in the comment) to use normalizeBunSnapshot
and snapshot matchers; keep existing variables (stdout, stderr) and test names
intact and only change the assertion style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf5013fd-e6d5-4c6d-a756-6b5051e19845
📒 Files selected for processing (2)
src/js_parser/p.rstest/regression/issue/31401.test.ts
Adopt the repo convention of normalizing spawned-process stdout through normalizeBunSnapshot (which collapses CRLF among other things) instead of ad-hoc replaceAll. The bug is still caught by the stderr 'No identifiers allowed directly after numeric literal' assertion, which is independent of stdout formatting.
There was a problem hiding this comment.
LGTM — targeted fix that aligns create_default_name with the Zig reference sanitizer, with solid regression coverage.
Extended reasoning...
Overview
This PR fixes a single function, create_default_name in src/js_parser/p.rs, swapping the non-allocating fmt_identifier() formatter for MutableString::ensure_valid_identifier so that filename-derived default-export names are valid identifiers even when the filename starts with a digit (e.g. 1.ts → _1_default instead of the invalid 1_default). It also adds a 3-case regression test (bun run, import, build --no-bundle) at test/regression/issue/31401.test.ts.
Security risks
None. The change only affects how an internal symbol name is constructed from a module filename for printing; no auth, crypto, permissions, or user-input parsing surface is touched. The sanitizer being swapped in is strictly stricter than the one being replaced.
Level of scrutiny
Low. This is a ~10-line mechanical fix that brings the Rust port back in line with the documented Zig reference (nonUniqueNameString = ensureValidIdentifier(nonUniqueNameStringBase())). I verified ensure_valid_identifier always returns a non-empty result (empty → _), so the arena slice math is sound, and the ? correctly propagates the existing AllocError → bun_core::Error path. Non-digit filenames are unchanged since the sanitizer is a no-op for already-valid-start identifiers.
Other factors
All review feedback has been addressed and resolved: the CodeRabbit normalizeBunSnapshot suggestion was adopted in b103c2a, and my earlier non-blocking heads-up about two sibling fmt_identifier call sites was acknowledged as intentionally out-of-scope (neither is observable on the no-renamer path). The PR description confirms the existing #2946 bundler regression test and transpiler suite remain green, and the new test fails on system Bun / passes on the patched build as expected.
CI status: diff is green, failures are pre-existing flake on unrelated lanesThe only repeatedly-failing test across builds #58031 and #58043 is This is unrelated to this PR:
Other red lanes were also unrelated flake:
The diff itself is green. This needs a maintainer to merge (or a re-run of the flaky Windows lane). |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete your code comments.
fmt_identifier shoudl handle numbers? Isn't that the correct fix?
…t char Per review feedback, fix the root formatter instead of special-casing create_default_name. `fmt_identifier` (FormatValidIdentifier) left the first character untouched when it was not a valid identifier start (e.g. a digit from a module named `1.ts`), emitting an invalid identifier like `1_default`. Prepend `_` in that case, matching MutableString::ensure_valid_identifier (the #2946 fix). create_default_name reverts to its original one-liner now that the formatter is correct.
|
Done in d47b834 — you're right, that's the correct fix. This also fixes the sibling Note: the flaky |
There was a problem hiding this comment.
LGTM — moving the leading-underscore fix into fmt_identifier itself is the cleaner root-cause fix and resolves the sibling call sites I'd flagged earlier.
Extended reasoning...
Overview
This PR fixes #31401: bun run 1.ts with export default function () {} produced SyntaxError: No identifiers allowed directly after numeric literal because the auto-generated name 1_default is not a valid identifier on the no-renamer (transpile/run) path.
The final diff is minimal:
src/bun_core/fmt.rs(+4):FormatValidIdentifier::fmtnow prepends_when the first character is not a valid identifier-start (thestart_i == 0branch). This mirrorsMutableString::ensure_valid_identifierline-for-line, including the same #2946 comment.src/js_parser/p.rs(−4): removes a now-stale port-note comment;create_default_nameis otherwise unchanged.test/regression/issue/31401.test.ts(+66): three concurrent tests coveringbun run,import, andbuild --no-bundlefor digit-named modules.
Security risks
None. This only affects how a filename-derived string is sanitized into an internal JS identifier. No auth, crypto, filesystem, or network surface.
Level of scrutiny
Low–medium. It's a 4-line logic addition that brings the non-allocating formatter in line with the allocating reference (ensure_valid_identifier). I audited all fmt_identifier / FormatValidIdentifier call sites: bare callers (create_default_name, the namespace_ref sites in p.rs/parse_stmt.rs, scanImportsAndExports.rs) now emit valid identifiers up-front instead of relying on the bundler renamer to repair them; prefixed callers (require_{}, export_{}, ${}) gain at most a cosmetic extra _ in internal generated names, which are still valid and go through the renamer anyway.
Other factors
- The author pivoted from a call-site patch to fixing
fmt_identifierat the root after review discussion — this directly resolves the non-blocking sibling-site concern I'd raised on the previous revision. - All inline review threads (CodeRabbit's
normalizeBunSnapshotrequest, my sibling-site note) are addressed and resolved. - Tests follow house style:
tempDirwithusing,bunEnv/bunExe,normalizeBunSnapshotfor stdout, stderr regression marker checked, exit code asserted last. - Existing bundler regressions (#2946) and the transpiler suite were verified green by the author; CI failures were unrelated pre-existing flakes (Windows stack-overflow fixture, S3 ASAN leak) with no parser frames.
- No CODEOWNERS for these paths; bug-hunting system found nothing.
* oven/main (10 new commits): Optimize TextEncoder.encode: restore SIMD ASCII fast paths lost in the Rust port (oven-sh#31385) js_parser: sanitize auto-generated default export name for digit-named modules (oven-sh#31403) fetch: run checkServerIdentity before writing the request (oven-sh#31325) ffi: avoid copying the threadsafe callback wrapper on the calling thread (oven-sh#31332) install: gate the exit-callback cache teardown to the main thread (oven-sh#31376) fix(node:module): don't register native helpers as their own constructors (oven-sh#31393) css: escape custom pseudo-class/element names when printing (oven-sh#31404) Deepen the lots-of-for-loop fixture so the transpiler stack-overflow tests throw on Windows (oven-sh#31382) Hardening: input validation and bounds tightening across 36 subsystems (round 4) (oven-sh#31339) Speed up FormData multipart serialization (oven-sh#31379) Auto-merged: src/install/PackageManager.rs, src/runtime/cli/upgrade_command.rs, src/runtime/webcore/Blob.rs, src/sys/lib.rs
2 participants
Fixes #31401
Repro
Two conditions are both required: the module filename starts with a digit (
1.ts,9abc.ts, …) and the source has an anonymous default function declaration (export default function () {}). Named defaults, anonymous classes, and arrows are unaffected.Cause
An anonymous
export default function () {}is given an auto-generated name derived from the module filename (<name>_default). The transpile /bun runpath does not run the renamer (only bundling /minify_identifiersdo), so the generated name is emitted verbatim:create_default_namebuilt this name with the non-allocatingfmt_identifier()formatter. Unlike the allocatingnonUniqueNameStringhelper the Zig reference used (MutableString.ensureValidIdentifier),fmt_identifierdoes not prepend an underscore when the name starts with a non-identifier-start character — so the leading1passed through unchanged, yielding1_default. JSC's lexer then rejects1_defaultas a numeric literal followed by an identifier.The bundler path was fine because its
NumberRenamerrunsensure_valid_identifieron every name, rewriting1_default→_1_default. This is effectively a regression of #2946 on the no-renamer path: names must be generated valid up-front there.Fix
In
create_default_name, sanitize the filename-derived base withMutableString::ensure_valid_identifier(which carries the #2946 leading-underscore fix) before appending_default, matching the ZigcreateDefaultNameand the bundler output:Non-digit filenames are unchanged (
foo.ts→foo_default).Verification
bun run 1.tsnow exits 0.test/regression/issue/31401.test.tscovers thebun run,import, and transpile-only paths. Fails withUSE_SYSTEM_BUN=1(3 fail), passes with the debug build (3 pass).test/bundler/bundler_regressions.test.ts(incl.InvalidIdentifierInFileName#2946) andtest/bundler/transpiler/transpiler.test.js(156 pass) remain green.