Repository navigation
Conversation
…s use counts
When the visit leaves a top-level statement empty, the parser rolls the
statement's uses back from `use_count_estimate`. Two counts that are
compared with `use_count_estimate` were not rolled back:
- the tracked uses of an import() / require() namespace local. One
removed `!ns;` or `if (ns && process.env.NODE_ENV !== "production") ...`
hid one use of the whole namespace, and the importee lost every export.
- the `module.exports` rewrites. One removed `if (module.exports.a) {}`
hid one other use of `module`, and the file lost its CommonJS wrapper.
Each count now keeps the share that the open part noted. `revert_dead_part`
replaces `clear_symbol_usages_from_dead_part` and takes that share back
together with the uses.
The roll-back no longer sets the use count of a symbol the part declared
to zero. An unwrapped `var x = require()` leaves its statement empty, and
a use of `x` in a function above it was lost.
`ignore_usage_of_runtime_require` calls `ignore_usage` for `require`. A
folded `typeof require` or `require.main === module` subtracted a use
outside the part's record, so a removed statement subtracted it twice, and
unreachable code subtracted a use that was never counted.
|
Updated 12:15 PM PT - Oct 20th, 2026
✅ @robobun, your commit 41a593a21af57d324f586c5f9a4b87ee6d7235f9 passed in 🧪 To try this PR locally: bunx bun-pr 45345That installs a local version of the PR into your bun-45345 --bun |
|
Status: ready for review. CI is green on
The description has the new test count (19 cases fail on How this was reproduced (a 1.4.3 canary build and a release build of the merge base): // lib.mjs
export const a = 1;
export const b = 2;
// entry.mjs
const ns = await import("./lib.mjs");
!ns;
console.log(JSON.stringify(Object.keys(ns)));
// lib.js (CommonJS), imported with `import { a } from "./lib.js"`
exports.a = 1;
if (module.exports.a) console.debug("x");
function k(m) { globalThis.out = typeof m; }
k(module);
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe parser now tracks usage totals and notes by part generation. It reverts tracked usage when a visited part has no statements, and uses the resulting totals in CommonJS and dynamic-import decisions. New bundler tests cover both transformation paths across CLI and API backends. ChangesParser usage accounting
Possibly related PRs
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the primary change: rolling back tracked uses and use counts for removed statements. |
| Description check | |
The description thoroughly explains the problem, fix, scope, trade-offs, and verification results. It does not use the template headings, but it includes the required information and is mostly complet… |
| 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. |
Full details: Description check
-
Explanation
The description thoroughly explains the problem, fix, scope, trade-offs, and verification results. It does not use the template headings, but it includes the required information and is mostly complete.
- 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}
`const r = require; return typeof r;` folds to "function" on the revisit that follows single-use substitution. `ignore_usage` does nothing on a revisit, so after the previous commit the use of `require` that the first visit recorded stayed, and a file with no other CommonJS sign got a `__commonJS` wrapper that it does not get on main. `ignore_usage_of_runtime_require` now takes the `require` use back from the count and from the part's record whenever control flow is live. A file like that is plain ESM again. The same fold inside a removed statement still takes exactly one use away.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
No code change.
There was a problem hiding this comment.
I reviewed the updated PR (including the new commit on the typeof require revisit path) and did not find any bugs; because this rewrites use-count bookkeeping that both the minifier's single-use substitution and the cjs2esm / dynamic-import DCE decisions key off, a human look would still be worthwhile.
What was reviewed:
revert_dead_partagainst the oldclear_symbol_usages_from_dead_part: thepart_use_slotliveness filter matchestake_symbol_uses, forgotten-then-re-recorded refs makerevertidempotent, and the dropped zeroing of declared symbols matches esbuild'sappendPart(which never zeroed).AccountedUses.note/revertgeneration handling across the threepart_generationbump sites, theu32::MAXsentinel path in visit/mod.rs, and all three.totalreaders.ignore_usage_of_runtime_requirenow takes therequire_refuse back on the substitution revisit, which addresses the earlier inline note; theis_control_flow_deadguard pairs correctly withrecord_usage_implskipping dead regions.__requirestill keeps its use on the revisit, same as the base branch.- New tests exercise cli/api backends, minify-syntax,
--drop/--define, andproduction, with exact stdout and kept-export assertions.
Extended reasoning...
The change touches the Rust JS parser's part-level use accounting (src/js_parser/p.rs, fold.rs, visit/mod.rs, parse/parse_entry.rs) and adds 15 bundler test cases; it touches no security-sensitive surface. The second commit addresses the one inline finding from the prior review, and no CODEOWNER covers these files. Deferral rather than approval because the accounting feeds minifier substitution and wrapper/export-narrowing decisions across every bundled file, and the removal of declared-symbol zeroing changes behavior beyond the two reported repros even though it aligns with esbuild.
…ved statement A removed statement that reads a property of a destructured import() or require() local left one use on that local. The roll-back zeroed the locals the statement declared, which hid it. The roll-back now takes the property reads back, and a read of a local declared outside the statement keeps no export either. The mark that single-use substitution leaves on a namespace local goes with its statement. `var module = require()` and `var exports = require()` of an unwrapped package make the wrapper's variable an import namespace. For these two symbols the roll-back still zeroes the use count, as before this branch: a use above the declaration is a use of the namespace.
There was a problem hiding this comment.
I reviewed the current revision and didn't find any bugs; the revisit fold now takes back the require_ref use through ignore_recorded_usage, which addresses my earlier inline note. Because this rewrites the parser's dead-part symbol accounting (use counts, namespace escape tracking, module.exports rewrite counts) and narrows the old zero-all-declared-symbols behavior, a human look from someone who owns the parser is still worthwhile.
What was reviewed:
revert_dead_partagainsttake_symbol_uses/part_use: the generation|index slot check only subtracts live entries, and every ref inpart_useshas a slot, so no out-of-bounds index.- Checked whether a dead part could carry
import_symbol_property_useswithout reaching the revert branch —record_import_property_userequires an existing part use, sopart_usesis non-empty; skippingexports_refis right since those entries are call records never moved out ofpart_uses. AccountedUsesescape/revert interplay:mark_escapedonly fires at total 0 andrevertrestores 0 for the same generation, no-op otherwise;ignore_usage_of_runtime_requirenow leavesrequire_refalone in dead control flow where it was never recorded.- Tests assert exact stdout and kept-export sets across cli/api backends and both
minifySyntaxstates.
Extended reasoning...
The diff touches four parser files (src/js_parser/p.rs, fold.rs, visit/mod.rs, parse/parse_entry.rs) plus two bundler test files, replacing plain counters with a per-part AccountedUses struct and replacing clear_symbol_usages_from_dead_part with revert_dead_part that walks part_uses, tracked namespace notes, import property reads and module.exports rewrites. It touches no security-sensitive surface. The change is behaviorally subtle (it stops zeroing use counts of all symbols a removed statement declared, keeping only module_ref/exports_ref, and gates the require_ref decrement on live control flow), and correctness depends on invariants spread across the minifier's single-use substitution and the CJS-to-ESM classifier, which I could not execute here. No CODEOWNERS entry covers these files and no third-party objection is outstanding, but the complexity and scope rule out an approval.
Problem
bun buildremoves!ns;and then drops every export of theimport()target:const ns = await import("./lib.js"); !ns; console.log(Object.keys(ns))prints[].if (module.exports.a) {}loses its wrapper while it still usesmodule:ReferenceError: module_lib is not defined.clear_symbol_usages_from_dead_part(src/js_parser/p.rs) takes back a removed statement's uses. The two counts thatparse_entry.rscompares with them keep its share (bundler: tree-shake unobserved exports of import() / require() targets #32557, bundler: keep the CommonJS wrapper when a file uses module beyond module.exports #41262).Fix
AccountedUses: the total and the open part's share.revert_dead_parttakes back the uses, that share, the property reads (a.x) and the "escapes" mark.ReferenceError: ns is not defined), except the wrapper'smoduleandexports. A removedtypeof require;builds again.bundler_dynamic_import_dce.test.tsandbundler_cjs2esm.test.tsfail onmain.test/bundler/has no new failure.Background
ns.a,!ns) counts inuse_count_estimateand in a second count. The namespace escapes when the first is larger.Downsides
module.exportsrewrite +12..textgrows 4,608 bytes.Notes
The branch has
cf414ea1e0(the roll-back),95583045fd(therequirefold on the revisit),65d3b8bcc0(comments only), a merge ofmainat91e7e3b140,40864409bd(property reads, the escape mark, themodule/exportsexception, more tests), and a second merge ofmainatea6a2c7b7c. Unless a line says otherwise, a number below compares40864409bdwith the parser sources ofmainat91e7e3b140in the same tree. After the second merge the two test files pass again (123 and 344, debug + ASAN build).Behaviour.
mainand pass here, on debug + ASAN builds and on release builds.bundler_dynamic_import_dce.test.ts: 344 pass.bundler_cjs2esm.test.ts: 123 pass.test/bundler/, release builds of both trees: the only failures here are twobytecode cache portabilitytests, which fail the same way with the parser ofmainin this container.native-plugin.test.tshit a 5 s setup timeout on its first run and passes 20 of 20 when it runs again.--minify-syntax: for 135 of them the bundle differs between65d3b8bcc0(merged withmain) and the last commit. In each, the new bundle is smaller (a property read or an escape mark of a removed statement), or it equals the bundle ofmainagain (avar module/var exportsfile). No program prints something thatmaingets right and the last commit gets wrong. In thevar modulefamily the last commit prints whatmainprints, also in the few programs where65d3b8bcc0was right andmainis not.What the last commit changes, and why.
(function () { const { a } = require("./lib.js"); return a.x; });, kept the exportaat65d3b8bcc0.record_import_property_usemoves such a read out of the part's record and leavesuse_count_estimate.mainhas the same leftover, but it zeroed every symbol the removed statement declared, which hid it for a local declared inside the statement. The roll-back now takes the property reads back. The export also goes when the local is declared outside the removed statement, wheremainkeeps it.--minify-syntax,(function () { const ns = require("./lib.js"); return ns; });as a removed statement kept every export: single-use substitution marksnsas escaped, and nothing took the mark back. The mark now belongs to its part.var module = require("./impl.js")in a package of the unwrap list makes the symbol of the wrapper'smodulean import namespace and removes the declaration. At65d3b8bcc0a use in a function above the declaration then read the wrapper'smodule, wheremainreads the namespace. For these two symbols the roll-back zeroes the count again, asmaindoes. On 63 programs of this family the bundles are byte-identical tomain.var exports = require("react"); typeof exports.createElement, whichmaingets right. It is not in this PR.Left out on purpose.
var module = require()/var exports = require()family is as onmain: wrong for a read that runs before the declaration and formodule.exportsafter it. bundler: keep the CommonJS wrapper when a file gives module or exports its own value #41635 and bundler: do not fold an unwrapped require() declaration when its variable is rebound #45343 are open for it.exports.a = 1; if (module.exports.b) {}with--minify-syntax. Same bytes asmain. It is reported separately (no PR yet).require()orimport()call inside a removed statement still counts as an import of the file. When nothing accounts for its result (var ns = require()), every export stays. Same asmain.module;in a function body, a fold on the second visit) stays counted, as onmain. With a removedmodule.exportsread beside it,mainlifted the file because the two miscounts cancelled. This change keeps the wrapper, asmaindoes without the removed read. The output is right in both.--minifynames can differ frommainwhen an earlier statement uses a symbol that a removed statement declares: the count that orders the names is no longer zero. Same size.import()call made in a removed statement stays. Its import record is in no live part.--minify-syntax,exports.a = 1; exports.get = function () { const m = module; return m.exports.a + 1; };imported asimport { get }throwsReferenceError: $a is not defined, as onmain. On the second visit the use of the__requirehelper is still kept, as onmain:function f() { const r = require; return typeof r; }emits an unused shim. Both were reported separately.Cost, release builds.
valgrind,perfandbpftracecould not be installed where this was built. A count is the number of instructions executed in one call ofappend_part, entry to return, callees included, from gdb single-stepping the stripped binary. Each input repeats one statement 12 times in a bundled file. The table has the value that repeats.append_partcallmainexport const v = 1;, JSexport const v = 1;, TypeScriptconsole.log(ns.b);, one tracked readexports.b = module.exports.a + 1;, one rewritev;, JSv;, TypeScriptv;, file with a tracked namespace local!ns;(function () { return a.x; });, a property readif (module.exports.a) {},--minify-syntaxvar v = 1;gives 1335 against 1338 (JS) and 1321 (TypeScript).v;in a file without one..textof the stripped binary: 81,665,765 to 81,670,373 bytes (+4,608).P(the parser struct): 4,032 to 4,040 bytes.AccountedUses: 12 bytes, so each tracked namespace local uses 8 more bytes in its map.cf414ea1e0against its merge base, and not again: calls to the mimalloc entry points perappend_partcall are 3 malloc and 1 free for a kept statement in both trees, and 5 and 2 against 2 and 2 for a removedv;. The later commits add no call that allocates.Review.
fold.rsmade a CommonJS file that lifts correctly keep its wrapper. Removed, andalias.cjsin the tests now pins it.requirefold, and a file with no other CommonJS sign got a wrapper. Fixed in95583045fd. Five added comments were longer than one line. Shortened in65d3b8bcc0.65d3b8bcc0merged withmain, with a build ofmainbeside it: 22 findings, 19 held. Two were regressions of that head, each found three times, and are fixed in the last commit (the property read, thevar moduleread). One was the same kind of leftover in a map this PR already changes (the escape mark) and is fixed too.pair,keptThenRemoved,aliasandaliasRemovedinbundler_cjs2esm.test.ts, andmixedandpairinbundler_dynamic_import_dce.test.ts. The seventh has no test: the generation bump at the end ofrevert_dead_part, which matters only when the removed statement is the last one of a file.exports.eliminatemoves the wrong statement), and a lifted file that reads the wholemodule.exportsvalue (reported separately, no PR yet).var modulefix (above) and three more lines without a test. The rowscall,live,viaSwitchand the second read inoutsideanswer them.call.cjsandviaSwitchpass onmaintoo. They pin theexportsexception of the property-read loop and the early return ofnote()by reading, not by a build with the line removed.Open PRs in the same code. #38352 calls
clear_symbol_usages_from_dead_parton a part that is already closed.revert_dead_partonly reverts the open part, so #38352 needs its own helper. #32417 also edits the dead-part branch. #45507 removes the second visit: the rowsalias,aliasRemovedand the testSubstitutedRequireReadFoldsOneUsedescribe that visit. #41635 and #45343 change thevar module = require()family: the testVarModuleIsTheUnwrappedRequireNamespacepins three reads thatmaingets right today. #45296, #41158, #41159, #41156, #45016 and #42402 touch the same files.[human-review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file