More slot impl #6618

youknowone · 2026-01-01T00:26:14Z

Summary by CodeRabbit

Bug Fixes
- Enhanced set and frozenset binary operations to properly handle mixed-type operands, ensuring correct results in edge cases.
- Refactored truthiness evaluation to use a protocol-driven approach, reducing unnecessary method lookups.
Changes
- Modified len method exposure on multiple built-in types; the len() function continues to work as expected.

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

coderabbitai · 2026-01-01T00:26:19Z

Warning

Rate limit exceeded

@youknowone has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 8 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between fbf02df and bb18d0c.

📒 Files selected for processing (11)

crates/vm/src/builtins/bool.rs
crates/vm/src/builtins/bytearray.rs
crates/vm/src/builtins/bytes.rs
crates/vm/src/builtins/dict.rs
crates/vm/src/builtins/list.rs
crates/vm/src/builtins/mappingproxy.rs
crates/vm/src/builtins/memory.rs
crates/vm/src/builtins/set.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/stdlib/ctypes/array.rs

📝 Walkthrough

Walkthrough

This pull request removes Python method exposure for __len__ across multiple builtin classes via #[pymethod] attribute removal, refactors boolean truthiness evaluation to use protocol-based slot lookups instead of method fallbacks, updates binary operations for mixed set/frozenset types with type checking, and improves slot machinery for method resolution based on whether a type defines its own methods.

Changes

Cohort / File(s)	Summary
Boolean Truthiness Refactor `crates/vm/src/builtins/bool.rs`	Replaced multi-path truthiness evaluation with simplified protocol-based approach: attempt `nb_bool` slot first, then `mp_length`, then `sq_length`, defaulting to truthy. Removed legacy `__bool__` and `__len__` fallback method lookups. Removed unused `malachite_bigint::Sign` import.
len Method Exposure Removal `crates/vm/src/builtins/{bytearray,bytes,dict,list,tuple}.rs`, `crates/vm/src/builtins/memory.rs`, `crates/vm/src/stdlib/ctypes/array.rs`	Removed `#[pymethod]` attribute from `__len__` methods across eight builtin types, eliminating direct Python method exposure while preserving internal Rust implementations.
Mapping Proxy Protocol Wiring `crates/vm/src/builtins/mappingproxy.rs`	Removed `#[pymethod]` from `__len__` and wired length exposure through sequence protocol via `PySequenceMethods.length` using `sequence_downcast`, maintaining external accessibility through protocol machinery instead of direct Python binding.
Set/Frozenset Binary Operations `crates/vm/src/builtins/set.rs`	Reworked binary operators (`__sub__/__rsub__`, `__and__/__rand__`, `__xor__/__rxor__`, `__or__/__ror__`) to validate operand types using new `AnySet::check()` helper, return `NotImplemented` for invalid type combinations, and wrap results to appropriate public types. Introduced unified `__contains__(&self, needle: &PyObject, vm)` signature for both `PySet` and `PyFrozenSet`; updated sequence containment to route through new `__contains__` implementation.
Collections Deque API Changes `crates/vm/src/stdlib/collections.rs`	Removed `#[pymethod]` exposure for `__len__` and `__add__` methods from `PyDeque`, eliminating Python-facing concatenation and length methods while retaining internal `concat` and `__iadd__` functionality.
Windows Registry Type Conversion `crates/vm/src/stdlib/winreg.rs`	Removed `#[pymethod]` attribute from `PyHkey::__int__` method, eliminating Python-level integer conversion exposure while preserving internal Rust method for internal protocol hooks.
Number Protocol Slot Copying `crates/vm/src/protocol/number.rs`	Extended `PyNumberSlots::copy_from` to mirror left-hand operator slots to right-hand counterparts for binary operators (Add, Subtract, Multiply, Remainder, Divmod, Power, Lshift, Rshift, And, Xor, Or, FloorDivide, TrueDivide, MatrixMultiply).
Slot Update Machinery `crates/vm/src/types/slot.rs`	Refactored slot update logic to collect slot definitions into temporary `Vec` before iteration, added `has_own` detection to determine if a type defines its own method for a slot, and implemented conditional wrapper/fallback selection based on ownership (ass_item/ass_subscript slots consider paired `__setitem__`/`__delitem__` methods).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Implement copyslot #6505 — Both PRs modify boolean/truthiness codepaths to use direct protocol slots (nb_bool, mapping, sequence) instead of legacy __bool__/__len__ fallbacks.
PyWrapperDescrObject and rewrite toggle_slot #6536 — Both PRs refactor VM slot/descriptor machinery with changes to slot update logic and wrapper/SlotFunc usage patterns in crates/vm/src/types/slot.rs.
RichCompare contains pymethod. No AtomicCell for slots #6562 — Both PRs modify slot machinery in crates/vm/src/types/slot.rs to centralize wrapper functions and change how slots are updated based on type definitions.

Poem

🐰 Protocol paths now guide our way, no more __len__ for the fray,
Sets and slots align just right, truthiness checked in protocol's light,
Simpler flows, fewer lookups dance, RustPython takes a cleaner stance! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.84% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'More slot impl' is vague and does not clearly convey the main changes, which involve refactoring truthiness evaluation, removing len method exposures, and restructuring set operations.	Consider a more descriptive title that captures the primary change, such as 'Refactor protocol-driven truthiness and remove len Python method exposures' or 'Implement slot-based protocols for truthiness and method access'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

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

685-700: Consider moving string interning outside the lock scope.

The ctx.intern_str() calls on lines 695-696 are made while holding the self.attributes.read() lock. If string interning acquires internal locks, this could introduce lock ordering concerns. Consider interning these strings before acquiring the attributes lock:

🔎 Suggested refactor

                     let has_own = {
+                        // Intern strings before acquiring the lock to avoid potential lock ordering issues
+                        let setitem = ctx.intern_str("__setitem__");
+                        let delitem = ctx.intern_str("__delitem__");
                         let guard = self.attributes.read();
                         // Check the current method name
                         let mut result = guard.contains_key(name);
                         // For ass_item/ass_subscript slots, also check the paired method
                         // (__setitem__ and __delitem__ share the same slot)
                         if !result
                             && (stringify!($slot) == "ass_item"
                                 || stringify!($slot) == "ass_subscript")
                         {
-                            let setitem = ctx.intern_str("__setitem__");
-                            let delitem = ctx.intern_str("__delitem__");
                             result = guard.contains_key(setitem) || guard.contains_key(delitem);
                         }
                         result
                     };

crates/vm/src/builtins/set.rs (1)

830-834: Consider using s.inner directly instead of s.inner.clone().

Since s (a PyFrozenSet) is consumed by the closure, you can move inner directly without cloning:
-                            r.map(|s| PySet {
-                                inner: s.inner.clone(),
-                            })
+                            r.map(|s| PySet { inner: s.inner })
This applies to similar patterns on lines 849-852, 867-870, and 885-888. The clone is cheap (just an Rc bump), but removing it is slightly more idiomatic.

📜 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 a4df238 and fbf02df.

📒 Files selected for processing (14)

crates/vm/src/builtins/bool.rs
crates/vm/src/builtins/bytearray.rs
crates/vm/src/builtins/bytes.rs
crates/vm/src/builtins/dict.rs
crates/vm/src/builtins/list.rs
crates/vm/src/builtins/mappingproxy.rs
crates/vm/src/builtins/memory.rs
crates/vm/src/builtins/set.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/protocol/number.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/stdlib/ctypes/array.rs
crates/vm/src/stdlib/winreg.rs
crates/vm/src/types/slot.rs

💤 Files with no reviewable changes (9)

crates/vm/src/builtins/bytes.rs
crates/vm/src/builtins/bytearray.rs
crates/vm/src/builtins/tuple.rs
crates/vm/src/stdlib/ctypes/array.rs
crates/vm/src/builtins/list.rs
crates/vm/src/builtins/memory.rs
crates/vm/src/stdlib/collections.rs
crates/vm/src/builtins/dict.rs
crates/vm/src/stdlib/winreg.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 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/protocol/number.rs
crates/vm/src/builtins/bool.rs
crates/vm/src/builtins/mappingproxy.rs
crates/vm/src/types/slot.rs
crates/vm/src/builtins/set.rs

🧠 Learnings (2)

📚 Learning: 2025-08-26T05:20:54.540Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.

Applied to files:

crates/vm/src/builtins/set.rs

📚 Learning: 2025-08-26T05:20:54.540Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Applied to files:

crates/vm/src/builtins/set.rs

🧬 Code graph analysis (3)

crates/vm/src/builtins/bool.rs (2)

crates/vm/src/protocol/mapping.rs (1)

slots (108-110)

crates/vm/src/protocol/sequence.rs (1)

slots (134-136)

crates/vm/src/builtins/mappingproxy.rs (6)

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

sequence_downcast (1905-1907)

crates/vm/src/builtins/set.rs (2)

__len__ (536-538)

__len__ (1008-1010)

crates/vm/src/builtins/dict.rs (2)

__len__ (215-217)

__len__ (766-768)

crates/vm/src/builtins/list.rs (1)

__len__ (185-187)

crates/vm/src/builtins/memory.rs (1)

__len__ (693-702)

crates/vm/src/builtins/tuple.rs (1)

__len__ (301-303)

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

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

find_slot_defs_by_name (918-920)

🔇 Additional comments (12)

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

610-612: Defensive collection looks reasonable.

Collecting into a Vec before iteration is a safe approach. Since find_slot_defs_by_name iterates over a static SLOT_DEFS array and returns &'static SlotDef references, there shouldn't be actual mutation-while-iteration issues here. However, this defensive pattern is low-cost and prevents potential future issues if the implementation changes.

701-713: LGTM!

The logic correctly prioritizes:

Use wrapper when the type has its own Python method definition (ensuring Python method dispatch)

Use native slot function from MRO when available (for efficiency)

Fall back to wrapper otherwise

This properly handles the case where a subclass defines __setitem__ or __delitem__ in Python while inheriting from a native type.

crates/vm/src/builtins/mappingproxy.rs (2)

237-237: LGTM! Proper sequence protocol integration.

The length binding correctly integrates __len__ into the sequence protocol using the sequence_downcast pattern, mirroring the existing mapping protocol implementation at line 223.

176-179: len implementation correctly shifts to protocol-based slot lookups.

Removal of #[pymethod] is consistent with the architectural shift to protocol-based access. The method properly delegates to the underlying object's length through both AsMapping (line 223) and AsSequence (line 237) protocol slots using the inherited downcast methods. The implementation matches the pattern used in other builtins.

crates/vm/src/protocol/number.rs (1)

356-476: LGTM! Consistent with From implementation pattern.

The addition of right-hand operator slot assignments correctly mirrors the pattern established in the From<&PyNumberMethods> implementation (lines 297-352). For native types, using the same function for both left and right slots is appropriate since these implementations perform their own type checking and handle mixed-type operations internally.

crates/vm/src/builtins/bool.rs (1)

44-66: Solid refactoring to protocol-based truthiness evaluation.

The slot-based approach correctly implements Python's truthiness semantics: attempts __bool__ via the number protocol first, then falls back to length checks via mapping/sequence protocols, finally defaulting to true. This aligns with the PR's broader shift away from method-based dispatch.

The use of mapping_unchecked() and sequence_unchecked() is appropriate given the preceding slot existence checks.

Protocol slot wiring is comprehensive across all major builtin types. All types that expose __len__ (list, dict, tuple, bytes, bytearray, set, frozenset, range, and others) have their length methods properly wired into the corresponding protocol slots via as_mapping(), as_sequence(), or as_number() implementations. No incomplete conversions detected.

crates/vm/src/builtins/set.rs (6)

536-542: LGTM - Slot-based method implementations.

Both __len__ and __contains__ are implemented without #[pymethod] attributes, which aligns with the PR's goal of using protocol-based slot lookups instead of method exposure. The implementations correctly delegate to the inner methods.

788-790: Correct routing through __contains__.

The sequence contains slot now properly routes through the __contains__ method, maintaining consistency with the slot-based design.

818-893: Binary operation handling looks correct for mixed set/frozenset types.

The implementation properly:

Guards with AnySet::check(a) && AnySet::check(b) to return NotImplemented for non-set operands

Handles the case where a is a PyFrozenSet (via reflected ops) by wrapping the result in PySet

The comments explaining the __rsub__ operand swapping are helpful for maintainability.

1008-1014: LGTM - Consistent with PySet implementation.

The __len__ and __contains__ implementations for PyFrozenSet follow the same slot-based pattern as PySet.

1168-1170: Consistent routing through __contains__.

1222-1272: LGTM - Correct handling for PyFrozenSet's AsNumber operations.

The implementation correctly:

Returns NotImplemented when operands aren't sets

Preserves result type based on the first operand (a)

Delegates to PySet methods when a is a PySet (via reflected ops)

crates/vm/src/builtins/set.rs

youknowone added 5 commits January 1, 2026 09:15

fix update_sub_slot

135e023

Fix __setitem__ __delitem__ handling

b2a3215

contains

59cab45

no __bool__ anymore

5d24adb

Add rops to wrapper

5871c72

youknowone force-pushed the update-slot branch 2 times, most recently from aefd83e to fbf02df Compare January 1, 2026 01:15

youknowone marked this pull request as ready for review January 1, 2026 01:51

coderabbitai bot reviewed Jan 1, 2026

View reviewed changes

crates/vm/src/builtins/set.rs Outdated Show resolved Hide resolved

youknowone added 2 commits January 1, 2026 11:08

Remove pymethod from set

9ae607f

remove len

bb18d0c

youknowone force-pushed the update-slot branch from fbf02df to bb18d0c Compare January 1, 2026 02:09

youknowone merged commit 020ff30 into RustPython:main Jan 1, 2026
23 of 24 checks passed

youknowone deleted the update-slot branch January 1, 2026 06:00

This was referenced Jan 1, 2026

sequence, mapping slots and fix separate __delitem__ slots #6621

Merged

__{get,set,del}item__ without pymethods #6622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

More slot impl #6618

More slot impl #6618

Uh oh!

youknowone commented Jan 1, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 1, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

More slot impl #6618

More slot impl #6618

Uh oh!

Conversation

youknowone commented Jan 1, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Jan 1, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 1, 2026 •

edited

Loading