Tidy: fold duplicated matches into one method, drop dead state (no behavior change) - #38841
Merged
Merged
Conversation
…ad and plain arguments alike
…vert_hoisted_lvalue_kind
…instead of its own copy
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 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 (62)
Comment |
Collaborator
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Each of the 38 commits reads as behavior-preserving, but given the breadth (62 files across install, bundler, resolver, react_compiler, and several parsers) a human skim would still be worthwhile.
Checked specifically:
primary_side_effectsin resolver.rs —SideEffects::has_side_effectscovers theUnspecified/False/Maparms identically to the removed inline matches.transpiler.rs— the droppedGlobal::crash()was unreachable (guarded byis_none()on the same field);ImportPathFormatisCopyso passing it directly matches the per-arm dispatch.- md
ansi_rendererlink state —link_hrefwas alwaysSomewhilelink_depth > 0, so the newOption<OpenLink>and the unconditional OSC 8 terminator are equivalent. cfg_utils::mark_instruction_idsandlowering::convert_locare byte-identical to the removed local copies.
Extended reasoning...
Overview
38 independent tidy commits, net −520 lines across 62 files. Three shapes: (1) a repeated match on an enum lifted to a method on that enum (LoadStep::verb, PathSep::apply, ParamPattern::place, PlaceOrSpread::place, InstructionKind::unhoisted, PackageIndexEntry::as_slice, BundleOptions::forced_jsx_development, ResultValue::source_index, CompilerErrorOrDiagnostic::category, EntriesOption::as_entries, NumberOrPercentage::unit_value, IdentifierName::promoted); (2) two identical private functions collapsed to one (match_any/match_any_comptime, yaml byte_literal, convert_loc, mark_instruction_ids, is_set_state_identifier, css parse_list_with_state, is_member_chain_of_module_level_or_global, collect_bundled_deps, resolved_folder_package, exit_for_root_package_json, trim_input_for_kind/trimmed_top_level_dir/assert_joinable); (3) redundant state removed (has_process_exited beside exit_status: Option, md renderer's link_depth+link_href merged into Option<OpenLink>, xml tape's parallel value/loc vectors merged into a Rows<T> pair type, checkbox_color and aliasing-node kind moved from string literals to enums, .expect("oom") markers replaced with bun_core::handle_oom, dead save_from_binary wrapper removed).
Security risks
None. No parsing of untrusted input changes shape, no auth/crypto/permission code, no new syscall paths. The security scanner change removes a redundant bool that was always set alongside exit_status.
Level of scrutiny
Medium-high. Each commit is individually mechanical and behavior-preserving by inspection, but the aggregate spans production-critical subsystems: the package manager (install/), bundler resolve/parse loop, resolver side-effects classification, and several parsers. I traced the non-obvious equivalences: has_side_effects covers all five PJSideEffects arms identically to the removed inline matches (including Map's contains_key); the removed crash() in transpiler.rs was dead code inside an else guarded by is_none(); the md link-state refactor holds because the old link_href was always Some while link_depth > 0; the xml Rows<T> wrapper preserves every paired push/truncate/index; cfg_utils::mark_instruction_ids and lowering::convert_loc are byte-identical to the removed copies; ResolvedPackageResult { ..Default::default() } gave is_first_time: false, task: None, matching the new explicit fields.
Other factors
The author ran the nearest test suites per area on the debug build and reports the only failures also fail on main. No prior human review comments to address. The one place I'd flag for a maintainer glance is scan_installing_scanner_if_needed: the two folded call sites were already identical after substituting scan_all=true, seeds=&[] for the second, so the retry now correctly threads scan_all/seeds through both — but it's the kind of merge worth a second pair of eyes since it's on the security-scanner install path.
alii
added a commit
that referenced
this pull request
Aug 15, 2026
#38846) Moves the pinned mordant revision to 4efc663, which adds sixteen lints (duplicated matches and helpers, parallel vecs, bool parameter runs, integer aliases used interchangeably, a Result collapsed to a bool and dropped, the same place narrowed two ways, and a few more; scarletindustries/mordant#12 has the list). mordant-baseline.toml is regenerated against main so the job stays advisory and only reports instances a change adds; the recorded counts are what exists today (largest: 81 same_match_twice, 38 collapsed_error, 20 bool_params). #38841 already removes a good share of the same_match_twice and reimplemented_helper entries and can shrink the file when it lands. Nothing else changes: same nightly, same dylint version, so the workflow's caches key over on the rev alone.
alii
added a commit
that referenced
this pull request
Aug 15, 2026
…the wrapper script (#38865) clippy.yml, miri.yml, lolhtml.yml and mordant.yml each spent about forty lines on the same setup (LLVM from apt.llvm.org, Bun, a pinned toolchain so rust-toolchain.toml's cross targets are not installed, bun install, codegen) before running one cargo command. They are now one workflow, rust-lints.yml, whose four jobs run in parallel and share a composite action, .github/actions/rust-lint-setup, for that setup. Check names are unchanged for cargo clippy, cargo miri test and mordant; lol-html's is now "lol-html cargo test". Triggers are the union of the four path lists on pull_request plus merge_group and workflow_dispatch; mordant stays advisory (continue-on-error) and off merge_group as before. Every cache key, version pin and env var carried over; .github/workflows/CLAUDE.md describes the new layout. The mordant job no longer goes through scripts/rust-mordant.ts, which is deleted. That script existed to assemble -A flags, cap lints so the workspace's deny(warnings) did not abort a crate on the first new finding, and grep the output for pass/fail. mordant does those itself as of scarletindustries/mordant#13, pinned here: the lints we keep off are `disabled = [...]` in dylint.toml with their reasons, findings over the baseline print as warnings that deny cannot escalate, and the run leaves target/mordant/over-baseline.txt when there were any, so the job is `cargo dylint --all --workspace -- --keep-going` followed by `test ! -s` on that file. `bun run rust:mordant` and `rust:mordant:baseline` are the same two commands for local use. mordant-baseline.toml is regenerated at the new pin; it is smaller than the one on main mostly because #38841 removed many of the duplicated matches it recorded. The one Linux-only entry (the shell rm builtin, whose fields differ under cfg) is kept by hand since the file was generated on macOS.
This was referenced Aug 15, 2026
Closed
alii
added a commit
that referenced
this pull request
Aug 15, 2026
…39148) ### Problem - mordant's `bare_bool_args` flags `H2FrameParser::send_data` in `src/runtime/api/bun/h2_frame_parser.rs`: it takes `close: bool`, `suppress_half_closed_local_dispatch: bool` and `defer_write_callback: bool`, and `no_trailers` calls it as `send_data(stream, b"", true, JSValue::UNDEFINED, false, false)`, where nothing says which flag is which (the other caller, `write_stream`, passes a bare `true` for the middle one). - The three flags are independent: `close` is data (it flows on into `queue_frame`'s `end_stream` and the END_STREAM flag), while the other two select how the result is reported to the caller, so they do not collapse into one enum. ### Fix - The three flags move into a `SendDataOptions` struct (`Copy`, it is three bools), which both callers build as a struct literal, so each call names what it sets. `send_data` destructures it on entry and the body is unchanged; `payload` and `callback` stay positional. `send_data`'s doc comment shrinks to the meaning of its return tuple; the two reporting modes were already explained at the `write_stream` call site and on `WRITE_FLUSHED_WITHOUT_CALLBACK`, so the fields just name them. - No behavior change: both calls pass the same values as before, in named form. - Two tests in `test/js/node/http2/node-http2.test.js` pin the flags the two callers set, which is the mistake this change is meant to make impossible (neither path had coverage in this file; they pass before and after, as a refactor's tests should): `write()` leaves its chunk in `writableLength` until the deferred write callback runs (`write_stream`'s `defer_write_callback`), and `sendTrailers({})` ends the stream with END_STREAM and no trailer block (`no_trailers`'s `close`). Inverting `close` in `no_trailers` makes the second test time out waiting for 'end'; inverting `defer_write_callback` in `write_stream` makes the first one see `writableLength` already 0 when `write()` returns. - The `bare_bool_args:src/runtime/api/bun/h2_frame_parser.rs` entry is removed from `mordant-baseline.toml`: the file was regenerated with `bun run rust:mordant:baseline` at the pinned mordant revision. The regeneration also drops two entries whose findings were already fixed on main after the baseline was recorded, `always_unwrapped_option:src/install/PackageInstall.rs` (gone since #38271 / #38841 touched that file) and `narrowed_two_ways:src/runtime/node/node_crypto_binding.rs` (gone since #37648). Happy to trim the file back to just the h2 line if you would rather keep this PR to one entry. - Verified with `bun bd`: - `bun bd test test/js/node/http2/node-http2.test.js`: the two new tests pass (and also pass on the unmodified binary, as expected); the full file before they were added: 349 pass, 6 skip. The 8 failures are the `describe.concurrent` block "DATA payload survives its ArrayBuffer being detached/resized" hitting its 5s per-test timeout in this debug+ASAN container; the same 9 tests pass when the block is run on its own (each takes 2.4s to 3.9s alone). - `bun bd test test/js/node/http2/h2-conformance.test.ts`: 67 pass. - 27 `test/js/node/test/parallel/test-http2-*.js` scripts covering trailers, `noTrailers`, write callbacks, empty and zero-length writes, backpressure and flow control, each run with the debug binary: all exit 0. - `cargo clippy -p bun_runtime --no-deps`: clean (the first push failed CI's `needless_pass_by_value` until the struct derived `Copy`). - `bun run rust:mordant` against the regenerated baseline: with the old `send_data` it reports exactly one finding over the baseline, this `bare_bool_args` at `h2_frame_parser.rs:7481` (`target/mordant/over-baseline.txt` contains `bun_runtime 1`); with this change it reports nothing and that file is not written. ### Background - `H2FrameParser` is the native side of `node:http2`. `send_data` writes one stream's DATA payload, splitting it into frames and either handing the frames to the socket or queueing them when flow control or socket backpressure blocks the write. - `write_stream` is the host function behind a stream's `_write`; it returns the stream state it settled on to JS instead of having the engine dispatch `onStreamEnd` back into JS in the middle of the call, and it can ask for the write callback to be left to JS (`defer_write_callback`) so a Writable's callback never completes synchronously inside `write()`. `no_trailers` sends the empty END_STREAM frame once JS decides not to send trailers, and wants the normal dispatch. - `mordant-baseline.toml` is the ratchet for the mordant lint pack: it records per-(lint, file) counts of the findings that predate the job, so CI fails only on new findings. Clearing a site means deleting or decrementing its entry. --------- Co-authored-by: Alistair Smith <hi@alistair.sh>
alii
added a commit
that referenced
this pull request
Aug 15, 2026
#39153) ### Problem - `parse` in `src/sql_jsc/shared/datetime_text.rs` took `allow_date_only: bool, allow_t_separator: bool`, and both callers passed bare literals: `parse(text, true, true)` in `parse_mysql`, `parse(text, false, false)` in `parse_postgres_timestamp`. At the call site nothing says which literal is which flag. - This is the one `bare_bool_args` finding recorded for this file in `mordant-baseline.toml`. ### Fix - Each flag becomes a two-variant enum: `TimePart::{Optional, Required}` (is the 10-byte `YYYY-MM-DD` form accepted) and `Separator::{Space, SpaceOrT}` (which byte may sit between the date and the time). MySQL passes `Optional, SpaceOrT`; Postgres passes `Required, Space`, matching the previous `true, true` / `false, false`. - The two checks inside `parse` match on the enums; the accepted inputs are unchanged. No behavior change. - Removes the `bare_bool_args:src/sql_jsc/shared/datetime_text.rs` entry from `mordant-baseline.toml`. This is what `bun run rust:mordant:baseline` now writes for the `[bun_sql_jsc]` section. - Verified: - `cargo dylint --all -p bun_sql_jsc` with the baseline entry removed: no findings with this change; with the old `parse` restored it reports exactly this finding over the baseline (`over-baseline.txt` contains `bun_sql_jsc 1`). - `bun bd test test/js/sql/sql-postgres-datetime-roundtrip.test.ts test/js/sql/sql-mysql-datetime-roundtrip.test.ts test/js/sql/postgres-datestyle.test.ts test/js/sql/postgres-infinity-date.test.ts` against local Postgres and MariaDB servers: all pass. Both round-trip files exercise the text protocol (`.simple()`), which is the path that goes through this parser. - An ad hoc script over both drivers on the text protocol: MySQL `DATE` (date-only form) and `DATETIME(6)` with fractional seconds, and a Postgres `timestamp` with fractional seconds, all decode to the same instants as the binary protocol. - `cargo clippy -p bun_sql_jsc --no-deps` is clean. - No new test: the change is type-level in a private function and accepts exactly the inputs it did before, so there is no input on which a test could distinguish the old code from the new. Both settings of each flag are already covered by existing tests on the text protocol: `sql-mysql-datetime-roundtrip.test.ts` and the `date` test in `sql-mysql.test.ts` (`TimePart::Optional`, `Separator::SpaceOrT` with the space form), `sql-postgres-datetime-roundtrip.test.ts` (`TimePart::Required`, `Separator::Space`). ### Background - `datetime_text.rs` is the parser both SQL drivers use for the wall-clock `YYYY-MM-DD HH:MM:SS[.ffffff]` strings their text protocols return. MySQL additionally returns bare dates for `DATE` columns and the parser tolerates `T` as the separator; Postgres `timestamp` text is always the full shape, and anything else must return `None` so the caller falls back to `Date.parse`. - 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. <details> <summary>Regenerating the whole baseline also drops two unrelated entries</summary> A full `bun run rust:mordant:baseline` run on top of this branch additionally removes `always_unwrapped_option:src/install/PackageInstall.rs` and `narrowed_two_ways:src/runtime/node/node_crypto_binding.rs`. Those findings were fixed on main after the baseline was recorded (#37648 for the crypto one; #38271 / #38841 touched `PackageInstall.rs`). They are left out of this PR to keep it to the one site; happy to include them or send them separately. </details> Co-authored-by: Alistair Smith <hi@alistair.sh>
alii
pushed a commit
that referenced
this pull request
Aug 15, 2026
…h_manager (#39154) ### Problem - mordant `same_match_twice` in `src/install/PackageManager/install_with_manager.rs` (the one `[bun_install]` entry for that lint in `mordant-baseline.toml`): "this `match` on `workspace_package_json_cache::GetResult` repeats an earlier one arm for arm". - The two copies were the root package.json lookup in the differ branch of `install_with_manager` (around line 165) and in `create_new_lockfile_and_enqueue` (around line 1939). #38841 folded what the error arms did into `exit_for_root_package_json`, but both sites still spelled out the lookup plus the `Entry` / `ReadErr` -> `"read"` / `ParseErr` -> `"parse"` mapping, and both then only did `entry.source.clone()`. ### Fix - One helper, `root_package_json_source(manager, root_package_json_path) -> crate::Result<Source>`, does the cache lookup, maps the error variant to its verb, and absorbs the body of `exit_for_root_package_json` (which had no other callers). Both sites become `let source_copy = root_package_json_source(manager, root_package_json_path)?;`. - No behavior change: the same `get_with_path` call with the same options, the same clone of `source`, the same log print before `failed to read/parse '<path>'`, the same exit code, and the same `Err` return in the one case where printing the log fails (`?` on `print` instead of the explicit `if let Err`). Returning the cloned `Source` rather than the cache entry is what lets the helper be a plain function: the callers reborrow `manager` mutably right after, and the entry borrow would otherwise have to stay alive across the error arms. - `mordant-baseline.toml`: dropped the `same_match_twice` line for this file. - Tests (`test/cli/install/bun-install.test.ts`, "root package.json that cannot be read or parsed"): the read failure (package.json replaced by a directory) and the parse failure, each once against a bun.lock that already lists a dependency and once without a lockfile. Only the fresh-install parse case had a test before. A temporarily instrumented build confirmed the bun.lock cases go through the differ and the others through `create_new_lockfile_and_enqueue`. Since the change is a refactor, these pass before and after it; they pin the output the helper now owns for both callers and both verbs. Verified on the debug build here and, for the `EISDIR` and path expectations, against the current canary on Windows. - Other verification: - `bun bd test` on `bun-workspaces`, `bad-workspace`, `frozen-lockfile-missing-workspace`, `frozen-lockfile-pruned`, `lockfile-only`: 207 pass. - Full `bun-install.test.ts`: everything passes except the bitbucket/gitlab/remote tarball/`--registry` cases that need network and fail identically here with `USE_SYSTEM_BUN=1`. - `cargo dylint --all -p bun_install --no-deps` (mordant at the pinned rev, baseline line removed): clean, no `target/mordant/over-baseline.txt`. The same command on the unmodified file reports exactly this finding at line 1939, and removing a different `[bun_install]` baseline line makes it report that one, so the run is exercising the baseline. ### Background - `WorkspacePackageJSONCache::get_with_path` reads and parses a package.json once and caches it; it returns `GetResult`, a three-way enum (`Entry(&mut MapEntry)`, `ReadErr`, `ParseErr`), and any parse diagnostics go into the package manager's `Log`. The install code prints that log before the final `failed to parse` line so the user sees where in the file the error is. - `bun install` reaches this lookup on one of two paths: the differ, when an existing bun.lock already has dependencies and package.json is re-read to diff against it, or `create_new_lockfile_and_enqueue`, when there is no usable lockfile yet. - `mordant-baseline.toml` records the per-(lint, file) finding counts that predate the mordant CI job; the job only fails on findings above those counts, so an entry can be deleted once the file is clean for that lint. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thirty-eight small commits, each independent and each meant to change nothing observable; if any one looks wrong it can be dropped on its own. Net about 520 lines go away across install, bundler, resolver, css, xml, md, shell, paths and react_compiler.
Most of them are the same shape: a
matchon some enum that was written out two or three times in different functions (LoadStep to its verb, ForceNodeEnv to jsx.development, ScanAttemptResult to a Result, ParamPattern to its place, SideEffects to the resolver's flag, PathSep to a byte) becomes one method on the enum and the copies call it. A few are a private function that existed twice with the same body (shell match_any / match_any_comptime, yaml's two literal encoders, react_compiler's convert_loc, mark_instruction_ids, promoted_name, is_set_state_identifier), reduced to one. The rest remove state the type did not need: the security scanner'shas_process_exitedflag beside itsexit_status: Option(exited is now "has a status"), the md renderer's link depth beside an optional href (oneOption<OpenLink>), the xml tape's parallel value/location vectors (one row type keeps them the same length), a checkbox colour and an aliasing-node kind that were strings compared against literals (enums), and the remaining.expect("oom") // bun.handleOommarkers from the port in install (real handle_oom calls).Checked with the tests nearest each area on the debug build: xml and the xml suite, yaml, css, shell parse/lex/bunshell, md spec and edge cases, hosted-git-info, esbuild default, update-interactive formatting, security provider, workspaces, install workspace/patch. The failures seen (RequireShimSubstitution in esbuild default, registry-backed pm scan/patch cases) fail the same way on main here.