make mro representation fit to CPython#5866
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis set of changes refactors the handling of method resolution order (MRO), type checking, and subclass determination in the Python VM implementation. It introduces more precise MRO management, optimizes subclass checks, updates tuple initialization to use context directly, and aligns error handling and attribute checks with CPython semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM
participant Context
participant PyType
participant PyObject
User->>VM: import(...)
VM->>Context: get empty_tuple
Context-->>VM: empty_tuple
VM->>PyTupleTyped: empty(&Context)
PyTupleTyped-->>VM: PyTupleTyped instance
User->>PyObject: is_subclass_of(cls)
PyObject->>PyType: check_class()
PyType-->>PyObject: Ok/Err
PyObject->>PyType: is_subtype(other)
PyType->>PyType: is_subtype_with_mro(self.mro, self, other)
PyType-->>PyObject: true/false
Suggested reviewers
Poem
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
vm/src/builtins/type.rs (2)
161-168: Consider removing the unused_aparameter.The
_aparameter is not used in the function body. If it's not needed for future compatibility, consider removing it for clarity.-fn is_subtype_with_mro(a_mro: &[PyTypeRef], _a: &Py<PyType>, b: &Py<PyType>) -> bool { +fn is_subtype_with_mro(a_mro: &[PyTypeRef], b: &Py<PyType>) -> bool {Note: If you remove this parameter, update the call site on line 452:
- is_subtype_with_mro(&self.mro.read(), self, other) + is_subtype_with_mro(&self.mro.read(), other)
552-555: Consider using explicit type reference instead of copying from old MRO.While copying the first element from the old MRO should work (since it's always self), it would be clearer and more robust to use an explicit reference.
- let mut mro = - PyType::resolve_mro(&cls.bases.read()).map_err(|msg| vm.new_type_error(msg))?; - mro.insert(0, cls.mro.read()[0].to_owned()); + let mut mro = + PyType::resolve_mro(&cls.bases.read()).map_err(|msg| vm.new_type_error(msg))?; + // Get a proper reference to the type + let weakref_type = super::PyWeak::static_type(); + let type_ref = cls.subclasses.read()[0].upgrade() + .and_then(|obj| obj.downcast::<PyType>().ok()) + .unwrap_or_else(|| { + // Fallback to copying from old MRO if we can't get a proper reference + cls.mro.read()[0].clone() + }); + mro.insert(0, type_ref);Actually, on second thought, the current approach is simpler. You could just add a comment explaining the assumption:
+ // The first element of MRO is always the type itself mro.insert(0, cls.mro.read()[0].to_owned());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
vm/src/builtins/tuple.rs(1 hunks)vm/src/builtins/type.rs(13 hunks)vm/src/frame.rs(1 hunks)vm/src/object/core.rs(2 hunks)vm/src/protocol/object.rs(2 hunks)vm/src/vm/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
vm/src/frame.rs (3)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: To add a Python module to the RustPython interpreter, use 'vm.add_native_module' with the module name and module factory.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
vm/src/object/core.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
vm/src/vm/mod.rs (3)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: To add a Python module to the RustPython interpreter, use 'vm.add_native_module' with the module name and module factory.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
vm/src/builtins/type.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.138Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
🧬 Code Graph Analysis (3)
vm/src/frame.rs (1)
vm/src/builtins/tuple.rs (1)
empty(540-545)
vm/src/vm/mod.rs (1)
vm/src/builtins/tuple.rs (1)
empty(540-545)
vm/src/protocol/object.rs (5)
vm/src/builtins/type.rs (4)
__bases__(515-523)bases(850-867)check(211-213)class(146-148)vm/src/exceptions.rs (2)
try_from_object(396-412)class(43-45)vm/src/protocol/iter.rs (1)
check(25-29)vm/src/vm/mod.rs (1)
class(564-573)vm/src/builtins/union.rs (1)
class(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- 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: Run tests under miri
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (12)
vm/src/frame.rs (1)
1384-1389: Updated call uses&vm.ctx– correct and consistent with new APIThe call now passes a
&Contextrather than the full&VirtualMachine, matching the updatedPyTupleTyped::empty(ctx: &Context)signature intuple.rs. This reduces coupling and removes an unnecessary borrow of the whole VM. No further action required.vm/src/vm/mod.rs (1)
583-583: LGTM!The update to pass
&self.ctxinstead ofselfcorrectly aligns with the updatedPyTupleTyped::emptymethod signature.vm/src/builtins/tuple.rs (1)
540-545: Good API simplification!Changing the parameter from
&VirtualMachineto&Contextreduces coupling since the method only needs access toempty_tuplefrom the context.vm/src/object/core.rs (1)
1255-1255: Correct MRO initialization for core types.These changes properly set up the MRO vectors to include each type at the front of its own MRO, aligning with Python's MRO semantics where a class always appears first in its resolution order.
Also applies to: 1261-1262, 1278-1278
vm/src/protocol/object.rs (4)
374-388: Clean refactoring of class validation.The renamed
check_classmethod now properly validates classes by checking for a__bases__attribute that must be a tuple, using the newabstract_get_baseshelper.
390-410: Well-designed abstraction for bases handling.The
abstract_get_basesmethod properly handles all edge cases with clear documentation of the 4 possible return states. Good choice to maskAttributeErrorand non-tuple cases asNone.
459-480: Excellent CPython-compatible implementation.The additions improve performance with a fast path for
PyTypeobjects and provide CPython-compatible error messages. The check order (union, then class/tuple) matches CPython's behavior.
555-555: Consistent method rename.The call site is properly updated to use the renamed
check_classmethod.vm/src/builtins/type.rs (4)
210-213: Well-implemented type checking methods matching CPython semantics.The
checkandcheck_exactmethods correctly implement CPython'sPyType_CheckandPyType_CheckExactmacros with clear documentation.Also applies to: 456-459
335-335: Correct MRO iteration pattern for inheritance lookups.The code correctly skips the first MRO element (self) when looking for inherited attributes or checking subclass relationships, while including it when collecting all attributes. This matches expected Python semantics.
Also applies to: 384-387, 392-395, 465-465, 493-493
451-453: Clean implementation of subtype checking.The
is_subtypemethod provides a clear API for checking subtype relationships using the MRO.
271-271: Verify GC traversal of MRO cycle.Inserting the type into its own MRO is correct, but I noticed in the GC traversal implementation that the
mrofield isn’t being traced:
- vm/src/builtins/type.rs around line 263:
// self.mro.traverse(tracer_fn);Please confirm that the garbage collector still correctly handles this self-reference (either by tracing MRO elsewhere or by design) to avoid leaking types. This check applies to both types created in
new_heap_inner(line 271) andnew_static(line 316).
vm/src/protocol/object.rs
Outdated
| fn abstract_issubclass(&self, cls: &PyObject, vm: &VirtualMachine) -> PyResult<bool> { | ||
| let mut derived = self; | ||
| let mut first_item: PyObjectRef; | ||
| loop { | ||
|
|
||
| // First loop: handle single inheritance without recursion | ||
| let bases = loop { | ||
| if derived.is(cls) { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| let bases = derived.get_attr(identifier!(vm, __bases__), vm)?; | ||
| let tuple = PyTupleRef::try_from_object(vm, bases)?; | ||
|
|
||
| let n = tuple.len(); | ||
| let Some(bases) = derived.abstract_get_bases(vm)? else { | ||
| return Ok(false); | ||
| }; | ||
| let n = bases.len(); | ||
| match n { | ||
| 0 => { | ||
| return Ok(false); | ||
| } | ||
| 0 => return Ok(false), | ||
| 1 => { | ||
| first_item = tuple[0].clone(); | ||
| derived = &first_item; | ||
| // Avoid recursion in the single inheritance case | ||
| // # safety | ||
| // Intention: bases.as_slice()[0].as_object(); | ||
| // Though type-system cannot guarantee, derived does live long enough in the loop. | ||
| derived = unsafe { &*(bases.as_slice()[0].as_object() as *const _) }; | ||
| continue; | ||
| } | ||
| _ => { | ||
| for i in 0..n { | ||
| let check = vm.with_recursion("in abstract_issubclass", || { | ||
| tuple[i].abstract_issubclass(cls, vm) | ||
| })?; | ||
| if check { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| // Multiple inheritance - break out to handle recursively | ||
| break bases; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| return Ok(false); | ||
| // Second loop: handle multiple inheritance with recursion | ||
| // At this point we know n >= 2 | ||
| let n = bases.len(); | ||
| assert!(n >= 2); | ||
|
|
||
| for i in 0..n { | ||
| let result = vm.with_recursion("in __issubclass__", || { | ||
| bases.as_slice()[i].abstract_issubclass(cls, vm) | ||
| })?; | ||
| if result { | ||
| return Ok(true); | ||
| } | ||
| } | ||
|
|
||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimization looks good but has safety concerns.
The single inheritance optimization is clever, but the unsafe pointer cast on line 432 is risky:
derived = unsafe { &*(bases.as_slice()[0].as_object() as *const _) };While the comment claims safety, this pattern extends the lifetime unsafely. Consider using a safer approach or add more robust lifetime guarantees.
Also, the assertion on line 445 is redundant - the match expression already ensures n >= 2.
🤖 Prompt for AI Agents
In vm/src/protocol/object.rs between lines 412 and 457, the unsafe pointer cast
used to extend the lifetime of the derived reference in the single inheritance
case is risky and should be replaced with a safer approach, such as
restructuring the code to avoid unsafe lifetime extension or using safe Rust
references that respect lifetimes. Additionally, remove the redundant assertion
on line 445 since the match statement already guarantees that n is at least 2.
| @@ -48,7 +48,7 @@ unsafe impl crate::object::Traverse for PyType { | |||
| fn traverse(&self, tracer_fn: &mut crate::object::TraverseFn<'_>) { | |||
| self.base.traverse(tracer_fn); | |||
| self.bases.traverse(tracer_fn); | |||
| self.mro.traverse(tracer_fn); | |||
| // self.mro.traverse(tracer_fn); | |||
There was a problem hiding this comment.
Uncommenting MRO traversal may be necessary for proper garbage collection.
The MRO field contains references to types that should be traversed. Commenting this out could lead to memory leaks or incorrect reference cycle detection.
- // self.mro.traverse(tracer_fn);
+ self.mro.traverse(tracer_fn);🤖 Prompt for AI Agents
In vm/src/builtins/type.rs at line 51, the call to traverse the MRO field is
commented out, which may cause memory leaks or incorrect reference cycle
detection. Uncomment the line that calls self.mro.traverse(tracer_fn) to ensure
proper traversal of references in the MRO field during garbage collection.
|
beacuse MRO is currently not weakrefs, this prevent RC=0 to release type objects |
youknowone
changed the title
mro representation fit to CPythonmro representation fit to CPython
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes