Repository navigation
Conversation
… when lowering
After the parser folds "pre" + `fix/${id}` (or `${"a"}b${x}`), the template's
head or tail is a rope E::String: `data` holds the first segment and the rest
is linked through `next`. convert_template_contents read `data` only, so the
compiled output kept just the first segment. Flatten the rope into the arena
the same way roped string literals already are.
A folded computed object key arrives as the same rope. lower_object_property_key
returned a Todo error for it, bailing the whole function out of compilation;
let it take the computed arm, which flattens it, like the object method path
already does.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe React Compiler now flattens rope-backed strings through a shared helper. Computed rope keys use computed-expression lowering. Regression tests cover folded templates, concatenated strings, object keys, and dynamic import specifiers. ChangesString rope lowering
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change is supported by regression coverage and no unresolved merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on the 1.4.0 release and again on a canary at 6a92015 (2026-09-11). Bundle a component that contains 2026-09-11: merged main (4b5862f) into the branch and shortened two comments that the comment check flagged. CI on 7bab183: 180 of 181 jobs pass. The one red job is the x64-asan test lane, which fails on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that flattens roped EString segments in template quasis and computed keys before HIR lowering.
What was reviewed:
arena_str_from_ropewalksnextand sizes viaEString::len(), which returnsrope_lenfor ropes;EString::pushasserts 8-bit soslice8()per segment is sound.lower_object_property_keynow matches the sibling infunction.rs(if s.next.is_none()→ rope falls through to thecomputedarm, wherelower_expression→convert_js_stringflattens it); checked that the object-pattern caller already filters computed keys before this point, socomputed=false+ rope is unreachable there.convert_js_stringrefactor is behaviour-preserving (same walk, now shared; arena buffer instead of heap Vec is fine — arena-lifetime, rare path).- Tests cover head/tail via
+, head/tail via template folding, joined templates, empty-data head, computed key, and the no-minifyimport()case; each asserts observable rendered output plus memo-cache allocation count.
Extended reasoning...
Overview
Two-file Rust change in src/react_compiler/lowering/build_hir/ plus a test addition. convert_template_contents gains a s.next.is_some() branch that flattens a roped cooked head/tail into a single arena string via a new arena_str_from_rope helper; convert_js_string (which already flattened ropes inline) is deduplicated to call the same helper. lower_object_property_key in helpers.rs restricts its static-string arm to non-roped EString so a folded computed key ({["a" + "b"]: x}) falls through to the existing computed arm, which routes through lower_expression → convert_js_string and flattens correctly. This mirrors the existing lower_object_property_key in function.rs:373, which already had the if s.next.is_none() guard.
Security risks
None. Pure AST-to-HIR string handling in the bundler's React Compiler pass; no untrusted input parsing, no syscalls, no allocation sizing driven by external data beyond source-file string lengths already bounded by the parser.
Level of scrutiny
Medium. It's a correctness fix in codegen (wrong output emitted), but the mechanism is well-understood — the PR description traces the rope invariant precisely, and I verified EString::len() returns rope_len (src/ast/e.rs:1838) and EString::push asserts is_utf8() on both operands (src/ast/e.rs:2021-2022), so the 8-bit walk and capacity presize are sound. The helpers.rs change is a one-line guard tightening that brings it in line with the identical function in function.rs.
Other factors
- The tests are strong: the first bundles under
minify.syntax, runs the output, and asserts exact JSON of rendered props for six distinct rope shapes plus a memo-cache allocation counter proving neither component bailed. The second covers the always-onimport()folding path without minify. The PR description states these fail on the unfixed build with the specific truncated outputs. - The
convert_js_stringrefactor trades a short-lived heapVecfor an arenaHirVecthatJsString::from_wtf8_bytesthen copies from. The arena buffer survives until arena reset, but this path is only reached for roped string literals (rare, per the deleted comment) and the deduplication is what REVIEW.md asks for when a block appears twice in a diff. - Checked the object-pattern caller of
lower_object_property_key(helpers.rs, destructuring path): it bails onIsComputedbefore calling, and passescomputed=false, so a rope cannot reach it withcomputed=falsefrom that path. The object-literal caller passes the realIsComputedflag, which is set for["a" + "b"]. - 3293 upstream fixtures + the rest of react-compiler.test.ts reported passing.
|
Updated 2:33 PM PT - Sep 11th, 2026
❌ @robobun, your commit 7bab183 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 38993That installs a local version of the PR into your bun-38993 --bun |
…ompiler-rope-template-quasis # Conflicts: # test/bundler/transpiler/react-compiler.test.ts
…th ends Add three shapes to FoldedTemplateAndKeyKeepAllSegments: a constant substitution at both ends of one template, folded arithmetic that lands in the head, and folded arithmetic that lands in a middle tail. Template folding turns the number into a string and links it onto the head or the tail as one more rope segment, so the unfixed build drops it the same way.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused rope-handling fix with good variant coverage.
What was reviewed:
arena_str_from_rope:EString::len()returnsrope_lenfor ropes, so theHirVeccapacity is correct; thebuf.leak()pattern matchesarena_utf8_from_utf16right above it.lower_object_property_keyfall-through: a folded["a" + "b"]keepscomputed == true, so the rope reaches the_ if computedarm and flattens vialower_expression→convert_js_string; a non-computed string key can never be a rope, so the final Todo arm is unreachable for this shape.convert_js_stringrefactor:from_wtf8_bytesstill receives the full joined bytes; only the backing allocation moved from a tempVecto the arena.- Tests cover head/tail/mid/both-ends/joined/empty-head/numeric folds, the computed-key bailout (memo-cache count = 2), and the always-folded
import()path withoutminify.syntax.
Extended reasoning...
Overview
This PR fixes a React Compiler HIR-lowering bug where roped E::EString values (produced by parser constant folding) were read only from their first segment. Three sites are touched: convert_template_contents gains a s.next.is_some() branch that flattens via a new arena_str_from_rope helper; convert_js_string is refactored to reuse that helper instead of an inline flatten loop; and lower_object_property_key guards its EString arm with s.next.is_none() so a folded computed key falls through to the existing computed arm rather than returning a cold_todo that bailed the whole component. Two itBundled tests are added covering nine template-rope shapes, the computed-key case (asserting both components allocate memo caches), and the always-folded import() specifier path.
Security risks
None. This is AST→HIR string handling in the bundler's React Compiler pass — no auth, crypto, network, filesystem, or untrusted-length arithmetic. The only allocation sizing (AstAlloc::vec_with_capacity(s.len())) is derived from the parser's own rope_len, and I confirmed in src/ast/e.rs that EString::len() returns rope_len when the string is roped, so the buffer is sized for the joined result.
Level of scrutiny
Low-to-medium. The Rust change is ~15 net lines and mirrors the existing arena_utf8_from_utf16 pattern (arena HirVec<u8> + .leak()), so the arena/Drop concern from CLAUDE.md doesn't apply — u8 has no destructor. I traced the helpers.rs fall-through: a folded ["a" + "b"] retains computed == true on the property, so it hits the _ if computed arm and lowers via lower_expression_to_temporary, which reaches convert_js_string and flattens correctly. A non-computed string-literal key cannot be a rope (folding only happens on +/template expressions, which are syntactically computed), so the final Todo arm is not newly reachable. The convert_js_string refactor is behavior-preserving: from_wtf8_bytes still receives the same joined bytes, just from an arena slice instead of a stack Vec.
Other factors
REVIEW.md's "extract a helper the second time a block appears" and "fix the whole class" rules are followed — the inline flatten loop in convert_js_string was deduplicated into arena_str_from_rope, and the PR description names #38944 as the sibling PR covering the js_parser/ast crate readers. Test coverage hits the variant matrix REVIEW.md asks for (head, tail, mid, both-ends, joined templates, empty-head const-enum, numeric folds in head and mid-tail, computed key, and the no-minify import() path), asserts exact stdout, and asserts the memo-cache count so a silent bailout would fail. No CODEOWNERS entry covers these paths. There is no outstanding third-party CHANGES_REQUESTED; the earlier COMMENTED review and bot inline threads were followed by three commits on this branch, and the current diff looks clean against the concerns I checked.
This branch has not been deployed
1 participant
Problem
"pre" + `fix/${id}`in a component is emitted as`pre${id}`,`${Route.Users}/${id}`(const enum) as`${id}`,`${id}/mid` + "dle"as`${id}/mid`. The same build withoutreactCompilerprints the full text.minify.syntax, and it is always on insideimport()/require()arguments, so a default build is affected too:import(Dir.Pages + `/${name}.js`)in a component is emitted asimport(`./pages${name}.js`).fold_string_addition(src/ast/fold_string_addition.rs) andTemplate::fold(src/ast/e.rs) build the folded head/tail as a rope (see Background).convert_template_contentsinsrc/react_compiler/lowering/build_hir/expr.rsread it withslice8(), which returnsdataalone, and codegen (template_contentsinsrc/react_compiler/codegen.rs) rebuilds the template from that truncated quasi.{ ["a" + "b"]: id }) is the same rope, andlower_object_property_keyinbuild_hir/helpers.rsreturned a Todo error for it, so the whole component silently went uncompiled (output correct, memoization lost). The object method path inbuild_hir/function.rsalready handled this shape.js_parser/ast; this PR covers thereact_compilercrate and does not overlap with that diff.Fix
convert_template_contentsflattens a roped cooked head/tail with a newarena_str_from_ropebefore storing it as the quasi;convert_js_string, which already flattened roped string literals inline, now uses the same helper.lower_object_property_key(helpers.rs) only takes the static-string arm for a non-ropedE::String; a rope falls through to the existing computed arm, wherelower_expressionflattens it. This matches what function.rs does for methods and what upstream does with the unfolded"a" + "b"(a computed key); it is emitted as{ ["ab"]: id }.EString::pushasserts it), so walkingdataof each segment covers the whole string.bun bd test:test/bundler/transpiler/react-compiler.test.ts:FoldedTemplateAndKeyKeepAllSegmentsruns aminify.syntaxbundle and checks nine rope shapes (head and tail from+, head and tail from template folding, one template folded at both ends, two templates joined, a head whose own text is empty, folded arithmetic in the head and in a middle tail) plus the computed key, and that both components allocated a memo cache.FoldedImportSpecifierKeepsAllSegmentschecks the no-minifyimport()case. On the unfixed build the first printspre7,7/mid,a7,7/x,p7r,7/one7,7,n7,7:7and one memo cache instead of two; the second emits./pages${name}.js.react-compiler.test.ts(47 tests in total) andreact-compiler-fixtures.test.ts(3293 upstream fixtures, each also run underminify.syntax) pass. Last run on this branch merged with main 4b5862f.Background
"a" + "b", it does not copy the bytes. It links the right operand onto the leftE::Stringthrough itsnextpointer and records the total length inrope_len;datastill holds only"a".slice8()returnsdata, so a reader that wants the whole string has to walknext. (resolve_rope_if_neededflattens in place, but it needs the parser arena and a mutable AST; React Compiler lowering has neither, so it copies into its own arena instead.)${}. Folding"x" + `y${a}`pushes onto the head,`${a}b` + "c"pushes onto the last tail, and`${"a"}b${x}`moves the constant part into the head, so a head can be a rope whose owndatais empty. The rope is only resolved when no${}remains and the template collapses to a plain string.src/react_compilerrewrites component and hook bodies by lowering the post-visit AST to HIR and generating new AST from it, so any text it fails to carry into HIR is missing from the output. A function whose lowering reports a Todo error is left as written.Repro on the unfixed build
[human-review] gate passed · iteration 0 · 3 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