GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Code nitpicks by ShaharNaveh · Pull Request #8058 · RustPython/RustPython

Conversation

ShaharNaveh commented Jun 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Closes #xxxx
This PR follows our AI policy

Summary

Summary by CodeRabbit

Optimization
- Buffer descriptor validation now runs only in debug builds, reducing runtime overhead in release builds.
Code Quality
- Added compiler hints to flag ignored return values across several protocol helpers.
- Multiple protocol implementations refactored for clearer control flow and consistency.
Behavioral
- Minor error-message wording and exception-construction consistency changes; iteration/sequence behaviors unchanged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Code nitpicks

54c4cbc

ShaharNaveh force-pushed the nits-4 branch from b56bd73 to 54c4cbc Compare

June 7, 2026 14:55

coderabbitai Bot reviewed

Jun 7, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/vm/src/stdlib/_functools.rs`:
- Line 47: Update the TypeError message in the functools.reduce implementation
so it matches CPython wording: replace the string passed to vm.new_type_error
from "reduce() of empty sequence with no initial value" to "reduce() of empty
iterable with no initial value" (locate the call to vm.new_type_error in the
reduce implementation in _functools.rs).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2c2afbec-1cb2-4611-bc4b-a4cac69cc74a

📥 Commits

Reviewing files that changed from the base of the PR and between b56bd73 and 54c4cbc.

📒 Files selected for processing (8)

crates/vm/src/protocol/buffer.rs
crates/vm/src/protocol/callable.rs
crates/vm/src/protocol/iter.rs
crates/vm/src/protocol/mapping.rs
crates/vm/src/protocol/number.rs
crates/vm/src/protocol/object.rs
crates/vm/src/protocol/sequence.rs
crates/vm/src/stdlib/_functools.rs

🚧 Files skipped from review as they are similar to previous changes (5)

crates/vm/src/protocol/iter.rs
crates/vm/src/protocol/buffer.rs
crates/vm/src/protocol/callable.rs
crates/vm/src/protocol/object.rs
crates/vm/src/protocol/sequence.rs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/stdlib/_functools.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh added 2 commits

June 7, 2026 18:15


          fix error message

51cc36e


          Use vm.new_.*_error methods

11305fa

youknowone approved these changes

Jun 11, 2026

View reviewed changes

youknowone merged commit b77ada9 into RustPython:main

Jun 11, 2026

26 checks passed

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot mentioned this pull request

Jun 20, 2026

More clippy rules #8127

Merged

2 tasks

youknowone pushed a commit that referenced this pull request

Sep 16, 2026


          Code nitpicks (#8058)

e6b1fd0

* Code nitpicks

* fix error message

* Use `vm.new_.*_error` methods

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ShaharNaveh commented Jun 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Jun 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.