Deprecate ::new_ref - #6046
Conversation
Sorry, something went wrong.
WalkthroughThis change deprecates the new_ref constructor methods in several built-in types, replacing their explicit PyRef::new_ref usage with idiomatic from(...).into_ref(ctx) patterns. Deprecation attributes and notes are added to guide users toward the new approach. Some internal helper methods are consolidated or refactored for consistency. Additionally, the new_ref method was removed from PyNamespace, and a new public new constructor was added for PyStaticMethod. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuiltinType
participant Context
Caller->>BuiltinType: from(data)
BuiltinType->>BuiltinType: construct instance
Caller->>BuiltinType: into_ref(ctx)
BuiltinType->>Context: create PyRef
Context-->>Caller: PyRef<BuiltinType>
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between ce73615 and 0a05a91. 📒 Files selected for processing (15)
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
Learnt from: CR Learnt from: CR vm/src/frame.rs (2)✨ 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. ❤️ Share 🪧 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
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 57029f6 and eea1a1f.
📒 Files selected for processing (9)- vm/src/builtins/bytearray.rs (1 hunks)
- vm/src/builtins/bytes.rs (1 hunks)
- vm/src/builtins/classmethod.rs (1 hunks)
- vm/src/builtins/complex.rs (1 hunks)
- vm/src/builtins/dict.rs (1 hunks)
- vm/src/builtins/function.rs (1 hunks)
- vm/src/builtins/list.rs (1 hunks)
- vm/src/builtins/staticmethod.rs (1 hunks)
- vm/src/builtins/str.rs (1 hunks)
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmt to format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
- vm/src/builtins/staticmethod.rs
- vm/src/builtins/dict.rs
- vm/src/builtins/bytes.rs
- vm/src/builtins/bytearray.rs
- vm/src/builtins/complex.rs
- vm/src/builtins/str.rs
- vm/src/builtins/list.rs
- vm/src/builtins/function.rs
- vm/src/builtins/classmethod.rs
vm/src/builtins/dict.rs (1)54-57: LGTM! Clean deprecation and simplification.
The deprecation note correctly guides users to the new pattern, and the implementation has been properly simplified to use Self::default().into_ref(ctx) directly. This is consistent with the broader refactoring effort.
vm/src/builtins/bytes.rs (1)102-105: LGTM! Consistent with the refactoring pattern.
The deprecation note correctly guides users to the new PyBytes::from(...).into_ref() pattern, and the implementation has been properly simplified to use exactly that approach. This change is consistent with the broader refactoring effort across built-in types.
vm/src/builtins/bytearray.rs (1)76-79: LGTM! Follows the established refactoring pattern.
The deprecation note correctly guides users to the new PyByteArray::from(...).into_ref() pattern, and the implementation has been properly simplified to use exactly that approach. This change is consistent with similar refactoring across other built-in types in this PR.
vm/src/builtins/str.rs (1)421-425: LGTM: Well-implemented deprecation pattern
The deprecation is properly implemented with:
- Clear guidance in the deprecation note directing users to use PyStr::from(...).into_ref() instead
- Simplified implementation that delegates to into_ref(ctx) while maintaining the same functionality
- Maintains backward compatibility during the transition period
This aligns well with the PR's objective to standardize instance creation patterns across built-in types.
vm/src/builtins/staticmethod.rs (2)64-68: LGTM: Clean constructor implementation
The new new method provides a clean, idiomatic constructor that:
- Takes the required PyObjectRef parameter
- Properly wraps it in PyMutex as expected by the struct
- Follows standard Rust constructor patterns
This serves as the foundation for the deprecation pattern being implemented across the codebase.
69-72: LGTM: Consistent deprecation implementation
The deprecation follows the established pattern:
- Clear deprecation note directing users to PyStaticMethod::new(...).into_ref()
- Simplified implementation that delegates to the new constructor method
- Maintains backward compatibility during the transition
This is consistent with the deprecation pattern being applied across all built-in types in this PR.
vm/src/builtins/function.rs (1)776-779: LGTM: Consistent deprecation pattern applied
The deprecation of PyBoundMethod::new_ref properly follows the established pattern:
- Clear deprecation note with specific guidance to use Self::new(object, function).into_ref(ctx)
- Simplified implementation that leverages the existing Self::new constructor
- Maintains backward compatibility during the transition period
This completes the consistent application of the deprecation pattern across built-in types as outlined in the PR objectives.
vm/src/builtins/classmethod.rs (1)114-117: LGTM! Clean deprecation implementation.
The deprecation follows Rust best practices with a clear migration path. The implementation correctly uses the idiomatic Self::from(callable).into_ref(ctx) pattern, which aligns with the PR objective to standardize constructor methods across builtin types.
vm/src/builtins/complex.rs (3)231-234: LGTM! Consistent deprecation implementation.
The deprecation follows the same pattern as other builtin types in this PR, providing a clear migration path with the idiomatic Self::from(value).into_ref(ctx) pattern.
236-238: Good optimization with const fn.
Making to_complex64 a const fn is a nice optimization that allows compile-time evaluation when possible, while maintaining the same functionality.
244-254: Good refactoring with helper method.
The number_op helper method consolidates the common pattern of converting two PyObject references to Complex64 values and applying a binary operation. This reduces code duplication and improves maintainability.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation