Fix subclass right-op dispatch for Python classes by rlaisqls · Pull Request #7462 · RustPython/RustPython

rlaisqls · 2026-03-18T12:27:25Z

When both operands are Python-defined classes, the inherited right-op was incorrectly given priority over the base class left-op, even when the subclass didn't override anything.

class A:
    def __floordiv__(self, other): return "A.__floordiv__"
    def __rfloordiv__(self, other): return "A.__rfloordiv__"

class B(A):
    pass

A() // B()
# Expected: "A.__floordiv__" (B didn't override, so base left-op wins)
# Got:      "A.__rfloordiv__" (inherited right-op was incorrectly prioritized)

The root cause is that binary_op1 and ternary_op compared left(A) vs
right(B) slot pointers to decide whether two types share the same implementation.

Since RustPython uses separate left/right slot functions (unlike CPython's single combined slot),
this comparison always returned "different" for Python classes, causing the
subclass right-op path to fire unconditionally.

Fix:

Compare left(A) vs left(B) instead, which correctly identifies same-implementation types
Add method_is_overloaded (similar to CPython method_is_overloaded) to check whether the subclass actually overrides the right-op at the Python method level
Add right_method_name to PyNumberBinaryOp / PyNumberTernaryOp for the op-to-dunder mapping needed by method_is_overloaded
Apply to both binary_op1 and ternary_op

Summary by CodeRabbit

New Features
- Added support for right-side numeric operator methods so objects can provide r* implementations for binary and ternary numeric ops.
- New accessors and helpers expose and identify right-side operator slots for use by extensions and tooling.
Bug Fixes
- Improved operator dispatch so right-side methods and method overloading between related types are resolved more accurately.

coderabbitai · 2026-03-18T12:27:43Z

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

Lib/test/test_collections.py is excluded by !Lib/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 49b2cab1-b180-4b4b-99ac-fd894d59a06c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds explicit reflected ("right-side") numeric slots and helpers mapping ops to r* method names, and updates VM operator dispatch to consider right-side slots and subclass overloads when resolving binary and ternary operations.

Changes

Cohort / File(s)	Summary
Number protocol: right-side slots & helpers `crates/vm/src/protocol/number.rs`	Added `right_method_name()` for `PyNumberBinaryOp` and `PyNumberTernaryOp`; introduced public `right_*` AtomicCell fields on `PyNumberSlots` (e.g., `right_add`, `right_subtract`, `right_multiply`, `right_power`, `right_matrix_multiply`, etc.) and wired `From<&PyNumberMethods>` to populate them.
VM operator dispatch changes `crates/vm/src/vm/vm_ops.rs`	Added `method_is_overloaded()` helper and updated binary/ternary dispatch to prefer right-side slots when operand classes differ and to check subclass overloads (aligning behavior with CPython's method resolution semantics).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Caller
    participant VM as VM (vm_ops)
    participant Left as LeftType.__op__
    participant Right as RightType.__rop__
    VM->>Left: lookup left slot (left_*)
    alt left slot exists and returns value
        Left-->>VM: result or NotImplemented
    else classes differ or left returned NotImplemented
        VM->>Right: lookup right slot (right_*)
        alt subclass and method_is_overloaded -> true
            Right-->>VM: result or NotImplemented
        else
            Right-->>VM: NotImplemented
        end
    end
    VM-->>Caller: final result or raise TypeError

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

PyWrapperDescrObject and rewrite toggle_slot #6536 — Adds and wires right-side numeric slots and r* handling, overlapping slot definitions and dispatch.
Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222 — Adjusts number-slot resolution and operator dispatch logic with MRO-aware slot lookup; closely related to the subclass overload behavior changes.

Suggested reviewers

youknowone

Poem

🐰 I hop from left to right with cheer,
New r doors open wide and clear.
VM listens, checks subclass and art,
Reflected slots play their part.
A rabbit applauds this numeric start.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix subclass right-op dispatch for Python classes' directly describes the main change: fixing incorrect dispatch of right-operator methods for Python-defined subclasses. It is specific, concise, and accurately reflects the primary objective of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

📝 Coding Plan

Generate coding plan for human review comments

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 · 2026-03-18T12:28:16Z

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/opcode.py
[ ] lib: cpython/Lib/_opcode_metadata.py
[x] test: cpython/Lib/test/test__opcode.py (TODO: 2)
[x] test: cpython/Lib/test/test_opcodes.py

dependencies:

opcode (native: _opcode, builtins)
- _opcode_metadata
- _opcode_metadata

dependent tests: (44 tests)

opcode: test__opcode test_code test_compile test_dis test_peepholer
- dis: test_ast test_compiler_assemble test_dtrace test_fstring test_inspect test_monitoring test_opcache test_patma test_positional_only_arg
  - bdb: test_bdb
  - inspect: test_abc test_argparse test_asyncgen test_buffer test_builtin test_collections test_coroutines test_decimal test_enum test_functools test_generators test_grammar test_ntpath test_operator test_posixpath test_pydoc test_signal test_sqlite3 test_traceback test_type_annotations test_types test_typing test_unittest test_yield_from test_zipimport test_zoneinfo
  - modulefinder: test_importlib test_modulefinder
  - trace: test_trace

[ ] test: cpython/Lib/test/test_class.py (TODO: 15)
[x] test: cpython/Lib/test/test_genericclass.py
[x] test: cpython/Lib/test/test_subclassinit.py

dependencies:

dependent tests: (no tests depend on class)

[ ] lib: cpython/Lib/collections
[x] lib: cpython/Lib/_collections_abc.py
[x] test: cpython/Lib/test/test_collections.py (TODO: 2)
[x] test: cpython/Lib/test/test_deque.py (TODO: 3)
[x] test: cpython/Lib/test/test_defaultdict.py (TODO: 1)
[x] test: cpython/Lib/test/test_ordered_dict.py (TODO: 8)

dependencies:

collections (native: _collections, _weakref, itertools, sys)
- _collections_abc
- _collections_abc, abc, annotationlib, copy, heapq, keyword, operator, reprlib, warnings

dependent tests: (302 tests)

[ ] test: cpython/Lib/test/test_descr.py (TODO: 43)
[ ] test: cpython/Lib/test/test_descrtut.py (TODO: 3)

dependencies:

dependent tests: (no tests depend on descr)

[x] lib: cpython/Lib/dis.py
[ ] test: cpython/Lib/test/test_dis.py (TODO: 37)

dependencies:

dis

dependent tests: (70 tests)

[x] test: cpython/Lib/test/test_format.py (TODO: 6)

dependencies:

dependent tests: (no tests depend on format)

[x] test: cpython/Lib/test/test_math.py
[x] test: cpython/Lib/test/test_math_property.py

dependencies:

dependent tests: (229 tests)

[x] lib: cpython/Lib/types.py
[ ] test: cpython/Lib/test/test_types.py (TODO: 6)

dependencies:

types

dependent tests: (52 tests)

types: test_annotationlib test_ast test_asyncgen test_asyncio test_builtin test_call test_code test_collections test_compile test_compiler_assemble test_coroutines test_decorators test_descr test_dis test_doctest test_dtrace test_dynamicclassattribute test_email test_enum test_exception_group test_fstring test_funcattrs test_generators test_genericalias test_hmac test_importlib test_inspect test_listcomps test_marshal test_monitoring test_opcache test_os test_positional_only_arg test_pprint test_pyclbr test_pydoc test_raise test_string test_subclassinit test_subprocess test_tempfile test_threading test_trace test_traceback test_type_aliases test_type_annotations test_type_params test_types test_typing test_unittest test_xml_etree test_xml_etree_c

Legend:

[+] path exists in CPython
[x] up-to-date, [ ] outdated

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/vm/vm_ops.rs`:
- Around line 13-25: Change method_is_overloaded to return PyResult<bool>
instead of bool and propagate any error from vm.identical_or_equal instead of
silencing it; specifically, in method_is_overloaded (and places calling it such
as binary_op1 and ternary_op) replace the unwrap_or(false) with the ? operator
(or explicit match to return Err) so exceptions from identical_or_equal bubble
up as PyErrs, update callers binary_op1/ternary_op to accept the PyResult<bool>
and propagate errors (or handle them) accordingly, and ensure the initial
get_attr checks still return Ok(false) when attributes are missing.
- Around line 183-197: The code is incorrectly dropping slot_bb when the
left-side slots compare equal, which loses the reflected fallback (right_*
trampoline). Always preserve slot_bb by assigning slot_b = slot_bb when slot_bb
is present, and separate that from the priority decision: use
class_b.fast_issubclass(class_a) && let Some(rop_name) =
op_slot.right_method_name(self) && method_is_overloaded(class_a, class_b,
rop_name, self) only to decide whether the RHS should run before the LHS (i.e.,
adjust ordering/priority), not to decide whether to queue the reflected slot at
all; update the logic around slot_a, slot_b, slot_bb, class_a, class_b, op_slot,
right_method_name, method_is_overloaded and fast_issubclass accordingly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 0b5c98b5-467c-45ff-92d6-5298b5f4eb29

📥 Commits

Reviewing files that changed from the base of the PR and between 216006b and 131e5aa.

⛔ Files ignored due to path filters (1)

Lib/test/test_descr.py is excluded by !Lib/**

📒 Files selected for processing (2)

crates/vm/src/protocol/number.rs
crates/vm/src/vm/vm_ops.rs

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>

ShaharNaveh

great!
ty for your recent contributions:)

youknowone

Thank you 👍 👍

* Fix subclass right-op dispatch for Python classes * Separate fallback queueing from subclass priority in op dispatch

$@rlaisqls$

Fix subclass right-op dispatch for Python classes

131e5aa

$@rlaisqls$

rlaisqls force-pushed the fix/subclass-right-ops branch from 216006b to 131e5aa Compare March 18, 2026 12:29

$coderabbitai[bot]$

coderabbitai Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread crates/vm/src/vm/vm_ops.rs

Comment thread crates/vm/src/vm/vm_ops.rs

$@rlaisqls$

Separate fallback queueing from subclass priority in op dispatch

b24d50e

$@rlaisqls$

rlaisqls force-pushed the fix/subclass-right-ops branch from 94c80c5 to b24d50e Compare March 18, 2026 13:23

$ShaharNaveh$

ShaharNaveh reviewed Mar 18, 2026

View reviewed changes

Comment thread crates/vm/src/vm/vm_ops.rs Outdated

$@rlaisqls$ $@ShaharNaveh$

Add CPython source link to method_is_overloaded doc comment

b26a464

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>

$ShaharNaveh$

ShaharNaveh approved these changes Mar 18, 2026

View reviewed changes

$@rlaisqls$

Remove expectedFailure from test_union_operators as it now passes

eeb8d14

$youknowone$

youknowone approved these changes Mar 19, 2026

View reviewed changes

$@youknowone$

youknowone merged commit 79e17cb into RustPython:main Mar 19, 2026
16 checks passed

Copilot AI pushed a commit that referenced this pull request Mar 25, 2026

$@rlaisqls$

Fix subclass right-op dispatch for Python classes (#7462)

e609658

* Fix subclass right-op dispatch for Python classes * Separate fallback queueing from subclass priority in op dispatch

Navigation Menu

Uh oh!

Fix subclass right-op dispatch for Python classes - #7462

Fix subclass right-op dispatch for Python classes#7462
youknowone merged 4 commits into
RustPython:mainfrom
rlaisqls:fix/subclass-right-ops

$@rlaisqls$ rlaisqls commented Mar 18, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 18, 2026 •

edited

Loading

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Mar 18, 2026 •

edited

Loading

Uh oh!

$@coderabbitai$ coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

$@ShaharNaveh$ ShaharNaveh left a comment

Uh oh!

$@youknowone$ youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rlaisqls commented Mar 18, 2026 • 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 Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Library Dependencies

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

$@rlaisqls$ rlaisqls commented Mar 18, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 18, 2026 •

edited

Loading

github-actions Bot commented Mar 18, 2026 •

edited

Loading

$@coderabbitai$ coderabbitai Bot left a comment

$@ShaharNaveh$ ShaharNaveh left a comment

$@youknowone$ youknowone left a comment