Repository navigation
Conversation
|
Updated 1:38 PM PT - Oct 27th, 2026
✅ @robobun, your commit 755cd73c2b5a18f53583746580fe86c8c1431fe8 passed in 🧪 To try this PR locally: bunx bun-pr 46903That installs a local version of the PR into your bun-46903 --bun |
|
Status: ready for review. Merge #41220 first. Without its source change, a barrel that this PR keeps throws at load if its How I reproduced #18008: the commands under "Repro" in the description, on main 179664e and on 1.4.3. Tests: 21 of the 29 new tests in CI: build 133868 on 755cd73 has no failed job. At 20:07 UTC, 181 of 183 jobs passed and 2 macOS x64 test jobs wait for an agent. The failure annotations name 5 tests that failed once and passed when they ran again. This PR touches none of them: |
|
#46911 (the resolver change for #8993) depends on this PR. It makes a package.json with no 10 layouts (5 shapes of the import, each below a package.json with and without a
#46911 asks for this PR to merge first. It has two |
…arrel's exports object An import that reaches a CommonJS or external module through `export *` prints as a read of the barrel's exports object (`exports_barrel.x`). The import tracker recorded no dependency for that read. In a package with `"sideEffects": false`, tree shaking dropped the barrel and the bundle threw `ReferenceError: exports_barrel is not defined` at load. The `DynamicFallback` arm of the import tracker now adds the part that declares the exports object to `re_exports`. A direct import keeps that list in `imports_to_bind`, so the existing readers of `re_exports` give the dependency to each part that prints the read.
…or namespace getters The dependency of such a read on the part that declares the exports object keeps the barrel in the bundle. `LoadTimeReads` took the same dependency as a read of every binding that the object's getters name. With `--splitting`, a chunk with an import cycle through the barrel then lost its safe order and printed `NaN`. Only a part that uses the object itself counts as a reader of those bindings. Also state what the step-6 arm does, document the shape of `imports_to_bind` entries, and add tests: the cycle shapes, a read in a chunk that `import()` loads, a read that does not run at load, and a JSX runtime behind `export *` of a CommonJS file.
…wser build disables
50174bd to
8b916d0
Compare
…e line The test of the reader of a lazily loaded module now names the step-6 check that keeps its barrel.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 12 minutes. View limit details
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 |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new .expect("unreachable") on the imports_to_bind.put — it mirrors the existing NormalAndNamespace put a few lines below, so it is not a new failure mode — and the LoadTimeReads dependents refactor in findAllImportedPartsInJSOrder.rs, which emits the same edges as before for every dependency except cross-file part-0 dependencies whose part never uses that file's exports object, which is the intended exclusion.
Extended reasoning...
The change touches the bundler linker's import matching (DynamicFallback arm and the Namespace bind in LinkerContext.rs), the LoadTimeReads dependents graph build, a comment in scanImportsAndExports.rs, a README bullet, and ~620 lines of new itBundled tests; it touches no security-sensitive surface. Two confirmed findings are posted inline (the ESM load-time regression for external/disabled export-star targets, which the author's own description acknowledges as a merge-order dependency on another PR, and a split-chunk cycle ordering case), so a human needs to weigh them; this note only records the two adjacent concerns examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/linker_context/findAllImportedPartsInJSOrder.rs— Users of split ESM bundles with a cycle through a"sideEffects": falsebarrel can getundefinedfrom a name that resolved on main's dropped-barrel path only by throwing.LoadEffect::ofat src/bundler/linker_context/findAllImportedPartsInJSOrder.rs:600 keepsSExportStarasHoisted, yet a barrel'sexport * from <CJS>prints__reExport(exports_barrel, ...), which writes the run-time names at load. Fix: classify an export-star part whose target is dynamic (is_dynamic()at scanImportsAndExports.rs:1354) asEvaluates/Assignssois_broken_bytreats it as a written part that readers must follow.Why this was flagged
Trigger: the newly kept barrel (this PR makes
exports_barrel.xreaders depend on part 0 at src/bundler/LinkerContext.rs:4087, so barrels in"sideEffects": falsepackages now stay live) sits in an import cycle inside a chunk shared by two entry points.find_safe_walks(:345) only reorders whenis_broken_byfinds anEvaluatesdependent positioned before a non-Hoistedwritten part. The barrel's__reExportstatement is the only thing that givesexports_barrel.xa value, butfor_each_part_backward(:525) skips it because :600 mapsSExportStartoHoisted, so it is never awrittencandidate. No order is ever judged broken for this read. The user getsundefinedforxwhere main's bundle threwReferenceError: exports_barrel is not definedon load. On base the population that reaches it (side-effect-free barrels) never had a live barrel; this PR creates the live-barrel-plus-reader shape and then exempts it from the only ordering safeguard.Verification:
LoadEffect::ofclassifies the export-star partHoisted(src/bundler/linker_context/findAllImportedPartsInJSOrder.rs:600), andfor_each_part_backward(:525) only visits non-Hoistedparts aswritten, sois_broken_by(:545-567) never starts from the__reExportwrite. On base a"sideEffects": falsebarrel throwsReferenceErrorunconditionally; a live barrel yields the sameundefinedby the same route.
| // Depend on the part that declares the exports object that this read prints | ||
| let deps = self.top_level_symbols_to_parts( | ||
| next_tracker.source_index.get(), | ||
| next_tracker.import_ref, | ||
| ); | ||
| re_exports.reserve(deps.len()); | ||
| for &dep in deps { | ||
| re_exports.push(Dependency { | ||
| part_index: dep, | ||
| source_index: bun_ast::Index::init(next_tracker.source_index.get()), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔴 Users of a "sideEffects": false barrel that does export * of an external or browser-disabled module get an ES-module bundle that throws at load, where main loads it. The new re_exports dependency keeps the barrel live for any read of a run-time name, even one inside a function nothing calls. The kept barrel prints __reExport(exports_barrel, ns) but scanImportsAndExports.rs:1347-1387 never sets CONTAINS_IMPORT_STAR for an external record, so ns is never declared. Fix: before this lands, make a kept export * of a not-in-bundle target declare its namespace in ESM output, and add the ESM twins of barrel/ExportStarOfExternalFormatCjs and barrel/ExportStarOfDisabledFormatCjs.
Why this was flagged
Input: /lib/package.json with "sideEffects": false, /lib/barrel.js = export * from 'node:path', and an entry that does import { basename } from './lib/barrel.js'; export function later() { return basename("/a"); } bundled with the default --format=esm through bun build. On the base branch the barrel is dropped and the bundle loads (the read never runs). With this diff src/bundler/LinkerContext.rs:4080-4091 pushes the barrel's part 0 onto re_exports, src/bundler/LinkerContext.rs:4641-4655 stores it in imports_to_bind, and step 6 at src/bundler/linker_context/scanImportsAndExports.rs:989 makes the reading part depend on it, so the barrel is live. For the external record src/bundler/linker_context/scanImportsAndExports.rs:1347-1387 sets happens_at_runtime and CALLS_RUNTIME_RE_EXPORT_FN but not CONTAINS_IMPORT_STAR (that flag is only set on the lifted-CommonJS path at :1331-1335), so the chunk prints __reExport(exports_barrel, node_path) beside a bare import "node:path", and the bundle throws ReferenceError: node_path is not defined at load.
Verification: normal — acknowledged in diff: the PR description and the new test comment at test/bundler/bundler_barrel.test.ts:2437-2439 ("With ES module output the kept statement still needs #41220") both flag this; #41220 is not in this diff or its base, so merging this head alone ships the regression.
There was a problem hiding this comment.
This is the merge order condition on line 2 of the description. The statement is wrong on main for every live barrel of this kind (#15997): without "sideEffects": false, the barrel of this input throws node_path is not defined at load on main too, with the same bytes as on this branch. #41220 is the change that declares the namespace. It sets CONTAINS_IMPORT_STAR on that record. This PR does not copy it, so this PR must merge after #41220.
I ran the input of this comment on three debug builds:
| build | result |
|---|---|
| main 9b8347a | loads, and later() throws exports_barrel is not defined |
| this branch (755cd73) | throws at load: node_path is not defined |
| this branch with the source change of #41220 | loads, and later() returns the name |
The same holds for a target that a browser build disables. The "Merge order" table in the description has the four shapes.
The ES module twins of barrel/ExportStarOfExternalFormatCjs and barrel/ExportStarOfDisabledFormatCjs fail until #41220 is on main. I add them when I rebase onto it. This thread stays open until then.
|
On the additional finding in this review ( Fixture, // a.js (entry point)
import { r } from './lib/barrel.js';
console.log("a", r);
// b.js (entry point)
import './reader.js';
console.log("b");
// reader.js
import { x } from './lib/barrel.js';
console.log("x", x);
export const r = 1;
// lib/barrel.js
export * from '../reader.js';
export * from './impl.cjs';
// lib/impl.cjs
const x = 200;
module.exports = { x };
The shared chunk takes the order of I did not change the check in this PR, for these reasons:
The shape is now in Downsides and under "Not fixed here" in the description. |
Fixes #18008
Merge after #41220.
Problem
ReferenceError: exports_barrel is not definedwhen a"sideEffects": falsebarrel doesexport *of a CommonJS or external module.exports_barrel.x. TheDynamicFallbackarm (src/bundler/LinkerContext.rs:4060) records no dependency on that object, so tree shaking drops the barrel.Fix
re_exports, which a direct import now keeps. Each part that prints the read keeps the barrel.LoadTimeReadsdoes not count it as a read of the getters. Split chunks keep their order.bundler_barrel.test.ts,bundler_jsx.test.ts(21 of 29 new tests fail on main), all oftest/bundler/.Background
export * from <CommonJS>are run-time names:__reExport()fills the exports object.scanImportsAndExports.rs:1290(named here) stays. The esbuild rule makes langiumimport { stream }981,548 B, not 11,206 B.Downsides
export *target throws at load in ES module output: 112 of 31,796 random-graph bundles.--splittingwith an import cycle through the barrel can run the read before__reExport()and printundefined, as main does without the flag (Notes).--ignore-dce-annotationskeeps (langiumimport { Emitter }: 932,625 B). Release.text+1,280 B, 7 moreis_empty()tests per 697-module link.Notes
Repro (no install, the shape of the issue)
bun test.tsokReferenceError: exports_cancellation is not definedokokOn main the whole output is
console.log(new exports_cancellation.CancellationTokenSource().ok());. This branch printsvar exports_cancellation = {}; __reExport(exports_cancellation, __toESM(require_cancellation(), 1));above it.langium 4.2.4 has
"sideEffects": falseandexport * from 'vscode-jsonrpc/lib/common/events.js'. An entry that builds its grammar services throwsReferenceError: exports_event is not definedfrom the bundle of main. The bundle of this branch prints what node prints.Merge order
A barrel that this PR keeps prints each run-time
export *it has. If the target is not in the bundle and the output keeps ES module syntax, that statement is__reExport(exports_barrel, ns)beside a bareimport "path", and it throwsReferenceError: ns is not defined. Main prints the same for every live barrel of that kind (#15997). #41220 setsCONTAINS_IMPORT_STARon the record, which names the namespace. #46786 carries the same commit. This PR does not copy it."Not in the bundle" means an external target (
node:path,--external,--packages=external) or a target that a browser build disables (fs, a"browser"field entry set tofalse). The second kind needs no flag.export * from "node:path", read at loadexports_barrel is not definednode_path is not defined"browser": { "./node.js": false }target, read at loadexports_barrel is not definednode is not definedundefinedWith
--format=cjsevery row loads on this branch. So a bundle whose read of the name never runs works on main and breaks here until the hunk is on main: 3 of 3 fixtures. I built this branch with the source change of #41220 on top to fill the last column. The four rows are the same on the rebased branch (debug builds of main 9b8347a, of 755cd73, and of 755cd73 with that source change).After #41220 is on main, this PR gets the ES module twins of
barrel/ExportStarOfExternalFormatCjsandbarrel/ExportStarOfDisabledFormatCjs.The check that stays
#42552 (comment) names the
export_kindcheck atscanImportsAndExports.rs:1290as the bug. esbuild tests the imported file there (linker.go:1958in 0.21.5), and so did the first port (636d2e4). 77eb61a (2023-05-11) changed it to the importing file.import { fromEsm }of a barrel with one CommonJSexport *and one ES moduleexport *is 72 B on main and 2,129 B in esbuild. langiumimport { stream }is 11,206 B on main and 981,548 B in esbuild. This PR adds the dependency per printed read, so both stay as on main.export *statement of a lazily loaded module when a name is read through it #46786 is in. I built that variant on top of this branch. langiumimport { Emitter }goes from 932,625 to 909,077 B and a mixed barrel from 2,852 to 2,699 B. One shape breaks: a file with run-time exports that reads a lazily loaded ES module through a side-effect-free barrel of ES modules printsx undefined, because the check keeps that barrel live by accident. bundler: keep theexport *statement of a lazily loaded module when a name is read through it #46786 does it on purpose.barrel/ExportStarOfCommonJSInReaderOfLazyModulepins the shape. In 15 linker test files no other test changes.DynamicFallbackarm. The comment ofbarrel/ExportStarOfCommonJSInReaderOfLazyModulesays what the check keeps.How the dependency reaches each reader
advance_import_trackerreturnsDynamicFallbackfor a name that a file with run-time exports does not list.match_import_with_export_innerthen makes the import a property read of that file's exports object (Symbol.namespace_alias). It is the only place that makes a read of the exports object of another file.re_exports.export { x } from "./barrel"in another file), the match isNormalAndNamespaceandbind_matched_importalready storesre_exports.Namespace, and the list was dropped. It is now stored in animports_to_bindentry that binds the import to itself. The import still prints through its alias.imports_to_bindthen add the dependency where the read is printed. None of them changed:scanImportsAndExports.rs:935),:1018),doStep5.rs:527), also in a third file throughexport *,X.namewhereXis a namespace that a file re-exports (LinkerContext.rs:4960).export *statement, which prints__reExport(...).--splittingthe dependency also gives the barrel the entry bits of each reader, so the barrel goes to a chunk that every reader loads.An entry of
imports_to_bindthat binds a ref to itself is not new.generate_symbol_import_and_useandadd_exportswrite such entries for a symbol of another file. New is that the ref is an import of the same file. I read every reader of the map for that shape:scanImportsAndExports.rs:716,:935-993(symbols.merge(ref, ref)returns at once),:1018-1028,doStep5.rs:173-185,:335,:527-533,computeCrossChunkDependencies.rs:228-243,:289-304,postProcessJSChunk.rs:955-972,LinkerContext.rs:4532,:4787,:4904,:4937.The load order check
LoadTimeReads(findAllImportedPartsInJSOrder.rs) decides if the order of a split chunk with an import cycle puts a part that runs at load ahead of a binding that it reads. It readspart.dependenciesas "this part names a binding of that part". The new dependency ends at part 0 of the barrel, and part 0 names each export that the namespace object lists. So the check saw the reader ofexports_barrel.xas a reader of all of them, found no safe order, and kept an order that printedNaN(right on main, also for a barrel without"sideEffects": false). The first version of this branch had that regression. The self-review found it.A read of a run-time name calls none of the getters.
LoadTimeReads::newnow skips a dependency on part 0 of another file unless the part uses the exports object itself (import * as nsas a value). With that, 3 of 3 cycle fixtures are byte-identical to main. The check does not know that the read must run after the__reExport(...)statement of the barrel. Main does not know it either. "Not fixed here" has a fixture.Shapes that threw on main and are right on this branch
import { x }, and the same read inside a function.import * as nswithns.x.export *hops, and two run-timeexport *in one barrel.export { x } from "./barrel"in another file, andimport { x }; export { x }.X.xwhereXisexport * as Xof a file that re-exports the name.var export_x = exports_barrel.x), formats esm and cjs.import(). A JSON file kept CommonJS byimport().--format=cjs.export *of a CommonJS file (jsxImportSource). Main throwsexports_jsx_runtime is not definedfor it with and without"sideEffects": false, because the JSX import is never kept for side effects.--format=esm,cjsandiife,--target=node,bunandbrowser,--minify.--splitting: two entry points that read through the barrel.Two shapes do not throw on main and change too:
--splitting, one entry point keeps the barrel in another way (its namespace object) and another chunk reads a name. On main that chunk importsexports_barrelfrom the entry point's file, which runs the entry point's code or, for a chunk thatimport()loads, imports the unhashed entry file (the symptom of --splitting: chunks import from the entry chunk, breaking entry URLs versioned with a query string #42290). Here the barrel goes to a chunk of its own.What does not change
Every output that prints no read of a run-time name is byte-identical to main. So is every output of a barrel without
"sideEffects": false, with or without such a read: 4,735 of 4,735 random-graph bundles (1,752 graphs). The JSX shape above is the one exception, because main drops that barrel. The unchanged shapes include:import { fromEsm }of a barrel with a CommonJSexport *and an ES moduleexport *: 72 B.import { fromEsm, movable }): 72 B.import()without--splitting), and a barrel that is wrapped.Measurements (x64 Linux. Release builds of main 179664e and of this branch. node 26.3.0 runs the bundles. The line numbers in this description are from the branch on main 9b8347a.)
Fixtures: 27 shapes, each as esm and cjs. 24 outputs throw on main. 23 of them print what node prints on the source. The last one is the external target with ES module output. The other 28 outputs are byte-identical (
cmp).Random graphs: 11,700 graphs of CommonJS and ES module leaves under 1 to 4 barrels (
export *,export { x } from,export * as, own exports). The readers are named imports,ns.x, a captured namespace, re-exports, entry point exports,require()andimport(). The generator also makes import cycles through a barrel, reads that do not run at load, external targets and targets that a browser build disables. Each graph is bundled as esm, cjs and--splittingwith two entry points: 31,796 bundles that node can judge. node on the source is the reference, line by line in order.export *that is not in the bundle, ES module output, and a read that does not run at load (72 with a disabled target, 40 with an external one). With--format=cjsthere are 0. I ran those 69 graphs again on this branch with the source change of bundler: name the namespace of an external export * in a non-entry module #41220: 0 of the 112 stay wrong.--splitting, 0 go from right to wrong. The first version of this branch had 4.--splittingentry point are compared as a set, 1 more bundle goes from right to wrong. It is theimport()chunk under "Not fixed here".exports_barrelon main, the namespace here).Real packages, no run-time name read: a 697-module bundle (hono, svelte/compiler, stripe, 1,640,435 B) and a 205-module bundle (express, lodash, axios, react-dom/server, immutable, 1,899,573 B) are byte-identical.
langium 4.2.4:
import { Emitter }(run-time name)langium/lib/utils/event.jsimport { stream }(binds statically)The
import { Emitter }bundle is byte-identical to the one that main prints with--ignore-dce-annotations.A read in a function that nothing calls, CommonJS target: 110 B on main, 2,225 B here, 1,837 B in esbuild. Here and in esbuild the CommonJS file runs at load.
test/bundler/, release builds, 108 files: main 7,767 pass and 24 fail, this branch 7,809 pass and 1 fail. The 1 is "encoder output is identical on every platform" inbundler_bytecode_portable.test.ts, which fails on main too in this container. The other 23 on main are the 21 new tests and 2 cold-start failures of the first run (native-plugin.test.tssetup, one bytecode cache test). Both pass on main when run again.15 linker test files with the debug builds of main and of the first version of this branch (1,703 tests): the new tests are the only difference.
default/TopLevelAwaitForbiddenRequireSourceMapCLIhits its 5 s timeout on both debug builds in this container. The two changed test files pass in full on the final debug build (164 pass).After the rebase onto main 9b8347a, debug build: the two changed test files pass (164 pass). With the
src/of that main, 21 tests fail.Link cost, counted with gdb breakpoints on the debug build:
is_empty()testsimport { Emitter }, 321 modulesThe test runs once per import that binds to a namespace (CommonJS, external, or a run-time exports object). Each run-time name adds two dependency entries to a part that reads it: the import statement of the name and part 0 of the barrel. The new code in
LoadTimeReads::newruns only for a split chunk with an import cycle and two or more entry points.Release binary:
.text82,164,617 to 82,165,897 B (+1,280 B,size -A). The file is 107,730,656 B on both builds.match_import_with_export_inner7,463 to 7,995 B,scan_imports_and_exports56,421 to 56,533 B (nm -S).Instruction counts are not measured: valgrind and perf are not in the build container. bloaty and strace are not there either.
Other PRs
export *statement of a lazily loaded module when a name is read through it #46786 carries the commit of bundler: name the namespace of an external export * in a non-entry module #41220: see "The check that stays".export *name depend on the barrel's exports object #46903 (comment).Not fixed here
--splitting, a chunk of two entry points, and an import cycle through the barrel. The chunk takes the order of one entry point. If that order runs the read ahead of the barrel's__reExport(...), the read isundefinedfor both entry points.--splitting --format=esma.jsb.jsx undefinedx 200libhas"sideEffects": falseexports_barrel is not definedx undefinedx undefinedx undefinedx undefinedx 200x 200Without
--splittingeach entry point prints what node prints. Withexport * from './impl.cjs'first in the barrel, every row but the one that throws printsx 200.LoadTimeReadshas no edge from a read of a run-time name to theexport *part that fills the object, andfind_safe_walkshas one verdict per order. The edge must stay apart from the edges for bindings: the first version of this branch mixed them and lost the one order that was safe for a binding. It is a change for barrels with and without the flag.--splitting,import()of a barrel that a static import also uses, where the barrel has an externalexport *: the chunk of theimport()is onlyimport "./shared.js", so the names of the external module are missing (xa undefined). Main does this when the static import binds a name statically, and when the barrel is not side-effect free. A read of a run-time name now counts as a use, so that shape joins them: 1 fixture that is right on main.One run-time name through a root barrel keeps as much as
--ignore-dce-annotationsdoes for that barrel. The read is a property of the root barrel's exports object, and__reExport(exports_root, exports_inner)needs each object on the way. langiumimport { Emitter }is 932,625 B here and 981,715 B in esbuild. The same name from the innermost file (langium/lib/utils/event.js) is 6,182 B. To bind such a read to the innermost file that can have the name is a change inadvance_import_trackerwith its own guards.bundler: take one copy per key for a top-level destructuring of a module namespace #46163 refuses a destructuring pattern over such a barrel because of this bug. It can drop that refusal after this PR. Its
mark_held_namespacesreads!imports_to_bind.contains(&item), and the entry that this PR adds changes the answer for an import of a run-time name.One CommonJS name, re-exported by name through two files and then through
export *of both, readsundefinedon main and here. No run-time exports object is involved.Self-review
23 concerns. 21 addressed: the load order check, the comment at the check, the JSX route, the split shapes, a read that does not run, the disabled target, an assertion in each test that the output has the read, and this description. Not done:
import()chunk above. The cause is in how a chunk of animport()target that lives in another chunk prints its exports. Main has it for a static binding.import()chunk. They are listed here.Tests
test/bundler/bundler_barrel.test.tshas 26 new tests andtest/bundler/bundler_jsx.test.tshas 3. 21 fail on main: 18 with aReferenceError, 3 with a wrong output or chunk graph. 8 pass on both builds and pin what must not change:StaticNameOnlyStaysTreeShakenUnreadRunTimeNameStaysTreeShakenSplittingImportCycle,...SideEffectsFalse,...WithLazyChunkLoadTimeReadschange)SplittingNamespaceInImportCycleInReaderOfLazyModulescanImportsAndExports.rs:1290keeps rightjsx/AutoImportDroppedWhenJsxUnusedExportStarOfCjs