feat(napi): provide unsafe as_mut on ArrayBuffer#3055
Conversation
WalkthroughA new unsafe method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR adds an unsafe as_mut method to the ArrayBuffer type, allowing mutable access to the underlying byte buffer from Rust. The method is properly marked as unsafe with clear documentation about the undefined behavior risks from concurrent JavaScript modifications.
Key changes:
- Adds
as_mut()method toArrayBufferwith appropriate safety documentation - Provides a working example function
mutate_arraybufferthat demonstrates usage - Includes comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs | Implements the core as_mut() method with unsafe marker and safety documentation |
| examples/napi/src/typed_array.rs | Adds example function demonstrating proper unsafe usage of the new method |
| examples/napi/tests/values.spec.ts | Adds test case verifying the mutation behavior works correctly |
| examples/napi/index.d.cts | Exports TypeScript type definition for the new function |
| examples/napi/index.cjs | Exports the new function in CommonJS format |
| examples/napi/example.wasi.cjs | Exports the new function for WASI environment |
| examples/napi/example.wasi-browser.js | Exports the new function for WASI browser environment |
| examples/napi/tests/snapshots/values.spec.ts.snap | Updates snapshot file with test results |
| examples/napi/tests/snapshots/values.spec.ts.md | Updates markdown snapshot with type definition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/napi/src/typed_array.rs (1)
309-315: ArrayBuffer mutation mirrors existing TypedArray pattern; looks goodImplementation is minimal, mirrors
mutate_typed_array, and keeps the unsafe usage tightly scoped to iterating overbuf.as_mut(). No issues from the example-layer perspective.crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (1)
331-338: TightenArrayBuffer::as_mutsafety surface and align with existing lint conventionsThe implementation itself looks fine and consistent with other raw mut accessors, but two small improvements would help:
- This is a
pubmethod namedas_mut, so Clippy’sshould_implement_traitlint will likely fire. Other similar APIs in this file use#[allow(clippy::should_implement_trait)]; it would be good to do the same here for consistency.- Since
ArrayBuffer<'env>isCopyand you’re exposing a mutable slice into JS-managed memory, it’s worth spelling out in the safety docs that callers must ensure there are no other active Rust references (including clonedArrayBuffers) used while the returned&mut [u8]is alive, in addition to the JS-side synchronization caveat you already mention.A minimal change along these lines could look like:
- /// # Safety - /// - /// This is literally undefined behavior, as the JS side may always modify the underlying buffer, - /// without synchronization. - pub unsafe fn as_mut(&mut self) -> &mut [u8] { - std::slice::from_raw_parts_mut(self.data.as_ptr() as *mut u8, self.data.len()) - } + #[allow(clippy::should_implement_trait)] + /// # Safety + /// + /// This is literally undefined behavior, as the JS side may always modify the underlying buffer, + /// without synchronization. + /// + /// In addition, the caller must ensure there are no other active Rust references + /// (including cloned `ArrayBuffer` values) used to access this buffer while the + /// returned slice is alive. + pub unsafe fn as_mut(&mut self) -> &mut [u8] { + std::slice::from_raw_parts_mut(self.data.as_ptr() as *mut u8, self.data.len()) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs(1 hunks)examples/napi/__tests__/__snapshots__/values.spec.ts.md(1 hunks)examples/napi/__tests__/values.spec.ts(2 hunks)examples/napi/example.wasi-browser.js(1 hunks)examples/napi/example.wasi.cjs(1 hunks)examples/napi/index.cjs(1 hunks)examples/napi/index.d.cts(1 hunks)examples/napi/src/typed_array.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (2)
crates/napi/src/bindgen_runtime/js_values/buffer.rs (1)
as_mut(424-429)crates/napi/src/js_values/arraybuffer.rs (1)
as_mut(231-236)
examples/napi/src/typed_array.rs (1)
crates/macro/src/lib.rs (1)
napi(29-43)
examples/napi/index.d.cts (1)
examples/napi/example.wasi-browser.js (2)
mutateArraybuffer(297-297)mutateArraybuffer(297-297)
examples/napi/__tests__/values.spec.ts (1)
examples/napi/example.wasi-browser.js (2)
mutateArraybuffer(297-297)mutateArraybuffer(297-297)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (41)
- GitHub Check: Agent
- GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
- GitHub Check: build - aarch64-unknown-linux-gnu
- GitHub Check: build - x86_64-unknown-linux-gnu
- GitHub Check: build - powerpc64le-unknown-linux-gnu
- GitHub Check: Build only test - aarch64-unknown-linux-ohos
- GitHub Check: build - s390x-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
- GitHub Check: Test freebsd target
- GitHub Check: Build only test - armv7-linux-androideabi
- GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: Build only test - aarch64-linux-android
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: Build only test - loongarch64-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
- GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
- GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
- GitHub Check: build - aarch64-unknown-linux-musl
- GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
- GitHub Check: build - x86_64-unknown-linux-musl
- GitHub Check: build-and-test-msys2 (CLANG64)
- GitHub Check: build-and-test-msys2 (MINGW64)
- GitHub Check: build-and-test-msys2 (UCRT64)
- GitHub Check: Test node wasi target (--cfg tokio_unstable)
- GitHub Check: Test node wasi target
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
- GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
- GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
- GitHub Check: Memory leak detect job
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: ASAN - windows-latest
🔇 Additional comments (6)
examples/napi/index.cjs (1)
806-806: CJS export formutateArraybufferis wired correctlyExport name and source (
nativeBinding.mutateArraybuffer) are consistent with the rest of the binding surface.examples/napi/__tests__/values.spec.ts (1)
145-145: NewmutateArraybuffertest is consistent and effectiveImport and test setup mirror the existing
mutateTypedArraycase, including the WASI skip guard, and correctly assert in‑place mutation of the underlying ArrayBuffer bytes.Also applies to: 1295-1309
examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)
891-892: Snapshot update formutateArraybuffermatches the new APIThe added declaration in the snapshot aligns with the TypeScript d.ts and the runtime export; no issues.
examples/napi/index.d.cts (1)
851-852: Type declaration formutateArraybufferis consistent with implementation
ArrayBufferparameter andvoidreturn type correctly describe the new native function and line up with the tests and exports.examples/napi/example.wasi-browser.js (1)
297-297: WASI browser wrapper correctly re-exportsmutateArraybufferRe-export follows the existing pattern and keeps the WASI/browser surface in sync with the main binding.
examples/napi/example.wasi.cjs (1)
342-342: WASI CJS wrapper export formutateArraybufferis correctly wiredThe new assignment cleanly exposes the underlying N-API export without altering existing behavior.
Brooooooklyn
deleted the
12-09-feat_napi_provide_unsafe_as_mut_on_arraybuffer
branch
|
|
||
| #[napi] | ||
| pub fn mutate_arraybuffer(mut buf: ArrayBuffer) { | ||
| for item in unsafe { buf.as_mut() } { |
There was a problem hiding this comment.
Should provide a user document explaining in what scenarios this capability needs to be used?
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.