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

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

• 📝 Generate docstrings

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

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 476b75d and 73ad0f4.

• 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

📄 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

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

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

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

• None (1687-1687)

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

• None (1152-1152)

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.

• X
• Mastodon
• Reddit
• LinkedIn

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