Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222

YashSuthar983 · 2025-10-27T16:51:14Z

…ed and dynamically added methods are found.

Use class().mro_find_map() to mimic the same behaviour as CPython.

test Cases

dynamic_patch_test

# Expected behavior: After patching A.__mul__ at runtime, B() * 2 should succeed.

class A:
    pass

class B(A):
    pass

b = B()
def mul(self, other):
    return ("mul", type(self).__name__, other)

A.__mul__ = mul

res = b * 2
assert res == ("mul", "B", 2)
print("dynamic_patch OK")

Cpython

Bravo:~$ python3 ../number_mro_dynamic_patch.py
dynamic_patch OK

RustPython

Bravo:~/contri/RustPython$ cargo run --quiet -- tests/number_mro_dynamic_patch.py
Traceback (most recent call last):
  File "tests/number_mro_dynamic_patch.py", line 20, in <module>
    res = b * 2
TypeError: '*' not supported between instances of 'B' and 'int'
Bravo:~/contri/RustPython$

Summary by CodeRabbit

Bug Fixes
- Numeric operations and type conversions now resolve methods by searching the full method resolution order, ensuring inherited or overridden numeric slots are respected.
- Binary and ternary operation dispatch correctly invokes overridden methods from parent classes when present, preserving previous behavior and error messages.

…ed and dynamically added methods are found. Use class().mro_find_map() to mimic the same behaviour as CPython. Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>

coderabbitai · 2025-10-27T16:51:36Z

Caution

Review failed

The pull request is closed.

Walkthrough

Replaced direct class slot access with MRO-aware lookups for number protocol methods and VM operation slots, so slot retrieval now traverses the method resolution order (using mro_find_map) before invoking or falling back to previous behavior.

Changes

Cohort / File(s)	Summary
Number Protocol MRO Lookups `vm/src/protocol/number.rs`	Replaced direct per-class slot reads with `mro_find_map`-based lookups in `check`, `is_index`, `int`, `index`, and `float`. Invocation path and result handling preserved; only slot retrieval now searches the MRO.
VM Operations MRO Lookups `vm/src/vm/vm_ops.rs`	Replaced direct operation-slot lookups with `mro_find_map` in binary/ternary op resolution (`binary_op1`, `binary_iop1`, `ternary_op`, `ternary_iop`). Added comments clarifying MRO-based resolution; fallbacks unchanged.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Protocol as Number/VM Ops
    participant MRO as mro_find_map
    participant ClassSlot as Class Slots

    rect rgb(245,245,255)
    note over Caller,ClassSlot: Old: direct class slot access
    Caller->>Protocol: request slot
    Protocol->>ClassSlot: read obj.class().slots[slot]
    ClassSlot-->>Protocol: slot or None
    Protocol-->>Caller: call slot or fallback
    end

    rect rgb(245,255,245)
    note over Caller,MRO: New: MRO-aware lookup
    Caller->>Protocol: request slot
    Protocol->>MRO: mro_find_map(class, find_slot)
    MRO->>ClassSlot: iterate classes -> check slots
    alt found
        ClassSlot-->>MRO: slot
        MRO-->>Protocol: slot
        Protocol-->>Caller: call slot
    else not found
        MRO-->>Protocol: None
        Protocol-->>Caller: fallback
    end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Review focus:
- Consistency of error and deprecation messages in int, index, float
- Correctness of mro_find_map usage and closure/map key for each slot kind
- Binary/ternary op resolution paths and fallback ordering in vm_ops.rs

Possibly related PRs

Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222 — Applies the same pattern: replacing direct class slot access with mro_find_map for number protocol and operator slots.

Poem

🐰
I hop through classes, nose to the sky,
Now slots I find by walking MRO high.
No sudden leaps — I follow the chain,
Parents guide methods, nothing in vain.
A tiny hop, a system made spry.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Resolve number slots via MRO in PyNumber and operator, ensure inherit..." directly and accurately reflects the main changes in the changeset. The modifications replace direct slot access with MRO-aware lookups in both `vm/src/protocol/number.rs` and `vm/src/vm/vm_ops.rs`, which enables inherited and dynamically added number methods to be properly discovered during numeric operations. The title is clear, specific, and sufficiently concise to convey the primary purpose without vague or generic terminology.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 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 21c7e15 and 914bd74.

⛔ Files ignored due to path filters (1)

Lib/ctypes/__init__.py is excluded by !Lib/**

📒 Files selected for processing (1)

vm/src/protocol/number.rs (1 hunks)

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

YashSuthar983 · 2025-10-27T16:54:30Z

@youknowone @arihant2math please take a look whenever you get a chance.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

vm/src/vm/vm_ops.rs (1)

163-172: Good: MRO-aware slot lookup for binary ops; fix pointer equality implementation for consistency.

Switching to class_a/class_b.mro_find_map aligns behavior with CPython and enables dynamic patches. However, comparing function pointers via as usize is brittle and inconsistent with fn_addr_eq used below. Prefer std::ptr::fn_addr_eq (or direct fn pointer ==) here as well.

Apply:
-    let slot_bb = class_b.mro_find_map(|x| x.slots.as_number.right_binary_op(op_slot));
-    if slot_bb.map(|x| x as usize) != slot_a.map(|x| x as usize) {
-        slot_b = slot_bb;
-    }
+    if let Some(slot_bb) = class_b.mro_find_map(|x| x.slots.as_number.right_binary_op(op_slot)) {
+        if !slot_a.is_some_and(|sa| std::ptr::fn_addr_eq(sa, slot_bb)) {
+            slot_b = Some(slot_bb);
+        }
+    }

vm/src/protocol/number.rs (3)

474-505: Match CPython error text: add space in “non-int (type …)”.

Great switch to MRO. The error string currently reads "non-int(type …)". CPython uses "non-int (type …)" (space before the parenthesis). Same deprecation text is fine.

Apply:

-                    Err(vm.new_type_error(format!(
-                        "{}.__int__ returned non-int(type {})",
-                        self.class(),
-                        ret_class
-                    )))
+                    Err(vm.new_type_error(format!(
+                        "{}.__int__ returned non-int (type {})",
+                        self.class(),
+                        ret_class
+                    )))

509-540: Also fix spacing in index error text.

Mirror CPython: "non-int (type …)".

-                    Err(vm.new_type_error(format!(
-                        "{}.__index__ returned non-int(type {})",
-                        self.class(),
-                        ret_class
-                    )))
+                    Err(vm.new_type_error(format!(
+                        "{}.__index__ returned non-int (type {})",
+                        self.class(),
+                        ret_class
+                    )))

544-575: And fix spacing in float error text.

Mirror CPython: "non-float (type …)".

-                    Err(vm.new_type_error(format!(
-                        "{}.__float__ returned non-float(type {})",
-                        self.class(),
-                        ret_class
-                    )))
+                    Err(vm.new_type_error(format!(
+                        "{}.__float__ returned non-float (type {})",
+                        self.class(),
+                        ret_class
+                    )))

📜 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 9a2792a and 21c7e15.

📒 Files selected for processing (2)

vm/src/protocol/number.rs (1 hunks)
vm/src/vm/vm_ops.rs (5 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

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

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

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

{vm,stdlib}/**/*.rs

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

Use RustPython macros (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

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

🧬 Code graph analysis (1)

vm/src/protocol/number.rs (3)

vm/src/types/slot.rs (3)

obj (1217-1218)

ret (202-202)

ret (261-261)

vm/src/builtins/memory.rs (4)

x (921-921)

x (925-926)

x (925-925)

x (930-930)

vm/src/object/core.rs (1)

class (671-673)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Run tests under miri
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Check Rust code with rustfmt and clippy
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (6)

vm/src/vm/vm_ops.rs (4)

234-237: LGTM: in-place binary ops now respect MRO for left-hand iop.

Using a.class().mro_find_map(...) matches how merged number tables behave in CPython. No issues spotted.

273-281: LGTM: ternary op MRO traversal mirrors CPython ordering.

MRO-based discovery for left/right ternary slots is correct; control flow preserves the subtype right-op preference before left-op, matching CPython’s ternary_op.

Also applies to: 284-298

346-349: LGTM: in-place ternary op uses MRO for left-hand slot.

Matches the binary iop pattern; behavior consistent.

307-315: MSRV check confirmed: std::ptr::fn_addr_eq is fully supported.

std::ptr::fn_addr_eq was stabilized in Rust 1.85.0, and the repository's MSRV is 1.89.0, so the code is correct and requires no fallback.

vm/src/protocol/number.rs (2)

446-460: LGTM: PyNumber_Check now respects MRO for int/index/float presence.

Correctly detects numeric behavior inherited or patched at runtime.

467-470: LGTM: PyIndex_Check via MRO.

Accurate and matches how merged number tables behave.

arihant2math · 2025-10-27T22:12:28Z

Oh nice, you might consider remove this patch:

RustPython/Lib/ctypes/__init__.py

Line 304 in 9a2792a

buftype = c_wchar.__mul__(init)

(if this fixes that).

youknowone

Awesome, thank you so much!

vm/src/protocol/number.rs

Resolve number slots via MRO in PyNumber and operator, ensure inherit…

21c7e15

…ed and dynamically added methods are found. Use class().mro_find_map() to mimic the same behaviour as CPython. Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>

coderabbitai bot reviewed Oct 27, 2025

View reviewed changes

Revert temp __mul__ fix

355604d

youknowone approved these changes Oct 28, 2025

View reviewed changes

vm/src/protocol/number.rs Show resolved Hide resolved

TODO comment

914bd74

youknowone merged commit b6e8a87 into RustPython:main Oct 28, 2025
11 checks passed

youknowone mentioned this pull request Nov 28, 2025

Ctypes __mul__ #6305

Merged

coderabbitai bot mentioned this pull request Dec 4, 2025

prefer nb_bool slot in try_to_bool instead of __bool__ #6328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222

Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222

Uh oh!

YashSuthar983 commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 27, 2025 •

edited

Loading

Review failed

Uh oh!

YashSuthar983 commented Oct 27, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

arihant2math commented Oct 27, 2025

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222

Resolve number slots via MRO in PyNumber and operator, ensure inherit… #6222

Uh oh!

Conversation

YashSuthar983 commented Oct 27, 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 Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

YashSuthar983 commented Oct 27, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

arihant2math commented Oct 27, 2025

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YashSuthar983 commented Oct 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 27, 2025 •

edited

Loading