Repository navigation
Conversation
…ction
`--drop=console` replaces each call through a dropped name with
`undefined`. `console.log.bind(console)` is such a call, but it does not
call `console.log`. It makes a function that the program calls later.
The output was `var log = undefined`, and the built program threw
`TypeError: log is not a function` at load.
In the drop branch of `e_call`, a call of `<dropped>.method.bind` now
becomes `() => {}`. Its arguments are removed, like the arguments of
each dropped call. Each other dropped call stays `undefined`:
`.call`, `.apply`, a method that is named `bind` on the dropped name
itself (`Mousetrap.bind(key, fn)` with `--drop=Mousetrap`), and a drop
key that ends in `.bind`.
The helper reads the drop list again and does not trust the flag alone,
because `import.meta.hot` sets the same flag with no drop list.
The runtime transpiler cache version is now 36, so that a cached entry
does not keep the old output.
… callee spelling
With `--drop=app --drop=app.keys.bind`, `app.keys?.bind(k, fn)` became
`() => {}`. The check for a callee that is itself on the drop list used
`is_dot_define_match`, which does not match through `?.`. The check now
compares the names of the callee chain with the drop key, so each
optional spelling stays `undefined`.
The guard arm in `e_call` tests the name `bind`, so the helper does not
test it a second time.
Tests: one table with a row for each callee spelling, the truthiness of
a bound dropped method, the index spellings, the single-use inlining
pass, arguments that contain scopes, and a drop key that ends in
`.bind` with its prefix on the drop list too.
|
Updated 1:05 PM PT - Oct 16th, 2026
❌ @robobun, your commit 070a0a8 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 45566That installs a local version of the PR into your bun-45566 --bun |
|
Status Reproduced on stock 1.4.2 and on release builds of // in.mjs
const log = console.log.bind(console);
log("hello");
process.stdout.write("ALIVE\n");
With this PR the output is Run the tests with |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe drop transform now replaces eligible ChangesDropped-function bind handling
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 identifies the main change: --drop now lowers dropped .bind(...) calls to a callable no-op instead of undefined. |
| Description check | |
The description explains the problem, implementation, behavior, scope, cache update, and verification results. It does not use the exact template headings, but it provides the required information and… |
Full details: Description check
-
Explanation
The description explains the problem, implementation, behavior, scope, cache update, and verification results. It does not use the exact template headings, but it provides the required information and is substantially complete.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 findings, I also checked two things and ruled them out: the early-return arm in e_call restores ignore_dce_annotations and should_fold_typescript_constant_expressions the same way the fallthrough path does, and the helper saves/restores is_control_flow_dead itself before that flag is set; and the behavior change where a guarded bound logger (if (log) log(dump())) now takes the truthy branch is intentional per the documented downside and matches esbuild 0.25+, not a defect in the arm.
Extended reasoning...
The change adds a .bind-on-dropped-callee arm to the --drop handling in src/js_parser/visit/visit_expr.rs with three new lookup helpers, bumps the transpiler cache EXPECTED_VERSION to 36, and adds bundler and transpiler-cache tests plus docs; it touches no security-sensitive surface. Inline findings cover gaps in the receiver guard (X.bind vs deeper .bind members), the EIndex string-key mismatch between is_drop_key and is_dot_define_match, and two test nits, so a human should weigh those before merge.
| // Not a bind of a dropped function: `Mousetrap.bind(k, f)`, or a callee on the drop list. | ||
| if !matches!(bind.target.data, Data::EDot(..)) |
There was a problem hiding this comment.
🟡 (optional) Users who drop a namespace whose API has a method literally named bind now get a bare () => {} where the base gave undefined, so guards like if (h) h.unbind() enter and throw. src/js_parser/visit/visit_expr.rs:1884 only rejects a direct X.bind receiver; app.keys.bind(k, fn) with --drop=app passes because app.keys is an EDot and is_dropped_name matches app. Fix: rewrite to an arrow only when the receiver is a dropped callable method shape the user asked for (for example require the exact a.b.bind not be a plain method of a dropped namespace, or default to undefined when the bind call carries more than one argument), and apply the same rule at every sibling receiver shape.
Why this was flagged
Trigger: --drop=app (or --drop=$, --drop=Mousetrap-style namespace entries) and a call app.keys.bind("ctrl+s", fn) reached through e_call at src/js_parser/visit/visit_expr.rs:2212. The guard at src/js_parser/visit/visit_expr.rs:1884 only requires bind.target to be an EDot; is_drop_key at :1901 is false unless the user also listed app.keys.bind; is_dropped_name at :1930 returns true as soon as the root app is a drop entry. The helper then emits () => {} at :1897. On the base the same call reached :2245 and became undefined. Code that stored the result and guarded it, const h = app.keys.bind(k, fn); if (h) h.unbind(), skipped the branch on the base and now calls unbind on a bare arrow, throwing TypeError. The PR text names this as a downside but sizes it as rare; jQuery-style $.fn.bind and hotkey libraries make bind a common method name under a dropped namespace.
Verification: Trigger: --drop=<ns> where <ns> has a nested object whose own method is literally named bind and the call's return value is used. At src/js_parser/visit/visit_expr.rs:1884 bind.target is an EDot; is_drop_key (:1901) is false with only app dropped; is_dropped_name (:1924) returns true; the call is rewritten to E::Arrow::NOOP_RETURN_UNDEFINED at :1897. On the base the same input became undefined.
| } | ||
| /// Whether `callee` spells a drop key. Unlike `is_dot_define_match`, `a?.b` counts as `a.b`. | ||
| fn is_drop_key(p: &Self, callee: Expr) -> bool { | ||
| let Data::EDot(last) = callee.data else { | ||
| return false; | ||
| }; | ||
| let Some(keys) = p.define.dots_for(last.name.slice()) else { | ||
| return false; | ||
| }; | ||
| keys.as_slice().iter().any(|key| { | ||
| let Some((root, members)) = key.parts.split_first() else { | ||
| return false; | ||
| }; | ||
| let mut expr = callee; | ||
| for member in members.iter().rev() { | ||
| match expr.data { | ||
| Data::EDot(dot) if strings::eql(member, &dot.name) => expr = dot.target, | ||
| _ => return false, | ||
| } | ||
| } | ||
| let Data::EIdentifier(id) = expr.data else { | ||
| return false; | ||
| }; | ||
| key.data.method_call_must_be_replaced_with_undefined() | ||
| && strings::eql(p.load_name_from_ref(id.ref_), root) | ||
| }) |
There was a problem hiding this comment.
🟡 (optional) Users who list both a.b.c and a.b.c.bind in --drop still get () => {} instead of undefined when the call is spelled a["b"].c.bind(x). The flag is raised by is_dot_define_match (src/js_parser/p.rs:7280), which accepts a UTF-8 string EIndex as a dot part, but is_drop_key at src/js_parser/visit/visit_expr.rs:1911 only walks EDot nodes, so the full-callee drop key is missed and is_dropped_name then matches the a.b.c entry. Fix: make is_drop_key recognise every callee spelling that can raise the flag (string EIndex parts as well as EDot), e.g. by walking the same shapes is_dot_define_match walks, so the documented 'add <callee>.bind to the drop list to keep undefined' escape hatch holds for all spellings.
Why this was flagged
Input lib["log"].info.bind(x) built with drop: ["lib.log.info", "lib.log.info.bind"] and no minify-syntax, through bun build or Bun.build. Visiting the call target in e_dot (src/js_parser/visit/visit_expr.rs:1337-1357) matches lib.log.info.bind because is_dot_define_match treats lib["log"] as lib.log (src/js_parser/p.rs:7280-7291) and raises method_call_must_be_replaced_with_undefined. In e_call the new arm calls replace_bind_of_dropped_function; is_drop_key (src/js_parser/visit/visit_expr.rs:1899-1924) walks bind then info as EDot, reaches the EIndex lib["log"] and returns false, so the explicit lib.log.info.bind drop entry is not seen. is_dropped_name then matches lib.log.info via is_dot_define_match and the call is rewritten to () => {}. On the base branch the same input became undefined. The PR text says adding <callee>.bind to the drop list keeps undefined; that only holds for the pure dot spelling (and ?.), not for the bracket spelling the flag-setting matcher already accepts.
Verification: Trigger: --drop lists both a dropped name/method and its .bind spelling, the call uses bracket notation (lib["log"].info.bind(x)), and minify-syntax is off. is_drop_key (visit_expr.rs:1901-1925) walks the chain matching only Data::EDot, so the EIndex at lib["log"] makes it return false even though the full key is listed. Result: () => {} where base gives undefined.
| console?.log.bind(console)("FAIL"); | ||
| console.log.bind?.(console)("FAIL"); | ||
| ["FAIL"].forEach(console.log.bind(console)); | ||
| Promise.reject(new Error("FAIL")).catch(console.error.bind(console)); | ||
| my.logger.bind(null)("FAIL"); |
There was a problem hiding this comment.
🟡 nit (optional): a maintainer gets a viaAlias check that prints "callable" with or without the fix, plus a comment that misdescribes the parser. m.logger.bind(null) at test/bundler/bundler_drop.test.ts:150 has a bound local m as its root, so e_identifier (src/js_parser/visit/visit_expr.rs:256) never sets the drop flag and the call is left as written. Single-use inlining (src/js_parser/visit/mod.rs:1963) swaps my in after the statement was visited; nothing is "visited again". Fix: either drop viaAlias or make it assert the negative contract it actually pins (an alias of a dropped name is not dropped), and reword the comment at test/bundler/bundler_drop.test.ts:143 so it does not claim a second visit.
Why this was flagged
The row out(viaAlias()) expects "callable" (test/bundler/bundler_drop.test.ts:160). viaAlias reads typeof m.logger.bind(null) where m is const m = my (test/bundler/bundler_drop.test.ts:148-150). Because m is a bound local, e_identifier's Unbound check at src/js_parser/visit/visit_expr.rs:256 never sets method_call_must_be_replaced_with_undefined, so e_call never reaches the undefined arm or the new bind arm. At run time my.logger is the real function from globalThis.my = { logger() {...} }, so typeof is "function" on the base branch and on this PR alike. The comment at test/bundler/bundler_drop.test.ts:143 says single-use locals are "inlined and visited again"; substitute_single_use_symbol_in_stmt (src/js_parser/visit/mod.rs:1963, src/js_parser/p.rs:2787) rewrites the already-visited AST and does not re-run the visitor, so the drop logic never sees the inlined form. The other rows in the same test (viaLocal, the bound calls) do fail on the base, so the test as a whole is live; only this row and its comment are vacuous.
Verification: The viaAlias row prints "callable" regardless of whether the .bind fix is present. m is a block-scoped const, so it is a bound symbol; src/js_parser/visit/visit_expr.rs:256-297 is gated on Kind::Unbound, so m.logger.bind(null) never reaches the drop branch of e_call and is emitted unchanged. The comment at test line 153 is wrong: nothing is visited again.
| // Transpiled on the first run, served from the cache on the second. | ||
| expect(run(temp_dir, ["--drop=console", "a.js"])).toBe("function"); | ||
| expect(run(temp_dir, ["--drop=console", "a.js"])).toBe("function"); |
There was a problem hiding this comment.
🟡 nit (optional): the new cache test cannot tell a cache hit from a re-transpile, so the EXPECTED_VERSION bump it accompanies is not exercised. The comment at test/cli/run/transpiler-cache.test.ts:467 says the second run is served from the cache, but no newCacheCount() assertion backs it, unlike the sibling test at test/cli/run/transpiler-cache.test.ts:445-455. Fix: assert newCacheCount() after each run (1 after the first write, 0 after the --drop=console runs) so the test follows the file's convention and the cache-served claim can fail.
Why this was flagged
The test at test/cli/run/transpiler-cache.test.ts:458-470 runs the file three times and only compares stdout. Nothing in it reads the cache directory, so the test passes identically whether the second --drop=console run reads the cached entry or transpiles again, and it passes if the transpiler cache were disabled entirely. Every neighbouring test in this describe block (for example test/cli/run/transpiler-cache.test.ts:446, :449, :452, :455) pairs each run with expect(newCacheCount()).toBe(...). The comment on line 467 therefore states something the test does not verify. Consequence is test-quality only: no runtime behaviour differs from the base branch.
Verification: nit. Triggering condition: whenever the new test runs, it exercises only stdout, so it passes with the on-disk cache engaged, bypassed, or disabled. At test/cli/run/transpiler-cache.test.ts:458-470 the three run(...) calls are wrapped in expect(...).toBe(...) on stdout only; nothing in the test reads cache_dir (no newCacheCount()), so the comment at :467 is not backed by any assertion.
Problem
--drop=consoleturnsconst log = console.log.bind(console)intovar log = undefined. The build succeeds, andlog(...)throwsTypeError: log is not a function.e_call(src/js_parser/visit/visit_expr.rs:2115) replaces each call through a dropped name withundefined. Here that call is.bind(...), which only makes a function.Fix
e_call, a call of<dropped>.method.bindbecomes() => {}. Each other dropped call staysundefined..bind(...)arguments are removed, as for each dropped call.ASSERT.ok.bind(ASSERT)with--drop=ASSERTwould throwReferenceError.undefined.test/bundler/bundler_drop.test.ts(9 new tests, 8 fail without the fix),test/cli/run/transpiler-cache.test.ts(1 new),bundler_minify,esbuild/dce. Self-reviewed: 11 concerns raised, 11 addressed.Background
e_callreplaces the call. The flag has no depth:console.log.bind(...)looks likeconsole.log(...).e_dot(bundler: keep console.METHOD.bind() callable under --drop=console #33852): +6 instructions on each property access. The drop branch ofe_callruns only for a dropped call.Downsides
--drop: 0 instructions per property access, 0 to -1 per call, text +2,304 bytes. With--drop: +3 to +6 per dropped call, +1,054 per rewritten.bind.if (log) log(dump())runsdump().x = app.keys.bind(k, fn)withdrop: ["app"]now gives a function, notundefined. Addapp.keys.bindto the drop list to keepundefined.Notes
Question for the reviewer. No maintainer has said what
.bindgives under--drop. This PR says: a function that does nothing. #36875 and #40857 touch the same flag and can follow that rule. There are two shapes for it:e_calland a helper that reads the drop list again, then js_parser: set the import.meta.hot call-drop flag only for a call target #36875 for the flag.Pin place of the bool, which the code that sets the flag fills in. It removes the helper's second look at the callee and also fixes the--definecase below. It changes each place that sets the flag.Output
--drop=consoleunless shown)const log = console.log.bind(console)undefined() => {}console.log.bind(console)("x")undefined("x"), throws(() => {})("x")p.catch(console.error.bind(console))p.catch(undefined)p.catch(() => {})typeof console.log.bind(console)"undefined""function"console.log?.bind(console),console?.log.bind(console),console.log.bind?.(console)undefined() => {}my.logger.bind(null)with--drop=my.loggerundefined() => {}ASSERT.ok.bind(ASSERT)with--drop=ASSERTundefined() => {}console.log.bind(console);as a statementconsole.log("x"),.call(...),.apply(...)undefinedundefinedconsole.bind(x),Mousetrap.bind(k, fn)with--drop=MousetrapundefinedundefinedMousetrap.bind(k, fn)with--drop=Mousetrap.bindundefinedundefinedapp.keys?.bind(k, fn)with--drop=app --drop=app.keys.bindundefinedundefinedconsole.log.bind.call(console.log, console)undefinedundefinedconsole["log"].bind(console)without--minify-syntaxA matrix of 54 shapes, with and without
--minify-syntax, gives 46 different outputs. Each of them is a.bindon a dropped name.drop/BindOfEachDroppedCalleepins 22 callee spellings in both modes.Why a bare arrow. esbuild prints
(() => {}).bind(console)and Terser prints(function(){}).bind(). They keep the call because they handle each method name the same way. Bun treats onlybindin a special way, so the value of the call is known.() => {}is callable, needs no pure annotation to go away when it is not used, and lets thetypeof,!and||folds see a function. The arguments are not kept, because--droptakes any name:ASSERT.ok.bind(ASSERT)with--drop=ASSERTwould read a global that does not exist.What a truthy value changes. Code that tests a bound console method now takes the other branch, as it does with no
--drop:if (log) log(dump())callsdump(), a library option{ debug: console.log.bind(console) }counts as set, and arequire()orimport()behind such a test is in the bundle again. No callable value is falsy, so no output form avoids this. esbuild made the same change in 0.25.0.Why the helper reads the drop list again.
import.meta.hot.acceptsets the same flag with no drop list when HMR is off (src/js_parser/fold.rs:765,:799), and a read of it with no call leaves the flag set for the next call (#36875). The helper gives() => {}only when the receiver is a dropped name. Without a drop list the output is the same as on main: 171 of 171 outputs are byte-identical (57 files, with no flag,--minify-syntaxand--minify; the files includetypescript.js,sass.dart.jsand theimport.meta.hotshapes).Three conditions in the receiver walk copy the conditions of the code that sets the flag: the symbol is unbound, it is not in a
withscope, and the chain ends in an identifier. After a drop key matched they are always true. They decide only after a staleimport.meta.hotflag, where the output of main is wrong too, so no test pins them.Why the helper has its own lookup loop.
e_dothas the same loop overdots_for. It runs for each property access, and this PR does not change it. A const parameter onis_dot_define_match(to let one matcher reada?.basa.b) changed the machine code of that matcher: +26 to +29 instructions for each dotted define lookup such asObject.keysorprocess.env.X. So the helper compares the names of the callee chain itself.Measurements. Release builds of the merge base
a407eae88aand of this PR (070a0a8440).bun build --no-bundle <file>, user-space instructions fromperf_event_open, 5 interleaved runs, median. The base spread is 594 to 3,829 instructions.--drop.js.tsa.b(),.jsa.b(),.tsa(),.jsa(),.tsa.b.c.d.e()a.b(c, d)a(c)x = Object.keys(a) + Math.abs(b)x = process.env.NODE_ENV + process.env.FOOtypescript.js(9.1 MB)The -1 per call with arguments is block layout: the new arm ends in a return.
--drop=consolex = console.log.bind(console)console.log(1)console.warn(1)x = console.bind(1)typescript.jsvisit_expr_in_out: 17,694 to 17,752 bytes (JS copy), 17,555 to 17,620 (TS copy). Prologuesubq $0x248, %rspin both builds.size).E::Arrownode for each rewritten.bind. No allocation on any other path.The
e_dotdesign of #33852, same procedure (relinks ofa407eae88a): +6.0 instructions per property access, +8.0 per callee access,typescript.js+416,317 (+0.044%),visit_expr_in_out+192 bytes.The shape in
e_callmatters. A call to the cold helper from the block that writesundefinedmade LLVM inlinehandle_react_refresh_hook_callandtranspose_known_to_be_if_requireintovisit_expr_in_out: +992 bytes and a frame of0x258. The guard arm ahead of the argument visit does not change those decisions.Tests. On a release build of the merge base, 8 of the 9 new tests in
bundler_drop.test.tsfail, and so does the new test intranspiler-cache.test.ts.drop/BindKeepsPlainCallsDroppedis a guard: it passes on main and pins the calls that stayundefined. Builds with one condition of the helper removed fail it or the table: the member receiver, the callee on the drop list, the dropped receiver name, and the visit of the arguments (panic: Scope mismatch while visiting). The single-use inlining pass visits the call a second time; theviaAliasandviaLocallines ofdrop/BindStaysCallable*cover it.Other suites.
test/bundler/bundler_edgecase.test.ts: 181 pass.edgecase/DeepImportDiamondDAGtimed out once in the full run of the debug build and passes alone (46 s).History. #33852 was the first attempt. The stale workflow (
close-stale-robobun-prs.yml) closed it after 90 days. Its only reviews were from bots, and no maintainer rejected it.Not changed here
--drop=X.mwith a--definethat renamesX: with--drop=global.console --define global=globalThis,global.console.log.bind(global.console)is stillundefined. The helper reads the visited callee, and the define already renamed its root. Shape (b) above fixes this.import.meta.hot.accept.bind(x)with HMR off staysundefined.ASSERT.bind(null)with--drop=ASSERTstaysundefined: it can not be told fromMousetrap.bind(k, fn)with--drop=Mousetrap.console.log.toString()and other calls on a dropped method stayundefined.drop/BecomesUndefinedNested2pins the drop at any depth.console["log"].bind(console)without--minify-syntaxis not dropped, as on main.Related. #36875 changes how the same flag is set, on the same lines of
e_call, and also takes cache version 36. The PR that lands second takes 37, and only the read of the flag ine_callchanges here. If #40857 makese_indexforward the drop request, the receiver walk in the helper needs anEIndexlevel, and the table row fordebug[key]shows it.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/transpiler-cache.test.ts