__hash__ to slot_wrapper #6480
__hash__ to slot_wrapper #6480
Conversation
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
7c2a0d9 to
2515109
Compare
| SlotFunc::Hash(func) => { | ||
| if !args.args.is_empty() || !args.kwargs.is_empty() { | ||
| return Err( | ||
| vm.new_type_error("__hash__() takes no arguments (1 given)".to_owned()) |
There was a problem hiding this comment.
| vm.new_type_error("__hash__() takes no arguments (1 given)".to_owned()) | |
| vm.new_type_error("__hash__() takes no arguments (1 given)") |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/class.rs (1)
156-171: Function pointer comparison may be unreliable; consider alternative approaches.The comparison
hash_func as usize != hash_not_implemented as usizeon line 159 casts function pointers tousizefor equality checking. While this may work in practice, comparing function pointers via cast to integers is not guaranteed to be reliable across all platforms and optimization levels.Consider these alternatives:
- Use
std::ptr::eqfor function pointer comparison- Add a marker trait or use a different mechanism to identify special sentinel functions
🔎 Proposed fix using std::ptr::eq
- if let Some(hash_func) = class.slots.hash.load() - && hash_func as usize != hash_not_implemented as usize - { + if let Some(hash_func) = class.slots.hash.load() { + let is_not_implemented = std::ptr::eq( + hash_func as *const (), + hash_not_implemented as *const () + ); + if !is_not_implemented { let hash_name = identifier!(ctx, __hash__); if !class.attributes.read().contains_key(hash_name) { let wrapper = PySlotWrapper { typ: class, name: ctx.intern_str("__hash__"), wrapped: SlotFunc::Hash(hash_func), doc: Some("Return hash(self)."), }; class.set_attr(hash_name, wrapper.into_ref(ctx).into()); } + } }crates/vm/src/builtins/descriptor.rs (1)
420-428: Clarify the error message for hash argument count.The error message on line 423 says
"__hash__() takes no arguments (1 given)", but this is potentially confusing since theselfargument has already been consumed at this point. The message should reflect the actual situation more accurately.🔎 Proposed improvement
if !args.args.is_empty() || !args.kwargs.is_empty() { return Err( - vm.new_type_error("__hash__() takes no arguments (1 given)".to_owned()) + vm.new_type_error(format!( + "__hash__() takes exactly one argument ({} given)", + args.args.len() + 1 + )) ); }Or simply:
- vm.new_type_error("__hash__() takes no arguments (1 given)".to_owned()) + vm.new_type_error("__hash__() takes exactly 1 argument (self)".to_owned())
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/vm/src/builtins/descriptor.rscrates/vm/src/class.rscrates/vm/src/types/slot.rs
🧰 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/class.rscrates/vm/src/builtins/descriptor.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (1)
crates/vm/src/class.rs (1)
crates/vm/src/types/slot.rs (1)
hash_not_implemented(374-376)
🔇 Additional comments (8)
crates/vm/src/class.rs (3)
5-8: LGTM!The import changes properly bring in
SlotFuncfrom the descriptor module and maintain logical grouping.
1 B35E 46-154: LGTM!The
__init__wrapper correctly uses the newSlotFunc::Initvariant to wrap the init function, maintaining the existing functionality while adopting the unified slot function abstraction.
158-171: The hash wrapper implementation is correct and properly handles all cases.The code correctly:
- Creates wrappers for all types with non-null hash slots (excluding
hash_not_implemented)- Prevents duplicates via the
contains_keycheck before insertion- Integrates properly with the slot dispatch mechanism through
SlotFunc::Hash- Has appropriate special handling for unhashable types (sets
__hash__to None whenhash_not_implementedis the slot function)The implementation aligns with the expected behavior for exposing slot-based hash functions to Python as
__hash__methods.crates/vm/src/builtins/descriptor.rs (4)
9-10: LGTM!The import of
HashFuncis properly added to support the newSlotFunc::Hashvariant.
395-411: LGTM! Well-designed type-erased slot function abstraction.The
SlotFuncenum effectively type-erases different slot function signatures, mirroring CPython'svoid* d_wrappedapproach. TheCloneandCopyderives are appropriate for function pointers, and theDebugimplementation is helpful without exposing internal details.
468-486: LGTM! Proper integration with SlotFunc.The
PySlotWrapper::callimplementation correctly:
- Extracts the first argument as
self- Validates the type of
selfagainst the expected type- Delegates to
SlotFunc::callwith the remaining argumentsThe unified dispatch mechanism is well-implemented.
543-549: LGTM! Bound method-wrapper correctly uses SlotFunc.The
PyMethodWrapper::callimplementation properly delegates toSlotFunc::callwith the bound object and arguments, maintaining consistency with the slot wrapper mechanism.crates/vm/src/types/slot.rs (1)
1101-1101: No issues found. All 27+ implementations of the Hashable trait properly define the requiredfn hash(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyHash>method, with zero remaining references to any removed__hash__method. TheSlotFunc::Hashwrapper is correctly integrated in the class system.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.