slot for numeric ops by youknowone · Pull Request #6619 · RustPython/RustPython

youknowone

📝 Walkthrough

Walkthrough

This PR systematically removes Python method exposure (via #[pymethod] attributes) from multiple operator methods including __add__, __iadd__, __mul__, __imul__, __rmul__, and OR-family operators across builtin and stdlib types, while preserving their Rust implementations. It also refactors sequence repeat slot handling and strengthens MRO-based compatibility checking in slot lookup.

Changes

Cohort / File(s)	Change Summary
Sequence types (array, list, tuple) `crates/stdlib/src/array.rs`, `crates/vm/src/builtins/list.rs`, `crates/vm/src/builtins/tuple.rs`	Removed `#[pymethod]` attributes from `__add__`, `__iadd__`, `__mul__`, `__imul__`, and `__rmul__` exposure, hiding these operator methods from Python API.
String and binary types `crates/vm/src/builtins/str.rs`, `crates/vm/src/builtins/bytes.rs`, `crates/vm/src/builtins/bytearray.rs`	Removed `#[pymethod]` from `__add__`, `__rmul__`, `__rmod__` bindings; renamed/reorganized `__mod__` exposure; updated internal calls to use `__mod__` instead of legacy method names.
Type and union operators `crates/vm/src/builtins/type.rs`, `crates/vm/src/builtins/union.rs`, `crates/vm/src/builtins/genericalias.rs`	Removed `#[pymethod]` attributes from `__or__` and `__ror__` (including reverse-OR bindings), eliminating Python-level OR operator exposure.
Mapping and view operators `crates/vm/src/builtins/dict.rs`, `crates/vm/src/builtins/mappingproxy.rs`	Removed `#[pymethod]` from `__or__`, `__ior__`, `__ror__` and set-operation methods (`__rxor__`, `__rand__`, `__rsub__`), reducing operator availability on dict/view types.
Collections and ctypes `crates/vm/src/stdlib/collections.rs`, `crates/vm/src/stdlib/ctypes/pointer.rs`, `crates/vm/src/stdlib/ctypes/simple.rs`, `crates/vm/src/stdlib/ctypes/structure.rs`	Removed `#[pymethod]` attributes from `__mul__`, `__imul__`, `__rmul__`, `__iadd__` in PyDeque and ctypes multiplication methods, hiding repeat/multiplication operators.
Descriptor and slot infrastructure `crates/vm/src/builtins/descriptor.rs`	Added `ArgSize` to public imports; updated `SeqRepeat` handling to use `ArgSize` with `into()` conversion; introduced type-compatibility runtime check in `PyMethodWrapper.call` to verify bound object type.
Slot wiring refactoring `crates/vm/src/types/slot.rs`	Added private `sequence_repeat_wrapper` and `sequence_inplace_repeat_wrapper` functions; refactored `SqRepeat`/`SqInplaceRepeat` slot handling; modified `lookup_slot_in_mro` signature to accept MRO slice parameter for compatibility verification; introduced `is_subclass_of` helper and cached `mro` variable for MRO-based extraction; tightened wrapper compatibility checks during MRO lookup with per-class compatibility validation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

RichCompare contains pymethod. No AtomicCell for slots #6562 — Modifies slot wiring and sequence/mapping wrapper functions in slot.rs, directly intersecting with the PR's sequence repeat refactoring and SqRepeat/SqInplaceRepeat handling.
Ctypes __mul__ #6305 — Adds/implements numeric __mul__ exposure and AsNumber integration for ctypes types, directly modifying the same ctypes multiplication methods now being hidden.
PyWrapperDescrObject and rewrite toggle_slot #6536 — Rewrites slot/descriptor infrastructure (SlotDef, SlotAccessor, SLOT_DEFS, MRO-driven slot wiring), closely related to the PR's lookup_slot_in_mro signature changes and MRO-aware extraction logic.

Suggested reviewers

arihant2math

Poem

🐰 ✨ Methods now hide from Python's eager sight,
Yet Rust keeps them ready, tucked out of light.
Slots rewire through MRO's careful chain,
Operators vanish—infrastructure's refrain!
Cleanup and care, with a rabbit's delight.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.25% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'slot for numeric ops' is vague and doesn't clearly convey the main purpose of the changes. While the PR does modify numeric operations, the term 'slot' is a technical implementation detail rather than a description of what the change achieves.	Consider a more descriptive title that explains the primary intent, such as 'Remove operator method exposure from Python API' or 'Refactor numeric operation slots for internal use'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3e2fa4 and 9dfc029.

📒 Files selected for processing (17)

crates/stdlib/src/array.rs
crates/vm/src/builtins/bytearray.rs
crates/vm/src/builtins/bytes.rs
crates/vm/src/builtins/descriptor.rs
crates/vm/src/builtins/dict.rs
crates/vm/src/builtins/genericalias.rs
crates/vm/src/builtins/list.rs
crates/vm/src/builtins/mappingproxy.rs
crates/vm/src/builtins/str.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/type.rs
crates/vm/src/builtins/union.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/stdlib/ctypes/pointer.rs
crates/vm/src/stdlib/ctypes/simple.rs
crates/vm/src/stdlib/ctypes/structure.rs
crates/vm/src/types/slot.rs

💤 Files with no reviewable changes (12)

crates/vm/src/builtins/genericalias.rs
crates/vm/src/stdlib/ctypes/structure.rs
crates/vm/src/stdlib/ctypes/simple.rs
crates/vm/src/builtins/dict.rs
crates/vm/src/builtins/type.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/list.rs
crates/stdlib/src/array.rs
crates/vm/src/builtins/union.rs
crates/vm/src/builtins/mappingproxy.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/stdlib/ctypes/pointer.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/vm/src/builtins/str.rs
crates/vm/src/builtins/descriptor.rs
crates/vm/src/builtins/bytes.rs
crates/vm/src/builtins/bytearray.rs
crates/vm/src/types/slot.rs

🧠 Learnings (1)

📚 Learning: 2025-12-27T14:03:49.034Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

crates/vm/src/builtins/descriptor.rs

🧬 Code graph analysis (3)

crates/vm/src/builtins/bytes.rs (3)

crates/vm/src/builtins/bytearray.rs (1)

__mod__ (534-537)

crates/vm/src/builtins/str.rs (1)

__mod__ (947-949)

crates/vm/src/builtins/int.rs (1)

b (659-659)

crates/vm/src/builtins/bytearray.rs (1)

crates/vm/src/builtins/bytes.rs (2)

__mod__ (518-521)

a (670-670)

crates/vm/src/types/slot.rs (2)

crates/vm/src/builtins/type.rs (1)

mro (1680-1682)

crates/vm/src/builtins/descriptor.rs (1)

cls (379-379)

🔇 Additional comments (7)

crates/vm/src/builtins/bytearray.rs (1)

534-537: Remainder (mod) wiring for PyByteArray looks correct

__mod__ simply wraps inner().cformat and AsNumber::remainder now delegates to it via to_pyresult(vm), matching the pattern used for bytes/str and keeping formatting logic centralized. No issues from a correctness or API-consistency standpoint.

Also applies to: 812-819

crates/vm/src/builtins/bytes.rs (1)

518-521: Bytes mod implementation and numeric remainder slot are consistent

The new __mod__ wrapper around inner.cformat plus AsNumber::remainder delegating to it mirrors the bytearray/str setup and keeps %-formatting behavior in one place. The conversion through into() and to_pyresult(vm) is type-safe and idiomatic here.

Also applies to: 669-675

crates/vm/src/builtins/str.rs (1)

947-949: String % formatting routed through mod and AsNumber cleanly

Renaming the helper to __mod__ and having AsNumber::remainder call it centralizes %-formatting via cformat_string while keeping the return as Wtf8Buf converted through to_pyresult(vm). This aligns string behavior with bytes/bytearray and looks semantically correct.

Also applies to: 1556-1562

crates/vm/src/builtins/descriptor.rs (2)

8-8: SeqRepeat now uses ArgSize for count – consistent with size/index semantics

Switching SlotFunc::SeqRepeat to bind (ArgSize,) and pass n.into() to the SeqRepeatFunc aligns sequence repeat with other ArgSize-based sites (e.g., str/bytearray __mul__), improving bounds/overflow handling without changing call sites of the slot itself.

Also applies to: 595-598

776-785: Type-compatibility check in PyMethodWrapper.call matches CPython’s descriptor rules

The added fast_isinstance guard before invoking the wrapped slot prevents calling a method-wrapper on an incompatible instance and produces a clear TypeError, mirroring CPython’s behavior for wrappers bound to the wrong class. This is a good safety improvement with no downside.

crates/vm/src/types/slot.rs (2)

441-449: Sequence repeat slots correctly delegate to __mul__ / __imul__ wrappers

sequence_repeat_wrapper and sequence_inplace_repeat_wrapper now forward to __mul__ / __imul__ via call_special_method, and SqRepeat / SqInplaceRepeat are wired through them with update_sub_slot. This keeps the C-API-style sequence slots in sync with Python-level dunder methods and respects existing type-specific SeqRepeat functions when present.

Also applies to: 1307-1316

1365-1368: MRO-based slot lookup now honors wrapper.typ compatibility

The new is_subclass_of + try_extract logic ensures a wrapper_descriptor’s slot is only reused when the target class’s MRO actually contains wrapper.typ, and it stops once a real method (non-wrapper) is found. Using appropriate MRO slices (&mro, &mro[i+1..]) keeps the subclass relationship checks accurate across the hierarchy. This tightens correctness around slot reuse without affecting existing valid cases.

Also applies to: 1371-1374, 1388-1402

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