Repository navigation
js_parser: include the hook binding in the React Fast Refresh signature - #43750
Conversation
The visit pass has a block that hashes the binding of a declaration whose initializer is a hook call. It compares the call with `react_refresh.last_hook_seen`, and nothing set that field, so the block never ran. `handle_react_refresh_hook_call` now records the call. `B::write_to_hasher` wrote the tag and the length of an identifier name, and not the name. It now writes the name too, so `[a, setA]` and `[b, setB]` get different signatures.
…he stack runs out `Data::write_to_hasher` recursed with no bound. The parser builds `a + b + ...` and `a.b.c...` in a loop, so a long chain in the initial state of `useState` overflowed the stack. With the binding in the signature, a default value or a computed key in the binding reaches the same recursion. It now takes a `StackCheck`, like `known_primitive`, and stops when it is not safe to recurse. The binding test also covers a hook call inside another call, and a hook call that is an argument of another hook call.
|
Status: CI is green (Buildkite build 119524) and the automated reviews found no open issue. The PR is ready for a maintainer. How I reproduced it on bun 1.4.3: mkdir /tmp/rfr && cd /tmp/rfr
echo 'import { useState } from "react"; export function C() { const [a, setA] = useState(0); return a; }' > a.jsx
echo 'import { useState } from "react"; export function C() { const [b, setB] = useState(0); return b; }' > b.jsx
bun build --react-fast-refresh --target=browser --external '*' a.jsx | grep '_s(C'
bun build --react-fast-refresh --target=browser --external '*' b.jsx | grep '_s(C'Both commands print The stack overflow: put |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.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; 6 remain after this review. WalkthroughChangesHashing and React Refresh
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change improves React Fast Refresh signature accuracy and prevents stack failures while hashing deeply nested expressions. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The stack check made the hash stop part way down a chain like `x + x + ...` with 100,000 terms, so an edit to the first terms did not change the signature. `Data::write_to_hasher` now walks the left spine of a binary expression on the heap, as the visitor and the printer do. The bytes that it writes are the same as before. The stack check stays for `a.b.c...`, which the visit pass reports as too deep. The test compares two chains that differ in the first term.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 9:12 PM PT - Sep 21st, 2026
✅ @robobun, your commit 9a553d5aedb9cbfe4c7214bb016260e16b039824 passed in 🧪 To try this PR locally: bunx bun-pr 43750That installs a local version of the PR into your bun-43750 --bun |
|
On the merge risk note from the automated review (a very deep initializer can keep the component state after an edit): the note was correct for the first version of the stack check. With the stack check alone, the hash stopped part way down a long chain. Two d3e78a4 changes The stack check stays for member chains ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
2 participants
Problem
const [a, setA] = useState(0)andconst [b, setB] = useState(0)both print_s(C, "9DqC+93o+Nw="). React keeps the state across the rename.react-refresh/babelresets it.visit_decls(src/js_parser/visit/mod.rs:369) hashes the binding only whenreact_refresh.last_hook_seenis the initializer call. Nothing sets that field.B::write_to_hasher(src/ast/b.rs:139) writes the length of an identifier name, not the name.Fix
handle_react_refresh_hook_callrecords the call inlast_hook_seen, after the hook context exists, so theunwraps invisit_declshold.B::write_to_hasherwrites the name bytes.Data::write_to_hasherrecursed with no bound:useState(x+x+…)with 100,000 terms is a segmentation fault on bun 1.4.3, and a binding default now reaches it. It now walks a binary chain on the heap, like the visitor and the printer. AStackCheckguards the other recursion.test/bake/dev/react-spa.test.ts. Both new tests fail on bun 1.4.3. Also25716.test.tsandbundler_jsx.test.ts.Background
_s(Component, "<signature>"). If an edit changes the signature, React remounts the component and its state resets./^use[A-Z]/call, and the initial state argument ofuseStateanduseReducer.Bis the AST node of a binding pattern: identifier, array or object.Downsides
const { data } = useQuery()toconst { data, error } = useQuery()remounts.useState(0) as T) Bun hashes the binding and Babel does not. A rename there resets in Bun only.Notes
Upstream.
react-refresh/babelbuilds the key asuseState{[a, setA](0)}: hook name, source text of the declarator id, initial state (ReactFreshBabelPlugin.js:422-436). React tests it: "resets state when renaming a state variable" (ReactFreshIntegration-test.js:593). The Zig sources had the same two gaps since the transform landed in #13215.No remount on upgrade. Signatures are not stored. A hook call that is not a whole initializer keeps its signature (
useState(0);stays9DqC+93o+Nw=).Which declarations take the binding. Only a hook call that is the whole initializer:
const x = useA(),const [a, b] = useA(),const { a: b } = useA(), each declarator ofconst a = useA(), b = useB(), and the outer call ofconst x = useA(useB()).const x = [useA()],const x = foo(useA()),const x = await useA()andx = useA()do not, as in Babel (path.parent.type === 'VariableDeclarator',ReactFreshBabelPlugin.js:423).Why the
unwraps hold.visit_declssetslast_hook_seentoNonebefore it visits each initializer.handle_react_refresh_hook_callsets it only after it created theHookContextof the current function. A hook call outside a function returns before that. The value is compared by address only. It is never read through. Equal addresses mean the call was handled during the visit of this initializer, with no function boundary between, so the hook context is the same one.Hash bytes. Per hook call: name, initial state argument,
\0, then the binding. An identifier binding is tag, length, name. The first byte after\0is a binding tag (0 to 2) or theuof the next hook name, so the stream is not ambiguous.Checks.
test/bake/dev/react-spa.test.ts(8 pass),test/bake/dev/hot.test.ts(11 pass),test/regression/issue/25716.test.ts(2 pass),test/bundler/bundler_jsx.test.ts(57 pass, 5 todo)..tsx/.jsxfixtures fromtest/throughbun build --react-fast-refresh --external '*'with bun 1.4.3 and with this branch (debug, ASAN). Exit codes are equal. The outputs are identical after the signature strings are masked. 50 files have a new signature. The 28 files with only unassigned hook calls keep theirs.react-refresh/babelover the 42 render/patch pairs of upstreamReactFreshIntegration-test.js(69 function/pair rows). Rows that disagree with Babel: 21 on main, 15 on this branch. No row regresses. The 6 fixed rows are the "resets state when renaming a state variable" tests (plain, in a HOC, with indirection, direct call), "does not get confused when custom hooks are reordered" and "does not get confused when component is called early".--minify,--minify-syntax,--minify-identifiersand--react-compiler.useStateargument,useReducerargument, binding default and computed key builds, and the whole chain is hashed. Two chains that differ only in the first term get different signatures. With the stack check alone they got the same one, because the hash stopped part way down. A 100,000 deepx.a.a.a…argument giveserror: Maximum call stack size exceededfrom the visit pass. bun 1.4.3 gives a segmentation fault for the argument cases. Nested binding patterns stop at the parser limit with the same error, below the depth where the hash could overflow.??, comma, assignment,**, unary) givesOUTRQVtcIug=on bun 1.4.3 and on this branch.Self-review, rejected concerns.
as,!,satisfies): the parser drops the wrapper, so Bun hashes the binding where Babel does not. A rename is still a rename. A reset is the safe direction.for (let i = useA(); ; ): the binding is not hashed, becausevisit_for_loop_initdoes not go throughvisit_decls. A hook in a loop header breaks the rules of hooks.null ?? useA(),false ? useB() : useA()) takes the binding only when the surviving call was visited last. These are conditional hooks.Not changed here (the same on main).
new, JSX and conditionals.useState({ count: 0 })touseState({ count: 1 })keeps the state.// @refresh resethas no effect (force_resetis never set)..tsfiles get no signatures, so a custom hook in a.tsfile is not tracked.useA?.()counts as a hook call in Bun, not in Babel.Bugs found outside this change. No open PR or issue covers them.
bun build --react-fast-refreshonexport default function App() { const [a] = useState(0); return a; }printsconst App = _s(function App() { … }, "…"); var App = App;. The output does not parse:SyntaxError: Identifier 'App' has already been declared. The dev server path is not affected.class K { m() { useState(1) } }) panics inlower_class(src/js_parser/p.rs:7399,internal error: entered unreachable code) for.tsand.tsx. For.jsxit printsm: _s(function() { … }, "…")inside the class body, which does not parse.[human-review] gate passed · iteration 0 · 4 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