Repository navigation
Conversation
When trimUnusedImports is enabled and every binding of an import statement has been removed (because its uses were dead-code-eliminated or the export using it was replaced via exports.replace), drop the whole import statement instead of leaving a bare side-effect import. Previously this only worked for js/jsx; ts/tsx kept the statement because the 'all bindings removed' branch in ImportScanner was gated on !is_typescript_enabled. TypeScript's separate ts_use_counts tracks value-position references including dead code, so an import used only inside a replaced export had ts_use_counts > 0 (not 'unused in TypeScript') but no surviving bindings. Also widen the exports.replace type to accept the [name, value] tuple form and non-string literals, matching what the runtime already accepts. Fixes #12892
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
ImportScanner runs for bundler parse tasks too; marking a TypeScript value import IS_UNUSED there would stop it being resolved and drop the module from the bundle even when it has side effects. Keep the new arm scoped to the transform-only path and let the linker decide side-effect inclusion when bundling. Adds an itBundled regression test.
There was a problem hiding this comment.
Both earlier concerns are addressed — gating on has_replace_exports (which only Bun.Transpiler populates via exports.replace/eliminate) means neither Bun.build nor the runtime module loader can reach the new arm, and the vacuous toContain("shared") assertion is now a real binding-clause match. Deferring to a human for the final call on using replace_exports.count() > 0 as the opt-in signal for TS import culling and on the widened exports.replace type in bun.d.ts.
What was reviewed: verified replace_exports is Default::default() at every non-JSTranspiler construction site (jsc_hooks.rs, RuntimeTranspilerStore.rs, transpiler.rs); confirmed exports.eliminate also populates the map so the eliminate test isn't accidentally passing; checked the itBundled test still pins bundler side-effect preservation; confirmed the JS-loader arm is unchanged (!is_typescript_enabled still short-circuits).
Extended reasoning...
Overview
The PR fixes #12892: Bun.Transpiler with a TS loader was leaving a bare side-effect import after exports.replace/eliminate removed the only code that referenced its bindings. Four files touched: the import-culling condition in src/js_parser/scan/scan_imports.rs, a widened type for TranspilerOptions.exports.replace in packages/bun-types/bun.d.ts, a new describe.each block over four loaders in test/bundler/transpiler/transpiler.test.js, and an itBundled regression guard in test/bundler/bundler_edgecase.test.ts.
Evolution across review rounds
- Round 1: original diff dropped the
!is_typescript_enabledguard entirely, which would have changedBun.buildoutput (TS imports referenced only in dead code would be dropped from the bundle). - Round 2 (f0b6b41): gated on
!p.options.bundle, which protectedBun.buildbut still reached the runtime module loader (bun runon a .ts file). - Round 3 (de531c6, current): gated on
p.options.features.replace_exports.count() > 0. I grepped every construction site —jsc_hooks.rs:2559,RuntimeTranspilerStore.rs:812, andtranspiler.rs:2951all passDefault::default(); onlyJSTranspiler.rspopulates it from user-suppliedexports.replace/exports.eliminate. So the new arm is now unreachable from both the bundler and the runtime loader, and the JS-loader path is byte-identical to before. The paragraph-long justification comment was also collapsed to two lines.
Security risks
None identified. This is transpiler output shaping; no untrusted-input parsing, no auth/crypto/permissions.
Level of scrutiny
Medium-high. ImportScanner::scan runs for every parsed file in every mode, and dropping an import changes runtime module-evaluation order/side-effects. The final gate is deliberately narrow (only fires when the user has explicitly configured exports.replace/eliminate on a Bun.Transpiler), which is exactly the #12892 use case, but the choice of replace_exports as the opt-in heuristic — rather than, say, a dedicated flag — is a design call a maintainer should sign off on. The bun.d.ts change also widens a user-facing type to accept literals and [name, value] tuples; the runtime already accepts these, but it's public API surface.
Other factors
- Test coverage is solid: 6 tests × 4 loaders covering replace/eliminate/define, named/default/namespace, and the two negative cases (live uses kept, originally-bare imports kept). The bundler regression guard asserts side effects survive under
define-driven DCE. - The remaining comment-cop bot flag on line 273 is on the two-line
// replace_exports is Bun.Transpiler-onlycomment; it reads as durable why-context, not a workaround justification, so I'm treating it as bot noise rather than a blocker. - CI build #80800 is still building per the robobun comment.
Gating on !bundle still admitted the runtime module loader (bun run), which sets bundle=false and trim_unused_imports=loader.is_typescript(). Use replace_exports.count() > 0 instead: only Bun.Transpiler populates that map (via exports.replace/eliminate), so Bun.build and bun run keep their existing behavior of loading the module for side effects. Adds a spawned runtime-transpiler test and a define-only Bun.Transpiler case pinning that TS keeps the bare import when exports.replace is not configured.
There was a problem hiding this comment.
The final revision addresses both earlier rounds — the TS culling arm is now gated on replace_exports.count() > 0, which is only ever non-empty via Bun.Transpiler's exports.replace/eliminate (verified: RuntimeTranspilerStore, jsc_hooks, bundler ParseTask, and transpiler.rs:2951 all pass Default::default()), and pinning tests now cover Bun.build and bun run. Deferring for a human to sign off on the one design coupling: once any exports.replace entry is present, TS drops all orphaned value imports, including those orphaned by unrelated define-driven dead code (pinned by the "drops imports only used in dead code when exports.replace is configured" test).
What was reviewed
- Traced every writer of
opts.features.replace_exports— onlyJSTranspiler.rspopulates it, so the new arm cannot fire in the bundler or runtime loader. - Confirmed the bundler (
bundler_edgecase.test.ts) and runtime (runtime-transpiler.test.ts) tests pin the previously-flagged side-effect-preservation behavior. - Checked the
bun.d.tswidening againstJSTranspiler.rs— tuple form and non-string literals are already accepted at runtime. - The vacuous
toContain("shared")assertion from the earlier round is nowtoMatch(/\{\s*shared\s*\}\s*from/).
Extended reasoning...
Overview
This PR changes the import-culling condition in src/js_parser/scan/scan_imports.rs so that TypeScript sources in Bun.Transpiler drop an import statement entirely (rather than emitting a bare import "./x") when every binding was DCE'd, gated on the presence of exports.replace/exports.eliminate. It also widens the exports.replace type in bun.d.ts and adds tests across transpiler.test.js (28 new cases via describe.each over 4 loaders), bundler_edgecase.test.ts, and runtime-transpiler.test.ts.
Review history
This is the third review round. Round 1 flagged that removing the !is_typescript_enabled gate leaked into Bun.build; round 2 flagged that the !bundle fix still leaked into bun run. The final commit (ceba7e2) replaced the flag-stacking approach with a single positive gate: has_replace_exports = p.options.features.replace_exports.count() > 0. I grepped every site that constructs parser Features / ParseOptions and confirmed only src/runtime/api/JSTranspiler.rs (the Bun.Transpiler API) ever populates this map — the runtime loader (RuntimeTranspilerStore.rs:812, jsc_hooks.rs:2559), the single-file transpiler (transpiler.rs:2951), the lazy stub (parse_entry.rs:192), and the bundler's ParseTask all leave it empty. Both regression tests added in earlier rounds are retained and now pass by construction rather than by flag.
Security risks
None. This is transpiler output shape only; no untrusted-input parsing, allocation, or FFI changes.
Level of scrutiny
Moderate-to-high. scan_imports.rs runs for every parsed file across the bundler, the runtime loader, and Bun.Transpiler, and the two prior review rounds demonstrated how easy it is for a change here to leak across consumers. The final gate is narrow and well-localized, but the coupling it introduces — "any exports.replace entry opts TS into JS-style whole-import culling for all orphaned imports, not just those orphaned by the replace" — is a product decision. It matches JS behavior and is explicitly tested, but a maintainer should confirm that's the intended contract for the exports.replace option before this ships.
Other factors
Test coverage is thorough: 4 loaders × 7 scenarios in transpiler.test.js including negative cases (live imports kept, originally-bare imports kept, TS-without-replace keeps bare import), plus bundler and runtime side-effect preservation tests. The comment-cop bot's complaints about verbose justification comments were resolved by extracting all_bindings_culled and has_replace_exports as named locals.
|
CI status: the Rust change compiles cleanly ("Finished release profile" in both timed-out jobs); the |
|
Heads up: #38352 touches the same condition in |
|
Superseded by #38352, which now contains this fix as a subset: the same |
Fixes #12892.
Problem
Bun.Transpilerwith ats/tsxloader keeps a bare side-effect import afterexports.replace/exports.eliminateremoves the only code that used its bindings, even withtrimUnusedImports: true. The same input with ajs/jsxloader drops the import entirely.Before:
After:
Cause
ImportScanner::scanhas two ways to mark an import record as unused:ts_use_counts == 0(only ever used in type positions).trim_unused_importsis on and every binding was culled (use_count_estimate == 0), so the statement has no star, no default, and no items left.Branch (2) was gated on
!is_typescript_enabled.ts_use_countsis incremented for value-position references and is never rolled back byignore_usage, so an import used only inside a replaced/eliminated export hasts_use_counts > 0(fails branch 1) but no surviving bindings. It fell through and was printed as a bareimport "./x";.Fix
Allow branch (2) to apply to TypeScript when
replace_exportsis non-empty. OnlyBun.Transpilerpopulates that map (viaexports.replace/exports.eliminate), soBun.buildandbun runare unaffected: they continue to resolve the module and keep its side effects, matching their current behaviour. Configuringexports.replacealready implies the user wants the replaced export and anything it alone pulled in removed from the output.Also widens the
exports.replacetype inbun.d.tsto accept non-string literals and the[name, value]rename tuple, which the runtime already accepts (raised in the same issue).Testing
describe.each(['jsx','js','tsx','ts'])block intest/bundler/transpiler/transpiler.test.jscovers named/default/namespace imports againstexports.replace,exports.eliminate, anddefine-driven dead code, plus negative cases for live imports, originally-bare side-effect imports, and define-only DCE withoutexportsconfigured (TS keeps the bare import there). Thets/tsxexports.replacecases fail on main and pass with this change;js/jsxpass on both.edgecase/TSImportUsedOnlyInDeadCodeKeepsSideEffectintest/bundler/bundler_edgecase.test.tspinsBun.buildstill bundling the side-effect module.bun runcase intest/bundler/transpiler/runtime-transpiler.test.tspins the runtime loader still evaluating the side-effect module.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts