Code nitpicks by ShaharNaveh · Pull Request #8058 · RustPython/RustPython

ShaharNaveh

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: e1c9059c-ca38-4d41-84aa-1250341b20ac

📥 Commits

Reviewing files that changed from the base of the PR and between 51cc36e and 11305fa.

📒 Files selected for processing (11)

crates/stdlib/src/array.rs
crates/stdlib/src/multiprocessing.rs
crates/stdlib/src/ssl.rs
crates/vm/src/builtins/int.rs
crates/vm/src/builtins/interpolation.rs
crates/vm/src/frame.rs
crates/vm/src/stdlib/_abc.rs
crates/vm/src/stdlib/_ctypes.rs
crates/vm/src/stdlib/marshal.rs
crates/vm/src/vm/vm_new.rs
src/lib.rs

📝 Walkthrough

Walkthrough

Modernize protocol modules: make buffer validation debug-only, promote several methods to const/#[must_use], refactor expression flows, adjust number-slot/operator access, and add VM exception-constructor helpers; update many sites to use specific vm.new_*_error helpers.

Changes

Protocol Implementation Modernization

Layer / File(s)	Summary
Buffer debug validation `crates/vm/src/protocol/buffer.rs`	BufferDescriptor::validate is compiled only under `debug_assertions` and uses `debug_assert_*`; `PyBuffer::new` calls validate only in debug builds; `is_last_dim_contiguous` annotated `#[must_use]`.
Callable, TraceEvent, Iterator hints `crates/vm/src/protocol/callable.rs`, `crates/vm/src/protocol/iter.rs`	Add `#[must_use]` to `PyObject::to_callable`/`is_callable` and `PyIter::new`; make `TraceEvent` derive `Clone, Copy, Eq, PartialEq` and promote predicate helpers to `const fn`; update iterator doc and async stop-iteration arg construction.
Mapping const / clippy `crates/vm/src/protocol/mapping.rs`	Promote `mapping_unchecked` to `pub const fn` and add `#[must_use]`; mark `PyMappingSlots::has_subscript` `#[must_use]`; switch clippy directive to `#[expect(...)]`.
Number protocol refactor `crates/vm/src/protocol/number.rs`	Make `PyNumberMethods::not_implemented` a `pub const fn` returning a static; add `Eq/PartialEq` to `PyNumberBinaryOp`; rewrite `right_method_name`; refactor `PyNumberSlots::copy_from` and accessor logic; add `#[must_use]` to `PyNumber::check`; reformat warnings and error-arm handling.
Object protocol expression refactor `crates/vm/src/protocol/object.rs`	Refactor `bytes`, `dir`, `get_aiter`, `_cmp_inner`, `ascii`, `str`, and class/bases handling to expression/early-return patterns; change some error constructions to direct vm helpers; spacing tweaks.
Sequence const & simplifications `crates/vm/src/protocol/sequence.rs`	Add module doc/imports; change clippy allow→expect; promote `sequence_unchecked` to `pub const fn`; simplify `list` return and refactor `index` to use `enumerate()` + `isize::try_from` for overflow detection; formatting tweaks.
Stdlib & VM exception helpers `crates/vm/src/vm/vm_new.rs`, `crates/stdlib/`, `crates/vm/src/`, `src/lib.rs`	Add `VirtualMachine::new_assertion_error` and `new_unbound_local_error`; replace many `vm.new_exception_msg(..., EXC, ...)` calls with specific `vm.new_*_error` helpers or the new VM helpers across stdlib and VM sites (functools, array, multiprocessing, ssl, int, interpolation, frame unbound-local paths, _abc, _ctypes, marshal, install_pip).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

RustPython/RustPython#8016: Related change touching sequence_unchecked promotion and C-API sequence wrappers.

Suggested reviewers

youknowone
fanninpm

🐰 I hopped through code with careful delight,
Debug asserts rest unless builders run night,
Consts and must_use flags tidy the trace,
Matches and slots find their tidy place,
A rabbit-approved refactor — neat and bright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title "Code nitpicks" is vague and generic, using a non-descriptive term that does not meaningfully convey what the changeset accomplishes.	Revise the title to be more specific about the main changes, such as "Replace manual exception construction with VM helper methods" or "Add const qualifiers and #[must_use] attributes across protocol modules".

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 84.48% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}