RichCompare contains pymethod. No AtomicCell for slots by youknowone · Pull Request #6562 · RustPython/RustPython

youknowone

📝 Walkthrough

Walkthrough

This PR removes atomic cell wrapping from function-pointer option fields across mapping and sequence protocol implementations and type slot definitions. In PyMappingMethods, PyMappingSlots, and PySequenceMethods, fields previously wrapped in AtomicCell<Option<fn...>> are now plain Option<fn...>. The change also introduces explicit wrapper functions for sequence/mapping operations and adds comparison dunder methods to the Comparable trait.

Changes

Cohort / File(s)	Summary
Mapping Protocol Refactoring `crates/vm/src/protocol/mapping.rs`	`PyMappingSlots` and `PyMappingMethods` struct fields (`length`, `subscript`, `ass_subscript`) changed from `AtomicCell<Option<fn...>>` to `Option<fn...>`. `PyMappingSlots::copy_from` updated to access methods directly instead of using `.load()`. `NOT_IMPLEMENTED` initialization simplified to use plain `None` values. Removed `#[allow(clippy::declare_interior_mutable_const)]` attribute.
Sequence Protocol Refactoring `crates/vm/src/protocol/sequence.rs`	`PySequenceMethods` fields (length, concat, repeat, item, ass_item, contains, inplace_concat, inplace_repeat) changed from `AtomicCell<Option<fn...>>` to `Option<fn...>`. `PySequenceSlots::copy_from` updated for direct field access. `NOT_IMPLEMENTED` constant simplified. Removed clippy attribute.
Context Variables & SQLite Integration `crates/stdlib/src/contextvars.rs`, `crates/stdlib/src/sqlite.rs`	Updated `PyContext` mapping and `Blob` sequence method initializations to use plain `None` instead of `AtomicCell::new(None)` for ass_subscript and sequence operation fields.
Slot Wrapper Centralization `crates/vm/src/types/slot.rs`	Introduced standalone wrapper functions (`mapping_setitem_wrapper`, `mapping_getitem_wrapper`, `mapping_len_wrapper`, `sequence_len_wrapper`, `sequence_getitem_wrapper`, `sequence_setitem_wrapper`, `sequence_contains_wrapper`) to replace inline closures. Enhanced `Comparable` trait with explicit dunder methods (`__eq__`, `__ne__`, `__lt__`, `__le__`, `__ge__`, `__gt__`) delegating to `Self::cmp`.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Implement copyslot #6505 — Modifies the same mapping/sequence method slot fields and initializations that are refactored in this PR's removal of AtomicCell wrappers.
__repr__ with slot wrapper #6486 — Updates slot-wrapper logic in crates/vm/src/types/slot.rs alongside this PR's wrapper function centralization changes.
iter with slot-wrapper #6488 — Affects slot-wrapper machinery in the same file, introducing macro-based slot insertion patterns related to the new wrapper functions.

Suggested reviewers

ShaharNaveh
arihant2math

Poem

🐰 No more atomic cells to guard our ways,
Plain options shine through protocol days,
Wrappers bundled, clean and bright,
Comparisons dance left and right,
A simpler VM, lighter and light! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: removing AtomicCell from protocol slots and adding rich comparison methods to PyMethods.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent 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 476b75d and 73ad0f4.

📒 Files selected for processing (5)

crates/stdlib/src/contextvars.rs
crates/stdlib/src/sqlite.rs
crates/vm/src/protocol/mapping.rs
crates/vm/src/protocol/sequence.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 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/stdlib/src/contextvars.rs
crates/vm/src/types/slot.rs
crates/stdlib/src/sqlite.rs
crates/vm/src/protocol/mapping.rs
crates/vm/src/protocol/sequence.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/stdlib/src/sqlite.rs
crates/vm/src/protocol/sequence.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/stdlib/src/sqlite.rs
crates/vm/src/protocol/sequence.rs

🧬 Code graph analysis (1)

crates/stdlib/src/contextvars.rs (2)

crates/vm/src/stdlib/ctypes/function.rs (1)

None (1687-1687)

crates/vm/src/stdlib/itertools.rs (1)

None (1152-1152)

🔇 Additional comments (10)

crates/stdlib/src/contextvars.rs (1)

267-267: LGTM!

The change from AtomicCell::new(None) to plain None correctly aligns with the updated PyMappingMethods struct definition where fields are now plain Option types instead of AtomicCell-wrapped options.

crates/stdlib/src/sqlite.rs (1)

2548-2567: LGTM!

The AsSequence implementation for Blob correctly updates the unimplemented sequence method slots from AtomicCell::new(None) to plain None, consistent with the new PySequenceMethods struct definition. The contains slot appropriately uses atomic_func! for its actual implementation.

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

114-118: LGTM!

The atomic_func! macro now returns Some($x) directly instead of wrapping in AtomicCell. This aligns with the migration from AtomicCell-wrapped function pointers to plain Option types in PyMappingMethods and PySequenceMethods.

388-439: LGTM!

Clean extraction of sequence/mapping operation wrappers into dedicated named functions. The #[inline(never)] annotations prevent these wrappers from being inlined, which helps with code size and debuggability. Each wrapper correctly delegates to the underlying generic len_wrapper, getitem_wrapper, setitem_wrapper, or contains_wrapper functions.

1194-1240: LGTM!

The slot accessor updates correctly reference the new named wrapper functions (sequence_len_wrapper, sequence_getitem_wrapper, sequence_setitem_wrapper, sequence_contains_wrapper, mapping_len_wrapper, mapping_getitem_wrapper, mapping_setitem_wrapper) instead of inline closures.

1564-1617: LGTM!

The addition of explicit dunder methods (__eq__, __ne__, __lt__, __le__, __ge__, __gt__) to the Comparable trait provides proper Python-level method exposure. Each method correctly delegates to Self::cmp with the appropriate PyComparisonOp variant, maintaining consistency with the slot-level slot_richcompare implementation.

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

37-47: LGTM!

The copy_from method correctly accesses the PyMappingMethods fields directly (no longer using .load()) since they are now plain Option types. The pattern if let Some(f) = methods.field cleanly handles the optional function pointers.

52-70: LGTM!

The PyMappingMethods struct now uses plain Option types for its function pointer fields, enabling const initialization of NOT_IMPLEMENTED. The separation between mutable PyMappingSlots (which retains AtomicCell for runtime slot updates) and static PyMappingMethods (now with plain Option for const definitions) is a clean architectural pattern.

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

44-69: LGTM!

The copy_from method correctly accesses the PySequenceMethods fields directly since they are now plain Option types. The implementation mirrors the pattern used in PyMappingSlots::copy_from.

72-103: LGTM!

The PySequenceMethods struct now uses plain Option types for all function pointer fields, enabling const initialization of NOT_IMPLEMENTED. This change is consistent with the PyMappingMethods refactoring and maintains the clean separation between mutable PySequenceSlots (retaining AtomicCell) and static PySequenceMethods (now with plain Option).

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.}