Skip to content

RichCompare contains pymethod. No AtomicCell for slots #6562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 2 commits into from
Dec 29, 2025
Merged

RichCompare contains pymethod. No AtomicCell for slots #6562

youknowone merged 2 commits into from
Dec 29, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 28, 2025 •
edited by coderabbitai bot
Loading

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
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025 •
edited
Loading

📝 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 youknowone force-pushed the pymethod branch 2 times, most recently from 382faaa to 519f527 Compare December 28, 2025 07:43
@youknowone youknowone changed the title Remove unnecessary #[pymethod] RichCompare contains pymethod. No AtomicCell for slots Dec 29, 2025
@youknowone youknowone marked this pull request as ready for review December 29, 2025 01:58
@youknowone youknowone merged commit d1ff2cd into RustPython:main Dec 29, 2025
13 checks passed
@youknowone youknowone deleted the pymethod branch December 29, 2025 02:21
This was referenced Dec 31, 2025
Merged
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone