Repository navigation
bundler: name the entry point flag at every construction and drop the unread is_html bit - #38819
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes 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 (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical refactor that names the is_entry_point flag at each construction and drops the write-only is_html bit.
What was reviewed:
- Verified all eight
EntryPoint::newcall sites were converted and each preserves its prioris_entry_pointvalue; no stragglers in the tree. - Grepped
src/for any reader of the removedis_htmlbit or anis_html()accessor onEntryPoint— none exist; every "is HTML chunk" check goes throughchunk.content. - Checked bit layout:
is_entry_pointstays at bit 62,set_entry_point_idand the other accessors are untouched and still consistent.
Extended reasoning...
Overview
This PR replaces chunk::EntryPoint::new(source_index, entry_point_id, is_entry_point: bool, is_html: bool) with two named constructors, EntryPoint::entry_point(si, id) and EntryPoint::non_entry_point(si, id), and drops the is_html bit (bit 63) from the packed u64. All eight call sites across src/bundler/bundle_v2.rs and src/bundler/linker_context/computeChunks.rs are updated. The is_entry_point() accessor is rewritten as self.0 & IS_ENTRY_POINT_BIT != 0, which is semantically identical to the old (self.0 >> 62) & 1 != 0.
Security risks
None. This is an internal bundler data-structure refactor with no user-facing surface, no parsing of untrusted input, and no change to any security-relevant path.
Level of scrutiny
Low-to-medium. The change is mechanical: two boolean positional args become named constructors, and a field that nothing reads is deleted. The only correctness question is whether every call site's is_entry_point value survived the rewrite unchanged — I checked each of the eight against the diff and they all match (the four true, false sites in computeChunks → entry_point; the code-split false, false → non_entry_point; bundle_v2's true, false JS chunk → entry_point, and its two false, * CSS/HTML dev-server chunks → non_entry_point). The one site that previously set is_html = true (bundle_v2 HTML dev-server chunk) had is_entry_point = false, so it correctly maps to non_entry_point, and the dropped is_html bit was never read anywhere.
Other factors
- Grep confirms no remaining
EntryPoint::newcallers and nois_htmlreader onEntryPointanywhere insrc/; the hits foris_htmlare all unrelated locals (main_is_html_entrypoint, markdown parser, HTMLBundle). - The packed layout is otherwise unchanged:
source_indexin bits 0..32,entry_point_idin bits 32..62,is_entry_pointat bit 62.set_entry_point_idstill masks the same range and does not touch bit 62. - The PR description states bundler_html, html-import-manifest, and bake dev tests pass on the debug build, and the repo's REVIEW.md "delete dead code in the same PR that makes it dead" guideline is exactly what this change follows.
- No prior human or bot review comments to address; CodeRabbit was rate-limited and left no findings.
…enums (#39166) ### Problem - `IntermediateOutput::code()` and `code_standalone()` in `src/bundler/Chunk.rs` take `force_absolute_path: bool` and `enable_source_map_shifts: bool`. Of their seven callers, four pass bare literals for both flags (`code_standalone(.., false, false, ..)` in `writeOutputFilesToDisk.rs` and `generateChunksInParallel.rs`, `code(.., false, false)` in `MetafileBuilder.rs` and `bake/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. - This is the `bare_bool_args:src/bundler/Chunk.rs` finding recorded in `mordant-baseline.toml`. ### Fix - Each flag becomes a two-variant enum next to `CodeResult`: `ReferencePathStyle::{ImporterRelative, OutdirRelative}` replaces `force_absolute_path` (`false` / `true`) and `SourceMapShiftTracking::{Disabled, Enabled}` replaces `enable_source_map_shifts`. Two constructors hold the derivations the callers used to spell out: `ReferencePathStyle::for_chunk(chunk, compile)` (the `compile && !IS_BROWSER_CHUNK_FROM_SERVER_BUILD` test both output loops had inline) and `SourceMapShiftTracking::for_source_map(option)` (the `!= SourceMapOption::None` test three callers had). - `code()` / `code_standalone()` match on `SourceMapShiftTracking` to pick the `code_with_source_map_shifts::<true / false>` instantiation, exactly as the `if` on the bool did, and pass `ReferencePathStyle` through 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. - Deletes the baseline entry. With `-p bun_bundler`, `MORDANT_BASELINE_WRITE=1` writes a `[bun_bundler]` section identical to this edit. - Tests, in the files that already cover these paths, pin the value each caller selects. They pass before and after this change (it is a refactor) and were checked against a build with the mappings deliberately inverted, where all of them fail while the pre-existing tests around them still pass: - `test/bundler/bundler_splitting.test.ts` (`splitting/ChunkReferencePaths`, the two `code()` output loops): entry points in nested directories import the shared chunk as `../../chunk-*.js` and each other as `../site/index.js`, or as `publicPath + outdir path` when 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 three `code_standalone()` callers): without source maps nothing map related appears in the document; with them, the whitespace-minified inlined script maps `function greet` back 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. - Other verification: - `cargo dylint --all -p bun_bundler` with the entry removed: clean on this branch; with the old `src/` restored it reports exactly this finding over the baseline (`over-baseline.txt`: `bun_bundler 1`). - Byte-for-byte comparison of this build's output against the installed release bun (main from 65 commits back; the two commits since then that touch these files, #38819 and #38604, rename a flag and change thread pool waiting, not output) over 48 output files: nested entry points with a shared chunk and an asset with no / linked / external / inline source maps and with a `--public-path`, a root-level entry, standalone HTML in every source map mode and with a public path, and a metafile. All identical, including `.map` mappings and `//# debugId` comments. - `bun bd test` on `bundler_splitting`, `bundler_compile_splitting`, `standalone`, `metafile`, `bundler_html`, `bundler_naming`, `bundler_files`, `test/js/bun/sourcemap/` and `test/bake/dev/css.test.ts`: pass. `bun build --compile` with and without `--sourcemap` (the `OutdirRelative` + `Enabled` combination) was checked by hand: `/$bunfs/root/...` frames without a map, original file names with one; `bun-build-compile-sourcemap.test.ts` asserts 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-deps` is clean. ### Background - After the bundler prints a chunk, references to other output files (imports of sibling chunks, asset URLs, server component boundaries) are still placeholders; `IntermediateOutput::code()` replaces them with real paths once every output path is known. `force_absolute_path` chose 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 pass `false`. - A source map shift records how much longer or shorter each substituted path is than its placeholder. The chunk's source map was generated against the placeholder text, so `output_source_map.finalize(&shifts)` needs them for any chunk that gets a map; `enable_source_map_shifts` turned that bookkeeping (and the `//# debugId=` comment) on. HTML chunks, CSS and the metafile never get a map, hence their literal `false`. - mordant is the advisory Rust lint pack CI runs; `mordant-baseline.toml` records the pre-existing findings per (lint, file) so only new ones are reported. Fixing a recorded finding lets its entry be deleted. - #37180 is a tree-wide pass over the same kind of parameter that also covers this site; it has been conflicting with main since it was opened. This PR is the one-site version, like the other `bare_bool_args` entries being retired individually. <details> <summary>Unrelated bugs noticed while writing the tests (not touched here, handed off separately)</summary> - `BuildArtifact.sourcemap` points at the next chunk instead of the chunk's map whenever a build has more than one chunk (`js_bundle_completion_task.rs` assumes 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`'s `snapshotSourceMap.mappings` check ends in `expect(a === b)` with no matcher, so it never compares against the map; the tests here use `SourceMapConsumer` directly. - `standalone.test.ts`'s two "falls back to normal compile" tests write an `app` executable into the test runner's cwd. </details>
1 participant
Chunk's EntryPoint was built with
new(source_index, entry_point_id, is_entry_point: bool, is_html: bool)and all eight call sites passed bare literals. The two HTML-chunk sites disagreed through exactly those slots: computeChunks passedtrue, false, the dev server path in bundle_v2 passedfalse, true, where the Zig set only.is_entry_pointat both and never touchedis_htmlanywhere. Looking for what that straytruedid turned up that nothing readsis_htmlat all, in Rust or in the Zig before it: every "is this chunk HTML" decision goes throughchunk.content.So the bit goes away (bit 63 documented as spare) and the constructor becomes
EntryPoint::entry_point(si, id)/EntryPoint::non_entry_point(si, id), so each site says by name what it sets and there is nothing left to transpose.is_entry_pointkeeps its position and value at every site.No behavior change. bundler_html, html-import-manifest and the bake dev html/css/esm tests pass on the debug build; a split HTML build produces the same files and metafile as before.