Deduplicate bundler dispatch and output glue, the JS string escaper, and the exports-map retry - #31998
Deduplicate bundler dispatch and output glue, the JS string escaper, and the exports-map retry#31998alii wants to merge 7 commits into
Conversation
|
Updated 6:22 PM PT - Aug 10th, 2026
✅ @alii, your commit c0e4c519b9d503a0af7926eb165ba9373d75dae3 passed in 🧪 To try this PR locally: bunx bun-pr 31998That installs a local version of the PR into your bun-31998 --bun |
|
@robobun adopt |
e0aa541 to
dfb8e9d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes duplicated helpers and delegations across components: moves JS string escaping into bun_core::printer with a const-generic Encoding and public can_print_without_escape; unifies ParseTask enqueue/dispatch via a shared helper; extracts linker output helpers and bytecode generation; adds ParseResult/export-default construction helpers; consolidates bundler Fs with bun_resolver::cache; refactors resolver fs to delegate to fs_full; simplifies standalone executable data validation; and adds bundler tests. ChangesConsolidate duplicated helpers and module delegations
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Adopted and, after main moved ~3000 PRs, rebuilt on current main rather than rebased. Two of the seven original dedups were made moot by #35002 and dropped; the other five were re-applied (two with small adaptations for #37071 and #36746) as five focused commits, net -234 lines. Details in the PR body. Review threads all resolved; CI running on the rebuilt head. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/resolver/resolver.rs`:
- Around line 3664-3674: The global-cache/module-folder branch in
load_node_modules resolves At/AtConditional using a different condition set
(conditions.import) than resolve_esm_exports (which uses
self.opts.conditions.style), causing inconsistent exports selection; update the
load_node_modules branch to call resolve_esm_exports (or otherwise reuse its
logic) for the global-cache path so both code paths use the same condition
resolution, or change resolve_esm_exports and load_node_modules to both consult
the same condition source (e.g., unify to self.opts.conditions.style) for
At/AtConditional handling; target symbols: resolve_esm_exports,
load_node_modules, At, AtConditional, self.opts.conditions.style,
conditions.import.
- Around line 3703-3705: Don't overwrite the resolved module type after
handle_esm_resolution(): remove the assignment that reassigns out.module_type
from the caller's module_type in the success path (the lines setting
out.is_node_module = true; out.module_type = *module_type; self.extension_order
= prev_extension_order). Instead, preserve the module type that
handle_esm_resolution() computed; if the inexact-resolution branch truly needs
the caller's module_type, set that value inside handle_esm_resolution() for only
that branch rather than resetting out.module_type here, and keep restoring
self.extension_order = prev_extension_order and setting out.is_node_module only
where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3658e8dc-9d9e-41f0-a551-e7caf3677c49
📒 Files selected for processing (11)
src/bun_core/string/mod.rssrc/bundler/bundle_v2.rssrc/bundler/cache.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/js_printer/lib.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/standalone_graph/StandaloneModuleGraph.rs
66e55c5 to
165595a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/linker_context/generateChunksInParallel.rs (1)
615-724:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't bake a single linked sourcemap URL into the standalone chunk cache.
standalone_chunk_contentsstores one rendered JS/CSS buffer per chunk, but this block appends//# sourceMappingURL=before that buffer is reused by every HTML output. Whenpublic_pathis empty and the same chunk is inlined into multiple standalone HTML files under different directories, the relative.mappath chosen here is only correct for the first matched HTML entry point and wrong for the others. Move the linked-URL injection to the HTML-specific assembly path, or keep the sourcemap payload separate and append the trailer per consuming HTML output instead of per shared chunk buffer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/linker_context/generateChunksInParallel.rs` around lines 615 - 724, This code currently mutates the shared chunk buffer by appending the linked sourceMappingURL inside the loop (see chunks[ci], buffer, source_map_final_rel_path, and standalone_sourcemaps), which bakes one relative .map path into a chunk reused across multiple HTML files; instead, stop writing the sourceMappingURL into buffer here—only finalize and store the external map bytes in standalone_sourcemaps (output_source_map) and, if needed, also store the computed source_map_final_rel_path (or its components a/b) alongside that map; then move the actual //# sourceMappingURL= trailer injection into the HTML assembly path where each HTML output renders the chunk (the code that consumes standalone_sourcemaps and writes standalone_chunk_contents), appending the correct per-HTML relative URL at that point.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 2509-2518: The code currently uses the original parameter `loader`
when doing copy-for-bundling bookkeeping after calling
`self.enqueue_on_load_plugin_if_needed(task)`, so tasks whose `task.loader` was
changed (e.g., from Loader::File to Loader::Js/Css/Json for dataurl: inputs) get
incorrect bookkeeping; update the branch to inspect and use the task's
post-dispatch loader (e.g., read `task.loader` or call the accessor that returns
the task's current loader) when calling `should_copy_for_bundling()` and when
updating `additional_files` (AdditionalFile::SourceIndex),
`items_side_effects_mut()`, and `self.graph.estimated_file_loader_count` so the
graph state reflects the actual parse path that will run instead of the original
`loader` parameter.
---
Outside diff comments:
In `@src/bundler/linker_context/generateChunksInParallel.rs`:
- Around line 615-724: This code currently mutates the shared chunk buffer by
appending the linked sourceMappingURL inside the loop (see chunks[ci], buffer,
source_map_final_rel_path, and standalone_sourcemaps), which bakes one relative
.map path into a chunk reused across multiple HTML files; instead, stop writing
the sourceMappingURL into buffer here—only finalize and store the external map
bytes in standalone_sourcemaps (output_source_map) and, if needed, also store
the computed source_map_final_rel_path (or its components a/b) alongside that
map; then move the actual //# sourceMappingURL= trailer injection into the HTML
assembly path where each HTML output renders the chunk (the code that consumes
standalone_sourcemaps and writes standalone_chunk_contents), appending the
correct per-HTML relative URL at that point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc65c06b-533f-4c49-a0f6-6864f783be7f
📒 Files selected for processing (14)
src/bun_core/string/mod.rssrc/bundler/bundle_v2.rssrc/bundler/cache.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/js_printer/lib.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/standalone_graph/StandaloneModuleGraph.rstest/bundler/bun-build-api.test.tstest/bundler/bundler_edgecase.test.tstest/bundler/bundler_string.test.ts
|
Re the outside-diff review note on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~900-line refactor across hot bundler/resolver/printer paths with several non-trivial helper extractions (parse-task dispatch, resolve_esm_exports loop, kind_impl closure-injection, the bun_core::printer move) plus a manual rebase over #32116 — worth a human pass to confirm the behavioral-equivalence claims.
Extended reasoning...
Overview
This PR deduplicates ~900 lines across the bundler, resolver, JS printer, and standalone-graph code. The main moves: (1) the const-generic string escaper migrates from js_printer into bun_core::printer and the old runtime-encoding write_pre_quoted_string becomes a dispatcher over it; (2) five parse-task enqueue sites in bundle_v2.rs collapse into configure_and_dispatch_parse_task; (3) linked-sourcemap/bytecode/standalone-placeholder glue is extracted into shared helpers in writeOutputFilesToDisk.rs; (4) the bundler's local cache::Fs is dropped in favor of the resolver's; (5) resolver::fs re-exports bun_core::immutable::BOM, adds ModKey::from_file, and extracts kind_impl with injected closures so the inline lib.rs RealFS and fs_full::RealFS share one body; (6) the two inline ESM-exports resolution blocks in load_node_modules become a single resolve_esm_exports loop; (7) the per-OS StandaloneModuleGraph::from_executable bodies collapse into one. Tests are added to pin the consolidated paths.
Security risks
None apparent. No auth, crypto, permissions, or untrusted-input parsing is introduced; the standalone-graph trailer/offset validation is unchanged, just deduplicated. The string escaper move preserves the same escaping rules.
Level of scrutiny
High. These are production-critical hot paths (module resolution, parse-task scheduling, output writing, string escaping) where a subtle field-initialization or loop-condition divergence would manifest as hard-to-reproduce bundler bugs. Several extractions are not purely mechanical: the consolidated parse-task helper sets io_task.node.next and is_entry_point for call sites that previously relied on ParseTask::init/Default; the resolve_esm_exports two-pass logic is rewritten as a loop with a length-comparison terminator; the two kind implementations being merged had minor Windows-side differences (ffi::zeroed vs zeroed_unchecked, Fd::from_system vs from_native, CreateFileW namespacing); and the branch was manually rebased over #32116's standalone-sourcemap changes.
Other factors
The bug-hunting system found nothing. CodeRabbit raised two concerns on resolve_esm_exports that were both shown to be pre-existing behavior and retracted. The PR description documents a careful field-by-field equivalence audit against ParseTask::init/Default/OutputFileInit::default(), and CI is green except for darwin runner flakes on unrelated files. All of that raises confidence, but given the breadth, the hot-path nature, and the rebase resolution, a human reviewer signing off on the equivalence claims is the right bar here.
165595a to
89759f1
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~900-line refactor across the bundler, resolver, and the hot JS-printer string escaper, with two manual rebase resolutions — worth a human pass to confirm the field-by-field equivalence claims.
Extended reasoning...
Overview
This PR deduplicates code across 14 files in the bundler, resolver, JS printer, and standalone-graph subsystems, netting roughly -900 lines. The main moves are: (1) the const-generic string-escape loop relocates from js_printer into bun_core::printer with the old runtime-encoding entry point becoming a dispatcher; (2) five parse-task enqueue sites in bundle_v2.rs collapse into configure_and_dispatch_parse_task; (3) linker output glue (linked-sourcemap trailer, bytecode generation, standalone placeholder) is extracted into shared helpers; (4) the bundler's local cache::Fs is dropped in favor of the resolver's; (5) the two ESM exports-map resolution blocks in load_node_modules become a loop in resolve_esm_exports; (6) RealFS::kind and several temp-dir/ulimit/ModKey bodies in the inline resolver/lib.rs delegate to fs_full; and (7) the per-OS StandaloneModuleGraph::from_executable bodies are unified.
Security risks
None introduced. The standalone-graph trailer/offset validation is consolidated verbatim, the resolver fs path-handling bodies are moved rather than rewritten, and no new untrusted-input parsing or auth/permission surfaces are touched.
Level of scrutiny
This warrants a careful human review. Although framed as pure code motion, several extractions are non-mechanical: resolve_esm_exports rewrites two sequential blocks as a retry loop, configure_and_dispatch_parse_task must match every prior call site's field initialization (and now unconditionally clears io_task.node.next, which two of the original sites did not), the string escaper is the hot path for all JS output, and the PR has been rebased twice over conflicting changes (#32116 standalone sourcemaps, #32507 AST shrink) with hand-resolved merges. The author's verification section is thorough and CodeRabbit's three findings were all correctly rebutted as pre-existing behavior, but the equivalence claims rest on per-field audits that a maintainer should spot-check.
Other factors
The bug-hunting system found nothing, new tests pin the consolidated paths (exports-map .js retry, linked-sourcemap publicPath, UTF-16 $\{ escaping), and the relevant bundler suites pass on the rebased head. All review threads are resolved. Still, the breadth (bundler dispatch + resolver + printer + linker output) and the manual rebase resolutions push this out of auto-approve territory.
|
On the one concrete point in the claude bot review ("unconditionally clears |
89759f1 to
2a448a0
Compare
2a448a0 to
757a2b7
Compare
86c32eb to
a4ef5e4
Compare
There was a problem hiding this comment.
No bugs found, but this is a large behavior-preserving refactor (~900 lines removed across bundler dispatch, resolver fs, js_printer, and standalone-graph) that has been rebased over six conflicting mainline changes — worth a maintainer's eyes on the equivalence claims.
What was reviewed:
configure_and_dispatch_parse_taskagainst all five pre-PR inline sites — the addedio_task.node.next = nullis a no-op at the two sites that omitted it (Node::default()already nulls it), andis_entry_pointdefaults tofalsewhere previously unset.resolve_esm_exportsloop: the retry guard (resolve_subpath.len() < subpath.len() || ext != ".js" || len <= 3) reproduces the original two-block sequence, andhandle_esm_resolutionstill receives the originalsubpathon the retry.- The moved
write_pre_quoted_string_innerinbun_core::printermatches the deleted js_printer body byte-for-byte on the escape arms; the UTF-16${lookahead and surrogate handling are unchanged. standalone_placeholder_output_file/OutputFileInit::default()— confirmed the elided fields (hash,input_path,display_size, etc.) default to the same values the explicit literals set.
Extended reasoning...
Overview
This PR deduplicates ~900 lines across four subsystems: (1) the bundler's parse-task enqueue sites collapse into configure_and_dispatch_parse_task; (2) the bundler-local cache::Fs is deleted in favor of the re-exported bun_resolver::cache::Fs; (3) linker output helpers (append_linked_sourcemap_url, generate_chunk_bytecode, standalone_placeholder_output_file) are extracted and shared between generateChunksInParallel and writeOutputFilesToDisk; (4) the const-generic string escaper moves from js_printer into bun_core::printer with the runtime-encoding wrapper becoming a dispatcher; (5) resolver/fs.rs extracts kind_impl with injected closures so the inline lib.rs RealFS can delegate to it, and ModKey::from_file / adjust_ulimit / temp-dir helpers become thin forwards; (6) resolve_esm_exports folds the two exports-map resolution passes into a loop; (7) StandaloneModuleGraph::from_executable unifies three per-platform copies of the trailer/offset validation. Three new tests pin the consolidated paths.
Security risks
None introduced. The standalone-graph consolidation touches unsafe pointer arithmetic reading the embedded module trailer, but the unified body is byte-identical to each of the three per-platform copies it replaces (same bounds check, same read_unaligned, same SAFETY comments). The resolver kind_impl extraction preserves the fd close-or-store scopeguard and the Windows reparse-point handling verbatim. No new user-controlled input paths, auth, or crypto surfaces.
Level of scrutiny
High. This touches the bundler's parse-task scheduling (correctness here affects every build), the resolver's filesystem cache (fd lifecycle, symlink resolution), the JS printer's hot escape loop, and the standalone executable loader. Each helper extraction is claimed behavior-preserving, but the equivalence depends on struct defaults (ParseTask::init, OutputFileInit::default(), Node::default()) matching the elided explicit assignments, and on the resolve_esm_exports loop's exit condition reproducing the original two-block control flow exactly. The PR has also been rebased six times over conflicting changes (#32116, #32507, #33035, #33072, #33909, plus the AST-shrink and BOM-dedup), and each resolution is a place a subtle divergence could slip in. A maintainer familiar with the pre-PR shape of these files should confirm the field-by-field equivalence the PR description claims.
Other factors
The bug-hunting pass found nothing. All three CodeRabbit findings were correctly refuted as pre-existing behavior (the out.module_type overwrite, the global-cache condition-set divergence, and the post-dispatch loader bookkeeping all match main). The prior concern about the unconditional io_task.node.next clear was resolved — ThreadPoolLib::Node is #[derive(Default)] so the two sites that didn't clear it explicitly already had it null. New tests cover the exports-map .js retry (both positive and the .mjs-doesn't-retry negative), the linked-sourcemap publicPath prefix, and the UTF-16 template-literal \${ escape. The relevant suites (bundler_string, bundler_loader, bundler_edgecase, bun-build-api, resolver-cache) are reported passing on the rebased head. Given the breadth and the number of rebase resolutions, this should not be auto-approved.
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
|
On it. Quick findings so far: main is ~3000 PRs past this branch's base and the rebase conflicts in 7 files, largely because #35002 (dead-code sweep) deleted ~1400 lines from resolver/fs.rs and ~1100 from js_printer/lib.rs. One of the seven dedups here (the bundler-local cache::Fs) already landed on main via that sweep; the other six still appear absent from main. Auditing each piece against current main now to decide between salvaging the still-relevant pieces and closing; will report back shortly. |
…-loader ParseResult construction Five parse-task enqueue sites repeated the same tail (scheduling fields, onLoad plugin hand-off, copy-for-bundling bookkeeping, pool schedule); fold it into configure_and_dispatch_parse_task. The linked-sourcemap trailer, bytecode generation, and standalone placeholder output file were each written out twice across generateChunksInParallel and writeOutputFilesToDisk; share one helper per concern, and drop the write-only fdpath in generateChunksInParallel. The text/md/data/wasm loaders built identical export-default statements and ParseResult literals; route them through export_default_parse_result and ParseResult::with_ast. No behavior change: the helpers set exactly the fields the inline copies set, and the elided fields match ParseTask::init / Default and OutputFileInit::default(). Co-authored-by: Alistair Smith <hi@alistair.sh>
a4ef5e4 to
337d0e4
Compare
robobun
changed the title
|
@alii Mergeable now. Main had moved ~3000 PRs, so instead of rebasing I audited each of the seven original pieces against current main and rebuilt the branch on top of it:
Now five focused commits on main, net -234 lines, co-authored to you. Workspace check / fmt / clippy clean, the bundler, build-api, edgecase, loader, plugin, standalone, and compile suites pass locally (the compile suite's one failure is a pre-existing debug-build version-string check, same as on main). PR title and body rewritten for the reduced scope. Format and clippy are already green on the new head; Buildkite build 91671 is running. |
bun_core::printer and bun_js_printer each carried a full copy of the quoted-string escape loop; the BEL/VT JSON fix had to be applied to both. Make the bun_core copy canonical: it gains the const-generic Encoding parameter and write_pre_quoted_string_inner (the body is the current js_printer loop, including the BEL/VT handling), the runtime write_pre_quoted_string becomes a dispatcher over it, and quote_for_json adopts the pre-reservation heuristic. js_printer re-exports Encoding, write_pre_quoted_string_inner, and quote_for_json; its const facade maps the bun_core error into the crate error. Co-authored-by: Alistair Smith <hi@alistair.sh>
…exports The node_modules exports-map resolution was two sequential copies of build-ESModule / resolve / handle_esm_resolution, the second with the .js extension stripped. Express it as one loop; handle_esm_resolution still receives the original subpath on both attempts and the success side effects are unchanged. Co-authored-by: Alistair Smith <hi@alistair.sh>
from_executable repeated the size check, trailer compare, and offsets read once per platform; only the get_data() source differs, so select that under cfg and validate once. Co-authored-by: Alistair Smith <hi@alistair.sh>
… and UTF-16 dollar-brace escaping Co-authored-by: Alistair Smith <hi@alistair.sh>
337d0e4 to
8479ae5
Compare
create_format already hands back a +1 reference, and bytecode generation only borrows the string (both C++ entry points call toWTFString on it, and NodeCompileCache pairs clone_utf8 directly with deref). The extra ref_() before wrapping in OwnedString therefore left every chunk's URL string at refcount 1 after the OwnedString dropped. All three original inline sites had the same extra ref_(); now that they share one helper, let OwnedString adopt the constructor's reference directly.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the pre-existing source_provider_url leak flagged in the previous run is now fixed in bfe5ec9). Because it's a wide behavior-preserving refactor across the bundler dispatch path, the linker output glue, the hot-path JS string escaper, the exports-map resolver, and the standalone loader — plus one intentional refcount change — a human look would still be worthwhile.
What was reviewed
resolve_esm_exportsloop: traced both iterations — second attempt strips.jsand passes the originalsubpathtohandle_esm_resolution, andresolve_subpath.len() < subpath.len()terminates after the retry, matching the original two sequential blocks;out.module_type = *module_typepreserves main's existing assignment.configure_and_dispatch_parse_task: the unconditionalis_entry_point/io_task.node.next = nullwrites are no-ops at the sites that omitted them (perParseTask::init/Node::default()).standalone_placeholder_output_file: elided fields checked againstOutputFileInit::default(); both original call sites' explicit values match.- String escaper: the moved
write_pre_quoted_string_innerbody is byte-identical to the deleted js_printer copy; the runtime-encoding wrapper now dispatches to it, andquote_for_jsonpicks up thegrow_if_neededheuristic that only the js_printer copy had.
Extended reasoning...
Overview
This PR consolidates duplicated code across five subsystems: (1) five parse-task enqueue tails in bundle_v2.rs into configure_and_dispatch_parse_task; (2) linked-sourcemap trailer, bytecode generation, and standalone placeholder output-file construction shared between generateChunksInParallel.rs and writeOutputFilesToDisk.rs; (3) text/md/data/wasm loader export default boilerplate in transpiler.rs; (4) the ~220-line quoted-string escape loop, moved from js_printer into bun_core::printer as canonical with js_printer re-exporting; (5) the two sequential exports-map resolution attempts in resolver.rs into a single loop; (6) three per-platform copies of the standalone trailer validation into one cfg-selected block. Net -236 lines. There is one intentional behavior change in its own commit: dropping an extra .ref_() on the source-provider URL in generate_chunk_bytecode, fixing a per-chunk WTF::StringImpl leak that all three original inline sites had (this was my finding from the previous run, now applied). Three tests are added pinning the consolidated paths.
Security risks
None identified. The changes are internal refactors with no new input parsing, no auth/crypto/permissions surface, and no new FFI signatures. The standalone trailer validation is unchanged in substance (same length check, same trailer comparison, same read_unaligned of Offsets); only the per-platform get_data() selection is factored out.
Level of scrutiny
High. Every touched path is production-critical: parse-task dispatch and copy-for-bundling bookkeeping affect every bundled file; the string escaper is an explicitly hot path used for every string literal printed; resolve_esm_exports gates package exports resolution; the linker helpers shape every chunk's emitted bytes and bytecode. REVIEW.md's "treat every refactor as guilty until proven behavior-preserving" applies directly. The PR description does provide per-helper equivalence arguments, and the CodeRabbit findings on this PR were all correctly rebutted as pre-existing behavior — but a maintainer should confirm the equivalence claims independently, particularly the OutputFileInit::default() field elisions and the string-escaper move (the bun_core quote_for_json now picks up a grow_if_needed pre-reservation it didn't have before, which is a small allocation-pattern change even if output-identical).
Other factors
- The refcount fix passed the debug-build bytecode/compile tests per the author, which would trip WTF refcount assertions on an over-deref, and matches the
NodeCompileCachereference pattern. - All prior review threads (CodeRabbit ×3, my leak finding) are resolved with no outstanding requests.
- Tests were added for the
.jsretry (positive and negative), linked-sourcemappublicPath, and the UTF-16${escape — good coverage of the consolidated edge cases, though theconfigure_and_dispatch_parse_taskand standalone-trailer consolidations have no new tests (they rely on existing suites). - The PR was rebuilt on current main after ~3000 intervening PRs; two of the original seven pieces were dropped as moot. The rebuild is well-documented but adds surface area for a human to verify against the current base.
Given the breadth across critical paths and the one intentional memory-lifetime change, this exceeds the "simple, mechanical, or obvious" bar for auto-approval.
There was a problem hiding this comment.
The refcount fix in bfe5ec9 addresses my earlier finding — OwnedString now adopts create_format's +1 directly, matching the NodeCompileCache pattern. This pass on the merged head found no bugs. Given the scope (five parse-task dispatch sites, the hot-path string escaper move to bun_core, the resolver exports-map loop reshaping, standalone trailer consolidation, plus the one intentional refcount behavior change), a human sign-off is still worthwhile.
What was reviewed:
configure_and_dispatch_parse_taskagainst all five original sites — the extrais_entry_point = false/io_task.node.next = nullare no-ops viaParseTask::init/Node::default().resolve_esm_exportsloop: second iteration passes the originalsubpathtohandle_esm_resolutionand only strips.js, matching the pre-PR inline paths; termination guaranteed by theresolve_subpath.len() < subpath.len()guard.- The moved escaper body is byte-identical to the js_printer copy (including the #36746 BEL/VT arms and the
${UTF-16 lookahead);quote_for_json's new pre-reserve is additive. standalone_placeholder_output_fileelides only fields equal toOutputFileInit::default().
Extended reasoning...
Overview
Behavior-preserving deduplication across five subsystems: bundler parse-task dispatch (bundle_v2.rs, five sites → one helper), linker output glue (generateChunksInParallel.rs / writeOutputFilesToDisk.rs, three shared helpers), transpiler loader helpers (transpiler.rs), the JS string escaper (bun_core/string/mod.rs becomes canonical, js_printer/lib.rs re-exports), the resolver exports-map .js retry (resolver.rs, two sequential attempts → one loop), and the standalone trailer validator (StandaloneModuleGraph.rs, three cfg blocks → one). Net -236 lines. Three new tests pin the consolidated paths. One intentional behavior change in its own commit (bfe5ec9): dropping the extra .ref_() in generate_chunk_bytecode, which I flagged in the prior review pass and which robobun confirmed and fixed.
Security risks
None identified. No auth, crypto, or permission code touched. The standalone trailer validation is unchanged in substance (same length check, same read_unaligned, same TRAILER comparison) — only the per-platform get_data() selection moved under cfg. The resolver change does not alter which paths are accepted, only how the two attempts are expressed.
Level of scrutiny
High. Every touched file is on a hot or correctness-critical path: the string escaper runs on every printed string literal, the resolver on every bare-specifier import with an exports map, parse-task dispatch on every bundled file, and from_executable on every compiled-binary startup. The PR is framed as behavior-preserving, and REVIEW.md's guidance is to treat refactors as guilty until proven so — which is why I traced each helper against its original sites rather than trusting the diff shape. The escaper move also has a knock-on effect on existing bun_core::quote_for_json callers (sourcemap chunks, macros): they now get the const-generic monomorphized loop and a ~12.5% pre-reserve, which the PR body calls out and which is output-identical.
Other factors
My prior review (2026-08-10) found the source_provider_url refcount leak; it was fixed in bfe5ec9 and verified under the debug build's WTF refcount assertions. Since then the only change is a merge from main (c0e4c51). All CodeRabbit threads are resolved (each was a pre-existing behavior, not a PR-introduced regression). The PR body's per-helper equivalence notes check out against the diff. Bundler/build-api/edgecase/loader/plugin/standalone/compile suites reported green locally. Given the breadth across five critical subsystems and the one deliberate behavior change, deferring to a human maintainer for final sign-off rather than auto-approving.
2 participants
What this does
Behavior-preserving dedup in the bundler, the JS string escaper, the resolver's exports-map retry, and the standalone loader. Net -236 lines. Rebuilt on current main (see below), as six commits:
bundle_v2.rs,linker_context/,transpiler.rs): five parse-task enqueue sites shared an identical tail (scheduling fields, onLoad plugin hand-off, copy-for-bundling bookkeeping, pool schedule), nowconfigure_and_dispatch_parse_task. The linked-sourcemap trailer, bytecode generation, and standalone placeholder output file were each written twice acrossgenerateChunksInParallelandwriteOutputFilesToDisk, now one helper each (also drops a write-onlyfdpath). The text/md/data/wasm loaders built identical export-default statements andParseResultliterals, nowexport_default_parse_result/ParseResult::with_ast.bun_core/string/mod.rs,js_printer/lib.rs):bun_core::printerandbun_js_printereach carried a full copy of the quoted-string escape loop; printer: emit \u escapes for BEL/VT when quoting for JSON #36746 (BEL/VT in JSON) had to patch both. The bun_core copy is now canonical, gaining the const-genericEncodingandwrite_pre_quoted_string_inner(body taken from current js_printer, so it carries the printer: emit \u escapes for BEL/VT when quoting for JSON #36746 fix), with the runtime entry point dispatching over it; js_printer re-exports. Two knock-on effects for the existingbun_core::quote_for_jsoncallers (sourcemap chunks, macros): it now pre-reserves ~12.5% slack up front like the js_printer copy did (same output, one fewer regrowth on typical input), and the runtime entry point is monomorphized once per encoding instead of branching on encoding inside the loop.resolver.rs): the two sequential exports-map attempts inload_node_modules(plain, then with.jsstripped) become one loop inresolve_esm_exports.handle_esm_resolutionis called unchanged, so resolver: auto-resolve extensions for wildcard exports/imports targets #36299's wildcard extension probing is unaffected.StandaloneModuleGraph.rs):from_executablevalidated the trailer once per platform; onlyget_data()differs, so it is selected under cfg and validated once.generate_chunk_bytecodeno longerref_()s the source provider URL before wrapping it inOwnedString.create_formatalready returns +1 and bytecode generation only borrows the string (both C++ entry points usetoWTFString;NodeCompileCachepairsclone_utf8directly withderef), so the extra ref leaked oneWTF::StringImplper bytecode chunk. All three original inline sites had it; consolidating them made it a one-line fix. Flagged by review..jsretry (and that.mjsdoes not retry), linked sourcemap withpublicPath, and the UTF-16${escape.Salvage notes
Main moved ~3000 PRs past the original base, so rather than rebase the branch was rebuilt on main after auditing each piece of the original against current main:
cache::Fsremoval and the resolverkind_impl/ModKey::from_file/ temp-dir delegation. Remove ~39k lines of dead Rust across the workspace #35002 (dead-code sweep) deleted the bundlercache::Fsand the entire secondRealFS, so that duplication no longer exists and there is nothing left to consolidate.transpiler.rshelpers lose theinput_fdparameter removed by transpiler: remove write-only ParseResult::input_fd plumbing #37071; the escaper move uses the post-printer: emit \u escapes for BEL/VT when quoting for JSON #36746 loop body and leavescan_print_without_escapeprivate (its only remaining caller is the loop itself, per Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184);generateChunksInParallel.rsdrops its duplicateBYTECODE_EXTENSIONin favor of the shared one.resolve_esm_exports, and the standalone consolidation apply to main as written; the code they replace is byte-identical to the original base.Verification
configure_and_dispatch_parse_tasksets exactly what each of the five sites set (the extrais_entry_point = false/io_task.node.next = nullat three sites are no-ops viaParseTask::init/Node::default());standalone_placeholder_output_fileelides only fields equal toOutputFileInit::default();resolve_esm_exportspasses the original subpath tohandle_esm_resolutionon both attempts as before; the moved escaper is the current js_printer body verbatim.cargo check --workspace, rustfmt, and clippy clean on all touched crates.bundler_string(60),bun-build-api(53, includes printer: emit \u escapes for BEL/VT when quoting for JSON #36746's BEL/VT test and the new publicPath test),bundler_edgecase(136),bundler_loader(50, includes the new.xmlloader arm),bundler_plugin(53),standalone(23),bundler_compile(60 of 61; the one failure,HelloWorldWithProcessVersionsBun, is a version-string check that fails identically on main's debug build and does not touchfrom_executable).no test proof · iteration 23 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts test/bundler/bundler_edgecase.test.ts