lgtm

Actionable comments posted: 0

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())

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

• crates/vm/src/builtins/descriptor.rs
• crates/vm/src/class.rs
• crates/vm/src/types/slot.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

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

crates/vm/src/types/slot.rs (1)

• hash_not_implemented (374-376)

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.