bundler: list tree-shaken files in the metafile inputs so import edges resolve - #38624
bundler: list tree-shaken files in the metafile inputs so import edges resolve#38624robobun wants to merge 2 commits into
Conversation
…iles left in a chunk The inputs section was seeded from chunk membership, so a file that tree shaking removed entirely (a sideEffects: false module with no used exports, or a module only reached from dead code) was missing from inputs while its importer's imports[] still pointed at it. Seed it from the linker's reachable files instead, which is also what esbuild lists; outputs[].inputs stays chunk-based.
|
Status: reproduced on bun 1.4.0 and on a debug build of main with the two repros in the description (sideEffects: false package with an unused import, and a module only reached from a dead require()). Fix and tests are in this PR; the three new tests in test/bundler/metafile.test.ts fail on the released build and pass with the fix. CI (build 96353): every lane that ran passed (177 jobs). The only red is |
|
Warning Review limit reached
Next review available in: 18 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 (2)
Comment |
There was a problem hiding this comment.
LGTM — focused fix that seeds metafile inputs from reachable_files instead of chunk membership, matching esbuild.
What was reviewed:
- Verified the superset claim:
files_with_parts_in_chunkis filled fromreachable_filesfiltered byfiles_live(computeChunks.rs:291-292), so nothing previously listed disappears. - Checked the dropped bounds guard is safe:
find_reachable_filesmarks indices via a bitset sized toinput_files.len(), so every reachable index is <sources.len(). - New tests assert exact
inputsshape for both tree-shaking paths and confirmoutputs[].inputsstays chunk-based; Windows path-separator concern ruled out (pretty paths use forward slashes here, same as neighboring--metafile-mdtests).
Extended reasoning...
Overview
The PR changes one loop in src/bundler/linker_context/MetafileBuilder.rs: the seen_sources bitset that decides which files appear as top-level inputs keys in the metafile is now seeded from c.graph.reachable_files instead of the union of chunk.files_with_parts_in_chunk across all chunks. Three new tests in test/bundler/metafile.test.ts cover a sideEffects: false package dropped by tree shaking, a module reached only from dead code (and its transitive import), and the --metafile-md rendering of the same. The import statement for bunEnv/bunExe is hoisted to the existing top-level harness import.
Security risks
None. The metafile is diagnostic JSON describing what the bundler parsed; no auth, no untrusted input parsing, no filesystem writes beyond what bun build already does.
Level of scrutiny
Low-to-medium. The Rust change is a net -3 lines swapping one iteration source for another in report-generation code — not on any hot path or affecting bundle output. I traced the two invariants that make the swap safe: (1) reachable_files is a superset of what chunks contain, since computeChunks.rs:291-292 fills chunk membership by iterating reachable_files and filtering on files_live, so no previously-listed input can disappear; (2) every index in reachable_files is < sources.len() because find_reachable_files (bundle_v2.rs:1952) uses a visited bitset sized to input_files.len(), so the removed < sources.len() guard was redundant and seen_sources.set() cannot go out of bounds. The runtime index is still filtered by the existing Index::RUNTIME check in the emission loop.
Other factors
The tests follow the file's conventions exactly: tempDir + using, bunEnv/bunExe, concurrent stdout/stderr/exit drain, test.concurrent for the subprocess-spawning cases, and toEqual on the full inputs object (strong assertion). They also assert the negative — outputs[].inputs still lists only entry.js — which pins down that the fix is scoped to the top-level inputs. The PR description confirms USE_SYSTEM_BUN=1 fails and bun bd test passes on all three, plus the rest of the file and esbuild/metafile.test.ts. The disclosed side effect (CSS entry-point stylesheets now appear in inputs) is a strict improvement in the same direction and is called out with the related PR numbers. No prior human review comments to address.
Problem
--metafile, a file that tree shaking removes entirely is missing frominputs, while theimports[]of the file that imports it still points at it: the edge has no target and is not markedexternal. Consumers that walkinputsedges (esbuild's analyzer,--metafile-md) hit a dangling edge. Reproduces on bun 1.4.0 and main.import { x } from "pkg"wherepkghas"sideEffects": falseandxis unused:inputshas onlyentry.js, whose imports listnode_modules/pkg/index.js.function unused() { return require("./lazy.js") }(nosideEffectsfield anywhere):lazy.jsand theeffect.jsit imports are both missing,entry.jsstill listslazy.js.inputs(with their ownimports) and leaves them out ofoutputs[].inputs, so the edges resolve.bun builditself already counts them ("Bundled 2 modules" for the first repro).generatein src/bundler/linker_context/MetafileBuilder.rs seeds the set of inputs to write fromchunk.files_with_parts_in_chunkof every chunk (lines 223-230 on main). That map is filled in src/bundler/linker_context/computeChunks.rs:291-292 from the reachable files that are also infiles_live, so it only holds files that survived tree shaking. The per-inputimportsare written from the importer's import records (MetafileBuilder.rs:299-312), whosesource_indexstill points at the removed file.Fix
generateseeds the inputs fromc.graph.reachable_filesinstead of chunk membership. Nothing else changes: the per-input fields, the index-order emission, andoutputs[].inputs(still derived fromfiles_with_parts_in_chunk) are as before.reachable_filesis the closure of the entry points over import records with a validsource_index, computed before tree shaking (find_reachable_files, src/bundler/bundle_v2.rs). The import records written intoimports[]are exactly the edges that traversal followed, so every bundled import edge now lands on aninputskey. Conversely every file that was in a chunk is reachable (the chunk maps are filled fromreachable_files, see above), so nothing that was listed before disappears. It is the set esbuild writes as well (checked against esbuild 0.25.12 on both repros) and the one the "Bundled N modules" summary counts (reachable_files_countin bundle_v2.rs).outputs[].inputsis deliberately left chunk-based: a removed file contributed nothing to any output, and esbuild omits it there too (covered by the tests).metafile lists a sideEffects: false module that tree shaking dropped from the output,metafile lists modules only reached from tree-shaken code, and what they import(also checks the removed module's own import edge and that outputs are unchanged), andmarkdown lists a module that tree shaking dropped from the output(--metafile-mdmodule list and reverse dependency). All three fail on the released build (USE_SYSTEM_BUN=1, the only diff being the missing entries) and pass withbun bd test; the rest of the file (47 tests), test/bundler/esbuild/metafile.test.ts andcss/MetafileCSSBundleTwoToOnepass with the debug build.cargo clippy -p bun_bundler --no-depsandcargo fmt --checkare clean.inputsas well; theoutputshalf of that is bundler: list CSS entry points in the metafile and report copied asset sizes #38341 (its source merges cleanly with this; the test file conflicts only on adjacent import lines). bundler: keep assets referenced by stylesheets and HTML documents out of JS chunks #38337 adds document assets to the same seeding loop; those assets are reachable, so this change covers its metafile part and that hunk can be dropped when the two meet. bundler: report a split dynamic import as its input file in the metafile #38465 builds a map inside the loop this removes and needs a small rebase. bundler: make --metafile output deterministic across build directories #36974 (input ordering) merges cleanly.Background
inputsmaps each source file to its size and the imports found in it (edges between source files, orexternal: true);outputsmaps each emitted file to the inputs it was built from (bytesInOutput) and the chunks it imports. Bun writesoutputsper chunk during chunk generation and assemblesinputsafterwards inmetafile_builder::generate.graph.reachable_files). This is the set of files that were parsed into the build."sideEffects": false, or one only referenced from code that is itself dead, is never marked live.compute_chunksplaces only live files into chunks, and a chunk'sfiles_with_parts_in_chunkis whatoutputs[].inputsand, until this change,inputswere generated from.Repro
Before (bun 1.4.0 and main),
inputs:{ "entry.js": { "bytes": 44, "imports": [{ "path": "node_modules/pkg/index.js", "kind": "import-statement", "original": "pkg" }], "format": "esm" } }After (same as esbuild's
inputsfor this project):{ "entry.js": { "bytes": 44, "imports": [{ "path": "node_modules/pkg/index.js", "kind": "import-statement", "original": "pkg" }], "format": "esm" }, "node_modules/pkg/index.js": { "bytes": 20, "imports": [], "format": "esm" } }outputs["./entry.js"].inputsis{ "entry.js": { "bytesInOutput": 19 } }before and after.Second shape, without any
sideEffectsfield:Before,
inputskeys:["entry.js"], withentry.jsimportinglazy.js. After:["entry.js", "lazy.js", "effect.js"], withlazy.jsimportingeffect.js. esbuild lists the same three.--metafile-mdfor the first repro, before:| Input modules | 1 |and an[IMPORT: entry.js -> node_modules/pkg/index.js]line with no matching[MODULE: ...]or[IMPORTED_BY: ...]. After:| Input modules | 2 |,[MODULE: node_modules/pkg/index.js],[IMPORTED_BY: node_modules/pkg/index.js <- entry.js].Also checked with the fixed build: an HTML entry point (scripts, stylesheet,
<img>,url()asset), a JS entry importing CSS, asideEffects: falsebarrel with an unused re-export, and the first repro under--splittingall produce the sameinputsas before plus, where applicable, the removed modules; a CSS entry point now lists its stylesheets (see the last Fix bullet).