Separate Debug from PyPayload #6320
Conversation
WalkthroughThis PR refactors the VM object system's trait bounds, replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)
1056-1061: Consider adding documentation for theleakmethod.The
leakmethod implementation is correct (standard Rust leak pattern), but it would benefit from documentation explaining its purpose, when it should be used, and that the leaked object will remain in memory indefinitely.Apply this diff to add documentation:
+ /// Leaks this reference, returning a static reference to the inner object. + /// + /// The reference count is not decremented, so the object will remain in memory + /// indefinitely. This is primarily useful for creating static singletons or + /// objects that need to live for the entire program duration. pub const fn leak(pyref: Self) -> &'static Py<T> {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/vm/src/builtins/dict.rs(1 hunks)crates/vm/src/object/core.rs(24 hunks)crates/vm/src/object/ext.rs(16 hunks)crates/vm/src/object/payload.rs(4 hunks)crates/vm/src/types/slot.rs(1 hunks)crates/vm/src/vm/context.rs(1 hunks)
🧰 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 Rust 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/types/slot.rscrates/vm/src/builtins/dict.rscrates/vm/src/vm/context.rscrates/vm/src/object/payload.rscrates/vm/src/object/ext.rscrates/vm/src/object/core.rs
🧬 Code graph analysis (2)
crates/vm/src/object/ext.rs (2)
crates/vm/src/object/core.rs (2)
payload(654-662)payload(912-914)crates/vm/src/object/payload.rs (1)
std(22-22)
crates/vm/src/object/core.rs (1)
crates/vm/src/object/payload.rs (2)
std(22-22)downcastable_from(27-29)
⏰ 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). (1)
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (16)
crates/vm/src/vm/context.rs (1)
380-386: LGTM - Debug bound appropriately added to generic helper.The
new_pyrefmethod correctly addsstd::fmt::Debugbound since it callsinto_ref, which now requiresDebugper the changes inpayload.rs. This is consistent with the PR's goal of adding Debug constraints only where needed.crates/vm/src/types/slot.rs (1)
796-805: LGTM - Debug bound correctly added to DefaultConstructor.The
Debugbound is necessary becauseconstruct_and_initcallsinto_ref, which now requiresSelf: std::fmt::Debug. This aligns with the PR's approach of adding Debug constraints at the method level where payloads are converted to references.crates/vm/src/builtins/dict.rs (1)
753-755: LGTM - Debug bound appropriately added to associated type.The
ReverseIterassociated type now requiresPyPayload + std::fmt::Debug, which is consistent with the concrete iterator types that already#[derive(Debug)](e.g.,PyDictReverseKeyIteratorat line 914). This makes the constraint explicit in the trait definition.crates/vm/src/object/payload.rs (3)
19-23: Clean separation of Debug from PyPayload trait.Removing
Debugfrom the supertrait allows types to implementPyPayloadwithout necessarily implementingDebug. The Debug constraint is now applied only to methods that actually need it (those that constructPyReforPyObjectRef).
52-57: Correct per-method Debug constraint.The
into_pyobjectmethod requires Debug because it callsinto_ref, which needs to store the payload in a debuggable object structure.
119-124: PyObjectPayload correctly retains Debug requirement.
PyObjectPayloadcontinues to requireDebugsince objects that are stored and potentially displayed/debugged need this capability. The blanket impl appropriately requiresPyPayload + std::fmt::Debug + 'static.crates/vm/src/object/ext.rs (4)
1-4: Import correctly updated to PyPayload.The import change from
PyObjectPayloadtoPyPayloadaligns with the broader refactor of usingPyPayloadas the primary trait for type-safe payload handling.
61-63: PyExact struct declaration correctly uses no explicit bounds.The
#[repr(transparent)]struct doesn't require bounds on the declaration itself; bounds are appropriately placed on the impl blocks.
582-590: ToPyObject blanket impl correctly requires Debug.This blanket implementation for
T: PyPayload + std::fmt::Debugis correct because it callsPyPayload::into_pyobject, which requires theDebugbound per the changes inpayload.rs.
261-271: Send/Sync impls appropriately constrained.The
SendandSyncimplementations forPyAtomicRef<T>correctly usePyPayloadbounds. These atomic reference types need thread-safety guarantees but don't require Debug for their safety invariants.crates/vm/src/object/core.rs (6)
79-95: LGTM! Trait bound refactoring is correctly applied.The function signatures correctly use
PyPayloadinstead ofPyObjectPayload, and theDebugbound is appropriately added only todebug_objwhere it's required for formatting.
531-577: LGTM! Downcast API refactoring is consistent.The downcast methods correctly use
PyPayloadbounds without requiringDebug, which is appropriate since these methods only perform type checking and casting, not object construction.
899-981: LGTM! Py trait implementations are correctly refactored.The trait bounds are consistently applied across all implementations. The
Debugbound is appropriately added only to theDebugimpl while other trait impls correctly propagate their respective bounds alongsidePyPayload.
1026-1061: LGTM! PyRef impl separation is well-designed.The separation of two impl blocks—one requiring only
PyPayloadand another requiringPyPayload + Debug—is correct. Thenew_refmethod needsDebugbecause it callsPyInner::new, while other methods likeleak,from_non_null, andfrom_rawdon't require debug-printability.
1147-1159: LGTM! PyWeakRef refactoring is consistent.The weak reference type correctly adopts the
PyPayloadbound, maintaining type safety while aligning with the broader refactoring objectives.
16-16: Verify that clippy passes with the refactored trait bounds.This broad refactoring changes trait bounds throughout the object model. Ensure
cargo clippyruns without new warnings on the changed code as required by the coding guidelines.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.