bundler: replace IntermediateOutput::code's two bool parameters with enums - #39166
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Status: ready for review. Refactor only, requested in chat for the
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change; the inline nit about a ReferencePathStyle::for_chunk helper is optional tidy-up, not blocking.
Extended reasoning...
Overview
This PR replaces the two trailing bool parameters of IntermediateOutput::code() / code_standalone() in src/bundler/Chunk.rs with two-variant enums ReferencePathStyle::{ImporterRelative, OutdirRelative} and SourceMapShiftTracking::{Disabled, Enabled}, adds SourceMapShiftTracking::for_source_map() to factor the shared != SourceMapOption::None test, updates all seven callers across MetafileBuilder.rs, generateChunksInParallel.rs, writeOutputFilesToDisk.rs, and bake/DevServer.rs, and deletes the corresponding bare_bool_args:src/bundler/Chunk.rs entry from mordant-baseline.toml.
Security risks
None. This is a type-only signature change in bundler output-assembly code; no untrusted input handling, no allocation/bounds logic, no FFI or lifetime changes are touched.
Level of scrutiny
Low-to-medium. The change is a mechanical bool→enum substitution whose correctness is fully checkable by mapping each caller's old value to the new variant. I traced every call site: every previous false for force_absolute_path becomes ImporterRelative, the two compile && !IS_BROWSER_CHUNK_FROM_SERVER_BUILD expressions become the same condition selecting OutdirRelative/ImporterRelative, every previous false for enable_source_map_shifts becomes Disabled, and the three != SourceMapOption::None sites become for_source_map(..) (which returns Enabled iff the option is not None). Inside code_with_source_map_shifts, the single read of the old bool (|| force_absolute_path ||) becomes || reference_path_style == OutdirRelative ||, and the if/else on enable_source_map_shifts becomes an exhaustive match picking the same const-generic instantiation. No path changes semantics.
Other factors
The PR description records byte-for-byte output comparison against release Bun over 48 output files (nested outdir with shared chunk/asset, all sourcemap modes, public path, standalone HTML, metafile) and passing bundler/sourcemap/dev-server test suites, which is the right verification for a no-behavior-change refactor. The one inline finding is a style nit (the compile && !IS_BROWSER_CHUNK... → enum block now appears as an identical ~8-line block at two sites; a ReferencePathStyle::for_chunk helper would mirror the for_source_map pattern the PR already introduces). That duplication existed before as inline bool expressions and doesn't affect correctness, so it's fine to land as-is or fold in.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flag: #37180 is the tree-wide version of this (a |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed two follow-ups since the first round of review:
The description is updated to match, including the unrelated bugs the tests ran into (tracked separately, not touched here). |
There was a problem hiding this comment.
LGTM — the for_chunk helper and shortened enum docs address the earlier feedback, and every caller's old bool value maps 1:1 onto the new enum variant.
What was reviewed:
- All seven
code()/code_standalone()call sites: each oldforce_absolute_path/enable_source_map_shiftsexpression maps to the identical variant viafor_chunk/for_source_mapor a literal. - The one read site in
code_with_source_map_shifts(use_outdir_relative_path):== OutdirRelativereplaces the old bool with the same truth table. - Imports left behind (
ChunkFlags,SourceMapOption) are still used elsewhere in their files. - New tests use
tempDir/describe.concurrent, look maps up by path (sidestepping theBuildArtifact.sourcemapbug noted in the description), and assert source-map columns viaSourceMapConsumerdirectly.
Extended reasoning...
Overview
This PR replaces two bool parameters on IntermediateOutput::code() and code_standalone() in src/bundler/Chunk.rs with two-variant enums (ReferencePathStyle, SourceMapShiftTracking) and two constructor helpers (for_chunk, for_source_map) that hold the derivations previously spelled out at call sites. Seven callers across MetafileBuilder.rs, generateChunksInParallel.rs, writeOutputFilesToDisk.rs, and bake/DevServer.rs are updated. The single consumer of the old force_absolute_path bool inside code_with_source_map_shifts now compares against ReferencePathStyle::OutdirRelative. The match shift_tracking picks the same const-generic instantiation the old if enable_source_map_shifts did. The corresponding mordant-baseline.toml entry is removed. Tests are added in three existing files that pin the value each caller now selects.
Security risks
None. This is a type-level refactor of two boolean flags into enums in the bundler's chunk-output-path resolution step. No user input parsing, no auth, no crypto, no filesystem path handling changes.
Level of scrutiny
Low-to-medium. The change is mechanical (bool → two-variant enum) and the PR description documents byte-for-byte output equivalence against a release build across 48 output files covering every source-map mode, public-path configuration, and standalone/outdir combination. I traced each of the seven callers' old expressions against the new enum/constructor and confirmed they compute the same value in every case. The one internal read site (use_outdir_relative_path) has an identical truth table. No control-flow, allocation, or lifetime changes.
Other factors
- My earlier inline comment (extract
ReferencePathStyle::for_chunkto deduplicate the two output loops) was addressed in aab5016; the comment-cop doc-length flags were addressed in e2a8451. All threads are resolved. - The added tests were verified (per the description) to fail on a build with the enum mappings inverted while surrounding tests still pass, satisfying the "test fails for the right reason" requirement for a refactor.
- The duplicate-PR flag against #37180 (a stalled tree-wide pass) is acknowledged in the description; this is the single-site version and does not conflict with it landing later.
- No CODEOWNERS or security-sensitive paths touched. No dead imports introduced (
ChunkFlagsandSourceMapOptionremain used at other sites in their files).
…with enums
`code()` and `code_standalone()` took `force_absolute_path: bool` and
`enable_source_map_shifts: bool`, and most callers passed bare literals, so
a call like `code_standalone(.., false, false, ..)` did not say which flag
was which. Each flag is now a two-variant enum defined next to `CodeResult`:
`ReferencePathStyle::{ImporterRelative, OutdirRelative}` and
`SourceMapShiftTracking::{Disabled, Enabled}`, the latter with a
`for_source_map(SourceMapOption)` constructor for the callers that derive
the flag from the chunk's source map setting.
Every caller maps its previous value onto the matching variant; the path
and source map shift logic inside `code_with_source_map_shifts` is
unchanged. Removes the now-fixed `bare_bool_args:src/bundler/Chunk.rs`
entry from mordant-baseline.toml.
…aller selects The compile-mode test that picks the path style was spelled out at both output loops; it now lives next to the enum, mirroring for_source_map. The tests cover the values the callers pass: nested entry points get importer-relative chunk paths (outdir-relative behind a public path), a chunk's source map is shifted by the paths and data: URIs written over its placeholders and gets a debugId only when a map is emitted, the standalone HTML document and dev server stylesheets get nothing appended, and the on-disk and in-memory output loops agree.
e2a8451 to
eccf321
Compare
|
Rebased onto main (no code changes; same three commits). #39119 had removed the neighbouring |
There was a problem hiding this comment.
LGTM — the for_chunk helper landed and the doc comments were trimmed; all seven call sites map one-to-one onto the previous bool values.
What was reviewed:
- Traced every
code()/code_standalone()caller: each literalfalse/false→ImporterRelative/Disabled, each computed pair →for_chunk/for_source_mapreproducing the same expression. - The one read site of the old
force_absolute_path(use_outdir_relative_path) now compares againstOutdirRelative, and thematch shift_trackingpicks the same const-generic instantiation theifdid. - New tests use
tempDir/describe.concurrent, look up maps by path (sidestepping the notedBuildArtifact.sourcemapbug), and assert columns viaSourceMapConsumerrather than the brokensnapshotSourceMaphelper.
Extended reasoning...
Overview
Replaces the two positional bool parameters on IntermediateOutput::code() / code_standalone() in src/bundler/Chunk.rs with two-variant enums ReferencePathStyle and SourceMapShiftTracking, plus for_chunk / for_source_map constructors that hold the derivations the two output loops previously spelled inline. All seven callers (MetafileBuilder.rs, two in writeOutputFilesToDisk.rs, three in generateChunksInParallel.rs, one in bake/DevServer.rs) are updated, the bare_bool_args:src/bundler/Chunk.rs mordant baseline entry is removed, and ~170 lines of tests across three existing test files pin what each caller selects.
Security risks
None. This is a type-level refactor of two flags in the bundler's chunk-assembly step; no parsing of untrusted input, no auth/crypto/permissions, no new I/O paths.
Level of scrutiny
Medium — bundler output correctness matters, but the change is a mechanical bool→enum substitution with no branching logic added or removed. I traced each of the seven call sites against the diff: every previous false maps to ImporterRelative/Disabled, every previous true-producing expression maps to OutdirRelative/Enabled via the constructor that reproduces it exactly (compile && !IS_BROWSER_CHUNK_FROM_SERVER_BUILD, != SourceMapOption::None). Inside code_with_source_map_shifts, the single read of the old force_absolute_path becomes == ReferencePathStyle::OutdirRelative, and the dispatch match selects the same <true>/<false> monomorphization the old if did. The PR description reports byte-identical output against a pre-change binary over 48 files covering every source-map mode, public path, and standalone/outdir combination.
Other factors
My earlier inline suggestion (extract ReferencePathStyle::for_chunk to dedupe the two 8-line blocks) was applied in aab5016 and both call sites now use it. The comment-cop bot flagged the new enum doc comments; the author shortened them in e2a8451 and the remaining three are 2–3 lines of genuine "what does this variant select" documentation, not workaround justification — reasonable to keep. All review threads are resolved. The added tests follow harness conventions (tempDir, describe.concurrent, no network, SourceMapConsumer for column checks) and the description states they fail on a build with the mappings inverted while surrounding tests still pass, which is the right shape for pinning a refactor. The duplicate-PR flag (#37180, tree-wide) is acknowledged in the description; this PR is the single-site retirement of one baseline entry, consistent with how other bare_bool_args findings are being cleared.
2 participants
Problem
IntermediateOutput::code()andcode_standalone()insrc/bundler/Chunk.rstakeforce_absolute_path: boolandenable_source_map_shifts: bool. Of their seven callers, four pass bare literals for both flags (code_standalone(.., false, false, ..)inwriteOutputFilesToDisk.rsandgenerateChunksInParallel.rs,code(.., false, false)inMetafileBuilder.rsandbake/DevServer.rs) and a fifth for one of them, so at the call site nothing says which flag is which; the metafile caller compensates with trailing comments.bare_bool_args:src/bundler/Chunk.rsfinding recorded inmordant-baseline.toml.Fix
CodeResult:ReferencePathStyle::{ImporterRelative, OutdirRelative}replacesforce_absolute_path(false/true) andSourceMapShiftTracking::{Disabled, Enabled}replacesenable_source_map_shifts. Two constructors hold the derivations the callers used to spell out:ReferencePathStyle::for_chunk(chunk, compile)(thecompile && !IS_BROWSER_CHUNK_FROM_SERVER_BUILDtest both output loops had inline) andSourceMapShiftTracking::for_source_map(option)(the!= SourceMapOption::Nonetest three callers had).code()/code_standalone()match onSourceMapShiftTrackingto pick thecode_with_source_map_shifts::<true / false>instantiation, exactly as theifon the bool did, and passReferencePathStylethrough to the one place the old bool was read (use_outdir_relative_path). Every caller maps its previous value onto the matching variant. No behavior change.-p bun_bundler,MORDANT_BASELINE_WRITE=1writes a[bun_bundler]section identical to this edit.test/bundler/bundler_splitting.test.ts(splitting/ChunkReferencePaths, the twocode()output loops): entry points in nested directories import the shared chunk as../../chunk-*.jsand each other as../site/index.js, or aspublicPath + outdir pathwhen a public path is set; a build without source maps has no//# debugId; with source maps,util("admin"), which follows the rewritten dynamic import on the same line, still maps to its source column, for both a shorter and a longer substituted path; the outdir and in-memory loops produce the same code and mappings.test/bundler/standalone.test.ts(sourcemaps, the threecode_standalone()callers): without source maps nothing map related appears in the document; with them, the whitespace-minified inlined script mapsfunction greetback to its source column across the data: URI written over the asset import, exactly one debugId is emitted and it is inside the script, and the document still ends at</html>; same through an outdir.test/bake/dev/css.test.ts(asset referenced in css, the DevServer caller): the served stylesheet has its asset reference resolved and no debugId trailer.cargo dylint --all -p bun_bundlerwith the entry removed: clean on this branch; with the oldsrc/restored it reports exactly this finding over the baseline (over-baseline.txt:bun_bundler 1).--public-path, a root-level entry, standalone HTML in every source map mode and with a public path, and a metafile. All identical, including.mapmappings and//# debugIdcomments.bun bd testonbundler_splitting,bundler_compile_splitting,standalone,metafile,bundler_html,bundler_naming,bundler_files,test/js/bun/sourcemap/andtest/bake/dev/css.test.ts: pass.bun build --compilewith and without--sourcemap(theOutdirRelative+Enabledcombination) was checked by hand:/$bunfs/root/...frames without a map, original file names with one;bun-build-compile-sourcemap.test.tsasserts the same but hits the 5 s default timeout under the ASAN debug build here, since each test copies the 830 MB debug binary.cargo clippy -p bun_bundler --no-depsis clean.Background
IntermediateOutput::code()replaces them with real paths once every output path is known.force_absolute_pathchose whether those paths are relative to the importing chunk's directory (default) or to the outdir (bun build --compile, whose chunks all load from one virtual root); a configured public path forces the outdir-relative form either way, which is why standalone HTML and the metafile could always passfalse.output_source_map.finalize(&shifts)needs them for any chunk that gets a map;enable_source_map_shiftsturned that bookkeeping (and the//# debugId=comment) on. HTML chunks, CSS and the metafile never get a map, hence their literalfalse.mordant-baseline.tomlrecords the pre-existing findings per (lint, file) so only new ones are reported. Fixing a recorded finding lets its entry be deleted.bare_bool_argsentries being retired individually.Unrelated bugs noticed while writing the tests (not touched here, handed off separately)
BuildArtifact.sourcemappoints at the next chunk instead of the chunk's map whenever a build has more than one chunk (js_bundle_completion_task.rsassumes the map follows its chunk in the output list; the list builder groups maps after all chunks). The tests here look maps up by path instead.Bun.build({ compile: true, target: "browser", sourcemap: "linked" })emits//# sourceMappingURL=/$bunfs/root/...(the executable public path leaks into standalone HTML builds; the CLI emits a relative URL). The tests here do not assert the URL.expectBundled'ssnapshotSourceMap.mappingscheck ends inexpect(a === b)with no matcher, so it never compares against the map; the tests here useSourceMapConsumerdirectly.standalone.test.ts's two "falls back to normal compile" tests write anappexecutable into the test runner's cwd.