__hash__ to slot_wrapper by youknowone · Pull Request #6480 · RustPython/RustPython · GitHub
Skip to content

__hash__ to slot_wrapper#6480

Merged
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:slot-hash
Dec 24, 2025
Merged

__hash__ to slot_wrapper#6480
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:slot-hash

Conversation

@youknowone
Copy link
Copy Markdown
Member

@youknowone youknowone commented Dec 24, 2025

Summary by CodeRabbit

  • Refactor
    • Improved internal organization of slot function handling to enhance code maintainability and consistency.
    • Strengthened integration of hash function support in class initialization.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 24, 2025

@youknowone youknowone marked this pull request as ready for review December 24, 2025 09:51
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())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 usize on line 159 casts function pointers to usize for 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:

  1. Use std::ptr::eq for function pointer comparison
  2. 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 the self argument 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3d7e521 and 2515109.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/descriptor.rs
  • crates/vm/src/class.rs
  • crates/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 running cargo fmt to 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.rs
  • crates/vm/src/class.rs
  • crates/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 SlotFunc from the descriptor module and maintain logical grouping.


146-154: LGTM!

The __init__ wrapper correctly uses the new SlotFunc::Init variant 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:

  1. Creates wrappers for all types with non-null hash slots (excluding hash_not_implemented)
  2. Prevents duplicates via the contains_key check before insertion
  3. Integrates properly with the slot dispatch mechanism through SlotFunc::Hash
  4. Has appropriate special handling for unhashable types (sets __hash__ to None when hash_not_implemented is 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 HashFunc is properly added to support the new SlotFunc::Hash variant.


395-411: LGTM! Well-designed type-erased slot function abstraction.

The SlotFunc enum effectively type-erases different slot function signatures, mirroring CPython's void* d_wrapped approach. The Clone and Copy derives are appropriate for function pointers, and the Debug implementation is helpful without exposing internal details.


468-486: LGTM! Proper integration with SlotFunc.

The PySlotWrapper::call implementation correctly:

  1. Extracts the first argument as self
  2. Validates the type of self against the expected type
  3. Delegates to SlotFunc::call with the remaining arguments

The unified dispatch mechanism is well-implemented.


543-549: LGTM! Bound method-wrapper correctly uses SlotFunc.

The PyMethodWrapper::call implementation properly delegates to SlotFunc::call with 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 required fn hash(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyHash> method, with zero remaining references to any removed __hash__ method. The SlotFunc::Hash wrapper is correctly integrated in the class system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants