fix(bundler): fix part liveness calculation by paperclover · Pull Request #12758 · oven-sh/bun

paperclover · 2024-07-24T02:25:09Z

What does this PR do?

This fixes a very subtle mistake when determining which parts are live. See #12571 (comment), After fixing this first bug, there were three or four other fixes which are all very subtle but now Bun is now correct when it comes to this dependency calculation instead of just getting lucky.

Not marking #12571 as fixed because want to keepthe issue open as it shows sourcemaps are broken, which this PR does not address.

Fixes #9037 (different repro, same underlying bug)

Note that for the provided reproduction in 12571, it now errors with when sending the callback.html

@davidgomes
For the file not found, I think that there is a bug with bun there, but this pattern works:

createReadStream(require.resolve("./callback.html")).pipe(response)

github-actions · 2024-07-24T03:19:51Z

❌ @paperdave, your commit has failing tests :(

💪 1 failing tests Darwin AARCH64

test/js/web/workers/worker.test.ts 1 failing

🪟💻 5 failing tests Windows x64 baseline

test/cli/install/registry/bun-install-registry.test.ts 1 failing
test/integration/next-pages/test/dev-server.test.ts 1 failing
test/js/node/child_process/child_process.test.ts 1 failing
test/js/node/worker_threads/worker_threads.test.ts STATUS_SEVERITY_ERROR
test/regression/issue/012360.test.ts 1 failing

🪟💻 3 failing tests Windows x64

test/cli/install/registry/bun-install-registry.test.ts 2 failing
test/js/node/child_process/child_process.test.ts 1 failing
test/regression/issue/012360.test.ts 1 failing

View logs

#32557) ### What does this PR do? Tree-shakes the exports of a module that is loaded with a string-literal `import()` or `require()` down to the names the program can actually observe. The parser records, for every `import("x")` / `require("x")` call, how its result is consumed. When every use of a call's result is one of the recognized shapes below, the set of property names read off it is attached to the import record. The linker unions those sets per importee (together with what static importers need) and, when the union is not "everything", narrows the importee's exported-names list. Whatever falls out of that list is no longer referenced by the namespace object / chunk export clause, so normal tree-shaking removes it along with anything only it depended on. Nothing about *how* the module is loaded changes. The `import()` still evaluates lazily behind its `__esm` wrapper (no `--splitting`) or as its own chunk (`--splitting`); `require()` is still synchronous. The parser does not rewrite or hoist anything, so evaluation order, top-level await, `try`/`catch` around the import, thenable importees and external specifiers behave exactly as written. This is deliberately *not* Rollup's `inlineDynamicImports`. #### Recognized shapes (narrowed) ```js const { a, b: c, d = 1, ...rest } = await import("./x") // a, b, d (+ whatever is read off rest.*) (await import("./x")).a // a const ns = await import("./x"); ns.a; ns["b"]; const { c } = ns // a, b, c export const { a } = await import("./x") // a (kept even though never read locally) import("./x").then(({ a }) => …) // a import("./x").then(ns => ns.a) // a import("./x").then(() => …) // nothing const [{ a }, ns] = await Promise.all([import("./x"), import("./y"), other]) // per element Promise.all([import("./x"), …]).then(([{ a }]) => …) import("./x"); await import("./x"); await import("./x").catch(…) // nothing (side effects only) const { a } = require("./x"); require("./x").a; const ns = require("./x"); ns.a require("./x"); // nothing ``` `let`/`var` destructuring, nested patterns (`{ a: { b } }` keeps `a`), string-literal and duplicate keys and default values are handled. A destructured local that is never read does not keep its export (unless a direct `eval` in scope or a hoisting merge could read it). A namespace local that the minifier inlines into its single use is treated as escaping. Always kept in addition to the recorded names: `then` for `import()` targets (the `await` itself calls it if present) and `module.exports` for `require()` targets (that is what `require()` of an ES module returns when present). #### Not narrowed (importee keeps every export) - The namespace escapes: passed or returned anywhere (`() => import("./x")`, `return await import("./x")`, `f(ns)`, `export default await import("./x")`, `export { ns }`), stored, spread, iterated, called, `Object.keys(ns)`, optional chaining `ns?.a`, computed access `ns[k]` (including constant ones), assignment through it, a spread inside `Promise.all([...])`. - `.then(function (ns) { … })` (a non-arrow can reach the namespace through `arguments`), `.then((...ns) => …)`. - A `var`-declared namespace local, a namespace or `require()` local referenced before its declaration, locals merged by hoisting, an exported `...rest`, direct `eval` in scope. - CommonJS importees (their `default`/named interop is synthesized from the export list itself), and any importee some importer reaches with `import * as ns` (used as a value) or `export * from`. Only the first level of an `export * as ns` barrel is narrowed. - User-specified entry points always export everything. One accepted divergence from unbundled semantics (shared with rolldown, and with esbuild's handling of static `import * as ns; ns.f()`): a function called *through* the namespace — `(await import("./x")).f()`, `ns.f()`, `require("./x").f()` — receives the narrowed namespace object as `this`, so an export reached only via `this.other` inside `f` is not seen. #### Interaction with `"sideEffects": false` A bare `import("./x");` / `await import("./x");` observes none of `x`'s exports. `x` itself is still evaluated (its own top-level side effects run once, matching webpack/rspack's `side-effect-free-dynamic-import` cases); what changes is that a `"sideEffects": false` module `x` merely *re-exports from* is no longer pulled in when none of those re-exports are observed. Previously the dynamic target kept every export and therefore dragged those in; three edge-case tests from #12758 change expectation accordingly. #### `require()` with `--splitting --target=bun` #40519 made `require()` of an ES module a chunk boundary (`import.meta.require("./chunk.js")`). The same narrowing applies to those chunks, so `require("./x").Foo` / `const { Foo } = require("./x")` only keep `Foo` (and `module.exports`) in the split-out chunk. Without `--splitting`, the wrapped ES module's `exports_x` object is narrowed the same way. #### Also - With `--splitting`, an `import("./data.json", { with: { type: "json" } })` whose target became a chunk no longer keeps the import attributes on the rewritten `import("./chunk.js")` (the runtime parsed the chunk as JSON); external targets keep them. - Fixes tsconfig `paths` substitution when the `*` is not on a path-segment boundary (e.g. `"~*": ["./src/*"]` with `~utils/x`), normalizing absolute (`${configDir}`) templates after substitution. ### Trade-offs - This is strictly an export-list narrowing; it never changes when a module runs. The cost of keeping laziness without `--splitting` is that the `__esm` wrapper and `exports_x` object stay (a hoisting design would remove them but changes evaluation order, breaks `try`/`catch` around the import, turns destructured snapshots into live bindings, and cannot be undone for specifiers that resolve external — all of which the test suite now pins). - Tracking is per call site and all-or-nothing per site: one escaping use of a namespace keeps everything for that importee. Thunks like `load: () => import("./cmd")` are the common untracked shape in real code; writing `() => import("./cmd").then(m => m.run)` makes them narrowable. - esbuild does not do this (evanw/esbuild#3987, #4255). rolldown, rspack and webpack do; their fixtures are ported here. Intentional differences: `.then(function (ns) {})` is not tracked (the `arguments` escape); a namespace local referenced from a function hoisted above its declaration keeps everything (rolldown over-shakes that case); `webpackExports` / `webpackMode` magic comments are ignored (usage alone decides); CommonJS importees and constant computed keys are not narrowed (webpack does both); context-module specifiers (`import(\`./dir/${x}\`)`) stay runtime imports. - Build time: flat vs main on three.js×10, a 3000-file × 30-named-import barrel, and 3000 narrowed `import()`s (±3% instructions); a pathological single namespace with 4,000 body destructures was O(n²) in an earlier revision and is now linear. ### Numbers Measured on a large internal application bundle (`--compile --splitting --bytecode --minify`, ~30 MB of minified JS across ~1,600 lazy chunks, ~765 `import()`/`require()` targets), same application commit, bun built from the same base commit with and without this branch: | | main | this branch | | ----------------------- | ----------- | ----------- | | executable | 215.6 MB | 211.2 MB (−4.4 MB, −2.0%) | | minified JS in payload | 30.1 MB | 29.1 MB | | lazy targets narrowed | 0 / 765 | 514 / 765 | Of the 251 targets still keeping every export in that build: ~157 are `() => import(x)` thunks whose namespace is consumed elsewhere, ~84 are `cond ? require(x) : null` style shapes, 34 are CommonJS. The remaining size after narrowing is mostly code reachable from the exports that *are* used; splitting had already isolated each target's module graph. ### How did you verify your code works? `test/bundler/bundler_dynamic_import_dce.test.ts` (152 cases: the shapes above in both splitting and non-splitting mode, the escape/bail cases, evaluation-order/TLA/try-catch/thenable/live-binding preservation, `Promise.all`, split and non-split `require()`, `sideEffects:false`, minifier single-use inlining, hoisting merges, `eval`, ports of rolldown `tree_shaking/dynamic_import_*` / `issue_4646/4682/5340` / `dynamic_import_body_destructure{,_bailout}` / `chunk_merging/dynamic_import_host_exporting_then`, rspack `statical-dynamic-import*`, and webpack `cases/chunks/statical-dynamic-import*` / `cjs-tree-shaking/*` / `configCases/cjs-tree-shaking/side-effect-free-dynamic-import*`), plus the existing `bundler_splitting`, `bundler_edgecase`, `bundler_barrel`, `bundler_cjs`, `bundler_minify`, `esbuild/{default,splitting,dce,tsconfig,importstar}` suites and `bundler_bytecode_portable`. The application above was built and exercised end-to-end with the resulting binary. Docs: `docs/bundler/index.mdx` (splitting → "Tree-shaking `import()` and `require()` results") and the esbuild migration table. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

paperclover added 3 commits July 23, 2024 01:28

tree shaking fix

30a4f28

yeah

f1b1b71

one more

5e763b9