Repository navigation
Conversation
|
Updated 5:08 AM PT - Oct 3rd, 2026
✅ @robobun, your commit 235bb5859b60a3de6926022ccc92e3b59cf975ad passed in 🧪 To try this PR locally: bunx bun-pr 38944That installs a local version of the PR into your bun-38944 --bun |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reached
This review includes 1 billable file and costs up to $0.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 21 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe change makes EString operations and parser and bundler consumers handle complete UTF-8 rope contents, including comparisons, hashing, paths, loader types, feature names, and folded property keys. Regression tests cover these cases across bundler, transpiler, and resolver behavior. ChangesFolded String Handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No confirmed issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix pushed (head c9684ba), waiting on CI. Reproduced on bun 1.4.0 and on main with the snippets in the description ( |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35680, but the two do touch the same function.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked that as_string_literal's switch from &self.data to self.data is sound (Data::EString holds a Copy StoreRef, so s.slice() flattens the arena node in place via DerefMut — same pattern as the printer). Also confirmed the four extra .data readers the nit names all bypass string(), so the new debug_assert won't fire on them.
Extended reasoning...
The three fixed sites each adopt the exact flatten-first pattern their working siblings (transpose_import, transpose_require) already use, and every fix ships with a test that fails on the unfixed build. The only finding is a nit about pre-existing same-class .data readers the .string() audit didn't cover; it doesn't affect the correctness of what this PR changes. I verified the StoreRef copy in as_string_literal and that the new debug assertion can't be tripped by those extra sites since they read the field directly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
robobun
changed the title
|
Pushed c9684ba, which restructures the fix following review:
|
There was a problem hiding this comment.
I reviewed the final revision (c9684ba) and didn't find any bugs. The refactor — making EString::string() join ropes itself instead of asserting and pushing the flatten to every caller — is cleaner than the earlier approach and subsumes the .string()-caller fixes (transpose_require_resolve_known_string, extract_dynamic_specifier_shape, as_string_literal). Given this touches parser/AST hot paths and coordinates with #35680 and #38787, a maintainer sign-off is still worthwhile.
What was reviewed:
join_ropeextraction and its two callers;string_cloned's new delegation preserves the fresh-copy contract for the non-rope UTF-8 case.eql_bytesnow walks the rope viaeql8_rope, soObject::getmatches folded computed keys (covered by the["wi" + "th"]loader test).- The new
debug_asserts onhash/order/eql_string— checked their callers (JSON sorting,alphabetize_*, mangled-prop dedup) all run on parser-produced or JSON-tape strings, never post-fold ropes. write_to_hasher's segment loop feeds bytes back-to-back with the trailing\x00, so"a"+"b"and"ab"hash identically as the test asserts.
Extended reasoning...
Overview
This PR fixes a class of bugs where post-visit readers of E::String saw only the first segment of a folded rope ("./fo" + "o" → "./fo"). It touches four Rust source files (src/ast/e.rs, src/ast/expr.rs, src/js_parser/p.rs, src/js_parser/visit/visit_expr.rs) and adds tests across eight files. The PR went through three substantive revisions: the initial three-site fix, a sweep of direct .data readers after my first review, and finally c9684ba which replaced the per-caller debug_assert approach with a rope-aware string() accessor plus a shared join_rope helper.
What changed since my last review
c9684ba is a design change: EString::string() now joins ropes into the arena on every call rather than asserting the caller flattened first. This is the right layering per the repo's own review rules ("fix bugs at the layer that owns the violated invariant") — every future .string() caller is now correct by construction. resolve_rope_if_needed and string() share join_rope; slice() remains the caching variant. string_cloned correctly keeps its own copy for the non-rope UTF-8 case (where string() returns a borrow, not a fresh allocation). The direct-.data fixes at import_record_loader, maybe_replace_bundler_feature_call, the e_index visitor, and rewrite_import_meta_hot_accept_string are still needed and remain in place. The new debug_asserts on hash()/order()/eql_string() cover the remaining rope-unaware readers; I traced their callers and none receive folded strings.
Security risks
None. The changes are to compile-time constant folding of string literals in the transpiler; no untrusted-input parsing, no auth/crypto/permissions.
Level of scrutiny
High — this is parser/AST code on the hot path of every transpile and bundle. The fix is well-tested (each of the eight fixed sites has a targeted test that fails on the unfixed build), but the surface area, the cross-PR coordination (#35680 rewrites extract_dynamic_specifier_shape; #38787 replaces the deferred decorator-key reader), and the two open comment-cop nags on the new string() doc comment mean a maintainer should sign off rather than an automated approval.
Other factors
Both of my earlier inline threads have justified responses (the decorator-key reader is owned by #38787 with tests there; the prev_should_fold = true pre-existing bug is filed separately as it changes when ropes are made, not how they're read). Test coverage is thorough and follows harness conventions. The two comment-cop comments at 14:45:40 look like false positives (one is a doc comment, one is pre-existing repositioned code) but are unaddressed.
### Problem
- `bun build` reads `ns["a" + "b"]` as the export `a`. With `const ns =
await import("./m.mjs")`, the bundle prints `A` where Node prints `AB`.
`ns["" + "x"]` prints `undefined`. The build shows no warning.
- The cause is in `e_index` (`src/js_parser/visit/visit_expr.rs:1036`).
It gives `s.data` of the key to `maybe_rewrite_property_access` and
`record_import_property_use`. For a folded key, `data` holds only the
first part.
### Fix
- Flatten the key with `resolve_rope_if_needed` before the visitor reads
it.
- The printer already reads the flattened key. So the parser and the
printer now use the same name. The minify path already flattened this
node through `is_identifier`.
- Verified: five new cases in
`test/bundler/bundler_dynamic_import_dce.test.ts` and
`test/bundler/esbuild/importstar.test.ts`. All five fail on main. The
other bundler suites I ran are in the notes.
### Background
- Constant folding makes `"a" + "b"` one `E::String` and copies no
bytes. The node keeps `"a"` in `data` and links `"b"` through `next`.
`resolve_rope_if_needed` joins the parts into `data`.
- Folding runs with `--minify-syntax`, in enum initializers, and in the
arguments of `import()` and `require()`. `e_import` restores its flag to
`true`, so folding stays on after an `import()`. #39018 fixes that flag.
- `import * as ns` has the same bug with an enum key, also in builds
from before #32557. #32557 records a key on an `import()` namespace as a
use. #41186 binds that key to the export. Together they make the bug
reachable from `await import()`.
<details><summary>Notes</summary>
Repro from the report:
```sh
printf 'export const a = "A"; export const ab = "AB"; export const x = "X";\nexport { x as "a-b" };\n' > m.mjs
cat > e.mjs <<'EOF'
const ns = await import("./m.mjs");
const v = ns["" + "x"];
console.log(JSON.stringify({ "ns['a'+'b']": ns["a" + "b"], "ns['a'+'-b']": ns["a" + "-b"], "ns[''+'x']": typeof v === "string" ? v : Object.prototype.toString.call(v) }));
EOF
bun build ./e.mjs --target=bun --outfile=o.js && bun o.js
```
- main (473335d):
`{"ns['a'+'b']":"A","ns['a'+'-b']":"A","ns[''+'x']":"[object
Undefined]"}`. The bundle has `"ns['a'+'b']": a`.
- This branch:
`{"ns['a'+'b']":"AB","ns['a'+'-b']":"X","ns[''+'x']":"X"}`, with and
without `--splitting`. The unused export `a` is dropped.
Static namespace, on a build from before #32557 (a6c4cc2):
```ts
import * as ns from "./m.mjs";
enum K { AB = "a" + "b", X = "" + "x" }
console.log(ns[K.AB], ns[K.X]);
```
The bundle prints `A undefined` and warns `Import "" will always be
undefined because there is no matching export in "m.mjs"`.
- The `"a" + "b"` tests depend on the flag that stays on after
`import()`. If #39018 lands, those keys are not folded without minify,
and the namespace stays whole. The output is still correct, so those
tests still pass. The enum keys fold in all cases, so the enum tests
keep this line covered.
- #38944 (open, conflicts with main) adds the same call as part of a
wider change to rope readers. If it lands first, this PR reduces to its
tests.
- Two other readers in the same visitor (`"str"[i]` and
`"str".charCodeAt(i)`) read only the first part. They compare the index
with the length of that part, so the result stays correct.
- Suites run with the debug build, all pass:
`bundler_dynamic_import_dce`, `esbuild/importstar`,
`esbuild/importstar_ts`, `esbuild/ts`, `esbuild/dce`, `bundler_string`,
`bundler_minify`, `transpiler_constant_fold_eqeq`, `bundler_edgecase`,
`transpiler/transpiler.test.js`, `bundler_barrel`, `bundler_splitting`.
</details>
…d other readers of the first segment
Constant folding turns "./fo" + "o" into a rope E::String whose `data` holds
only the first segment. Readers that went through EString::string() or read
`data` directly saw only "./fo":
- transpose_require_resolve_known_string recorded "./fo" as the import
record path, so require.resolve("./fo" + "o") was emitted (and resolved)
as require.resolve("./fo").
- extract_dynamic_specifier_shape built --allow-unresolved shapes from rope
template heads and tails.
- Expr::as_string_literal, used to match destructuring keys against an
inlined macro result, dropped the property for a computed folded key.
- Import::import_record_loader looked up the "js" loader for
`with: { type: "js" + "on" }`.
- feature() from "bun:bundle" looked up the first segment of a folded name.
- The React Fast Refresh signature hash covered only the first segment.
string() and string_cloned() now return the whole folded string, and
eql_bytes compares the whole rope. The remaining direct readers flatten
first. The comparators that still read only `data` (hash, order,
eql_string) assert that they are not handed a rope.
c9684ba to
a3dcbad
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I re-checked the post-visit .data readers raised in the earlier review: import.meta.hot.accept (src/js_parser/p.rs:8563), the e_index visitor (src/js_parser/visit/visit_expr.rs:984), feature() and import_record_loader all call resolve_rope_if_needed before reading data, and import_record_loader's only caller passes the arena. I also confirmed eql8_rope checks rope_len against the input length before slicing, so the new eql_bytes dispatch cannot index out of range, and transpose_require_resolve_known_string reaches the fix through string() with no call-site change needed.
Extended reasoning...
The change touches the EString rope accessors in src/ast/e.rs, the refresh-signature hasher in src/ast/expr.rs, and three parser call sites, plus tests across eight suites; it touches no security-sensitive surface. The earlier-flagged first-segment readers are now all normalized in this squashed commit, and the one confirmed finding is a test-redundancy nit, but the accessor semantics (per-call arena join in string(), debug-only rope asserts on hash/order/eql_string) are core parser behavior worth a maintainer's look rather than a bot approval.
| const signatureOf = async (file: string) => { | ||
| const build = await Bun.build({ | ||
| entrypoints: [join(String(dir), file)], | ||
| reactFastRefresh: true, | ||
| minify: { syntax: true }, | ||
| external: ["react"], | ||
| }); | ||
| const output = await build.outputs[0].text(); | ||
| const match = output.match(/_s\w*\(C, "([^"]+)"\)/); | ||
| if (!match) throw new Error(`no refresh signature in ${file}:\n${output}`); | ||
| return match[1]; |
There was a problem hiding this comment.
🔴 Maintainers get a new test that errors in the harness whenever react-refresh/runtime is not resolvable from the OS temp dir, so it cannot pass on this branch either. The component in each fixture calls useState, which makes the parser emit import { createSignatureFunctionForTransform } from "react-refresh/runtime"; Bun.build at bun-build-api.test.ts:377 then fails with "Could not resolve: "react-refresh/runtime"" and throws, since only react is external. Fix: make the refresh runtime resolvable in every fixture, e.g. add a node_modules/react-refresh/runtime.js stub to the tempDir like bundler_jsx.test.ts:1445 does, or add react-refresh/runtime to external.
Why this was flagged
The test at test/bundler/bun-build-api.test.ts:368 writes three .tsx files under tempDir, which lives in os.tmpdir() (test/harness.ts:523), with no node_modules. Each file calls useState inside function C, so handle_react_refresh_hook_call (src/js_parser/p.rs:8702) sets signature_used, and parse_entry.rs:2400-2406 calls generate_react_refresh_import with b"react-refresh/runtime", emitting an ordinary S::Import. The bundler resolves that record like any package import; on ModuleNotFound, it logs "Could not resolve: "{}". Maybe you need to "bun install"?" (bundle_v2.rs:6880). Bun.build defaults to throw_on_error: true (src/runtime/api/JSBundler.rs:221), so signatureOf rejects before the regex at line 384 runs. external: ["react"] does not cover react-refresh/runtime. The existing API-backend refresh test at test/bundler/bundler_jsx.test.ts:1441-1451 supplies a /node_modules/react-refresh/runtime.js stub for exactly this reason. Result: the test fails on both base and this branch in the standard harness, so it guards nothing.
Verification: The fixtures at test/bundler/bun-build-api.test.ts:372-374 each call useState(...) inside function C. parse_entry.rs:2401-2420 then calls generate_react_refresh_import with b"react-refresh/runtime". Bun.build defaults throw_on_error: true (src/runtime/api/JSBundler.rs:221), so the promise rejects with an AggregateError; the test errors rather than asserting anything.
The 8-bit rule for macro string results, two reader repairs and the cache version bump leave this branch: open #38944 owns the readers and #42019 the string storage. The table gets its own admission rule, a declaration inside an argument is recorded by role, a binding that takes its default is never recorded, and a call under a with object or a direct eval keeps the old path. An ASCII string from a macro joins in a template or a + while the arguments are visited.
This branch has not been deployed
1 participant
Problem
require.resolve("./fo" + "o")is transpiled torequire.resolve("./fo")and throwsCannot find module './fo'at runtime, whilerequire("./fo" + "o")next to it works. Same throughBun.Transpiler(transformSyncandscan()report./fo),bun build, and each branch ofrequire.resolve(x ? "./fo" + "o" : "./ba" + "r"). Node resolves./foo."./fo" + "o"produces a rope (see Background) whosedataholds only the first segment, andEString::string()(src/ast/e.rs) returneddatawithout walking the rope.transpose_require_resolve_known_stringrecords the import record path throughstring();transpose_importandtranspose_requirehappen to flatten first, which is whyrequire()andimport()work.string(): the--allow-unresolvedshape ("./lo" + \cales/${x}.json`was reported as./lo*.json, so a pattern matching the real specifier was rejected) and macro result destructuring (const { ["a" + "b"]: x } = m()dropped theab` property).datadirectly:import()attributes (with: { type: "js" + "on" }selected thejsloader, andObject::getdid not match folded computed keys),feature()frombun:bundleunderminify.syntax, the React Fast Refresh signature hash (useState("a" + "b")anduseState("a" + "c")got the same signature), andimport.meta.hot.accept()matching (not reachable with a rope today, HMR is only on in dev mode where folding is off).Fix
EString::string()(andstring_cloned()) join the rope into the arena whennextis set, through the same helperresolve_rope_if_needednow uses, andeql_bytescompares the whole rope likeeql_comptimealready did. That alone fixesrequire.resolve, the shape and the macro case; those call sites are unchanged from main.slice()keeps storing the joined bytes back into the node for callers that read it repeatedly.dataitself callresolve_rope_if_neededfirst (import_record_loader, which now takes the arena from its one caller;feature();hot.accept), and the refresh hasher feeds the segments back to back so a folded string hashes like the equivalent literal.hash(),order()andeql_string()still readdataonly; they nowdebug_assertthey are not handed a rope. Every caller normalizes first (Data::eqlandextract_string_valuesresolve both sides, the react compiler'sJsStringasserts the same at construction, package.json sorting never sees ropes), so this only guards future callers.require.resolvenow records the same pathrequirerecords for the same argument, which is what Node resolves.["a" + "b"]test, in js_parser: name lowered anonymous classes after numeric, non-ASCII and private property keys #38787), the_namehelper variables for decorated auto-accessors (cosmetic), and the"str"[i]/charCodeAt(i)folds (they bail past the first segment, so they only miss an optimization). Four different rope bugs found while sweeping are tracked separately (React Compiler template text loss,Template::foldmutating an inlined enum's rope,.lengthfolding counting bytes, and the fold flag staying on after animport()).extract_dynamic_specifier_shape; that function is untouched here, so there is no conflict, and its per-segmentstring()calls keep working sincestring()now handles ropes.bun bd test; every new test fails on a debug build of the base commit without thesrc/change:test/bundler/transpiler/transpiler.test.js: printed output for two- and three-segment concatenations and the ternary form,scan()paths, andrequire()/import()controls.test/js/bun/resolve/resolve.test.ts:require.resolveof folded specifiers returns the right files at runtime.test/bundler/bundler_allow_unresolved.test.ts: rope head from+, rope head from template folding, rope tail, and therequire.resolve()path accept a pattern matching the full specifier.test/bundler/transpiler/macro-test.test.ts: a computed folded key keeps its property when destructuring a macro result.test/bundler/bundler_loader.test.ts: foldedtypevalue, and folded computedwith/typekeys, select the JSON loader.test/bundler/bundler_feature_flag.test.ts: a folded flag name is looked up whole (CLI and API backends).test/bundler/bun-build-api.test.ts:"a" + "b"gets the same refresh signature as"ab"and a different one from"a" + "c".test/bundler/transpiler/*,test/bundler/esbuild/{default,ts,dce}.test.ts,bundler_edgecase,bundler_minify,bundler_string,bundler_loader,bun-build-api, the TOML tests andtest/cli/install/npmrc.test.tson the debug build.Background
"a" + "b"it does not copy bytes. It links the right operand onto the leftE::Stringthrough itsnextpointer and bumpsrope_len;datastill holds only"a". Template literals get the same treatment for their head and for the text after each${}. Ropes are produced in two places (fold_string_additionandTemplate::fold) and read in many, which is why the fix goes into the accessor.minify_syntaxor inlining is on (bun runandtarget: "bun"builds enable both); always, whatever the flags, for enum initializers and for the arguments ofrequire(),require.resolve(), macro calls andimport()including its options object. A string enum member built with+is a rope everywhere it is inlined.E::RequireResolveStringfrom the record's path, so the recorded text is both what gets resolved and what ends up in the output.--allow-unresolvedshape: for a dynamic specifier written as a template literal, the parser joins the literal parts with\0standing in for each interpolation and matches that against the user's glob patterns.const { a } = someMacro()inlines the macro's return value, the parser keeps only the properties the pattern names, matching each binding key against the inlined object.Repros on the unfixed build
Earlier revisions of this description
The first push flattened at the three
string()call sites and added adebug_asserttostring(); the second push added the directdatareaders after review; the third push moved the fix intostring()itself (review pointed out that the invariant lives inEStringand that per-call-site flattening is how this bug keeps recurring), reverted the three call sites to their original code, and replaced the contrived index-visitor test with the enum cases above.Rebase notes
Rebased onto main (519963e). The four commits are now one commit, because main changed the same lines.
src/ast/e.rs: main hasflatten_rope(js_printer: resolve string ropes into a local copy instead of the shared AST node #40168).string()calls it, and thejoin_ropeof this PR is gone.src/js_parser/visit/visit_expr.rs: main already flattens the rope in the index visitor (bundler: read ns["a" + "b"] as the export "ab", not "a" #41250). That line of this PR is gone.src/change, on a debug build of main 519963e: 12 of the 15 new tests fail.transpiler.test.js,macro-test.test.ts,bundler_feature_flag,bundler_loader,bundler_allow_unresolved,resolve.test.ts): 496 pass, 0 fail. No rope assertion fired.bundler_edgecase.test.tson this head: 184 pass, 0 fail.ns[K.X],exports[K.X]andE["fo" + "o"]go through the index visitor, which bundler: read ns["a" + "b"] as the export "ab", not "a" #41250 already fixed and tested. Those three tests are removed. The 12 that remain all fail on the base.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/resolve/resolve.test.ts, test/bundler/bun-build-api.test.ts