solved test case error for test_dictviews.py from object.rs #4779
Conversation
|
i have no idea what's going on |
That's one of the flaky tests. |
Dunno if its flaky-ness here, triggering re-runs hasn't helped. |
📝 WalkthroughWalkthroughIntroduces 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)
✅ Passed checks (1 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_dictviews.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/protocol/object.rsvm/src/protocol/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 runningcargo fmtto 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.rsvm/src/protocol/object.rs
🧠 Learnings (1)
📚 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:
vm/src/protocol/object.rs
🧬 Code graph analysis (1)
crates/vm/src/protocol/object.rs (9)
crates/stdlib/src/array.rs (2)
other(1061-1061)other(1080-1080)crates/vm/src/builtins/memory.rs (1)
other(332-332)crates/vm/src/builtins/tuple.rs (1)
other(271-271)crates/vm/src/stdlib/collections.rs (1)
other(351-351)crates/vm/src/builtins/str.rs (1)
other(545-545)crates/vm/src/builtins/int.rs (3)
other(302-303)other(313-313)other(582-583)crates/vm/src/builtins/complex.rs (1)
other(306-306)crates/vm/src/builtins/float.rs (2)
other(348-348)other(352-352)crates/vm/src/types/structseq.rs (1)
other(375-375)
⏰ 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 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
🔇 Additional comments (2)
crates/vm/src/protocol/object.rs (1)
335-346: Short-circuit optimization for identity-based equality looks correct.This optimization aligns with CPython's
PyObject_RichCompareBoolbehavior where identity implies equality. Whenself.is(other):
EqreturnstrueimmediatelyNereturnsfalseimmediatelyFor other comparison operators (
Lt,Le,Gt,Ge), the full comparison logic correctly proceeds since identity doesn't determine ordering.vm/src/protocol/object.rs (1)
336-347: Short-circuit optimization for identity-based equality is correct.This optimization matches CPython's
PyObject_RichCompareBoolbehavior, correctly short-circuitingEq/Necomparisons for identical objects while allowing other comparison operators to proceed through the full comparison logic.
| // 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.
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)
}| 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.
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),
+ }
}📝 Committable suggestion
‼️ 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), | |
| } | |
| } |
🤖 Prompt for AI Agents
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.
| _ => { | ||
| 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.
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);
}📝 Committable suggestion
‼️ 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); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
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.
solved test case error for
test_dictviews.pyfromfn rich_compare_boolinobject.rsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.