| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
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 configurationConfiguration 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:
WalkthroughAdds 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
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
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/opcode.py dependencies:
dependent tests: (44 tests)
[ ] test: cpython/Lib/test/test_class.py (TODO: 15) dependencies: dependent tests: (no tests depend on class) [ ] lib: cpython/Lib/collections dependencies:
dependent tests: (302 tests)
[ ] test: cpython/Lib/test/test_descr.py (TODO: 43) dependencies: dependent tests: (no tests depend on descr) [x] lib: cpython/Lib/dis.py dependencies:
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 dependencies: dependent tests: (229 tests)
[x] lib: cpython/Lib/types.py dependencies:
dependent tests: (52 tests)
Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0b5c98b5-467c-45ff-92d6-5298b5f4eb29
📥 CommitsReviewing files that changed from the base of the PR and between 216006b and 131e5aa.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
There was a problem hiding this comment.
great!
ty for your recent contributions:)
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you 👍 👍
Sorry, something went wrong.
* Fix subclass right-op dispatch for Python classes * Separate fallback queueing from subclass priority in op dispatch
* Fix subclass right-op dispatch for Python classes * Separate fallback queueing from subclass priority in op dispatch
* Fix subclass right-op dispatch for Python classes * Separate fallback queueing from subclass priority in op dispatch
| Back | FazBrowse Home | New Git URL |
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.
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:
Summary by CodeRabbit
New Features
Bug Fixes