Complete tp_repr #5852
Complete tp_repr #5852
Conversation
WalkthroughThe changes refactor how object representations ( Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm/src/types/structseq.rs (1)
50-54: Verify the downcast approach is robust.The downcast from
&PyObjecttoPyTupleis necessary for the slot-based approach, but the error message could be more descriptive for debugging purposes.Consider improving the error message to include the actual type information:
- .ok_or_else(|| vm.new_type_error("unexpected payload for __repr__"))?; + .ok_or_else(|| vm.new_type_error(format!( + "expected PyTuple for struct sequence __repr__, got {}", + zelf.class().name() + )))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vm/src/protocol/object.rs(1 hunks)vm/src/types/structseq.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
vm/src/protocol/object.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Follow Rust best practices for error handling and memory management in all RustPython Rust code.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
vm/src/types/structseq.rs (5)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
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.109Z
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.109Z
Learning: Follow Rust best practices for error handling and memory management in all RustPython Rust code.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When comparing behavior with CPython, use the 'python' command explicitly for CPython and 'cargo run --' for RustPython to avoid confusion.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: To add a Python module to the RustPython interpreter, use 'vm.add_native_module' with the module name and module factory.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (5)
vm/src/protocol/object.rs (1)
335-338: Verify this breaking change is intentional and won't cause runtime failures.The removal of the fallback mechanism for missing
__repr__slots is a significant behavioral change. This now enforces that every object must have a properly definedreprslot, which could cause runtime errors for existing objects that previously relied on the dynamic fallback.Please verify that all built-in types and user-defined classes have proper
reprslots defined:#!/bin/bash # Description: Search for pyclass definitions that might be missing repr slots # Expected: All classes should have either explicit repr slots or inherit them echo "Searching for pyclass definitions..." ast-grep --pattern $'#[pyclass] $$$ trait $NAME { $$$ }' echo -e "\nSearching for explicit slot_repr implementations..." rg -A 5 "slot_repr|#\[pyslot\].*repr" echo -e "\nSearching for __repr__ method implementations..." rg -A 3 "fn __repr__"vm/src/types/structseq.rs (4)
2-3: Import changes look appropriate.The addition of
PyObjectandPyStrRefimports aligns with the new slot-based approach that returnsPyStrRefand accepts generic&PyObjectparameters.
60-76: LGTM! Recursion guard and formatting logic preserved correctly.The refactored code maintains the original logic for:
- Recursion detection using
ReprGuard- Special handling for single required field with trailing comma
- Proper field formatting with name-value pairs
The change from
zelf.as_ref()is appropriate for the new parameter type.
77-79: Return type change aligns with slot requirements.The change from returning a
StringtoPyStrRefis correct for the slot-based approach and maintains consistency with other slot implementations.
81-84: Simplified wrapper method is clean and consistent.The
__repr__method is now appropriately simplified to just delegate to the slot implementation, which follows the established pattern for slot-based methods.
Summary by CodeRabbit
Refactor
__repr__method for consistent representation.Bug Fixes