bundler: resolve a barrel's import records once - #39874
Conversation
schedule_barrel_deferred_imports resolved a barrel again whenever the requested record had no source_index. An external record, a record whose resolution failed, and a record waiting for an onResolve plugin never get one, so every importer of such a re-export ran the resolver (and the onResolve plugins) over the barrel again. A pass that did un-defer a record also resolved those records again, because resolve_import_records only skipped records with a source_index. Resolve a barrel only when the BFS item un-deferred a record in it, and mark every record that resolve_import_records handles with RESOLVE_STARTED so a later pass skips it. The barrels_to_resolve map and the trailing loop are gone: each un-deferral resolves inline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesBarrel un-deferral now resolves only newly selected import records and patches their source indices immediately. Standard parsing keeps full-record resolution. Regression tests cover duplicate diagnostics, external resolution reuse, deferred records, and fallback behavior. Barrel resolution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 5:20 AM PT - Aug 21st, 2026
❌ @robobun, your commit e94e9a0 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39874That installs a local version of the PR into your bun-39874 --bun |
|
Status: ready for review. Reproduced on Bun 1.4.0 in two ways:
Current shape: the barrel BFS resolves only the records it just un-deferred and passes their indices to the resolve and patch passes ( The four new tests in CI on e94e9a0: every lane is green except Windows 2019 x64, where |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The RESOLVE_STARTED idempotency flag and the switch from "resolve on invalid source_index" to "resolve only on un-deferral" both check out, and the three new tests pin exact call/error counts. Because this reworks control flow in the bundler's resolution path — including a dev-server behavior change that leans on a non-local dedup argument — a human look would still be worthwhile.
Checked: all three un_defer_record sites now resolve immediately, so the removed trailing loop is dead; bundleErrors in expectBundled.ts rejects unexpected errors, so the third test does pin "reported once"; the export-star and named-resolution paths previously read .source_index directly (not via record_target), so the dev-server read path there is unchanged.
Extended reasoning...
Overview
Adds ImportRecordFlags::RESOLVE_STARTED (bit 10) and sets it in resolve_import_records so a record is never resolved twice. In barrel_imports.rs, drops the barrels_to_resolve map and its trailing loop; the BFS now calls resolve_barrel_records only when un_defer_record actually flipped a record. Three new tests in bundler_barrel.test.ts cover: external re-export resolved once across multiple importers, external record skipped when a later un-deferral re-runs the pass, and an unresolvable re-export reported exactly once.
Security risks
None. This is bundler bookkeeping — no untrusted input parsing, no auth/crypto, no filesystem writes outside the existing resolution machinery.
Level of scrutiny
High. resolve_import_records is on the critical path for every bundled module, and the barrel BFS has ordering-sensitive interactions with the dev server (records there carry no source_index after normal resolution). The PR description reasons through the dev-server case — a request for a never-deferred record used to force-resolve and produce a duplicate work item that dedup dropped, so skipping it is equivalent — but that argument depends on phase-1 seeding behavior in the barrel's own parse, which a maintainer familiar with bake/ should confirm. The author reports bake/dev/bundle and bake/dev/plugins pass.
Other factors
The mechanics look correct: every site that returns true from un_defer_record now calls resolve_barrel_records immediately, so the removed trailing loop was redundant; the new flag makes resolve_import_records idempotent for external / failed / plugin-pending records that legitimately lack a source_index. Tests use established harness patterns (the plugins(builder) callback form already appears in barrel/ResolvePlugin), and expectBundled's bundleErrors handling fails on unexpected errors, so the "reported once" test is not vacuous. No CODEOWNERS cover these paths. The PR notes a bit-10 collision with #35053's proposed IS_EXTERNAL; that's a coordination note for whichever lands second, not a defect here.
|
For the dev server point, the code that carries the argument is all in
The removed pass ran in a later importer's call. Its only output was a work item for the same (target, alias) pair, which is a propagated item and is dropped by the The dev server tests that cover barrel chains ( |
Replace the RESOLVE_STARTED import record flag with an index list. The BFS knows which records it just un-deferred, so resolve_barrel_records hands exactly those to resolve_import_records and to patch_import_record_source_indices (only_records on both contexts, which also replaces force_save). The barrel's other records are not resolved or patched again, and no ImportRecord flag bit is used. Add a plugin-free test: un-deferring a second record of a barrel used to report its unresolvable sibling a second time.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the barrel BFS resolve path — replacing the barrels_to_resolve drain and the !rec_si.is_valid() inline-resolve fallback with per-index only_records calls, and leaning on the dev-server invariant that the barrel's own schedule_barrel_deferred_imports call already covered every never-deferred record — a human familiar with the barrel optimization should also take a look.
What was reviewed:
- Traced that every
un_defer_record → truesite now resolves exactly that index; the removed drain loop and the!rec_si.is_valid()fallback are subsumed. - Checked the dev-server argument for the removed star-record fallback:
export *targets are seeded toRequestedExports::Allviarecord_target()in the barrel's own call, so a later BFS push would have been dropped by the dedup check anyway. - Verified
only_recordsis sorted at every call site (single-element or the ascending0..lenloop) sobinary_searchis sound;force_save's only caller now passesSome, sosave_import_record_source_indexis unchanged. - Confirmed
bundleErrorsinexpectBundleddoes exact-count matching, so the two "reported once" tests fail on a duplicate error.
Extended reasoning...
Overview
The PR changes how the barrel BFS in src/bundler/barrel_imports.rs resolves un-deferred import records. Previously, un-deferring a record put the barrel into a barrels_to_resolve set; when the BFS needed a source_index and it was invalid, it re-ran resolve_import_records on the barrel's entire record list (skipping only records that already had a source_index). External, failed, and plugin-pending records never get one, so they were re-resolved — duplicate error diagnostics, and onResolve plugins fired 2–3× for one record.
The fix threads an only_records: Option<&[u32]> through ResolveImportRecordCtx / PatchImportRecordsCtx (src/bundler/bundle_v2.rs) and, at each of the three un-defer sites in the BFS, immediately calls resolve_barrel_records with just the index(es) it un-deferred. The barrels_to_resolve map and its drain loop are deleted, and force_save on the patch ctx is replaced by only_records.is_some() (same single caller). Four new itBundled tests pin the exactly-once behavior for both the plugin-free and plugin cases.
Security risks
None. This is bundler resolution ordering; no untrusted input parsing, auth, crypto, or filesystem-boundary logic is touched.
Level of scrutiny
Medium-high. The barrel BFS is one of the more parse-order-sensitive parts of the bundler, and the correctness of removing the !rec_si.is_valid() fallback on the dev-server path rests on a cross-function invariant: the barrel's own call to schedule_barrel_deferred_imports already seeded its export * targets via record_target() (which has the path-map fallback), so the propagated item a later importer would have pushed hits RequestedExports::All in the dedup check and is dropped. I traced this and it holds, but it is exactly the kind of non-local reasoning a maintainer who owns this code should confirm. The item_is_star propagation arm already uses record_target() and is unchanged in that respect.
Other factors
- The
only_recordsslice is sorted at every call site (either a single-element literal or built byfor idx in 0..len), matching thebinary_searchinonly_selected_recordand the twodebug_assert!(is_sorted)guards. bundleErrorsinexpectBundled.tsmatches by exact count (unexpected extras fail, leftover expectations fail), so the two "reported once" tests genuinely assert one diagnostic, not at-least-one.- The plugin tests reset the
resolvedarray inplugins()and asserttoEqual(["react"]), so they pin exactly-once and won't leak state across the harness's build invocations. - All the comment-cop bot threads and my earlier note about the stale description are resolved. CI was still building at the last status update.
- The PR author ran the barrel, plugin, edgecase, splitting, regressions, and
bake/devbundle suites on a debug build; the four new tests fail on main and on the debug build with thesrc/changes stashed.
|
For a human reviewer, the places to check on the current head (e94e9a0). The earlier pointer comment has line numbers from the first revision.
CI on this head: 178 of 179 jobs green. The one failure is |
A barrel un-defer now resolves and patches only the un-deferred records (only_records), so neither pass reaches a record an earlier answer left external. The flag no longer guards anything, and bit 10 is the last free ImportRecordFlags bit, which #39874 chose not to take. This restores the external arm to its previous shape: an unchanged specifier is left alone, and a queued ExternalPath is applied after the path map pass so that a plugin path is never looked up as a module. The barrel test stays. It now pins that #39874's index list covers the rewritten record and the bundled module that shares its path text.
Problem
sideEffects: falsebarrel is resolved again, as a whole, when an importer asks for a re-export withoutsource_indexor un-defers one of its records. With no plugins, Bun 1.4.0 reportsCould not resolve: "./missing.js"twice for one broken re-export. AnonResolveplugin for an external re-export runs 2 to 3 times for one record.schedule_barrel_deferred_imports(src/bundler/barrel_imports.rs:852,:887on main) reads a missingsource_indexas "never resolved". An external, failed, or plugin-pending record never gets one.resolve_barrel_recordspasses the whole list.resolve_import_records(src/bundler/bundle_v2.rs:6002) skips only records with asource_index.Fix
barrels_to_resolvemap and the final loop are gone.ResolveImportRecordCtxandPatchImportRecordsCtxgetonly_records. Both loops skip every other record. It replacesforce_save, which covered the same case. NoImportRecordflag is added.test/bundler/bundler_barrel.test.ts, each fails on main. Other suites in Notes.Background
sideEffects: false(oroptimizeImports) package whose exports are all re-exports. Before it is resolved,apply_barrel_optimizationmarks the re-exports nobody asked for yetIS_UNUSED: they are deferred.schedule_barrel_deferred_importsclears the flag (un-defers) and callsresolve_barrel_records:resolve_import_records, thenpatch_import_record_source_indices, which writes thesource_indexof each module found. The BFS follows it into the next barrel.source_index. An external record gets none, a failed one is disabled, and anonResolvematch is answered later by the JS thread.Notes
Plugin-free repro. Bun 1.4.0 prints 2, this branch prints 1.
a.jsis found through the barrel, so its request always arrives after the barrel deferredBrokenandC:When the second importer is an ordinary file instead, the count depends on whether that file is parsed before or after the barrel, so the same build reports 1 or 2 errors from run to run.
Plugin repro: a barrel with
export { default as React } from "react"and anonResolveplugin that returns{ path, external: true }. The plugin runs when the barrel is resolved, again from the barrel's ownschedule_barrel_deferred_importscall (the seeded request forReactfinds nosource_index), and again for each later importer. The plugin case that needs the index list and not only the barrel change is a later importer that un-defers a different record:barrel/ExternalReExportNotResolvedAgainOnUnDefer.The second pass could also fail.
export { x } from "bun:whatever"in a barrel, targetbun, fails on main withCould not resolve: "whatever": thebun:arm strips the prefix, and the second pass resolves the stripped name as a package. It builds with this change. No test for it, the same pass is what the other tests pin.Relation to #35053: its barrel test hits the same second pass and adds
IS_EXTERNALso that pass skips its rewritten record. With this change the pass does not reach that record. This PR takes no flag bit.Dev server: import records there get no
source_indexfrom the normal parse path, so before this change a request for a never-deferred record also resolved the barrel again, andforce_savewrote indices onto all of its records. The work item that pass produced duplicated the request the barrel's own call made in phase 1 (barrel_imports.rs:462-510and:550-586) and was dropped by therequested_exportscheck. Un-deferred records still get their indices written. The barrel tests intest/bake/dev/bundle.test.tspass.Not changed: a plugin answer that arrives after the BFS does not request anything from the module it resolves to. Same before and after.
The first revision of this PR used an
ImportRecordFlags::RESOLVE_STARTEDbit instead of the index list. Bit 10 is the last free bit of theu16, and #35053 and #38461 both take it, so the self-review asked for this shape. The barrel change alone fixes two of the four tests. The index list is what fixesbarrel/UnDeferReportsUnresolvableSiblingOnceandbarrel/ExternalReExportNotResolvedAgainOnUnDefer.Suites run on the debug build:
bundler_barrel,bundler_plugin,bundler_plugin_chain,bundler_edgecase,bundler_npm,bundler_browser,bundler_bun,bundler_cjs,bundler_splitting,bundler_regressions,bundler_allow_unresolved,metafile,bun-build-api,native-plugin,bake/dev/bundle,bake/dev/plugins,bake/dev/esm,bake/dev/hot,regression/issue/29264. With thesrc/changes stashed, the new tests fail on the same debug build. The plugin-free test failed 15 of 15 runs on Bun 1.4.0.