Replace boolean flag parameters with two-variant enums - #37180
Replace boolean flag parameters with two-variant enums#37180dylan-conway wants to merge 4 commits into
Conversation
…t workspace
Call sites like `eql_long(a, b, true)`, `link(true)`, or
`GlobWalker::init(.., true, true, false, true, true)` don't say what the
booleans mean. This introduces `bun_core::bool_enum!`, which declares a
`Copy` two-variant enum (`Name::{No, Yes}` or domain-named variants, false
variant first and `Default`) with a `from_bool` constructor for runtime
values, and uses it to retype boolean parameters, mode-like fields, opaque
tuple slots and a few non-predicate `-> bool` returns throughout `src/`.
Call sites now read `CheckLen::Yes`, `Scope::Global`, `StopMode::Abrupt`,
`Chunk::Last`, etc.
Behaviour is unchanged. Left as `bool` on purpose: FFI/`repr(C)` and
codegen-dictated signatures, wire/lockfile layouts, const generics,
predicates (`is_*`/`has_*`), plain locals, named on/off option fields and
single-argument setters, and parameters where every caller passes the same
literal (the unused variant would be dead code).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. The changes I inspected (the bool_enum! macro, ResetOutcome, HttpScheme, ConnectionHeader, ModulePhase, Subdomains, IdentOrRefKind, Expanded, and the CheckLen/WsBefore/Chunk call-site rewrites) all preserve polarity correctly. That said, at 300 files and ~21k diff lines spanning TLS/proxy verification, the HTTP client, the package manager, and the parser/bundler, plus a new workspace-wide bool_enum! convention, a maintainer sign-off is warranted.
Extended reasoning...
Overview
This PR introduces bun_core::bool_enum!, a macro that declares a two-variant Copy enum (false-variant first, Default, from_bool, deliberately no Into<bool>), and applies it across ~300 files in the Rust workspace to replace positional bool parameters with named enums (CheckLen::Yes, Chunk::Last, TlsRole::Client, etc.). A handful of non-predicate -> bool returns are also retyped to ControlFlow, Result, or a named enum (ResetOutcome, Expanded). No behaviour change is intended.
Security risks
The refactor touches security-sensitive paths: boringssl hostname matching (Subdomains), TLS role and reject_unauthorized plumbing in uws/ssl_wrapper/WebSocketProxyTunnel, HTTP proxy selection (HttpScheme, AllowProxyUrl), HPACK never-index for sensitive headers, and package-install/lockfile code. A single polarity inversion in any of these could silently weaken a security check. I spot-checked several of the trickier variant orderings (HttpScheme { Https, Http } vs url.is_http(), Subdomains::from_bool(flags & SINGLE_LABEL_SUBDOMAINS != 0), RejectUnauthorized, ConnectionHeader { Close, KeepAlive }, ModulePhase { Evaluation, Defer } at the FFI boundary, ResetOutcome vs the old !retained test, IdentOrRefKind packing) and each preserves the original semantics — but I have not read every one of the ~300 files.
Level of scrutiny
High. The change is mechanical per-hunk, but the aggregate is a workspace-wide type-signature change across every major subsystem, including hot paths (parser, printer, resolver) and safety-critical ones (TLS, install). The primary failure mode — a swapped variant order or an inverted == Yes — would compile cleanly and might only surface as a subtle runtime divergence. The Rust type system catches missed call sites but not polarity errors. Per the repo's review guidance, new cross-cutting abstractions and workspace-wide conventions should have maintainer agreement.
Other factors
- No new tests are added; the PR description states behaviour is unchanged and verified by byte-identical output against the release binary on a scripted workload plus spot-running existing test files. That's reasonable for a pure refactor, but CI is the real gate here.
- The
bool_enum!macro itself is small and correct (false-variant = discriminant 0 =Default;from_bool(true)→ second variant). - The diff is far too large (~889k characters, truncated in my context) for me to have verified every hunk; I sampled the visible portion and the higher-risk semantic renamings.
- No prior review comments or CI results are visible on the timeline yet.
…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>
|
@robobun update |
|
Branch is current with main as of 8ed4565. History on top of dylan's commit 3d809d1: f4f845e (merge of 325 commits of main, 114 conflicted files, described below), 2ce8911 (records the Second merge (8ed4565), 4 conflicts: #39310 replaced diff-match-patch with a Myers diff, so How the first merge's conflicts were resolved:
Call sites added on main since the base that pass a bool to a function this PR retypes, fixed after the merge: Verification on the merged tree:
|
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it's a ~300-file mechanical refactor whose entire risk surface is polarity inversions the type system can't catch — and it touches TLS verification, the package manager, bundler, parser, and HTTP client — a human spot-check across a sample of hunks is still warranted before merge.
Checked the bool_enum! macro (false-variant first, from_bool maps true→second variant) and spot-checked polarity on ResetOutcome vs. ast_memory_allocator, HttpScheme in env_loader, Subdomains in boringssl, ConnectionHeader, and the ControlFlow conversion in HTMLScanner — all preserve the original branch direction.
Extended reasoning...
Overview
This PR introduces bun_core::bool_enum! and retypes boolean flag parameters, mode-like fields, opaque tuple slots, and a handful of non-predicate -> bool returns to two-variant enums across ~300 files spanning essentially every Rust crate in the workspace: bun_core, ast, bundler, js_parser/js_printer, css, http/http_jsc, install, jsc, resolver, react_compiler, shell, sql, runtime/api, bake, and more. The diff is ~888k characters. The stated invariant is that behaviour is unchanged.
Security risks
Several touched paths are security-sensitive: boringssl/lib.rs hostname matching (Subdomains), http/lib.rs check_server_identity / AllowProxyUrl, RejectUnauthorized in the WebSocket proxy tunnel, NeverIndex for HPACK sensitive headers, and reject_unauthorized plumbing in the SSL context cache. A polarity flip in any of these would fail open. I spot-checked Subdomains, HttpScheme, and AllowProxyUrl and they preserve the original branch direction, but I have not exhaustively verified every security-adjacent hunk in a diff this size.
Level of scrutiny
This is a pure refactor with no intended behaviour change, so per-hunk the review is mechanical: confirm the false-variant maps to what was previously false and that every == Yes / == SomeVariant / from_bool(...) preserves polarity. However, the type system cannot catch a swapped variant order or a == No that should have been == Yes, and there are hundreds of such conversions across critical subsystems. That combination — huge surface, silent-on-error failure mode, security-relevant paths — puts it well outside what I'm comfortable approving without a human sampling the diff.
Other factors
- No prior human review on the PR; the only timeline activity is a robobun merge-from-main.
- The PR ships no new tests (by design — behaviour is unchanged), so correctness rests on the existing suite plus the author's byte-identical output check.
cargo check/clippyclean on all targets guarantees no type-level mistakes but says nothing about polarity.- The bug-hunting system found nothing, and my own spot-checks (
ResetOutcome,HttpScheme,Subdomains,ConnectionHeader,ControlFlowinHTMLScanner,CloneText,Chunk) all check out — but that covers a small fraction of 300 files.
|
Updated 1:55 PM PT - Aug 16th, 2026
✅ @robobun, your commit 8ed4565321bbb35e3d0dfd875d6c8714812920e1 passed in 🧪 To try this PR locally: bunx bun-pr 37180That installs a local version of the PR into your bun-37180 --bun |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Given the scale (529 files, +7630/-4482, with 114 merge conflicts resolved against 325 commits of drift) and that several retyped flags sit on security-sensitive paths (RejectUnauthorized, AllowProxyUrl, TlsRole, cert verification in check_server_identity), a human sign-off is still warranted — both for the polarity audit and for the workspace-wide convention this introduces.
What was reviewed:
bool_enum!macro: false-variant is discriminant 0 andDefault,from_bool(true)→ second variant — consistent.- Spot-checked polarity on
HttpScheme { Https, Http }vsurl.is_http(),ResetOutcome::Recycledvs old!retained,Subdomains,ConnectionHeader,Chunk::{More,Last},CloneText { Adopt, Clone }— all match the original bool sense. ControlFlowconversion inHTMLScanner(on_*_tagreturningBreak→ stop) preserves the previoustrue → stopcontract.
Extended reasoning...
Overview
This PR introduces bun_core::bool_enum! and mechanically retypes positional bool parameters, mode-like fields, and a few non-predicate -> bool returns as two-variant enums across ~529 files in the Rust workspace. The visible diff (truncated at ~30%) covers ast, bundler, css, http/h2/h3, install, js_parser/printer, jsc, react_compiler, resolver, runtime, and more. A bot merge against main resolved 114 conflicting files and dropped 31 enum declarations superseded by types main had since introduced.
Security risks
Several retyped flags gate security decisions: RejectUnauthorized in the WebSocket proxy tunnel, AllowProxyUrl feeding check_server_identity and SNI selection, TlsRole::Client for the SSL wrapper, NeverIndex for HPACK sensitive headers, and HttpScheme for proxy env selection. Every one I traced preserves polarity, but a single inverted from_bool or swapped variant order in the ~70% of the diff I could not read would silently weaken TLS verification or flip a security default.
Level of scrutiny
High. Each hunk is trivial in isolation, but this is (a) a repo-wide code-style decision that maintainers should ratify, (b) an 888k-character diff far exceeding what automated review can exhaustively verify for polarity, and (c) freshly rebased over 325 commits with conflict resolution done by automation. The PR description's own verification is diligent (byte-identical output vs release, per-hunk polarity re-read, targeted test files) but is self-reported.
Other factors
No new tests are added — coverage relies entirely on existing suites, which is reasonable for a behaviour-preserving refactor but means any polarity flip in a rarely-exercised branch would not be caught. No human has reviewed yet; CI build #99254 is in progress. The bool_enum! macro itself is sound and the sampled call sites are correct, so I have no specific concern to flag — the deferral is purely about scale, security surface, and the architectural sign-off this warrants.
|
Two follow-ups on the merged head. Source lints. The only red check on f4f845e was Polarity. Both reviews above point at the same risk: a variant used the wrong way round is invisible to the compiler, and the merge re-applied a few hundred hunks by hand. So I checked the whole diff against main mechanically rather than by sampling:
The change on top of the original commit remains the conflict resolution described in my earlier comment plus the inventory line; nothing here alters the refactor itself, so the sign-off the reviews ask for is on the convention, which is dylan's call. |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; robobun's mechanical polarity reconstruction (2888 lines round-tripping to main, plus the 117 hand-checked non-reconstructing lines) also came back clean. Because this is a 529-file workspace-wide refactor that introduces a new bool_enum! convention, retypes flags on security-sensitive TLS/cert-verification paths (TlsRole, RejectUnauthorized, AllowProxyUrl, NeverIndex), and was hand-merged across 114 conflicting files, a maintainer sign-off on the convention itself is still warranted — as the last robobun comment already notes.
What was reviewed:
- Spot-checked polarity on the visible diff hunks (
ResetOutcome,HttpScheme,Subdomains,ConnectionHeader,Chunk,AllowProxyUrl,CloneText,Strength) — all match the original bool sense. - Confirmed
bool_enum!places the false-like variant first with discriminant 0/Default, sofrom_booland zero-init stay consistent with the priorboolsemantics. - Checked that FFI boundaries unwrap with
== Variantbefore crossing to C (lshpack,us_socket_t::adopt_tls,BunString__createExternal, visible-width) rather than casting the enum.
Extended reasoning...
Overview
This PR adds a bun_core::bool_enum! macro that declares two-variant Copy enums (false-like variant first, discriminant 0, Default; from_bool for runtime values; deliberately no Into<bool>) and applies it across ~529 files in the Rust workspace to replace positional bool parameters, mode-like fields, opaque tuple slots, and a handful of non-predicate -> bool returns. The diff is +7630/-4482 and touches essentially every subsystem: parser/printer, bundler, CSS, HTTP (h1/h2/h3, proxy tunnels, TLS), install/lockfile, glob, react-compiler, io, jsc, and more. The branch was 325 commits behind main and robobun hand-resolved 114 conflicting files during the update merge, deleting 31 enums that main had since typed independently.
Security risks
The refactor retypes flags on TLS and certificate-verification paths: TlsRole { Server, Client } (fed to adopt_tls/start_tls/SSL wrapper init), RejectUnauthorized, AllowProxyUrl (gates SNI/hostname selection in check_server_identity), Subdomains (X.509 host matching), HttpScheme (proxy env-var selection), and HPACK NeverIndex (RFC 7541 sensitive-header literal). A silent polarity flip on any of these would weaken security without a compile error. robobun's mechanical reconstruction reports every one of these lines round-trips to main's bool form, and I spot-checked the visible hunks — the two proxy tunnels pass TlsRole::Client where main passed true, resolve_reject_unauthorized unwraps with role == Server where main took is_server: bool, and the FFI sites cast via (x == Variant) as c_int rather than transmuting the enum. I did not find an inversion, but the sheer number of security-relevant call sites plus the hand-merged conflicts means a maintainer should confirm.
Level of scrutiny
High. This is not a mechanical rename the compiler enforces — the whole risk class (variant used the wrong way round) is invisible to rustc. It is also a cross-cutting API-design decision: bool_enum! becomes the house style for every future flag parameter, and the repo's own review guidance says new cross-cutting abstractions need maintainer agreement. robobun's final comment explicitly defers the convention sign-off to a maintainer.
Other factors
No automated tests are added (behaviour is claimed unchanged; existing suites are the coverage). Buildkite/clippy/miri/format are reported green on the merge head, and robobun ran a substantial subset of test files against a debug build. The one source-lint failure was addressed by recording the macro's #[allow(dead_code)] in the escape inventory. Given the scale, the hand-merged conflicts, the security-adjacent surface, and the pending convention decision, this falls squarely outside what an automated approval should cover.
3 participants
What does this PR do?
Positional booleans such as
eql_long(a, b, true),link(true)orGlobWalker::init(.., true, true, false, true, true)don't say what they mean at the call site. This addsbun_core::bool_enum!, which declares aCopytwo-variant enum (Name::{No, Yes}or domain-named variants likeScope { Local, Global }; false variant first andDefault;from_boolfor values computed at runtime, deliberately noInto<bool>), and uses it to retype boolean parameters, mode-like fields, opaque tuple slots and a few non-predicate-> boolreturns (→Result/ControlFlow/ named enum) across the Rust workspace. Call sites now readCheckLen::Yes,StopMode::Abrupt,Chunk::Last,TlsRole::Client.Behaviour is unchanged. Deliberately left as
bool: FFI/repr(C)and codegen-dictated signatures (.classes.tshooks, host fns), wire/lockfile layouts, const generics, predicates (is_*/has_*), plain locals, named on/off option fields and single-arg setters, and parameters where every caller passes the same literal (the other variant would trip-D dead_code). C++ and TS sources are out of scope.How did you verify your code works?
cargo check --workspace(+--tests) clean on aarch64/x86_64 macOS, x86_64/aarch64 Linux, x86_64 Windows;cargo clippy --workspaceclean.from_bool,== Yesvs oldif flag,ControlFlowdirection, tuple/argument order).Bun.buildproduces byte-identical output to the released binary; spot-ran shell, glob, toml, inspect, serve static/routes/file, body-clone, css color, fs-mkdir and cli env test files, all passing.