Remove duplicated richcompare pymethods and add missing rops' wrapper, getattr, proper bool impl #6579

youknowone · 2025-12-29T14:05:30Z

Summary by CodeRabbit

New Features
- Added support for __getattr__ and __rmul__ operations via slot definitions.
Refactor
- Standardized boolean evaluation across built-in types to use numeric protocol instead of individual methods.
- Removed explicit comparison operators from the base object level; comparison mechanisms continue via internal slot handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-29T14:05:36Z

📝 Walkthrough

Walkthrough

This PR refactors Python object truthiness evaluation by systematically migrating __bool__ methods to the AsNumber numeric protocol across multiple builtin types, removes Python-level comparison and representation operators from PyBaseObject in favor of slot-based mechanisms, and enhances the slot system with recursive subclass updates and improved __getattr__ resolution logic.

Changes

Cohort / File(s)	Summary
Comparison/representation removal from base object `crates/vm/src/builtins/object.rs`	Removed 8 public PyMethods: `__eq__`, `__ne__`, `__lt__`, `__le__`, `__ge__`, `__gt__`, `__repr__`, `__hash__`. Comparison and hashing now rely on slot-based mechanisms rather than explicit Python-level methods.
Migration of bool to AsNumber protocol `crates/vm/src/builtins/range.rs`, `crates/vm/src/builtins/singletons.rs`, `crates/vm/src/builtins/tuple.rs`, `crates/vm/src/stdlib/collections.rs`, `crates/vm/src/stdlib/ctypes/function.rs`, `crates/vm/src/stdlib/ctypes/pointer.rs`, `crates/vm/src/stdlib/ctypes/simple.rs`, `crates/vm/src/stdlib/winreg.rs`	Consistent pattern across 8 files: added `AsNumber` to pyclass traits, implemented `AsNumber` with boolean handlers (truthiness checks), and removed explicit `__bool__` methods. Each type's boolean behavior now routes through the numeric protocol.
Weak proxy numeric protocol `crates/vm/src/builtins/weakproxy.rs`	Added `AsNumber` to `PyWeakProxy` class declaration and implemented `AsNumber::as_number()` with a boolean handler that delegates to the proxied object's `is_true()`. Removed `__bool__` method.
Slot system enhancements `crates/vm/src/types/slot.rs`	Added `update_subclasses<const ADD: bool>()` to propagate slot updates recursively through subclass hierarchies. Enhanced `TpGetattro` slot resolution to check for `__getattr__` presence in MRO and use wrapper when available, falling back to local/MRO slot resolution otherwise.
Slot definitions expansion `crates/vm/src/types/slot_defs.rs`	Added two new slot entries: `__getattr__` (mapped to `TpGetattro`) and `__rmul__` (mapped to `SqRepeat`).
Operator wrapping logic `crates/vm/src/class.rs`	Modified `add_operators()` to skip creating wrapper descriptors for `__getattr__` slot, aligning with Python's semantics where `__getattr__` is not present on `object` by default.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

RustPython/RustPython#6505: Modifies slot inheritance machinery and introduces AsNumber/PyNumber protocol changes that directly parallel the protocol migrations in this PR.
RustPython/RustPython#6562: Moves comparison dunders (__eq__, __ne__, __lt__, etc.) into slot/Comparable machinery, directly complementing the removal of these methods from PyBaseObject.
RustPython/RustPython#6536: Implements the foundational slot/descriptor refactor (PyWrapper, SLOT_DEFS reorganization) that this PR builds upon for recursive slot updates and new slot definitions.

Suggested reviewers

arihant2math

Poem

🐰 No more bools scattered about,
AsNumber makes truthiness devout,
Slots cascade through families deep,
Where __getattr__ secrets keep,
A cleaner class hierarchy we reap! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 74.36% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: removal of duplicated richcompare pymethods from object, addition of missing right-hand operation wrappers (rmul), migration of bool to numeric protocol, and getattr handling.

✨ Finishing touches

📝 Generate docstrings

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

github-actions · 2025-12-29T14:05:54Z

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin update-slot

abubakarabid816-alt · 2025-12-29T17:04:33Z

Continue to python

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

crates/vm/src/types/slot_defs.rs (1)

989-994: New __getattr__ and sequence __rmul__ slot defs are consistent with existing patterns

__getattr__ mapped to TpGetattro mirrors how __getattribute__ is exposed in slots, and combined with the add_operators skip in class.rs keeps default object from gaining an unintended __getattr__.

Adding a SqRepeat‑backed __rmul__ mirrors the existing dual definitions for __add__ (number + sequence), which should help right‑hand repetition for sequence types behave more like CPython.

Consider adding a small test alongside test_find_by_name that asserts the expected count for "__rmul__" now that it appears both as a number and sequence slot, to guard future refactors of SLOT_DEFS.

Also applies to: 1429-1433

crates/vm/src/stdlib/ctypes/function.rs (1)

15-17: PyCFuncPtr numeric/boolean behavior via AsNumber looks sound

Using AsNumber for CFuncPtr and defining boolean as Ok(zelf.get_func_ptr() != 0) aligns with ctypes’ expectation that a function pointer is falsy only when it’s NULL. The new #[pyclass(... with(Callable, Constructor, AsNumber, Representable, AsBuffer))] block correctly surfaces this behavior at the type level, and the implementation reuses the existing pointer‑extraction logic.

It may be worth adding/adjusting a small test to cover bool(CFuncPtr(0)) vs bool(CFuncPtr(<nonzero addr>)) to lock in this behavior, especially since the truthiness path has moved from an explicit __bool__ method into the numeric protocol.

Also applies to: 1755-1854, 1856-1867

📜 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 37c47fc and 0aa1801.

📒 Files selected for processing (13)

crates/vm/src/builtins/object.rs
crates/vm/src/builtins/range.rs
crates/vm/src/builtins/singletons.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/builtins/weakproxy.rs
crates/vm/src/class.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/stdlib/ctypes/function.rs
crates/vm/src/stdlib/ctypes/pointer.rs
crates/vm/src/stdlib/ctypes/simple.rs
crates/vm/src/stdlib/winreg.rs
crates/vm/src/types/slot.rs
crates/vm/src/types/slot_defs.rs

💤 Files with no reviewable changes (2)

crates/vm/src/stdlib/ctypes/simple.rs
crates/vm/src/builtins/object.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/class.rs
crates/vm/src/types/slot_defs.rs
crates/vm/src/stdlib/winreg.rs
crates/vm/src/stdlib/ctypes/function.rs
crates/vm/src/stdlib/ctypes/pointer.rs
crates/vm/src/builtins/singletons.rs
crates/vm/src/types/slot.rs
crates/vm/src/builtins/range.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/builtins/weakproxy.rs
crates/vm/src/builtins/tuple.rs

🧠 Learnings (3)

📚 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/range.rs
crates/vm/src/builtins/weakproxy.rs

📚 Learning: 2025-08-26T05:20:54.540Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Applied to files:

crates/vm/src/stdlib/collections.rs

📚 Learning: 2025-08-26T05:20:54.540Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.

Applied to files:

crates/vm/src/stdlib/collections.rs

🧬 Code graph analysis (9)

crates/vm/src/stdlib/winreg.rs (7)

crates/vm/src/stdlib/ctypes/pointer.rs (3)

a (195-196)

zelf (73-74)

zelf (168-168)

crates/vm/src/stdlib/ctypes/simple.rs (2)

a (503-504)

zelf (250-251)

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

a (1566-1566)

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

zelf (278-282)

crates/vm/src/exceptions.rs (1)

zelf (1720-1720)

crates/vm/src/stdlib/io.rs (1)

zelf (3682-3682)

crates/vm/src/exception_group.rs (1)

zelf (212-213)

crates/vm/src/stdlib/ctypes/function.rs (6)

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

as_number (422-431)

number (425-425)

zelf (278-282)

crates/vm/src/stdlib/ctypes/pointer.rs (5)

as_number (192-208)

as_number (777-786)

number (780-780)

zelf (73-74)

zelf (168-168)

crates/vm/src/stdlib/ctypes/structure.rs (2)

as_number (409-425)

zelf (136-137)

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

as_number (604-607)

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

as_number (176-184)

crates/vm/src/stdlib/ctypes/union.rs (1)

zelf (305-306)

crates/vm/src/stdlib/ctypes/pointer.rs (7)

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

as_number (68-74)

as_number (110-119)

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

as_number (422-431)

number (425-425)

zelf (278-282)

crates/vm/src/stdlib/ctypes/function.rs (2)

as_number (1857-1866)

number (1860-1860)

crates/vm/src/stdlib/winreg.rs (1)

as_number (255-288)

crates/vm/src/stdlib/ctypes/structure.rs (2)

as_number (409-425)

zelf (136-137)

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

as_number (604-607)

crates/vm/src/stdlib/ctypes/union.rs (1)

zelf (305-306)

crates/vm/src/builtins/singletons.rs (4)

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

as_number (1893-1893)

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

as_number (171-180)

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

as_number (604-607)

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

as_number (1760-1766)

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

crates/vm/src/types/slot_defs.rs (1)

find_slot_defs_by_name (918-920)

crates/vm/src/builtins/range.rs (7)

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

as_number (68-74)

as_number (110-119)

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

as_number (422-431)

number (425-425)

zelf (278-282)

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

as_number (171-180)

number (174-174)

crates/vm/src/stdlib/ctypes/pointer.rs (5)

as_number (192-208)

as_number (777-786)

number (780-780)

zelf (73-74)

zelf (168-168)

crates/vm/src/stdlib/winreg.rs (1)

as_number (255-288)

crates/vm/src/stdlib/ctypes/simple.rs (3)

as_number (498-516)

as_number (1455-1469)

zelf (250-251)

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

as_number (677-689)

crates/vm/src/stdlib/collections.rs (3)

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

as_number (426-435)

number (429-429)

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

as_number (422-431)

number (425-425)

zelf (278-282)

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

as_number (171-180)

number (174-174)

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

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

as_number (426-435)

number (429-429)

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

as_number (422-431)

number (425-425)

zelf (278-282)

crates/vm/src/builtins/tuple.rs (4)

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

as_number (426-435)

number (429-429)

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

as_number (171-180)

number (174-174)

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

as_number (604-607)

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

as_number (1760-1766)

zelf (1718-1718)

zelf (1720-1720)

🔇 Additional comments (15)

crates/vm/src/class.rs (1)

37-42: Skipping __getattr__ wrapper from slots looks correct

The special-casing here avoids giving object (and other types that only define tp_getattro) a synthetic __getattr__, which matches CPython’s behavior that only __getattribute__ is present by default. This aligns with the comment and the new slot def for __getattr__.

If you haven’t already, it’s worth double‑checking that types which explicitly define __getattr__ still get their method via the normal method/slot plumbing and don’t rely on this wrapper path.

crates/vm/src/stdlib/winreg.rs (1)

266-269: Truthiness for PyHkey via AsNumber looks correct

Using !zelf.hkey.load().is_null() matches the expected bool(HKEY) semantics (non‑null handle is truthy, closed/detached is falsy), and the downcast is safe given this is PyHkey’s AsNumber implementation.

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

11-15: PyRange truthiness via AsNumber is correctly wired

Exposing AsNumber on range and implementing boolean as Ok(!zelf.is_empty()) is semantically correct (bool(range) is false only when empty) and consistent with the container pattern used for tuple/deque. The static PyNumberMethods setup also follows the existing idiom in this crate.

Also applies to: 175-185, 425-435

crates/vm/src/stdlib/collections.rs (1)

15-22: Deque AsNumber implementation aligns with container truthiness

Hooking PyDeque into AsNumber and implementing boolean as Ok(!zelf.borrow_deque().is_empty()) matches Python’s bool(deque) semantics and mirrors the existing pattern for other containers. The use of a read lock keeps it non‑intrusive w.r.t. existing mutation/state tracking.

Also applies to: 63-68, 495-506

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

740-777: TpGetattro getattr detection logic is correct.

The enhanced logic properly handles the case where __getattr__ may be defined anywhere in the MRO:

Line 745-757: Correctly checks both self's attributes and all MRO classes (skipping index 0 which is self)

Line 759-761: When __getattr__ is found, forces the wrapper to ensure the fallback behavior works

Lines 762-776: Standard slot inheritance when no __getattr__ is present

This aligns with CPython's behavior where native tp_getattro implementations don't call __getattr__, requiring the wrapper when __getattr__ is defined.

618-643: Lock implementation is safe; no deadlock risk from lock ordering.

The method properly handles weak references and skips subclasses with their own definitions. The hierarchical lock acquisition pattern (parent → child only) is safe. RustPython's single-threaded Cell-based lock implementation detects potential re-entrancy issues at runtime rather than causing deadlocks, making the original concern about cross-thread lock ordering inapplicable. Recursion depth is bounded by class hierarchy depth and poses no practical risk.

crates/vm/src/stdlib/ctypes/pointer.rs (2)

776-787: AsNumber implementation for PyCPointer is correct.

The boolean semantics (non-NULL pointer = true, NULL = false) correctly matches Python's ctypes behavior where pointer types are truthy when non-NULL. The implementation pattern is consistent with other types in the PR (e.g., PyCFuncPtr at crates/vm/src/stdlib/ctypes/function.rs lines 1856-1865).

262-265: Pyclass declaration correctly includes AsNumber.

The addition of AsNumber to the with(...) clause properly wires the AsNumber implementation into the type's slots. The ordering (Constructor, Initializer, AsNumber, AsBuffer) follows the existing pattern in the codebase.

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

67-75: AsNumber for PyNone correctly implements falsy semantics.

None is always falsy in Python, and returning Ok(false) is the correct implementation. The pattern matches other singleton types in the codebase.

52-53: Empty impl block after bool removal is fine.

The empty impl PyNone {} block is acceptable - it's required for the #[pyclass(with(...))] attribute to attach to. Rust allows empty impl blocks and this pattern is common when methods are provided through traits rather than direct implementations.

109-120: AsNumber for PyNotImplemented with deprecation TODO.

The implementation correctly returns true for current Python semantics. The TODO comment accurately documents the breaking change from bpo-35712:

Python 3.9: DeprecationWarning when using NotImplemented in boolean context

Python 3.14: TypeError

Consider tracking this with an issue to implement the deprecation warning before Python 3.14 compatibility is targeted.

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

421-432: AsNumber implementation for PyTuple is correct and consistent.

The boolean semantics (!zelf.elements.is_empty()) correctly implements Python's tuple truthiness where empty tuples are falsy.

The use of static AS_NUMBER: PyNumberMethods (without LazyLock) is appropriate here since no runtime initialization is needed - this is a const-initializable struct. This matches the pattern in PyRange (lines 425-434 in range.rs).

260-264: Pyclass declaration correctly adds AsNumber to protocol list.

The addition maintains alphabetical ordering of the traits (AsMapping, AsNumber, AsSequence, ...) which aids readability and consistency with other type declarations.

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

170-181: AsNumber for PyWeakProxy correctly delegates to referent's truthiness.

The implementation appropriately:

Uses LazyLock since the closure captures runtime behavior (unlike static const patterns used for simpler types)

Calls try_upgrade(vm)? which will raise ReferenceError if the referent is dead

Delegates to is_true(vm) on the referent, correctly proxying the truthiness

This maintains the proxy semantics where the weak proxy behaves like the underlying object for boolean contexts.

66-76: Pyclass with clause properly includes AsNumber.

The ordering of traits in the with(...) clause is sensible, grouping related functionality together. AsNumber is placed after Comparable following an approximate pattern of core attributes → protocols.

youknowone changed the title ~~Update slot~~ Remove duplicate pymethods Dec 29, 2025

youknowone force-pushed the update-slot branch 3 times, most recently from 6618b1f to 2f297e4 Compare December 29, 2025 16:40

youknowone force-pushed the update-slot branch from 8b5151f to 19adaa2 Compare December 30, 2025 08:21

youknowone changed the title ~~Remove duplicate pymethods~~ Remove duplicated richcompare pymethods and add missing rops' wrapper Dec 30, 2025

youknowone added 3 commits December 31, 2025 12:28

slots: remove duplicate pymethods from object

7519cf9

Add missing __rmul__

1f2ba99

migrate __bool__

a798467

youknowone force-pushed the update-slot branch from 19adaa2 to a798467 Compare December 31, 2025 03:28

subclass_update, __getattr__

0aa1801

youknowone marked this pull request as ready for review December 31, 2025 11:11

youknowone changed the title ~~Remove duplicated richcompare pymethods and add missing rops' wrapper~~ Remove duplicated richcompare pymethods and add missing rops' wrapper, __getattr__, proper __bool__ impl Dec 31, 2025

coderabbitai bot reviewed Dec 31, 2025

View reviewed changes

youknowone merged commit a8cfa36 into RustPython:main Dec 31, 2025
13 checks passed

youknowone deleted the update-slot branch December 31, 2025 11:26

coderabbitai bot mentioned this pull request Jan 1, 2026

sequence, mapping slots and fix separate __delitem__ slots #6621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove duplicated richcompare pymethods and add missing rops' wrapper, getattr, proper bool impl #6579

Remove duplicated richcompare pymethods and add missing rops' wrapper, getattr, proper bool impl #6579

Uh oh!

youknowone commented Dec 29, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 29, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions bot commented Dec 29, 2025

Uh oh!

abubakarabid816-alt commented Dec 29, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove duplicated richcompare pymethods and add missing rops' wrapper, __getattr__, proper __bool__ impl #6579

Remove duplicated richcompare pymethods and add missing rops' wrapper, __getattr__, proper __bool__ impl #6579

Uh oh!

Conversation

youknowone commented Dec 29, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 29, 2025 • 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

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 29, 2025

Uh oh!

abubakarabid816-alt commented Dec 29, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove duplicated richcompare pymethods and add missing rops' wrapper, getattr, proper bool impl #6579

Remove duplicated richcompare pymethods and add missing rops' wrapper, getattr, proper bool impl #6579

youknowone commented Dec 29, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 29, 2025 •

edited

Loading