GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Switch to Cell::update, slice::{split_first_chunk,split_off}, where appropriate by coolreader18 · Pull Request #6974 · RustPython/RustPython 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
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) + }
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.
There was an error while loading. Please reload this page.
| if let Some(&bytes) = buffer.first_chunk::<{ size_of::<usize>() }>() { | ||
| return Ok(usize::from_ne_bytes(bytes)); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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).
- 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.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
find . -name "Cargo.toml" -type f | head -20
Repository: 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 f
Repository: 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.toml
Repository: 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 -100
Repository: 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ppropriate (#6974) * Use Cell::update, slice::{split_first_chunk,split_off} * Use more array -> slice methods
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.