Skip to content

napi: add the missing safety comment on free_orphaned's unsafe block - #34125

Closed
cirospaciari wants to merge 1 commit into
mainfrom
ciro/clippy-napi-safety
Closed

cirospaciari wants to merge 1 commit into
mainfrom
ciro/clippy-napi-safety

Conversation

@cirospaciari

Copy link
Copy Markdown
Member

cargo clippy is failing on main again, and therefore on every open pull request that touches a .rs file:

error: unsafe block missing a safety comment
  --> src/runtime/napi/napi_body.rs:2811:14
   = note: requested on the command line with `-D clippy::undocumented-unsafe-blocks`
error: could not compile `bun_runtime` (lib) due to 1 previous error

undocumented_unsafe_blocks = "deny" is set workspace-wide. #34067 added ThreadSafeFunction::free_orphaned, whose doc comment carries a SAFETY: paragraph describing the function's own contract — but the lint wants a comment attached to the unsafe block. Its sibling free, ten lines above, already has exactly the comment this one needs:

    // SAFETY: `this` was allocated by heap::alloc in `new`.
    drop(unsafe { bun_core::heap::take(this) });

Why it keeps happening

.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. Since pull_request lints the PR merged into main, breaks introduced on main surface on contributors' PRs and look like their fault.

This is the third such breakage in a few days (#33951 fixed let_and_return in bun_paths and large_stack_frames in bun_runtime; #33958 fixed the aarch64/macOS-only lints). Adding a push: trigger — or a scheduled run on main — would catch these at the source. Happy to send that separately if wanted.

Verification

  • Reproduced on a clean main tip (16c557635b) before changing anything; cargo clippy -p bun_runtime --no-deps reports the error, and reverting the one-line fix reproduces it again (2 errors → 0).
  • After the fix, cargo clippy --workspace --no-deps --keep-going exits 0 on all three: macOS arm64 (host), x86_64-unknown-linux-gnu (what the Clippy workflow runs), and aarch64-unknown-linux-gnu.
  • cargo fmt --check -p bun_runtime clean.

Comment-only; no runtime surface.

`cargo clippy` fails on main, and so on every open pull request that touches a
.rs file:

    error: unsafe block missing a safety comment
      --> src/runtime/napi/napi_body.rs:2811:14
    error: could not compile `bun_runtime` (lib) due to 1 previous error

`undocumented_unsafe_blocks` is denied workspace-wide. #34067 added
`free_orphaned`, whose doc comment carries a `SAFETY:` paragraph for the
function's own contract, but the lint wants a comment on the unsafe *block*.
Its sibling `free` ten lines up already has exactly the one this needs.

The Clippy workflow has no `push:` trigger, so main never lints itself and
this only surfaces on contributors' PRs, attributed to their commits.
@robobun

robobun commented Jul 14, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 3:33 AM PT - Jul 14th, 2026

✅ @cirospaciari, your commit 1afd910a71efd84702d4592b624cfbdf51b205a7 passed in Build #72756! 🎉


🧪   To try this PR locally:

bunx bun-pr 34125

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

bun-34125 --bun

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

A safety comment was added to ThreadSafeFunction::free_orphaned, documenting that the pointer was allocated via heap::alloc in new. No logic or public API signatures changed.

Changes

Thread-safe function cleanup

Layer / File(s) Summary
Document allocation safety
src/runtime/napi/napi_body.rs
Adds a SAFETY comment in ThreadSafeFunction::free_orphaned describing the allocation contract established by new.

Possibly related PRs

  • oven-sh/bun#34067: Related TSFN teardown, freeing, and orphaning lifecycle changes.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly states the main change: adding the missing safety comment to free_orphaned.
Description check ✅ Passed The description explains the fix and includes verification steps, covering the required information despite different headings.
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.

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

@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/napi/napi_body.rs`:
- Line 2811: Update the SAFETY comment in ThreadSafeFunction::new to state that
the pointer originates from bun_core::heap::into_raw(Box::new(init)), while
preserving the note that heap::take is valid for reclaiming it.
🪄 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: b4a8cdd1-5abc-46ea-95e1-8edc63093ef5

📥 Commits

Reviewing files that changed from the base of the PR and between 16c5576 and 1afd910.

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

/// SAFETY: `this` is a live allocation from `new`, the caller holds no
/// lock on it, and no other thread holds a reference.
unsafe fn free_orphaned(this: *mut ThreadSafeFunction) {
// SAFETY: `this` was allocated by heap::alloc in `new`.

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the actual allocation path.

ThreadSafeFunction::new uses bun_core::heap::into_raw(Box::new(init)) at Line 2473, not heap::alloc. heap::take is still valid for this pointer, but this SAFETY comment should accurately describe the provenance.

Proposed fix
-        // SAFETY: `this` was allocated by heap::alloc in `new`.
+        // SAFETY: `this` was allocated by `heap::into_raw(Box::new(init))` in `new`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// SAFETY: `this` was allocated by heap::alloc in `new`.
// SAFETY: `this` was allocated by `heap::into_raw(Box::new(init))` in `new`.
🤖 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/napi/napi_body.rs` at line 2811, Update the SAFETY comment in
ThreadSafeFunction::new to state that the pointer originates from
bun_core::heap::into_raw(Box::new(init)), while preserving the note that
heap::take is valid for reclaiming it.

@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 — comment-only clippy fix.

What was reviewed:

  • Verified the added // SAFETY: comment on free_orphaned's unsafe block is verbatim identical to the sibling at napi_body.rs:2800 (free), which wraps the same bun_core::heap::take(this) call.
  • Confirmed no code changes — the drop(unsafe { ... }) line itself is untouched; only the comment line is inserted.
Extended reasoning...

Overview

This PR adds a single // SAFETY: comment above the unsafe { bun_core::heap::take(this) } block inside ThreadSafeFunction::free_orphaned in src/runtime/napi/napi_body.rs. The workspace has clippy::undocumented_unsafe_blocks = "deny", and #34067 introduced this function with a doc-comment SAFETY paragraph on the function but no comment on the inner block, breaking clippy on main. The added comment is copied verbatim from the sibling free function ten lines above (line 2800), which wraps an identical heap::take call.

Security risks

None. This is a comment-only change with zero effect on generated code or runtime behavior.

Level of scrutiny

Minimal. This is the canonical trivial-approve case: a one-line comment addition to satisfy a lint, matching an existing adjacent pattern exactly. The PR description documents reproduction on clean main, verification that clippy passes on three targets after the fix, and cargo fmt --check cleanliness.

Other factors

  • No prior reviews or outstanding comments on the PR (only the robobun build notification).
  • The bug hunting system found no issues.
  • I verified in the current tree that both the sibling comment at line 2800 and the new comment at line 2811 are present and identical, and that the drop(unsafe { ... }) line was not modified.
  • The PR description's suggestion about adding a push: trigger to the clippy workflow is explicitly out of scope for this PR ("Happy to send that separately"), so there's nothing else to review here.

@cirospaciari

Copy link
Copy Markdown
Member Author

Superseded — main already carries a safety comment on free_orphaned (different wording, same effect), so this is now a no-op. Verified cargo clippy --workspace --no-deps --keep-going --target x86_64-unknown-linux-gnu exits 0 on cc0c1e8355. Closing.

The underlying issue stands though: this is the third clippy break to reach main unnoticed (after #33951 and #33958), because .github/workflows/clippy.yml has no push: trigger — main never lints itself, so each break is attributed to whichever PR runs next. A push: trigger (or a scheduled run on main) would catch these at the source.

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.

2 participants