Switch to Cell::update, slice::{split_first_chunk,split_off}, where appropriate - #6974
Switch to Cell::update, slice::{split_first_chunk,split_off}, where appropriate#6974youknowone merged 2 commits into
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughThis PR refactors multiple crates to use safer, more idiomatic Rust patterns for buffer/slice operations. Changes replace manual bounds checks and unwraps with first_chunk(), map_or(), and other standard library methods, along with adding a new read_array API to the marshalling trait and improving scope-guarded recursion depth management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/compiler-core/src/marshal.rs (1)120-122: ⚠️ Potential issue | 🟡 Minor
Guard against overflow and panic in read_array.
N as u32 truncates for large N, and unwrap() can panic if the slice length doesn’t match. Prefer a fallible conversion and error mapping in this public API.
🛡️ Suggested fixBased on learnings: Follow Rust best practices for error handling and memory management.- fn read_array<const N: usize>(&mut self) -> Result<&[u8; N]> { - self.read_slice(N as u32).map(|s| s.try_into().unwrap()) - } + fn read_array<const N: usize>(&mut self) -> Result<&[u8; N]> { + let n = u32::try_from(N).map_err(|_| MarshalError::Eof)?; + let slice = self.read_slice(n)?; + slice.try_into().map_err(|_| MarshalError::Eof) + }
In `@crates/vm/src/stdlib/ctypes/function.rs`:
- Around line 570-572: The call to buffer.first_chunk::<{ size_of::<usize>()
}>() uses an unqualified size_of and will fail to compile; update that generic
const to use the fully qualified core::mem::size_of (or add a use) so it becomes
buffer.first_chunk::<{ core::mem::size_of::<usize>() }>() and ensure the
surrounding code that returns usize::from_ne_bytes(bytes) remains unchanged.
In `@crates/vm/src/stdlib/io.rs`:
- Around line 2509-2513: Confirm formatting and linting are clean: run cargo fmt
and cargo clippy, and ensure the slice APIs used in get_field (the as_array()
call on buf, the first_chunk() usage, and the BYTE_LEN constant on the type
implementing this method) are correct per MSRV 1.93.0; if clippy flags any style
or safety issues around the macro get_field or the unchecked unwrap() on
first_chunk(), address them (e.g., handle the Option instead of unwrap or add a
clear comment/expect) so the build and lint checks pass.
Sorry, something went wrong.
| if let Some(&bytes) = buffer.first_chunk::<{ size_of::<usize>() }>() { | ||
| return Ok(usize::from_ne_bytes(bytes)); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Unqualified size_of will cause a compilation error.
size_of is not imported and needs to be fully qualified. The rest of the file uses core::mem::size_of consistently (e.g., lines 138, 145, 157, 502).
🐛 Proposed fix- if let Some(&bytes) = buffer.first_chunk::<{ size_of::<usize>() }>() {
+ if let Some(&bytes) = buffer.first_chunk::<{ core::mem::size_of::<usize>() }>() {
return Ok(usize::from_ne_bytes(bytes));
}‼️ 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.
| if let Some(&bytes) = buffer.first_chunk::<{ size_of::<usize>() }>() { | |
| return Ok(usize::from_ne_bytes(bytes)); | |
| } | |
| if let Some(&bytes) = buffer.first_chunk::<{ core::mem::size_of::<usize>() }>() { | |
| return Ok(usize::from_ne_bytes(bytes)); | |
| } |
In `@crates/vm/src/stdlib/ctypes/function.rs` around lines 570 - 572, The call to
buffer.first_chunk::<{ size_of::<usize>() }>() uses an unqualified size_of and
will fail to compile; update that generic const to use the fully qualified
core::mem::size_of (or add a use) so it becomes buffer.first_chunk::<{
core::mem::size_of::<usize>() }>() and ensure the surrounding code that returns
usize::from_ne_bytes(bytes) remains unchanged.
Sorry, something went wrong.
| let buf: &[u8; Self::BYTE_LEN] = buf.as_array()?; | ||
| macro_rules! get_field { | ||
| ($t:ty, $off:ident) => {{ | ||
| <$t>::from_ne_bytes( | ||
| buf[Self::$off..][..core::mem::size_of::<$t>()] | ||
| .try_into() | ||
| .unwrap(), | ||
| ) | ||
| }}; | ||
| ($t:ty, $off:ident) => { | ||
| <$t>::from_ne_bytes(*buf[Self::$off..].first_chunk().unwrap()) | ||
| }; |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
find . -name "Cargo.toml" -type f | head -20Repository: RustPython/RustPython
Length of output: 639
🏁 Script executed:
cat Cargo.toml | grep -A 5 -B 5 "rust-version\|package.version"Repository: RustPython/RustPython
Length of output: 700
🏁 Script executed:
find . -name "rust-toolchain*" -type fRepository: RustPython/RustPython
Length of output: 86
🏁 Script executed:
rg -n "rust-version" --type tomlRepository: RustPython/RustPython
Length of output: 1141
🏁 Script executed:
cat rust-toolchain.tomlRepository: RustPython/RustPython
Length of output: 95
🏁 Script executed:
rg -n "as_array|first_chunk" crates/vm/src/stdlib/io.rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 480
🏁 Script executed:
cd crates/vm && cargo clippy --lib 2>&1 | head -100Repository: RustPython/RustPython
Length of output: 204
🏁 Script executed:
cd crates/vm && cargo fmt -- --check 2>&1 | grep -A 5 "io.rs"Repository: RustPython/RustPython
Length of output: 47
🌐 Web query:
as_array() first_chunk() Rust stabilization version 1.76 1.77
💡 Result:
- first_chunk() (and related first_chunk_mut, split_first_chunk, etc.) was stabilized in Rust 1.77.0. [1]
- as_array() (and as_mut_array()) is not from 1.76/1.77; slice::as_array was stabilized much later, in Rust 1.93.0. [2]
Sources: [1] [2]
MSRV and API compatibility confirmed; ensure linting and formatting checks are clean.
The repository's MSRV (1.93.0) supports both slice APIs: as_array() was stabilized in Rust 1.93.0, and first_chunk() in Rust 1.77.0. No toolchain compatibility issues.
Per coding guidelines, run cargo fmt and cargo clippy to verify formatting and linting are clean before submission.
🤖 Prompt for AI AgentsIn `@crates/vm/src/stdlib/io.rs` around lines 2509 - 2513, Confirm formatting and linting are clean: run cargo fmt and cargo clippy, and ensure the slice APIs used in get_field (the as_array() call on buf, the first_chunk() usage, and the BYTE_LEN constant on the type implementing this method) are correct per MSRV 1.93.0; if clippy flags any style or safety issues around the macro get_field or the unchecked unwrap() on first_chunk(), address them (e.g., handle the Option instead of unwrap or add a clear comment/expect) so the build and lint checks pass.
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes