Skip to content

clippy: unbreak cargo clippy on main (let_and_return + large_stack_frames) - #33951

Merged
Jarred-Sumner merged 2 commits into
mainfrom
ciro/clippy-let-and-return
Jul 11, 2026
Merged

Jarred-Sumner merged 2 commits into
mainfrom
ciro/clippy-let-and-return

Conversation

@cirospaciari

@cirospaciari cirospaciari commented Jul 11, 2026 •

Copy link
Copy Markdown
Member

Problem

cargo clippy fails on main, and therefore on every open pull request that touches a .rs file. There are two breakages, stacked — the second was hidden because bun_runtime depends on bun_paths, so it was never checked while bun_paths failed to compile.

1. bun_paths — clippy::let_and_return

error: returning the result of a `let` binding from a block
    --> src/paths/resolve_path.rs:1148:5

#33874 removed the debug_assert that used to sit between the binding and the return, leaving let result = ...; followed by result.

2. bun_runtime — clippy::large_stack_frames

error: this function may allocate 221049 bytes on the stack
  --> src/runtime/jsc_hooks.rs:301:11

Introduced by #31216. The frame is bun_bundler::Transpiler::init's return value: a Transpiler carries a Resolver whose BSSMapInner<DirInfo, 2048> is ~229 KB, and it is returned by move then ptr::writen straight into (*vm).transpiler. Optimized builds elide the copy; the function runs once per VM init. Only Linux crosses the 128 KB stack-size-threshold from clippy.toml, which is why local macOS clippy never flagged it.

Why neither was caught

.github/workflows/clippy.yml triggers on workflow_call, workflow_dispatch, pull_request and merge_group — there is no push: trigger, so main never lints itself. Because pull_request lints the PR merged into main, both breaks surface on contributors' PRs instead, attributed to their commits.

Fix

  1. Return the slice directly. The binding had no other use.
  2. Suppress large_stack_frames on init_runtime_state with the reason recorded, exactly as find_context_identifiers::walk_expr already does. #[allow] rather than #[expect] because the lint does not fire on macOS, where an expectation would go unfulfilled. Removing the temporary for real needs an in-place Transpiler::init_in_place(dst, ..) — a larger change owned by the bundler, and worth doing separately now that every Worker thread runs this init.

Verification

  • Reproduced both failures on a clean main tip before fixing; cargo clippy -p bun_paths --no-deps is clean after (1), and cargo check -p bun_runtime + cargo fmt --check are clean after (2). CI's ubuntu clippy job is the oracle for (2), since macOS never fires it.
  • Built bun-debug and drove the surface normalize_string_generic_tz actually backs, against a built node v26.3.0 — identical on every case:
    a/b/../c            => "a/c"
    /a/./b//c/          => "/a/b/c/"
    ../../x             => "../../x"
    resolve             => "/foo/bar/baz"
    resolve-up          => "/foo/baz"
    win32 normalize     => "C:\a\c"
    win32 UNC resolve   => "\\server\share\b"
    normalize("")       => "."
    normalize(".")      => "."
    normalize("/")      => "/"
    
  • bun bd test test/js/node/path/ → 122 pass, 2 skip, 0 fail. All 16 vendored test-path*.js pass.

Notes

A workspace-wide cargo clippy --workspace --no-deps --keep-going also reports derivable_impls for Sendfile in src/runtime/server/FileResponseStream.rs, but only on macOS: socket_fd: Fd::INVALID is a Linux/Android-gated field that is not Default, so the impl is genuinely not derivable on the Ubuntu runner. Left alone.

Adding a push: trigger to clippy.yml would stop this class of breakage from being invisible on main. Happy to do that here or in a follow-up.

#33874 removed the `if cfg!(debug_assertions) && is_windows { debug_assert!(..) }`
block that sat between the binding and the return, leaving

    let result = &mut buf[0..buf_i];

    result

which `clippy::let_and_return` rejects under `-D warnings`.

The Clippy workflow only runs on `pull_request` and `merge_group`, never on a
push to main, so main does not lint itself and this surfaces on every open pull
request that touches a .rs file — `pull_request` lints the merge ref.

Return the slice directly.
@robobun

robobun commented Jul 11, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 7:09 PM PT - Jul 10th, 2026

@cirospaciari, your commit e9d5087 is building: #71756

@coderabbitai

coderabbitai Bot commented Jul 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The changes simplify a path normalization return and add a targeted Clippy allowance to runtime initialization.

Changes

Path normalization

Layer / File(s) Summary
Direct slice return
src/paths/resolve_path.rs
The function returns &mut buf[0..buf_i] directly instead of assigning it to result first.

Runtime lint handling

Layer / File(s) Summary
Large stack frame allowance
src/runtime/jsc_hooks.rs
init_runtime_state now allows Clippy’s large_stack_frames warning with an explanatory reason for Transpiler initialization.
🚥 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 accurately names the two Clippy fixes and the overall goal of unbreaking cargo clippy on main.
Description check ✅ Passed The description covers what changed and how it was verified, with problem, fix, and verification details matching the template's intent.

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

@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.

LGTM — mechanical clippy fix with zero behavioral change.

What was reviewed:

  • Confirmed let result = &mut buf[0..buf_i]; result → &mut buf[0..buf_i] is semantically identical; the binding had no other use after #33874 removed the intervening debug_assert.
  • Checked that ZERO_TERMINATE still writes the NUL at buf[buf_i] before the returned slice is taken — unchanged.
  • No CODEOWNERS entry for src/paths/.
Extended reasoning...

Overview

Single-file, 3-lines-to-1-line change in src/paths/resolve_path.rs at the tail of normalize_string_generic_tz. It replaces let result = &mut buf[0..buf_i]; result with the direct expression &mut buf[0..buf_i]. This is the exact transformation clippy's let_and_return lint suggests, and it became necessary after #33874 deleted the debug_assert that previously sat between the binding and the return.

Security risks

None. This is a syntactic refactor of a return statement in a path-normalization helper; no new inputs, no new control flow, no changed bounds. The slice range [0..buf_i] is computed identically to before.

Level of scrutiny

Minimal. The change is provably behavior-preserving by inspection — the two forms compile to the same thing. The PR description documents that clippy was reproduced failing on main and passes with the fix, plus test/js/node/path/ and vendored test-path*.js all pass, which is more verification than a lint fix strictly needs.

Other factors

  • No CODEOWNERS coverage for src/paths/.
  • No prior reviewer comments to address.
  • The bug hunting system found nothing.
  • The PR is already the tip of main in the checked-out repo (commit fe93867), so the change has effectively been validated in-tree.
  • The PR description's note about the macOS-only derivable_impls warning and the missing push: trigger on clippy.yml are informational and correctly left out of this diff.

With bun_paths compiling again, clippy reaches bun_runtime and rejects
`init_runtime_state`:

    error: this function may allocate 221049 bytes on the stack
      --> src/runtime/jsc_hooks.rs:301:11

The frame is `bun_bundler::Transpiler::init`'s return value: a `Transpiler`
carries a `Resolver` whose `BSSMapInner<DirInfo, 2048>` is ~229 KB, and the
value is returned by move and then `ptr::write`n straight into
`(*vm).transpiler`. Optimized builds elide the copy, and the function runs once
per VM init.

Suppress it the same way `find_context_identifiers::walk_expr` does, with the
reason recorded. Removing the temporary for real means an in-place
`Transpiler::init_in_place(dst, ..)`, which is a larger change owned by the
bundler.

Only Linux crosses the 128 KB `stack-size-threshold`; macOS clippy does not
flag this function, so `#[allow]` (not `#[expect]`) — an expectation would go
unfulfilled there.
@cirospaciari cirospaciari changed the title paths: fix cargo clippy let_and_return in normalize_string_generic_tz clippy: unbreak cargo clippy on main (let_and_return + large_stack_frames) Jul 11, 2026

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/jsc_hooks.rs`:
- Around line 301-306: Replace the by-value Transpiler::init call in the VM
initialization path with Transpiler::init_in_place, constructing directly in
(*vm).transpiler. Remove the clippy::large_stack_frames allowance and update the
surrounding initialization logic to use the in-place API’s signature.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 67af85a0-c6a6-4f63-a90d-a777a9142f18

📥 Commits

Reviewing files that changed from the base of the PR and between fe93867 and e9d5087.

📒 Files selected for processing (1)
  • src/runtime/jsc_hooks.rs

Comment thread src/runtime/jsc_hooks.rs
Comment on lines +301 to +306
#[allow(
clippy::large_stack_frames,
reason = "`Transpiler::init` returns a Transpiler by value (~216 KB: its Resolver embeds \
BSSMapInner<DirInfo, 2048>) and it is `ptr::write`n straight into `(*vm).transpiler`. \
The copy is elided in optimized builds, and this runs once per VM init."
)]

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.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Use Transpiler::init_in_place instead of suppressing this warning.

This allowance leaves the large by-value Transpiler temporary in place. The supplied src/bundler/transpiler.rs implementation provides init_in_place specifically to construct directly in VirtualMachine.transpiler; that avoids the large stack temporary and remains important for debug/ASAN builds, where optimized copy elision does not protect the stack budget. Remove the allowance and route this initialization through the in-place API.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/jsc_hooks.rs` around lines 301 - 306, Replace the by-value
Transpiler::init call in the VM initialization path with
Transpiler::init_in_place, constructing directly in (*vm).transpiler. Remove the
clippy::large_stack_frames allowance and update the surrounding initialization
logic to use the in-place API’s signature.

@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.

LGTM — two lint-only fixes with no runtime behavior change.

What was reviewed:

  • resolve_path.rs: inlines let result = ...; result → direct return; identical codegen.
  • jsc_hooks.rs: adds #[allow(clippy::large_stack_frames, reason = ...)] matching the existing pattern at find_context_identifiers.rs:380; #[allow] over #[expect] is correct since the lint is Linux-only per clippy.toml's stack-size-threshold.
  • Confirmed large_stack_frames = "deny" in workspace Cargo.toml and no CODEOWNERS on either file.
Extended reasoning...

Overview

Two-file, lint-only PR unbreaking cargo clippy on main:

  • src/paths/resolve_path.rs: 3 lines → 1 line, removing an intermediate let result binding whose only consumer (a debug_assert) was deleted in #33874. The returned expression &mut buf[0..buf_i] is byte-identical.
  • src/runtime/jsc_hooks.rs: adds a 6-line #[allow(clippy::large_stack_frames, reason = ...)] attribute above init_runtime_state. No code inside the function changes.

Security risks

None. Neither change alters control flow, data flow, allocation, or any value observable at runtime. The first is a pure syntactic refactor of a return expression; the second is a compile-time lint attribute.

Level of scrutiny

Low. These are the canonical shapes for their respective clippy fixes:

  • let_and_return → inline the expression (clippy's own machine-applicable suggestion).
  • large_stack_frames → the repo already suppresses this identically at src/react_compiler/lowering/find_context_identifiers.rs:380-383. The reason = string is accurate (verified Transpiler::init is called and ptr::writen into (*vm).transpiler per the surrounding doc comment), and the choice of #[allow] over #[expect] is correctly justified — an #[expect] would fail on macOS where the lint doesn't fire.

Other factors

  • Workspace Cargo.toml:242 sets large_stack_frames = "deny", confirming why this is a hard clippy failure rather than a warning.
  • No CODEOWNERS entries match either file.
  • The PR description's verification (path tests, node parity, clippy repro) is thorough but almost superfluous given zero behavioral surface — nothing here can change what normalize_string_generic_tz returns or how init_runtime_state executes.
  • No outstanding reviewer comments; only bot activity in the timeline.

@Jarred-Sumner
Jarred-Sumner merged commit e3896ab into main Jul 11, 2026
21 of 40 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the ciro/clippy-let-and-return branch July 11, 2026 02:15
Jarred-Sumner pushed a commit that referenced this pull request Jul 11, 2026
Follow-up to #33951, which unbroke `cargo clippy` on the Clippy
workflow's runner (`x86_64-unknown-linux-gnu`). The workspace is still
not clean on the other hosts, so `bun run rust:clippy` fails for anyone
developing on an Apple Silicon Mac, and for the aarch64 Linux target.

Measured on `main` (`55ba6e453c`) vs this branch, `cargo clippy
--workspace --no-deps --keep-going`:

| | macOS arm64 (host) | aarch64-unknown-linux-gnu |
x86_64-unknown-linux-gnu |
| --- | --- | --- | --- |
| `main` | ❌ 2 errors | ❌ 2 errors | ✅ |
| this PR | ✅ | ✅ | ✅ |

## `ptr_cast_constness` — aarch64 only

`c_char` is `u8` on aarch64 and `i8` on x86_64, so these casts change
*only* constness on aarch64 and the lint fires there but not on the
runner:

- `bun_core::util::dupe_z` — `p as *const c_char`
- `bun_alloc::free_sensitive_cstr` — `p as *mut u8`, `p as *mut c_void`

Rewritten with `.cast()` / `.cast_const()` / `.cast_mut()`. These are
not "smarter" than `as`: in `core` they are literally `#[inline(always)]
pub const fn cast_const(self) -> *const T { self as _ }`, so each
rewrite is the same single `PtrToPtr` cast with the same address and the
same provenance.

The `*const` → `*mut` laundering in `free_sensitive_cstr` is
pre-existing and unchanged by this PR. The pointer's root is a
`default_alloc::malloc` allocation that `dupe_z` already wrote through,
so writing through it here was sound before and is sound now — Rust does
not track mutability in raw-pointer provenance.

## `derivable_impls` — macOS only

`Sendfile`'s `Default` impl is only derivable where the
`#[cfg(any(target_os = "linux", target_os = "android"))]` fields are
absent. On linux/android the impl carries `socket_fd: Fd::INVALID`, and
`Fd` has no `Default` impl at all — `#[derive(Default)]` would not
compile there. Suppressed with the reason recorded.

`#[allow]` rather than `#[expect]` in both suppression sites, because an
expectation would go *unfulfilled* on the platform where the lint
doesn't fire, and `warnings = "deny"` would turn that into an error.

## Verification

`cargo clippy --workspace --no-deps --keep-going` exits 0 on all three
targets above (and `cargo fmt --check` is clean). Built `bun-debug` and
exercised the touched crates through the CLI —
`path.resolve`/`path.win32.normalize`, `process.env`, `Bun.spawnSync` —
plus `bun bd test test/js/node/path/` → 122 pass, 0 fail.

Reverting just this commit reproduces both failures on macOS and
aarch64, so the changes are load-bearing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants