make mro representation fit to CPython - #5866
Conversation
Sorry, something went wrong.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command. You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file. WalkthroughThis 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. ❤️ 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: 2
🧹 Nitpick comments (2)vm/src/builtins/type.rs (2)📜 Review details161-168: Consider removing the unused _a parameter.
The _a parameter 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());
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 7a6e5c4 and 672e9a4.
📒 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)
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.
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.
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.
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/tuple.rs (1)vm/src/vm/mod.rs (1)
- empty (540-545)
vm/src/builtins/tuple.rs (1)vm/src/protocol/object.rs (5)
- empty (540-545)
vm/src/builtins/type.rs (4)⏰ Context from checks skipped due to timeout of 90000ms (11)vm/src/exceptions.rs (2)
- __bases__ (515-523)
- bases (850-867)
- check (211-213)
- class (146-148)
vm/src/protocol/iter.rs (1)
- try_from_object (396-412)
- class (43-45)
vm/src/vm/mod.rs (1)
- check (25-29)
vm/src/builtins/union.rs (1)
- class (564-573)
- class (32-34)
- 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
vm/src/frame.rs (1)1384-1389: Updated call uses &vm.ctx – correct and consistent with new API
The call now passes a &Context rather than the full &VirtualMachine, matching the updated PyTupleTyped::empty(ctx: &Context) signature in tuple.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.ctx instead of self correctly aligns with the updated PyTupleTyped::empty method signature.
vm/src/builtins/tuple.rs (1)540-545: Good API simplification!
Changing the parameter from &VirtualMachine to &Context reduces coupling since the method only needs access to empty_tuple from 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_class method now properly validates classes by checking for a __bases__ attribute that must be a tuple, using the new abstract_get_bases helper.
390-410: Well-designed abstraction for bases handling.
The abstract_get_bases method properly handles all edge cases with clear documentation of the 4 possible return states. Good choice to mask AttributeError and non-tuple cases as None.
459-480: Excellent CPython-compatible implementation.
The additions improve performance with a fast path for PyType objects 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_class method.
vm/src/builtins/type.rs (4)210-213: Well-implemented type checking methods matching CPython semantics.
The check and check_exact methods correctly implement CPython's PyType_Check and PyType_CheckExact macros 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_subtype method 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 mro field 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) and new_static (line 316).
Sorry, something went wrong.
| 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
⚠️ Potential issue
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 AgentsIn 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.
Sorry, something went wrong.
| @@ -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.
⚠️ Potential issue
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);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.
Sorry, something went wrong.
|
beacuse MRO is currently not weakrefs, this prevent RC=0 to release type objects |
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes