solved test case error for test_dictviews.py from object.rs - #4779
solved test case error for test_dictviews.py from object.rs#4779JaeyoungAhn wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
i have no idea what's going on test_weakref |
Sorry, something went wrong.
That's one of the flaky tests. |
Sorry, something went wrong.
Dunno if its flaky-ness here, triggering re-runs hasn't helped. |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughIntroduces comprehensive Python object protocol implementation in the VM with optimized rich comparison short-circuiting for identical objects, alongside extensive new methods for attribute management, iteration, type checking, subscripting, comparison, and representation across PyObjectRef and PyObject. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
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.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between e1b22f1 and 0965d94.
⛔ Files ignored due to path filters (1)- Lib/test/test_dictviews.py is excluded by !Lib/**
- crates/vm/src/protocol/object.rs
- vm/src/protocol/object.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/protocol/object.rs
- vm/src/protocol/object.rs
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:
- vm/src/protocol/object.rs
crates/stdlib/src/array.rs (2)⏰ 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)crates/vm/src/builtins/memory.rs (1)
- other (1061-1061)
- other (1080-1080)
crates/vm/src/builtins/tuple.rs (1)
- other (332-332)
crates/vm/src/stdlib/collections.rs (1)
- other (271-271)
crates/vm/src/builtins/str.rs (1)
- other (351-351)
crates/vm/src/builtins/int.rs (3)
- other (545-545)
crates/vm/src/builtins/complex.rs (1)
- other (302-303)
- other (313-313)
- other (582-583)
crates/vm/src/builtins/float.rs (2)
- other (306-306)
crates/vm/src/types/structseq.rs (1)
- other (348-348)
- other (352-352)
- other (375-375)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
crates/vm/src/protocol/object.rs (1)vm/src/protocol/object.rs (1)335-346: Short-circuit optimization for identity-based equality looks correct.
This optimization aligns with CPython's PyObject_RichCompareBool behavior where identity implies equality. When self.is(other):
- Eq returns true immediately
- Ne returns false immediately
For other comparison operators (Lt, Le, Gt, Ge), the full comparison logic correctly proceeds since identity doesn't determine ordering.
336-347: Short-circuit optimization for identity-based equality is correct.
This optimization matches CPython's PyObject_RichCompareBool behavior, correctly short-circuiting Eq/Ne comparisons for identical objects while allowing other comparison operators to proceed through the full comparison logic.
Sorry, something went wrong.
| // PyObject *PyObject_GetAIter(PyObject *o) | ||
| pub fn get_aiter(&self, vm: &VirtualMachine) -> PyResult { | ||
| if self.payload_is::<PyAsyncGen>() { | ||
| vm.call_special_method(self, identifier!(vm, __aiter__), ()) | ||
| } else { | ||
| Err(vm.new_type_error("wrong argument type".to_owned())) | ||
| } | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
get_aiter implementation is too restrictive.
This only works for PyAsyncGen objects, but any async iterable with __aiter__ should be supported. Compare with the other file's implementation which properly checks for __aiter__, validates the result has __anext__, and provides meaningful error messages.
The current error message "wrong argument type" is also non-specific compared to Python's expected "'X' object is not an async iterable".
🔎 Suggested implementation pattern pub fn get_aiter(&self, vm: &VirtualMachine) -> PyResult {
- if self.payload_is::<PyAsyncGen>() {
- vm.call_special_method(self, identifier!(vm, __aiter__), ())
- } else {
- Err(vm.new_type_error("wrong argument type".to_owned()))
+ // Check if object has __aiter__ method
+ let aiter_method = self.class().get_attr(identifier!(vm, __aiter__));
+ if aiter_method.is_none() {
+ return Err(vm.new_type_error(format!(
+ "'{}' object is not an async iterable",
+ self.class().name()
+ )));
}
+
+ let iterator = vm.call_special_method(self, identifier!(vm, __aiter__), ())?;
+
+ // Check that the result has __anext__
+ if !iterator.class().has_attr(identifier!(vm, __anext__)) {
+ return Err(vm.new_type_error(format!(
+ "'{}' object is not an async iterator",
+ iterator.class().name()
+ )));
+ }
+
+ Ok(iterator)
}
Sorry, something went wrong.
| pub fn has_attr<'a>(&self, attr_name: impl AsPyStr<'a>, vm: &VirtualMachine) -> PyResult<bool> { | ||
| self.get_attr(attr_name, vm).map(|o| !vm.is_none(&o)) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
has_attr implementation has incorrect semantics.
If an attribute exists but its value is None, hasattr will still return True. However, this implementation returns false when the attribute value is Python's None object.
Additionally, if get_attr raises an AttributeError for a non-existent attribute, this propagates the error instead of returning false. Python's hasattr catches AttributeError and returns false.
🔎 Proposed fix pub fn has_attr<'a>(&self, attr_name: impl AsPyStr<'a>, vm: &VirtualMachine) -> PyResult<bool> {
- self.get_attr(attr_name, vm).map(|o| !vm.is_none(&o))
+ match self.get_attr(attr_name, vm) {
+ Ok(_) => Ok(true),
+ Err(e) if e.class().is(vm.ctx.exceptions.attribute_error) => Ok(false),
+ Err(e) => Err(e),
+ }
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn has_attr<'a>(&self, attr_name: impl AsPyStr<'a>, vm: &VirtualMachine) -> PyResult<bool> { | |
| self.get_attr(attr_name, vm).map(|o| !vm.is_none(&o)) | |
| } | |
| pub fn has_attr<'a>(&self, attr_name: impl AsPyStr<'a>, vm: &VirtualMachine) -> PyResult<bool> { | |
| match self.get_attr(attr_name, vm) { | |
| Ok(_) => Ok(true), | |
| Err(e) if e.class().is(vm.ctx.exceptions.attribute_error) => Ok(false), | |
| Err(e) => Err(e), | |
| } | |
| } |
In vm/src/protocol/object.rs around lines 103-105, has_attr currently returns false for attributes whose value is Python None and lets AttributeError propagate; change it so that it returns Ok(true) whenever get_attr succeeds (regardless of the attribute value), and only returns Ok(false) when get_attr fails with an AttributeError; any other error should continue to propagate. Implement this by calling self.get_attr(...), mapping an Ok(_) to Ok(true), and matching Err(err) to detect AttributeError (returning Ok(false)) versus re-raising other errors.
Sorry, something went wrong.
| _ => { | ||
| if let Some(i) = (0..n).next() { | ||
| let check = vm.with_recursion("in abstract_issubclass", || { | ||
| tuple.fast_getitem(i).abstract_issubclass(cls, vm) | ||
| })?; | ||
| if check { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Bug: Multiple inheritance case only checks the first base class.
The expression (0..n).next() always returns Some(0), so this only checks the first base at index 0. After the match block, it returns Ok(false) without checking the remaining bases.
For example, if a class inherits from (A, B) where B is the target class, this would incorrectly return false because it only checks A.
🔎 Proposed fix _ => {
- if let Some(i) = (0..n).next() {
+ for i in 0..n {
let check = vm.with_recursion("in abstract_issubclass", || {
tuple.fast_getitem(i).abstract_issubclass(cls, vm)
})?;
if check {
return Ok(true);
}
}
+ return Ok(false);
}
}
-
- return Ok(false);
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ => { | |
| if let Some(i) = (0..n).next() { | |
| let check = vm.with_recursion("in abstract_issubclass", || { | |
| tuple.fast_getitem(i).abstract_issubclass(cls, vm) | |
| })?; | |
| if check { | |
| return Ok(true); | |
| } | |
| } | |
| } | |
| } | |
| return Ok(false); | |
| } | |
| _ => { | |
| for i in 0..n { | |
| let check = vm.with_recursion("in abstract_issubclass", || { | |
| tuple.fast_getitem(i).abstract_issubclass(cls, vm) | |
| })?; | |
| if check { | |
| return Ok(true); | |
| } | |
| } | |
| return Ok(false); | |
| } | |
| } | |
| } |
In vm/src/protocol/object.rs around lines 423 to 436, the code uses (0..n).next() which always yields index 0 and thus only checks the first base class; change this to iterate over all base indices (e.g., for i in 0..n or while let Some(i) = iter.next() over the full range) and perform the vm.with_recursion check for each base, returning Ok(true) as soon as any check succeeds and only returning Ok(false) after all bases have been checked.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
solved test case error for test_dictviews.py from fn rich_compare_bool in object.rs
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.