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

youknowone · 2025-12-28T06:54:18Z

Summary by CodeRabbit

Refactor
- Simplified internal storage and access mechanisms for protocol method handling in mappings and sequences, reducing runtime overhead.
- Restructured and centralized comparison operation implementations for improved consistency and maintainability.

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

coderabbitai · 2025-12-28T06:54:23Z

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

youknowone force-pushed the pymethod branch 2 times, most recently from 382faaa to 519f527 Compare December 28, 2025 07:43

slots: add comparison pymethods to Comparable trait

765133c

youknowone force-pushed the pymethod branch from 519f527 to 8ff35a9 Compare December 28, 2025 13:38

No AtomicCell

73ad0f4

youknowone force-pushed the pymethod branch from 8ff35a9 to 73ad0f4 Compare December 29, 2025 01:05

youknowone changed the title ~~Remove unnecessary #[pymethod]~~ RichCompare contains pymethod. No AtomicCell for slots Dec 29, 2025

youknowone marked this pull request as ready for review December 29, 2025 01:58

youknowone merged commit d1ff2cd into RustPython:main Dec 29, 2025
13 checks passed

youknowone deleted the pymethod branch December 29, 2025 02:21

This was referenced Dec 31, 2025

Remove duplicated richcompare pymethods and add missing rops' wrapper, __getattr__, proper __bool__ impl #6579

Merged

More slot impl #6618

Merged

slot for numeric ops #6619

Merged

solved test case error for test_dictviews.py from object.rs #4779

Open

coderabbitai bot mentioned this pull request Jan 23, 2026

derive Copy where possible #6844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RichCompare contains pymethod. No AtomicCell for slots#6562

RichCompare contains pymethod. No AtomicCell for slots#6562
youknowone merged 2 commits intoRustPython:mainfrom
youknowone:pymethod

youknowone commented Dec 28, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 28, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Dec 28, 2025 • 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 Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 28, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 28, 2025 •

edited

Loading