Repository navigation
Conversation
Since #41186 the bundler binds a local from import() / require() to the export it names. Four shapes bound a local that must stay a local: - A `var` in a nested scope that names a `.then` parameter. hoist_symbols links the `var` symbol to the parameter symbol, so the parameter's own symbol has no link. The parser now sets IS_LINK_TARGET on a symbol that another symbol links to, and parse_entry asks use_count_is_exact(), which covers both ends of a link. - A pattern off a `{...rest}` copy. The copy does not have the names that the pattern beside it took, so the pattern makes no import items. - A pattern off `c ? require(x) : null`. The local can be null, so the pattern must run. is_item now rejects a record that needs the namespace object. - A read that runs before the declaration: in a function declared below it that earlier code already refers to, in a later case of the same switch, or in a default value of a `.then` parameter. That read throws. The parser watches the locals of such a declaration, and one that the early code reads is not bound. A top-level declaration prints as `var` and is exempt.
|
Warning Review limit reached
On-demand reviews are free for the next 8 days. After that, they cost $0.25 per reviewed file. Or wait 8 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
Comment |
|
Status: this PR is ready for review (#42473). How I reproduced it: for each entry in the table under Notes, I ran |
|
Updated 5:32 AM PT - Sep 12th, 2026
✅ @robobun, your commit 48362b59da82e40238646badcb46c7cf22600564 passed in 🧪 To try this PR locally: bunx bun-pr 42473That installs a local version of the PR into your bun-42473 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find bugs. Given it reworks the dynamic-import DCE gating in the parser visit pass and swaps two shared conditions (is_inside_switch → switch_scope, records.len() == 1 → dynamic_import_needs_object), a human familiar with #41186 should also look.
What was reviewed:
is_inside_switchremoval — greppedsrc/, no remaining readers; the one other consumer (break-outside-switch) readsswitch_scope.is_none()at the same set/restore points.records.len() == 1→dynamic_import_needs_objectgate — confirmedregister_dynamic_import_namespace_local_multiinserts every record whenlen() > 1, so the new check subsumes the old and only adds the new conditional/early-read cases.- Hot-path cost —
watched_reads_in_scope()allocates per function-decl/case but iteratesdynamic_import_early_read_watch, which stays empty unless a tracked declarator hitscan_read_local_before_init; files with no dynamic-import tracking pay an empty-vec collect. IS_LINK_TARGET— set at all threelink.set()merge sites (two inhoist_symbols, one indeclare_symbolReplaceWithNew);use_count_is_exact()folds the priorhas_link()/must_not_be_renamed()checks plus the new flag.
Extended reasoning...
Overview
This PR hardens the bundler's dynamic-import DCE (introduced in #41186) so locals bound from import()/require() are not rewritten to direct export references when the read can observe TDZ, a {...rest} copy missing keys, or a conditional/null value. It touches src/ast/symbol.rs (new IS_LINK_TARGET flag + use_count_is_exact()), src/js_parser/p.rs (~130 new lines: early-read watch state + five helpers + three set_is_link_target calls at merge sites), src/js_parser/parse/parse_entry.rs (folds link/rename checks into use_count_is_exact() and swaps the records.len() == 1 gate for dynamic_import_needs_object), src/js_parser/parser.rs (is_inside_switch: bool → switch_scope: Option<StoreRef<Scope>>), the visit pass (visit/mod.rs, visit_expr.rs, visit_stmt.rs), and adds ~350 lines of itElides cases to test/bundler/bundler_dynamic_import_dce.test.ts.
Security risks
None. This is bundler output-correctness work with no auth, crypto, network, or filesystem surface. The failure mode of a bug here is miscompiled bundled output (wrong runtime value or a lost TDZ throw), not a security exposure.
Level of scrutiny
High. The change sits in the parser visit hot path and alters two shared conditions that other code depends on: FnOrArrowDataVisit::is_inside_switch is replaced wholesale, and the is_item gate in parse_entry.rs changes from a structural records.len() == 1 check to a set-membership check. Per REVIEW.md, refactors are guilty until proven behavior-preserving — I verified the is_inside_switch replacement has no orphaned readers and the dynamic_import_needs_object gate strictly subsumes the old one (every multi-record local is inserted at registration). The new per-declarator/per-case work is gated so files without tracked dynamic imports pay effectively nothing. Still, the interaction between watch_early_reads, bindable_reads (which counts import-item use estimates), and keep_locals_read_since across nested scopes and revisits is subtle enough that someone who owns the #41186 machinery should confirm the demotion rules match intent.
Other factors
Test coverage is strong: eight new itElides scenarios exercise nested-var merges across every hosting statement form, {...rest} re-destructure, conditional-null locals, hoisted-function TDZ reads (including through a merged var and an indirect call chain), later-case reads, .then parameter defaults, and a negative case proving unrelated hoisted functions and top-level var-printed declarations keep the #41186 output. The PR description documents a 72-variant × 6-mode matrix run and names the known --minify-syntax gap it does not close (#18477). No prior review activity on the PR. dynamic_import_early_read_watch is never pruned on scope exit, but entries are filtered by current_scope at read time and the vec is bounded by tracked-declarator count, so that's not a concern.
|
The review found no bugs, so the branch does not change. Two notes on its points:
CI: Buildkite build 114717 passed. The I agree that someone who knows #41186 should check the two rules that decide when a local stays a local: |
|
Closing: #45016 replaces this. It rebuilds the link flag and the |
Problem
bun buildbinds a local fromimport()/require()to the export in four shapes that need the local (1.4.0 is correct). Example:const { a, ...rest } = require("./m"); const { a: x } = rest;givesxthe value ofa.varthat names a.thenparameter, a pattern off a{...rest}copy or offc ? require(x) : null, a read that runs before the declaration.is_item(src/js_parser/parse/parse_entry.rs:1318) andnote_destructured_locals(src/js_parser/p.rs:1191) check only the local's own symbol and reads above the declaration.Fix
IS_LINK_TARGET).parse_entrythen checks both ends of a link.{...rest}copy makes no import items.is_itemrejects a record indynamic_import_needs_object(a conditional local).const/letcan run first (a function declared below it that earlier code refers to, a latercase), a read there keeps the local. Top-level declarations print asvarand are exempt.test/bundler/bundler_dynamic_import_dce.test.ts(8 new cases, 28 of 32 variants fail on 1.4.3), 13 more suites. Self-reviewed: 3 changes asked, 3 made.Background
varthat names a parameter is the same variable.hoist_symbolslinks its symbol to the parameter's, which gets no link itself.dynamic_import_needs_objectholds the import records whose local can hold no namespace.Notes
No user reported these. A comparison of bundled output with unbundled runs found them. 1.4.1 and 1.4.2 have them. "1.4.3" below is 1.4.3-canary.1+4ff919377.
Repro.
bun build ./entry.js --target=bun --outdir out && bun out/entry.js, compared withbun run entry.js. It also reproduces with--minify,--format=cjs,--target=nodeand--target=browser.import("./m.js").then(({ a }) => { if (retry) { var a = "fallback"; } console.log(a); })Aundefinedimport("./m.js").then(ns => { for (var ns of [{ a: "last" }]) {} console.log(ns.a); })lastAconst { a, ...rest } = require("./m.js"); const { a: x, b } = rest; console.log(a, x, b);A undefined BA A Bconst ns = c ? require("./a.js") : null; try { const { x } = ns; … } catch …withcfalsethrew TypeErrorgot ax, from a module that did not loadget()aboveconst { a } = require("./m.js"), withfunction get() { return a; }below itReferenceErrorundefinedswitch (k) { case 0: const { a } = require("./m.js"); return a; case 1: return a; }withk = 1ReferenceErrorA, andm.jsnever runsVariant matrix. 72 variants of the four shapes: each statement that can hold a nested
var, theawait import()/require()/.then/Promise.allsources,let, a generator,typeof, nested and reversed ternaries, a rest of a rest, acasetest, a function declared in acase. Each one was built with--target=bun,--minify,--format=cjs,--target=node,--target=browserand--minify-syntax, and compared with the unbundled run. On 1.4.3, 211 of the first 383 builds differ. On this branch none of 423 differ (5 differ only in howconsole.logprints a namespace object).The read-before-declaration rule.
note_destructured_localsalready keeps a local that has a read above its declaration, "since that read must not see the export". Code below the declaration can also run first. There are two ways:can_read_local_before_initchecks that: a member of the current scope that is a function, is declared after the local, and has a use (or an inexact use count, for{ var get; get(); }merged intofunction get).caseof the sameswitch, or its test. All cases share one scope, and theswitchcan jump past the declaration. A declaration insidecase 0: { … }is not affected.When one of the two holds,
watch_early_readsrecords the locals that the declaration binds.s_functionands_switchtakewatched_reads_in_scope()before each function declared in that scope and before eachcase, and callkeep_locals_read_since()after it. A destructured local with a new use leavesdynamic_import_destructured_locals. A namespace local with a newns.aitem read gets its records intodynamic_import_needs_object, so its items printns.a, the same as forc ? require(x) : null. The rule does not follow calls: once some function declared below is referred to early, a read in any function declared below counts.What the rule costs. It changes output only when code that can run early reads the local. These layouts keep the #41186 output (zod 4,
const { z } = await import("zod"), bytes unminified / minified):log("start")above,function logbelow,logdoes not readzconst zod = await import("zod")andzod.zconst { z }in an unbracedcase,default:does not readzprocess.on("exit", report)above,function reportbelow readszThe last row is the cost. There
reportcan run before theawaitfinishes. That read throws in the source, and main reads the export of a module that is still loading. A top-level declaration is exempt, becauseselect_local_kindprints it asvar: 1.4.0 already returnedundefinedfor an early read there, also with a CommonJS importee that never binds. Somodule.exports = fnabove a top-levelrequire()keeps the #41186 output. The scan reads the members of one scope, only for a declarator thatimport()/require()tracking matched.Not covered:
--minify-syntaxin aswitchcase. With--minify-syntax(alsobun run),case 0: const ns = require("./m.js"); return ns.a; case 1: return ns.a;still loses the declaration. Single-use substitution runs when onecasebody ends, before the other cases are visited, and moves the initializer intoreturn.case 1then reads a freens. That is older than #41186 and is tracked in #18477, with #30936 and #40791 open for it.ReadInLaterSwitchCasegives each local two uses in its owncase, so the substitution does not apply, and it checks forbefore initializationin the message.IS_LINK_TARGET. #41158 (open) adds a flag with the same meaning on another bit. This one has the same name and is set at the same three places (hoist_symbolstwice,declare_symbolonReplaceWithNew), so that PR can reuse it. Only the first place changes what this PR's tests see. The other two keep the name true.is_itemandrecords.len() == 1.register_dynamic_import_namespace_local_multiputs every record of a local with more than one record intodynamic_import_needs_object. The new check covers that case, so it replaces the old one.switch_scopereplacesis_inside_switch. Both would be set and restored together, so thebreakcheck now readsswitch_scope.is_none()..thenparameter defaults..then(({ b = a, a }) => …)readsabefore the pattern binds it. The tracking runs before the parameter is visited, soahad no use yet and was bound. Now a default value that is not a primitive literal stops the pattern's names from being items (binding_default_can_read_names).Promise.all([…]).then(([x = ns.a, ns]) => …)is not tracked at all.Self-review. Asked before merge: do not claim the
switchshape closed under--minify-syntaxand make the test say why it throws (done, above). State the cost on correct code (an earlier version of this branch kept every local of a declaration once any function below it was referred to early. That cost zod 140 KB to 757 KB in thelog("start")layout. The watch replaced it, and the table above is the cost now). Name the flag for its invariant and fold the threehas_link()checks (done). Follow-ups done here: the.thendefault value, the mergedvar get. Not done: one shared predicate for every "can this namespace local bind" check, and a checked-in matrix with a Node oracle.Each clause is needed. I removed each of the 23 conditions that the fix adds, one at a time, rebuilt, and ran the new tests. Each removal makes at least one new test fail.
No change on real packages. I bundled rollup, express, fastify, hono, axios, mongodb, pg, lodash, acorn, react-dom, svelte, undici, happy-dom, immutable and the corpus of
bundler_bytecode_portable.test.tswith main and with this branch (46 outputs, 39 MB, minified and not). The outputs are byte-identical.Not changed. A sloppy-mode
{ function a() {} }that names a parameter is merged into the parameter, as in esbuild. The flag makes such a parameter stay a local too.Suites run with the debug build:
bundler_dynamic_import_dce(306 pass),bundler_promiseall_deadcode,bundler_minify,bundler_cjs2esm,bundler_edgecase,bundler_barrel,bundler_splitting,esbuild/dce,esbuild/importstar,esbuild/splitting,esbuild/default,transpiler/transpiler,transpiler/react-compiler,regression/issue/cyclic-imports-async-bundler.bundler_bytecode_portabletimes out on my machine in a debug build (itsbun build --bytecodesteps take longer than the test allows). Its bundler inputs are part of the byte-identical comparison above.[human-review] gate passed · iteration 0 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file