Skip to content

bun_core: fix clippy on the Windows and FreeBSD targets and lint them in CI - #37605

Open
robobun wants to merge 5 commits into
mainfrom
farm/fc23061e/clippy-windows-bun-core
Open

robobun wants to merge 5 commits into
mainfrom
farm/fc23061e/clippy-windows-bun-core

Conversation

@robobun

@robobun robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On a Windows host, after bun run build --configure-only and ninja -C build/debug clone-lolhtml:

$ cargo clippy -p bun_core
error: could not compile `bun_core` (lib) due to 36 previous errors

Every one of the 36 is in #[cfg(windows)] code. The Clippy workflow runs on ubuntu only, so that code is never linted in CI, and since every workspace crate depends on bun_core, clippy on a Windows host stops there and reports nothing about any other crate. The same failure reproduces from Linux with cargo clippy -p bun_core --target x86_64-pc-windows-msvc; --target x86_64-unknown-freebsd has 2 more hits of the same kind in the FreeBSD branch of fd_path_raw.

Breakdown (all deny-level in the workspace [lints]):

lint sites
undocumented_unsafe_blocks 22 Windows Zeroable impls (one prose comment above the group; the lint wants a // SAFETY: per impl, which the libc group right above already has) + os::{take_environ, set_environ, environ} + the _strnicmp block + FreeBSD fd_path_raw
borrow_as_ptr CommandLineToArgvW(.., &mut argc), RtlLookupFunctionEntry / RtlVirtualUnwind out-params in debug.rs
ptr_as_ptr (*pp).hStdInput as *mut c_void x3 (HANDLE already is *mut c_void), FreeBSD kf_path cast
assigning_clones self_exe_path: s = rest.to_owned()
large_stack_frames PathBuffer::uninit: 2 x MAX_PATH_BYTES = 196604 bytes on Windows (98302 there vs 4096 on Linux)

Fix

src/bun_core only. SAFETY comments where they were missing, &raw mut / .cast() for the pointer lints, and the identity HANDLE casts removed. Two sites deserve a note:

  • self_exe_path now strips the \\?\ / \\?\UNC\ prefix in place (replace_range / drain) instead of building a second String while borrowing the first, which is what the lint is about (its own clone_into suggestion would not borrow-check here). Result is unchanged: \\?\C:\x\bun.exe -> C:\x\bun.exe, \\?\UNC\srv\share\bun.exe -> \\srv\share\bun.exe.
  • PathBuffer::uninit keeps its by-value shape and gets #[allow(clippy::large_stack_frames, reason = ..)] (the form jsc_hooks.rs / WindowsWatcher.rs already use): the two locals are the MaybeUninit temp and the return slot, neither is written, optimized builds emit nothing for it, and every caller is holding a MAX_PATH_BYTES buffer regardless. WPathBuffer::uninit is the same shape at 131068 bytes, 4 bytes under the configured threshold, so it is left alone.

Keeping it that way

bun_core is only the bottom layer of the drift. Clippy per target for the whole workspace, after this PR:

target hits
x86_64-pc-windows-msvc (aarch64-pc-windows-msvc reports the identical set) 710 in 22 crates: bun_runtime 280, bun_sys 130, bun_install 93 (51 of them in the windows-shim sources it compiles via #[path]), bun_libuv_sys 85, bun_io 30, bun_spawn 21, bun_watcher 14, ...
x86_64-unknown-freebsd 22 in 6 crates
x86_64-unknown-linux-musl 1 (bun_crash_handler)
aarch64-unknown-linux-gnu, aarch64-apple-darwin, x86_64-apple-darwin 0 (#33958 fixed darwin-only and c_char = u8-only hits last month, so these do regress; aarch64-unknown-linux-gnu is the only CI triple with c_char = u8)
aarch64-linux-android 43, of which 42 are thread_local_initializer_can_be_made_const firing on initializers that already are const { .. } (clippy false positive on that target), so it is not worth wiring up

Cleaning up the other crates is left to follow-ups (self-contained per crate); what this PR adds is the guard rail that makes every one of them stick:

  • scripts/rust-clippy-cross.ts (bun run rust:clippy-cross) lints the whole workspace with --target for every triple in scripts/rust-clippy-cross-budgets.json (the five non-android targets above) and counts the diagnostics per <file> <lint>. The JSON is the budget, in the shape test/internal/source-lints/dead-code-escape-limits.json already uses: a pair above its number fails and prints exactly that pair's diagnostics (in rustc's normal format, so the workflow's problem matcher annotates them), anything not listed is allowed zero hits, and a pair below its number fails until --update rewrites the file, so the file tracks the real state and otherwise only shrinks. Raising an entry is possible but is a reviewable diff line that needs the same justification as an #[allow]. Today that is 710 hits in 257 entries for Windows, 22 in 10 for FreeBSD, 1 for musl and {} for the two clean targets, so for example every file without a mem_forget / not_unsafe_ptr_arg_deref / undocumented_unsafe_blocks entry is pinned at zero for those lints now, without the existing hits having to be fixed first. Clippy only needs the triple's rust-std, so this runs from any host.
  • The run caps lints to warnings (-- --cap-lints=warn): under the workspace's deny-level lints the first failing crate would hide all of its dependents, which is the onion that made this hard to see in the first place. Always linting the full workspace in one invocation also keeps the counts honest: with --no-deps, a crate that was compiled as a dependency of some -p selection is not linted, and cargo then reuses that artifact when the crate is selected itself (reproduced: after cargo clippy --no-deps -p bun_ptr --target x86_64-pc-windows-msvc, cargo clippy --no-deps -p bun_core --target x86_64-pc-windows-msvc reports the unfixed bun_core as clean). So the script has no -p mode, and the test below builds into its own target directory. Diagnostics are deduplicated by their rendered text because a crate that proc-macros or build scripts depend on is compiled more than once per run (bun_output_tags three times); a hit added to it counts once.
  • .github/workflows/clippy.yml reads the triples out of the budget file, installs their std along with the toolchain, and runs the script after the host lint (also when the host lint failed, so a PR sees both sets of diagnostics at once). Cost, from the workflow's run on the final version of this PR: installing the five triples' std is inside a 19s Setup Rust step, the host lint step took 66s, the new step 4m56s for the five targets, job total 7m03s (it was about 2 minutes before). The Buildkite build (20 minutes) is what PR CI waits on, so this stays off the critical path.
  • test/internal/rust-clippy-cross.test.ts runs the plain deny-level cargo clippy -p bun_core --target <triple> for each budgeted triple, i.e. the command from the report, and expects no diagnostics. On main it fails for Windows and FreeBSD with exactly the diagnostics above (also with a poisoned shared target/, thanks to the private target dir); it skips where cargo, the configure output, or the triple's std is missing, same prerequisites as rust-windows-sys-link.test.ts / linear-fifo.test.ts.
  • One sentence in CLAUDE.md next to the existing rust:check-all advice.

The drift is live, not hypothetical: the first run of the workflow on this design came back red because the workflow lints the merge commit, and in the day between this branch's base and that run main had picked up six new Windows-only hits, cast_ptr_alignment in bun_cares_sys from #37490 and five borrow_as_ptr in bun_runtime from #31829 (ipc.rs, node_cluster_binding.rs). Replaying that last case against the committed file (budgets minus the two entries #31829 introduced, current tree) prints those five diagnostics and two budget lines, 64 lines in total, which is what #31829's own Clippy job would have shown. The branch is rebased onto main and the budgets were generated there; the numbers CI reports for the merge commit are the same as the local ones on all five targets (the counts depend on the sources alone). If main gains more cfg-gated hits before this merges, the workflow on this PR goes red the same way and needs a rebase plus --update; after it merges, the PR adding the hit is the one that goes red.

Verification

  • Linux: bun run rust:clippy-cross is within budget on all five targets and produces the same budget file twice in a row; its failure paths were exercised by hand (the cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829 replay above; bun_core with main's debug.rs: its 4 diagnostics and src/bun_core/debug.rs clippy::borrow_as_ptr: 4 hits, budget is 0, exit 1; an entry set above the real count: run --update, exit 1; a compile_error! on FreeBSD: error printed, exit 1; unknown triple argument: exit 1; one hit added to bun_output_tags counts once). cargo fmt --all -- --check is clean. bun bd test test/internal/rust-clippy-cross.test.ts passes (5 targets) and fails on main for Windows (36) and FreeBSD (2).
  • Windows Server x64 host: the repro above fails unpatched (36 errors) and is clean with this branch, and the test passes natively. A native debug build of the branch prints C:\workspace\bun\build\debug\bun-debug.exe for process.execPath when launched normally (the \\?\ branch of self_exe_path) and \\localhost\C$\workspace\bun\build\debug\bun-debug.exe when launched through the admin share (the \\?\UNC\ branch); process.argv[0] matches in both cases, and test/js/node/process/process-args.test.js compares parent and child argv[0] across 50 spawns before running into its 5s budget on the debug build.

[decide:dep] gate passed · iteration 0 · 9 files touched

fails on main (without fix)
ASAN without fix: 2 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/rust-clippy-cross.test.ts
bun test v1.4.0 (cbc78b7bc)

test/internal/rust-clippy-cross.test.ts:
74 |       const diagnostics = stderr.split(/\r?\n/).filter(line => /^\S+\.rs:\d+:\d+: (error|warning)/.test(line));
75 |       if (exitCode !== 0 && diagnostics.length === 0) {
76 |         // Failed for a non-lint reason (build script, dependency), which the assertions below cannot show.
77 |         console.error(stderr || stdout);
78 |       }
79 |       expect(diagnostics).toEqual([]);
                               ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/bun_core/util.rs:745:12: error: this function may allocate 196604 bytes on the stack",
+   "src/bun_core/util.rs:1551:28: error: `as` casting between raw pointers without changing their constness: help: try `pointer::cast`, a safer alternative: `(*pp).hStdInput.cast::<c_void>()`",
+   "src/bun_core/util.rs:1552:29: error: `as` casting between raw pointers without changing their constness: help: try `pointer::cast`, a safer alternative: `(*pp).hStdOu
... (truncated)

release without fix: 2 FAILED
bun test v1.4.0-canary.1 (da3851e57)

test/internal/rust-clippy-cross.test.ts:
74 |       const diagnostics = stderr.split(/\r?\n/).filter(line => /^\S+\.rs:\d+:\d+: (error|warning)/.test(line));
75 |       if (exitCode !== 0 && diagnostics.length === 0) {
76 |         // Failed for a non-lint reason (build script, dependency), which the assertions below cannot show.
77 |         console.error(stderr || stdout);
78 |       }
79 |       expect(diagnostics).toEqual([]);
                               ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/bun_core/util.rs:745:12: error: this function may allocate 196604 bytes on the stack",
+   "src/bun_core/util.rs:1551:28: error: `as` casting between raw pointers without changing their constness: help: try `pointer::cast`, a safer alternative: `(*pp).hStdInput.cast::<c_void>()`",
+   "src/bun_core/util.rs:1552:29: error: `as` casting between raw pointers without changing their constness: help: try `pointer::cast`, a safer alternative: `(*pp).hStdOutput.cast::<c_void>()`",
+   "src/bun_core/util.rs:1553:28: error: `as` casting between raw pointers without changing their constness: help: try `pointer::cast`, a 
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/rust-clippy-cross.test.ts
bun test v1.4.0 (cbc78b7bc)

test/internal/rust-clippy-cross.test.ts:
(pass) bun_core is clippy-clean for x86_64-pc-windows-msvc [2632.89ms]
(pass) bun_core is clippy-clean for x86_64-unknown-freebsd [2699.80ms]
(pass) bun_core is clippy-clean for x86_64-unknown-linux-musl [2747.78ms]
(pass) bun_core is clippy-clean for aarch64-unknown-linux-gnu [2642.83ms]
(pass) bun_core is clippy-clean for aarch64-apple-darwin [4946.45ms]

 5 pass
 0 fail
 10 expect() calls
Ran 5 tests across 1 file. [18.21s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     cbc78b7bc4
  features     baseline

22 deps, 107 codegen, 1176 objects in 975ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1238] install /workspace/bun
bun install v1.4.0-canary.1 (da3851e57)

Checked 107 installs across 153 packages (no changes) [7.00ms]
[2/1238] gen ErrorCode+*.h
[3/1238] gen bindgenv2
[4/1238] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (da3851e57)

Checked 1 install across 2 packages (no changes) [2.00ms]
[5/1238] fetch zlib
[zlib] up to date
[6/1238] gen .bind.ts → GeneratedBindings.cpp
[7/1238] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[8/1238] fetch tinycc
[tinycc] up to date
[9/1237] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (da3851e57)

Checked 129 installs across 147 packages (no changes) [14.00ms]
[10/1237] gen ProcessBindingConstants.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp

... (truncated)
diff hotspot
.github/workflows/clippy.yml            |  23 ++-
 CLAUDE.md                               |   2 +-
 package.json                            |   1 +
 scripts/rust-clippy-cross-budgets.json  | 278 ++++++++++++++++++++++++++++++++
 scripts/rust-clippy-cross.ts            | 181 +++++++++++++++++++++
 src/bun_core/debug.rs                   |  12 +-
 src/bun_core/lib.rs                     |  36 ++++-
 src/bun_core/util.rs                    |  28 ++--
 test/internal/rust-clippy-cross.test.ts |  89 ++++++++++
 9 files changed, 631 insertions(+), 19 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file
file                                     reads  edits  tests
.github/workflows/clippy.yml                 6     13      0
CLAUDE.md                                    3      5      0
package.json                                 1      2      0
scripts/rust-clippy-cross-budgets.json       0      1      0
scripts/rust-clippy-cross.ts                 7     12      0
src/bun_core/debug.rs                        1      2      0
src/bun_core/lib.rs                          6      2      0
src/bun_core/util.rs                         5      5      0
test/internal/rust-clippy-cross.test.ts      3      8      0

self-review · no surviving concerns

4 concerns were raised and did not survive verification.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f2650a15-4bc3-4edd-9ecf-10015995fc86

📥 Commits

Reviewing files that changed from the base of the PR and between 626034f and cbc78b7.

📒 Files selected for processing (9)
  • .github/workflows/clippy.yml
  • CLAUDE.md
  • package.json
  • scripts/rust-clippy-cross-budgets.json
  • scripts/rust-clippy-cross.ts
  • src/bun_core/debug.rs
  • src/bun_core/lib.rs
  • src/bun_core/util.rs
  • test/internal/rust-clippy-cross.test.ts

Walkthrough

The pull request adds target-specific Rust Clippy budgets and a Bun validation CLI. It updates platform-specific Rust pointer syntax and safety comments. GitHub Actions and developer guidance now run the cross-target checks.

Changes

Cross-target Clippy checks

Layer / File(s) Summary
Platform-specific Rust lint fixes
src/bun_core/debug.rs, src/bun_core/lib.rs, src/bun_core/util.rs
Platform-specific Rust code now uses updated raw-pointer syntax and expanded safety documentation without changing runtime behavior.
Clippy budget validation
scripts/rust-clippy-cross.ts, scripts/rust-clippy-cross-budgets.json, test/internal/rust-clippy-cross.test.ts
The Bun CLI and test run Clippy for configured targets, parse diagnostics, compare per-target budgets, and report failures.
CI and developer workflow integration
.github/workflows/clippy.yml, package.json, CLAUDE.md
CI installs required target libraries and runs cross-target Clippy after code generation. The package script and development checklist expose the new command.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the bun_core Clippy fixes for Windows and FreeBSD and the addition of CI linting.
Description check ✅ Passed The description clearly covers the problem, implementation, CI approach, and verification, despite not using the template headings.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:05 AM PT - Aug 12th, 2026

❌ @robobun, your commit cbc78b7 has some failures in Build #93023 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37605

That installs a local version of the PR into your bun-37605 executable, so you can run:

bun-37605 --bun

@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on a Windows Server x64 host (cargo clippy -p bun_core -> 36 errors) and from Linux with --target x86_64-pc-windows-msvc (same 36) / --target x86_64-unknown-freebsd (2); clean with this branch on both hosts, and a native Windows debug build reports the expected process.execPath for a normal and a UNC launch (details in the description).

The CI side went through review twice: it is now a per-file, per-lint budget file (scripts/rust-clippy-cross-budgets.json, cbc78b7) over five targets, so a failure prints only the new hits, and the workflow reads the triples from that file. The branch is rebased onto main because the first run of the budget version correctly flagged six Windows-only hits main gained from #37490 and #31829 in the meantime.

CI on cbc78b7: the Clippy workflow (host plus the five cross targets) and the other GitHub checks are green. Buildkite build 93023 finished with 179 of 181 jobs passed; the only recorded test failures are retry-passed flakes unrelated to this change (ipc handle, require-cache under ASAN, napi, cluster, grpc), and the two jobs that make the build red are the darwin 26 aarch64 - test-bun shards, which expired without ever getting an agent. That lane is not picking up work for any build at the moment (its jobs are queued or waiting on all 30 most recent builds of the pipeline), so re-running would not change anything, and nothing in this diff is compiled on darwin apart from an #[allow] attribute. Ready for a maintainer to decide on the CI piece and merge; if main gains more cfg-gated hits first, the Clippy workflow on this PR will say so and the fix is a rebase plus bun run rust:clippy-cross --update. The remaining hits on the Windows target (710, listed in the budget file) are tracked separately.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The Rust-side clippy fixes are mechanical and behavior-preserving, and the self_exe_path in-place prefix strip produces the same output as the previous allocation-based version for both \\?\ and \\?\UNC\ inputs. Deferring to a human because it introduces new CI infrastructure (the per-target --exclude ratchet in rust-clippy-cross.ts and a new step in .github/workflows/clippy.yml) that a maintainer should sign off on, and the author's own status note indicates the native Windows process.execPath check is still pending.

What was reviewed:

  • self_exe_path: verified replace_range(..8, r"\\") / drain(..4) yield identical results to the old format!/to_owned for both verbatim-prefix cases, and the UNC-before-plain check order is preserved.
  • &raw mut / .cast() / dropped HANDLE identity casts in debug.rs and util.rs — semantically equivalent to the &mut/as forms they replace.
  • Workflow: rustup toolchain install --target a,b accepts comma-separated triples; if: !cancelled() && steps.codegen.outcome == 'success' correctly runs the cross step even when the host clippy step fails.
  • New test skips cleanly when cargo/rustup/std/codegen are absent, matching the linear-fifo.test.ts / rust-windows-sys-link.test.ts prerequisite pattern.
Extended reasoning...

Overview

This PR fixes 36 Windows + 2 FreeBSD clippy failures in bun_core (all in #[cfg(...)]-gated code invisible to the ubuntu-only Clippy CI job) and adds cross-target clippy linting to CI. Files touched: src/bun_core/{lib,util,debug}.rs (lint fixes), scripts/rust-clippy-cross.ts (new), .github/workflows/clippy.yml (new step + target install), test/internal/rust-clippy-cross.test.ts (new), plus one-line additions to package.json and CLAUDE.md.

Security risks

None. The Rust changes are comment additions (// SAFETY:), pointer-syntax modernization (&raw mut, .cast()), removal of identity casts, an #[allow] attribute, and an in-place-vs-reallocate refactor of verbatim-path prefix stripping. No new unsafe operations, no input-handling changes, no auth/crypto/permissions code. The new script and workflow step only run cargo clippy.

Level of scrutiny

Medium. The bun_core changes are individually mechanical and low-risk — I traced each one and they're behavior-preserving. The self_exe_path refactor is the only one that rewrites logic rather than syntax; I verified both branches produce identical strings, and the branch order (UNC checked before plain \\?\) is unchanged. However, the PR also introduces a new CI mechanism: a per-target ratchet with hardcoded exclude lists (22 crates for Windows, 6 for FreeBSD) that will need ongoing maintenance as crates are cleaned up. That's a workflow/process design decision, not a bug-or-not question.

Other factors

  • The author's own status comment says a native Windows debug build is still running to double-check process.execPath after the self_exe_path change, so full verification is not yet reported complete.
  • The new test and script follow established sibling patterns (rust-check-all.ts, linear-fifo.test.ts, rust-windows-sys-link.test.ts), which reduces novelty risk.
  • Changes to .github/workflows/ and CLAUDE.md are repo-wide policy surfaces that typically warrant maintainer sign-off regardless of correctness.

Given the new CI infrastructure design and the pending Windows verification, deferring rather than approving.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

The Windows check mentioned in the review is done: a native debug build of this branch prints C:\...\bun-debug.exe for process.execPath when launched normally and \\localhost\C$\...\bun-debug.exe when launched through the admin share, i.e. both prefix-strip branches of self_exe_path behave as before. Details are in the Verification section of the description; the status comment above is updated as well.

Comment thread scripts/rust-clippy-cross.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix in 4b030a4 — the seen.has()/seen.add() dedup now does what the comment says, and this run found no further issues.

I'd still like a human to sign off here: beyond the mechanical clippy fixes in bun_core, this introduces a new per-crate/per-target ratchet (rust-clippy-cross-budgets.json) that every future PR touching cfg-gated Rust will be held to, plus a five-target cross-clippy step in the workflow. That's a tooling/policy decision worth a maintainer's explicit ack.

What was reviewed

  • self_exe_path prefix-strip refactor: replace_range(..8, r"\\\\") / drain(..4) produce the same strings as the old strip_prefix + reallocate; verified against the Windows-host process.execPath checks in the description.
  • &raw mut / .cast() / dropped HANDLE as *mut c_void casts are semantically identical to what they replace; the FreeBSD kf_path change only moves the SAFETY comment and swaps as for .cast().
  • packageName() handles both cargo package-id shapes (#name@ver and directory-named #ver); --cap-lints=warn + exit-code check means real compile errors still fail the script.
Extended reasoning...

Overview

The PR has two halves. The src/bun_core half is 36 Windows + 2 FreeBSD clippy fixes: per-impl // SAFETY: comments on 22 Zeroable impls and 4 unsafe blocks, &mut → &raw mut at 5 FFI out-param sites, three identity HANDLE as *mut c_void casts removed, one as *const u8 → .cast::<u8>(), an #[allow(clippy::large_stack_frames, reason = ..)] on PathBuffer::uninit, and a rewrite of self_exe_path's verbatim-prefix strip to mutate in place. The tooling half adds scripts/rust-clippy-cross.ts + rust-clippy-cross-budgets.json (a per-crate hit-count ratchet across five --target triples), a new step in .github/workflows/clippy.yml, a rust:clippy-cross package script, test/internal/rust-clippy-cross.test.ts, and a CLAUDE.md sentence.

Prior review

My earlier inline comment on scripts/rust-clippy-cross.ts flagged that !seen.add(...) is always falsy (JS Set.add returns this), so the per-configuration dedup never fired. Commit 4b030a4 replaced it with if (seen.has(key)) continue; seen.add(key); and the commit message confirms no budget entries changed (the multi-configuration crates currently have zero hits). The current diff reflects the fixed code, and this run's bug hunt found nothing new.

Security risks

None identified. The Rust edits are lint-driven and do not change observable behavior: &raw mut x and &mut x as *mut _ produce the same raw pointer for FFI out-params; the removed HANDLE casts were identity casts; the added lines are comments or an #[allow]. self_exe_path is the only control-flow change and was exercised on a Windows host for both the \\?\ and \\?\UNC\ branches per the PR description. The new script only shells out to cargo clippy with fixed arguments and reads a checked-in JSON file.

Level of scrutiny

Medium-high. The bun_core edits themselves are low-risk mechanical clippy fixes, but the ratchet is a new CI policy: 705 baked-in Windows counts across 22 crates that will gate every subsequent PR, an "under budget also fails" rule that forces --update commits, and a workflow step that lints the full workspace five extra times. Those are the kind of tooling/process decisions a maintainer should explicitly approve rather than have auto-merged.

Other factors

The test file follows the test/internal/ conventions (skips when cargo/rustup/std or the configure output is missing, private CARGO_TARGET_DIR, drains stdout/stderr/exited concurrently, asserts diagnostics before exit code) and its 120s timeout is justified inline. The workflow's if: !cancelled() && steps.codegen.outcome == 'success' correctly runs the cross step even when the host clippy fails but not when codegen fails. Given the scope and the new CI surface, deferring to a human reviewer.

…hem in CI

`cargo clippy -p bun_core` fails with 36 errors on a Windows host (and 2 on
FreeBSD): every lint is in cfg-gated code that the ubuntu-only Clippy job
never compiles. Since every other crate depends on bun_core, clippy on a
Windows host reports nothing else until this crate is clean.

- lib.rs: SAFETY comments for the `os::ENVIRON` accessors, the `_strnicmp`
  block and each Windows `Zeroable` impl (one per impl, as the libc group
  above already does).
- util.rs: `PathBuffer::uninit` allows `large_stack_frames` (two
  MAX_PATH_BYTES locals, ~196 KB on Windows, neither written); drop the
  no-op `HANDLE as *mut c_void` casts; strip the `\\?\` prefix in
  `self_exe_path` in place instead of reallocating; `&raw mut` for the
  `CommandLineToArgvW` out-param; `.cast()` plus a SAFETY comment in the
  FreeBSD `fd_path_raw` branch.
- debug.rs: `&raw mut` for the RtlLookupFunctionEntry/RtlVirtualUnwind
  out-params.

scripts/rust-clippy-cross.ts runs the workspace lint with --target for
x86_64-pc-windows-msvc and x86_64-unknown-freebsd from any host, excluding
the crates that still fail there (a ratchet: entries are removed as crates
get cleaned up). The Clippy workflow installs those two targets' std and
runs it after the host lint; test/internal/rust-clippy-cross.test.ts pins
bun_core itself on each target.
…ive targets

The exclude list left the 22 crates with existing hits entirely unlinted
(most of the cfg(windows) code in the tree) and could only be tightened by
hand. The script now lints the whole workspace per target with lints capped
to warnings, counts the diagnostics per crate and compares them with
scripts/rust-clippy-cross-budgets.json: over budget fails, under budget fails
until `--update` lowers the entry, unlisted crates are budgeted at zero.
Linting every crate in one invocation is also what keeps the numbers honest:
with --no-deps, a crate compiled as a dependency of a `-p` selection is not
linted and cargo reuses that artifact when the crate is selected later, so
the `-p` mode is gone and the test builds into its own target dir.

Targets: the two with hits (windows, freebsd) plus musl (1 hit),
aarch64-unknown-linux-gnu (the only CI triple where c_char is u8) and
aarch64-apple-darwin (the cfg(apple) and cfg(target_arch = "aarch64") code),
which are at zero today and stay there. aarch64-linux-android is left out:
clippy's thread_local_initializer_can_be_made_const fires on 42 initializers
that already are `const { .. }` on that target.
Set.prototype.add returns the set, so the `!seen.add(key)` guard never
skipped anything. No budget changes: none of the crates that are currently
compiled in more than one configuration (bun_output_tags is built three
times per --target run) has a hit today, but one new hit in such a crate was
counted three times.
CI lints the merge commit, and between this branch's base and now main
gained Windows-only hits: #37490 (net +1 in bun_cares_sys, cast_ptr_alignment)
and #31829 (+5 borrow_as_ptr in bun_runtime, net +4), which is exactly what
the first run of the workflow reported. Regenerated on the rebased tree the
budgets match that run on all five targets. The over-budget message now also
names the stale-budget case.
@robobun
robobun force-pushed the farm/fc23061e/clippy-windows-bun-core branch from 4b030a4 to fa74b7a Compare August 12, 2026 07:21

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR; the earlier Set.add() dedup bug I flagged is fixed (now has + add) and the current bug-hunting pass found nothing new. The bun_core lint fixes are mechanical and the one semantic-adjacent change (self_exe_path prefix stripping) was verified on a Windows host for both branches. Because this also introduces a new CI ratchet that will gate every future PR touching cfg-gated Rust, a maintainer sign-off on that piece would still be worthwhile.

Checked: &raw mut / .cast() / identity-cast removals are semantically identical to what they replace; replace_range/drain produce the same strings as the old strip_prefix + realloc for both \\?\ and \\?\UNC\; the workflow's if: !cancelled() && steps.codegen.outcome == 'success' correctly runs the cross step even when the host clippy step fails; packageName() handles both cargo package-id shapes.

Extended reasoning...

Overview

Two halves: (1) mechanical clippy fixes in src/bun_core/{debug,lib,util}.rs for #[cfg(windows)] / #[cfg(target_os = "freebsd")] code — SAFETY comments on 22 Zeroable impls and 5 unsafe blocks, &mut → &raw mut at FFI out-params, as *const u8 → .cast::<u8>(), three identity HANDLE as *mut c_void casts dropped, an #[allow(large_stack_frames)] with a reason, and self_exe_path reworked to strip verbatim prefixes in place; (2) new cross-target clippy infrastructure — scripts/rust-clippy-cross.ts (~180 lines), a per-crate budget JSON for 5 targets, a new step in .github/workflows/clippy.yml, a package.json script, a test in test/internal/, and a sentence in CLAUDE.md.

Security risks

None. No user-input parsing, no auth/crypto, no network. The only runtime-behavior change is the in-place string edit in self_exe_path (Windows only, called once at startup), which produces byte-identical output to the old code and was verified on a real Windows host for both the \\?\ and \\?\UNC\ branches.

Level of scrutiny

The Rust edits are low-risk lint conformance: &raw mut x at an FFI call site is the explicit form of the implicit &mut x → *mut T coercion that was already happening; .cast() is as without the integer-cast footgun; the removed HANDLE casts were identity casts. The added SAFETY comments are documentation only. The self_exe_path refactor is small and independently verified.

The CI half warrants more scrutiny because it is new required infrastructure: it adds a step that will fail PRs when any crate's cross-target clippy count moves in either direction, across five targets. The script itself reads correctly after the dedup fix — --cap-lints=warn so one crate's deny-level hits don't hide dependents, JSON message parsing that filters to level === "warning" with non-null code, per-crate budget comparison in both directions, and --update regenerates the file deterministically (sorted keys). The workflow wiring uses steps.codegen.outcome == 'success' under !cancelled() so the cross step runs even when host clippy failed. The test file skipIfs on missing cargo/rustup/std and uses a private CARGO_TARGET_DIR to avoid the artifact-reuse false-negative the description documents.

Other factors

My previous review's finding (!seen.add(...) never dedupes) was addressed in 4b030a4 and the thread is resolved; the current diff has the correct has/add pattern. The budget file was regenerated after the fix with no changes (author confirmed), and later rebased onto main (fa74b7a, hence bun_runtime: 280 vs the description's 276). The description references an earlier design review ("b4f5722 reworks the CI side after review") that moved from an exclude-list to the budget shape, but that discussion is not visible in the timeline, so I can't confirm a maintainer has signed off on the final ratchet design — which is the main reason I'm deferring rather than approving.

…e budget file

Keyed per crate, a regression in bun_runtime printed (and annotated) all
280 of its pre-existing hits, so the new one was not what showed up, and a
hit of one lint replacing a hit of another in the same crate was invisible.
The budget is now keyed by `<file> <lint>` per target, the shape the
dead-code ratchet already uses, so every (file, lint) pair without an entry
is pinned at zero and a failure prints exactly the pair's diagnostics: with
the budgets as of before #31829, the current tree reports that PR's five
borrow_as_ptr sites and nothing else. Same 710 / 22 / 1 / 0 / 0 hits as
before, now in 257 / 10 / 1 / 0 / 0 entries.

The workflow reads the triples to install from the budget file instead of
carrying its own copy of the list, which the script's header had already
claimed it did.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed after the dedup fix and the switch to per-file/per-lint budgets — no new issues found. The bun_core clippy fixes are mechanical and the self_exe_path rewrite was verified on Windows for both prefix branches. Since this adds a new CI ratchet (five cross targets, ~4.5min added to the Clippy job) that every future PR touching cfg-gated code will interact with, a maintainer sign-off on that design would still be worthwhile.

What was reviewed

  • seen.has()/add() dedup now correct; keying on rendered alone is sufficient since it embeds file:line.
  • --update with a per-target compile failure preserves that target's old budget and still exits 1 — reasonable.
  • self_exe_path replace_range/drain produces identical output to the old strip_prefix+to_owned for both \\?\ and \\?\UNC\.
  • Workflow if: !cancelled() && steps.codegen.outcome == 'success' correctly runs cross-lint even when the host lint fails.
Extended reasoning...

Overview

Fixes 36 Windows + 2 FreeBSD clippy hits in src/bun_core (SAFETY comments on Zeroable impls and os::environ helpers, &raw mut for out-params, identity HANDLE cast removal, in-place verbatim-prefix strip in self_exe_path, #[allow] on PathBuffer::uninit), and adds a cross-target clippy ratchet: scripts/rust-clippy-cross.ts + a 278-line per-file/per-lint budget JSON, wired into .github/workflows/clippy.yml and package.json, with a test/internal/ test that asserts bun_core is clean per target.

Changes since my last review

My earlier finding (!seen.add(...) always falsy) was fixed. After that, two more commits reworked the budget granularity from per-crate to per-<file> <lint> and made the workflow read the triple list from the budget file's keys. I re-read the current lint() / checkBudget() / main loop for the reworked shape: the dedup key is now just rendered (sufficient — it includes the path and span); the file-path extraction handles missing/non-primary spans and normalizes backslashes for Windows hosts; --update on a partial compile failure writes the successful targets' budgets, leaves the failed target's entry untouched, and exits 1. No new issues.

Security risks

None. No user-facing runtime behavior changes; the Rust edits are lint-driven syntax changes to already-unsafe blocks with no semantic difference (&raw mut x vs &mut x as *mut _, .cast() vs as, identity-cast removal). The script only shells out to cargo clippy on the local workspace.

Level of scrutiny

Medium-high. The bun_core edits themselves are low-risk and well-verified (native Windows build tested for both process.execPath branches). The higher-scrutiny part is the new CI guard rail: it adds ~4.5 minutes to the Clippy workflow, will start failing unrelated PRs that add cfg-gated lint hits, and encodes a specific budget shape (per-file-per-lint, five targets, --cap-lints=warn, no -p mode) that evolved through several iterations during this PR. That's a repo-wide policy decision a maintainer should ratify rather than something I should approve unilaterally.

Other factors

The PR is thoroughly documented and self-verified (Linux + native Windows, failure paths exercised by hand, test fails on main for the right reasons). The one prior review finding is resolved. Deferring solely on the CI-policy dimension.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant