Repository navigation
Conversation
When HMR is disabled, reading import.meta.hot.accept (or dispose/prune/on/off/send/decline/invalidate) without calling it would set p.method_call_must_be_replaced_with_undefined unconditionally. That flag is parser-wide and only consumed by e_call after visiting its target, so the write leaked into the next unrelated call expression and silently dropped it from the output. Gate the flag write on identifier_opts.is_call_target() so it is only set when an enclosing e_call will actually consume it. The --drop path already gates on the equivalent condition.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe parser now tracks whether disabled ChangesHMR property folding
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly summarizes the main parser change: the import.meta.hot call-drop flag is now limited to call targets. |
| Description check | |
The description explains the problem, fix, scope, trade-offs, cache change, and verification results. It does not use the exact template headings, but it provides the required information in equivalen… |
Full details: Description check
-
Explanation
The description explains the problem, fix, scope, trade-offs, cache change, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 12:53 PM PT - Oct 17th, 2026
✅ @robobun, your commit 534c2573630aa7468065c544ead70b5bf3ec7128 passed in 🧪 To try this PR locally: bunx bun-pr 36875That installs a local version of the PR into your bun-36875 --bun |
|
Reproduced on 1.4.3 and on a debug build of main: With this PR: An indirect call of a method is no longer removed. With this PR PR: #36875 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — surgical gate on the parser-wide drop flag; the leak into unrelated calls is fixed and the direct-call stripping path is unchanged.
What was reviewed
- Traced
is_call_targetat bothmaybe_rewrite_property_accesscall sites (e_dot/e_index) back top.call_target, whiche_callsets before visiting and whose consumer clears the flag on return — the gate matches the flag's lifetime exactly. - Confirmed the
--dropwriters of the same flag already gate onin_.property_access_for_method_call_maybe_should_replace_with_undefined, so this brings theimport.meta.hotpath in line. - Checked the third caller in
p.rs(define-chain builder) passes default opts, so it can't reach theHotDisabledarm withis_call_targettrue. - New tests cover every affected method name, both bundled and
--no-bundle, plus a re-assertion that direct calls are still stripped.
Extended reasoning...
Overview
Two-line fix in src/js_parser/fold.rs (maybe_rewrite_property_access, the ESpecial::HotEnabled|HotDisabled arm): the writes to p.method_call_must_be_replaced_with_undefined in the accept and decline|dispose|prune|invalidate|on|off|send branches are now gated on identifier_opts.is_call_target(). Two new itBundled cases in test/bundler/bundler_minify.test.ts sit next to the existing ImportMetaHotTreeShaking test.
Mechanism check
method_call_must_be_replaced_with_undefined is a parser-wide bool. Its only consumer is e_call (visit_expr.rs:1965-1968), which reads and clears it immediately after visiting its own target. is_call_target at the two visit_expr.rs call sites (887, 1343) is computed by pointer-comparing the current node against p.call_target, which e_call sets before that visit. So identifier_opts.is_call_target() is true exactly when an enclosing e_call will consume the flag on return — the gate is precise. The other two writers of this flag (visit_expr.rs:302, 1368, for --drop) already gate on the analogous in_.property_access_for_method_call_maybe_should_replace_with_undefined, so the hot path was the outlier.
The change is a strict narrowing: when is_call_target() is true, behavior is byte-identical to before (flag set, EUndefined returned, enclosing call dropped). When false, the property still folds to EUndefined but the flag stays clear, so the next unrelated call in the file survives. The one behavioral corner case I noted — import.meta.hot.accept.call(...) — now emits undefined.call(...) instead of being silently dropped, which is arguably more correct given import.meta.hot.accept is undefined when HMR is off.
Security risks
None. Bundler-output correctness only; no untrusted-input parsing, no allocation, no FFI.
Level of scrutiny
Medium — js_parser is production-critical, but the diff is a two-token guard on a niche branch (HMR-disabled import.meta.hot property access) with a well-understood consumer. The bug it fixes (silently dropping user code) is far more serious than any edge the narrowing could introduce.
Other factors
Tests are placed alongside existing coverage per repo convention, exercise every method name in the lookup table, cover both bundling modes, run the output, and negative-assert that direct calls are still stripped and import.meta.hot is gone from output. PR description reports bundler_minify, bundler_drop, and bake/dev-and-prod suites passing; robobun confirmed the repro before/after.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that gates the parser-wide drop flag on is_call_target(), matching how the --drop path already guards it.
What was reviewed:
- Traced
p.call_targetset ine_call(visit_expr.rs:1845) →is_call_targetcomputed on the EDot node → passed through tomaybe_rewrite_property_access; the flag is now only set when the enclosinge_callwill read-and-clear it at visit_expr.rs:1965-1968. - Confirmed the non-call path still returns
EUndefined(bare reads fold correctly), and the existingImportMetaHotTreeShakingtest plus the newFAIL-*-callassertions guard against regressing the call-stripping behavior. - Both callers in visit_expr.rs (EDot line 1429, EIndex line 1037) thread
is_call_targetthrough, so the gate applies toimport.meta.hot["accept"]too.
Extended reasoning...
Overview
Two-line fix in src/js_parser/fold.rs wrapping p.method_call_must_be_replaced_with_undefined = true in an if identifier_opts.is_call_target() guard for both the accept branch and the decline|dispose|prune|invalidate|on|off|send branch of the HotEnabled | HotDisabled arm. Two new itBundled tests in test/bundler/bundler_minify.test.ts covering bundled+run and --no-bundle variants.
Security risks
None. This is bundler/transpiler constant folding for import.meta.hot — no auth, crypto, filesystem, or untrusted-input parsing surface.
Level of scrutiny
Low-medium. The mechanism is well-explained and I verified it against the source: e_call sets p.call_target = target.data (visit_expr.rs:1845) before visiting, the EDot/EIndex visitors compute is_call_target by pointer equality against that, and e_call consumes and clears the flag immediately after the target visit (visit_expr.rs:1965-1968). Gating on is_call_target() therefore guarantees an enclosing consumer exists. The change is strictly a narrowing — the flag is set in a subset of the cases it was before — so the only risk is that call-stripping stops working, which both the pre-existing ImportMetaHotTreeShaking test and the new not.toContain("FAIL") / not.toContain("import.meta.hot") assertions cover.
Other factors
The comment-cop inline about a paragraph-long explanatory comment was addressed in 3964047 (comment removed; thread resolved). Tests are placed next to existing import.meta.hot coverage per repo convention, exercise all eight affected method names, assert both output contents and runtime stdout, and re-verify the positive case (direct calls still stripped). PR description reports bundler_minify, bundler_drop, and bake/dev-and-prod suites green.
…ept-read-flag-leak
With HMR off, reading an import.meta.hot method set the parser-wide "replace the method call with undefined" flag even when no call was being visited. The next call expression in the file took the flag and was removed. The flag is now a type with one gated writer. The import.meta.hot writer raises it only for a node in the callee of the call that is being visited, as the --drop writers already did. ExprIn carries that position as CalleeChain (None, Dots, Index). e_index forwards it, so a call such as import.meta.hot.accept["call"](...) is still removed, and --drop keeps following `.` only. e_index holds the flag back while it visits its index, so a call in the index cannot take it. The runtime transpiler cache version is bumped because cached output of the unfixed parser has the call removed.
robobun
changed the title
…ept-read-flag-leak # Conflicts: # src/jsc/RuntimeTranspilerCache.rs
A call whose callee folds to a disabled import.meta.hot method, such as `(0, import.meta.hot.accept)(cb)` with minify.syntax or `(true ? import.meta.hot.accept : x)(cb)`, was removed before the flag got its gate. With the gate the method read is not in the callee chain, so the call stayed as a call on `undefined`. The operand that takes the place of a conditional, logical or comma expression now takes its place in the callee chain too. `--drop` does not follow it, as it does not follow `[]`.
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 also checked the comma-callee path added in 75a6189: visit_right_and_finish now visits the right operand of , / || / && / ?? with callee_chain.indirect(), and callee_chain_at hands the call position only to the outermost op on the binary stack, so a left-nested (a, b), c callee is not mis-marked. I also traced e_call: the flag is take()n before any argument is visited, so no argument subtree sees it raised and the new debug assertion cannot fire from that path.
Extended reasoning...
The diff replaces the parser's method_call_must_be_replaced_with_undefined bool with a MethodCallDrop newtype and threads a CalleeChain enum through ExprIn, e_dot, e_index, e_if, e_binary and the binary-expression visitor so the import.meta.hot / --drop call removal only raises inside a callee; it also bumps the runtime transpiler cache version to 36 and adds bundler and runtime-transpiler tests. No security-sensitive surface is touched. The latest commit adds comma/logical propagation that covers the case flagged in the prior run, with a dedicated minifySyntax test. One nit about missing dead-branch test coverage is posted inline; the parser hot-path change is non-trivial enough that a human look remains worthwhile.
…ept-read-flag-leak
Replace the callee-chain gate with the gate of the first version of this change. With HMR off, the import.meta.hot arm of maybe_rewrite_property_access sets method_call_must_be_replaced_with_undefined only when the access is the target of the call that is being visited. p.rs, parser.rs, visit_expr.rs and visit_binary.rs are as on main again. A call through a property of a disabled method, such as import.meta.hot.accept.call(null, fn), is no longer removed. It prints as a call on undefined. The docs say that these methods must be called directly. Tests: keep the read-without-call cases and the dropped-call case, add the two other spellings of a direct call to them, and remove the cases that pinned the removal of indirect calls.
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline finding, I also checked the two --drop sites that set the same flag (src/js_parser/visit/visit_expr.rs:293 and :1352) for the same leak — both are already scoped by in_.property_access_for_method_call_maybe_should_replace_with_undefined, which e_call sets only along the callee chain, so they do not need the new gate. Note that the diff at HEAD contains only the is_call_target gate in src/js_parser/fold.rs plus the cache version bump; the MethodCallDrop / ExprIn::callee_chain mechanism the PR description describes is not in this code, and no test covers import.meta.hot.accept.call(...).
Extended reasoning...
The change gates the method_call_must_be_replaced_with_undefined flag on identifier_opts.is_call_target() at the two import.meta.hot fold sites in src/js_parser/fold.rs, bumps the runtime transpiler cache version to 36, and adds bundler, runtime-transpiler and Bun.Transpiler tests for read-without-call shapes. It touches no security-sensitive surface. The confirmed inline finding (methods invoked through .call/.apply/.bind now reach runtime as undefined.call) is the reason a human should weigh in, and the PR description describes a callee-chain design that the current diff does not contain.
Problem
import.meta.hotmethod that is not a call can delete another call.if (import.meta.hot) { register(import.meta.hot.dispose); } realWork(); more();printsif (undefined) {} more();.p.method_call_must_be_replaced_with_undefined(src/js_parser/fold.rs:765,:799). The next call that the visitor reaches takes the flag and, for most callees, is deleted.Fix
if identifier_opts.is_call_target(). Only the callee of a call sets the flag, and that call takes it at once.bundler_minify.test.ts,bundler_drop.test.ts(7 new tests fail on 1.4.3). Alsotranspiler.test.js,esbuild/dce,cli/run/transpiler-cache.Background
import.meta.hotisundefined. Bun removes a directimport.meta.hot.accept(cb). The docs say the methods "must be called directly".e_callrecords its callee inp.call_targetbefore it visits it.is_call_targetis true only for that node.34d735d07e) keeps indirect calls removed. It costs instructions on each.and[]visit.Downsides
import.meta.hot.accept.call(null, fn). It prints as a call onundefinedand throwsTypeErrorat load. Main removed it through the leak.bun build, the callback of such a call is live again, so its imports are bundled.Notes
Shapes,
bun build --no-bundle, HMR off (misimport.meta.hot.accept. The other seven methods behave the same.)const f = m; init(); start();init()deletedif (import.meta.hot) { register(import.meta.hot.dispose); } realWork(); more();realWork()deleteddoSomething(m); after();after()deletedif (typeof m !== "function") { setup(); }setup()deletedx[m](); after();x[...]()deletedx[undefined]()(import.meta.hot ? m : noop)(fn);noop(fn)m(() => { import.meta.hot.invalidate; }); a();a()deletedconsole.log(m); after();with--drop=consoleafter()deletedm(cb),import.meta.hot["accept"](cb),m?.(cb)import.meta.hot?.accept(cb)undefined?.accept(cb)undefined?.accept(cb)m.call(null, fn),m.apply(null, [fn]),m["call"](null, fn)undefined.call(null, fn)m.bind(null),m.toString()undefinedundefined.bind(null)(true ? m : x)(fn),(null ?? m)(fn),[m][0](fn)undefined(fn),[undefined][0](fn)(0, m)(fn)(0, undefined)(fn), removed with--minify-syntax(0, undefined)(fn),(void 0)(fn)with--minify-syntaxm[foo()]()undefined[undefined]()undefined[foo()]()const accept = m; accept(cb);accept(cb)deletedThe rows from
m.calldown are the accepted change. With this PR,import.meta.hot.accept.call(null, () => {})fails at load withTypeError: undefined is not an object (evaluating '(void 0).call').Which call the leak deletes. The flag is taken by the next call expression that the visitor reaches. That call is deleted when its visited callee is an identifier, a member access or
undefined. A call with another callee shape clears the flag and stays.newand tagged templates do not take the flag.Why not keep indirect calls removed. Earlier versions of this PR did: a callee-chain state in
ExprIn, forwarded bye_dot,e_indexand folded?:,||,&&,??,,(last at34d735d07e). A review of that version found these points.docs/bundler/hot-reloading.mdxlists indirect use as invalid and says the methods must be called directly..visit, 8 to each[]visit and 5 to each binary expression (release x64 assembly of the parser crate), for every file that Bun parses.[m][0](fn)throw, so the line it drew was its own.If a maintainer wants indirect calls to stay removed, the smaller option is to gate the two writes on the armed
ExprInbool that--dropuses, passed through the freeIdentifierOptsbit.Bundle effect of a kept call. For
import.meta.hot.accept.call(null, () => import("./dev.js")), 1.4.3 prints a bundle withoutdev.js. This PR keeps the call, sodev.jsis in the bundle.Cache. The cache key has the source hash, the features hash and the cache version, and no Bun version. I patched the version byte of an entry that 1.4.3 wrote for a 6 KB file with the first table row, to stand for an entry from an unfixed parser at the same version. The fixed parser restored it and the program still printed only
start.Related. #45658 has the same two gate lines in the same arm, inside a larger change, and also uses cache version 36. The second of the two PRs to merge needs a rebase in that arm and the next cache version. #40857 (
--dropthrough[]) and #45566 (--dropand.bind) edit the same flag. This PR does not change--drop.Seen and not changed here.
import.meta.hot?.accept(cb)is not removed. It printsundefined?.accept(cb), so the callback and its imports stay in the bundle. Fiximport.meta.hot?.accept()throwing in dev server #35009 is about this form in the dev server.import.meta.hot["acc" + "ept"](fn)with--minify-syntaxprints(void 0).accept(fn). The latea["b" + ""]toa.brewrite ine_indexdoes not callmaybe_rewrite_property_access.import.meta.hot.accept = 1printsundefined = 1. js_parser: keep assignment and delete targets as references #45658 changes that.Suites run with the debug build.
bundler_minify71 pass,bundler_drop15,transpiler/transpiler.test.js238,esbuild/dce87,bake/dev/hot11,bake/dev-and-prod12,cli/run/transpiler-cache20.[review] 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