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 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 |
Sorry, something went wrong.
![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).
Sorry, something went wrong.
|
beacuse MRO is currently not weakrefs, this prevent RC=0 to release type objects |
Sorry, something went wrong.
youknowone
changed the title
mro representation fit to CPython
youknowone
merged commit b98f062
into
RustPython:main
Dec 30, 2025
Summary by CodeRabbit
New Features
Refactor
Bug Fixes