Switch to Cell::update, slice::{split_first_chunk,split_off}, where appropriate - #6974
Conversation
π WalkthroughWalkthroughThis PR refactors multiple crates to use safer, more idiomatic Rust patterns for buffer/slice operations. Changes replace manual bounds checks and unwraps with Changes
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 warning)
β Passed checks (2 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ 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 |
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 | π‘ MinorGuard against overflow and panic in
read_array.
N as u32truncates for largeN, andunwrap()can panic if the slice length doesnβt match. Prefer a fallible conversion and error mapping in this public API.Based on learnings: Follow Rust best practices for error handling and memory management.π‘οΈ Suggested fix
- 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) + }
π€ Fix all issues with AI agents
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.
| if let Some(&bytes) = buffer.first_chunk::<{ size_of::<usize>() }>() { | ||
| return Ok(usize::from_ne_bytes(bytes)); | ||
| } |
There was a problem hiding this comment.
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));
}π 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.
| 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)); | |
| } |
π€ Prompt for AI Agents
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.
| 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.
𧩠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 relatedfirst_chunk_mut,split_first_chunk, etc.) was stabilized in Rust 1.77.0. [1]as_array()(andas_mut_array()) is not from 1.76/1.77;slice::as_arraywas 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 Agents
In `@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.
Summary by CodeRabbit
New Features
Bug Fixes