sequence, mapping slots and fix separate __delitem__ slots #6621
Conversation
📝 WalkthroughWalkthroughThis PR removes PyO3 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin update-slot |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/builtins/getset.rs (1)
121-132: Remove the unused__set__and__delete__wrapper methods.These methods have no callers in the codebase and serve no purpose now that their
#[pymethod]attributes have been removed. Sincedescr_set(with#[pyslot]) already implements the descriptor protocol for both set and delete operations, these wrappers are redundant. Removing them will simplify the code without any functional impact.
🧹 Nitpick comments (3)
crates/vm/src/stdlib/ctypes/array.rs (1)
12-14: AsMapping wiring forPyCArraylooks consistent; minor style nit onPyMappingMethodsinitThe new
AsMappingimplementation correctly mirrors the existing sequence semantics (length viaStgInfo.length, subscript via__getitem__, and assignment/deletion via__setitem__/__delitem__with a TypeError on delete). AddingAsMappingto the#[pyclass(with(...))]list cleanly exposes this to the slot machinery.One optional polish: consider initializing
PyMappingMethodswith..PyMappingMethods::NOT_IMPLEMENTED(as in other types likePyCPointer/bytes/tuple) to make this resilient to future field additions and keep the style uniform across mapping implementations.Also applies to: 471-493, 495-498
crates/vm/src/types/slot.rs (2)
693-724: Shared ass-item slot update logic matches design; minor duplication but acceptableThe custom handling for
SqAssItemandMpAssSubscriptinupdate_one_slotdoes the right thing:
- Detects any own
__setitem__/__delitem__on the class and forces use of the generic wrappers (sequence_setitem_wrapper/mapping_setitem_wrapper), so Python-defined methods are always respected.- Otherwise, pulls a
SeqSetItem/SeqDelItemorMapSetSubscript/MapDelSubscriptslot out of the MRO vialookup_slot_in_mro, preserving native slots for builtin types and subclasses without overrides.- Falls back to wrappers when no wrapper-descriptor exists (e.g., pure Python implementations), which is consistent with the rest of the slot machinery.
There is a bit of overlap between the generic
update_sub_slot!helper and these bespoke branches, but it’s localized and clear, so I wouldn’t push to deduplicate unless this grows more complex.Also applies to: 1322-1349, 1367-1394
392-405: Code implementation is correct; add tests for subclass overrides of only__setitem__or__delitem__The SqAssItem/MpAssSubscript slot reinterpretation is sound:
update_sub_slot!(and explicit logic at lines 1322-1345, 1367-1390) correctly checks for both__setitem__and__delitem__on shared ass_item/ass_subscript slots- When either method is defined on a class, the wrapper is used to dispatch to the correct Python method
- Otherwise, the code looks up the corresponding slot function (
SeqSetItem/SeqDelItemorMapSetSubscript/MapDelSubscript) in the MRO- Falls back to the wrapper if no native slot is found, preserving Python-level semantics
This design mirrors CPython's
sq_ass_item/mp_ass_subscriptand correctly handles the set vs. delete distinction.Tests should cover subclass overrides where only
__delitem__is defined (no__setitem__), and vice versa, including subclasses of built-in sequences/mappings, to confirm correct dispatch and MRO inheritance.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
crates/stdlib/src/array.rscrates/stdlib/src/contextvars.rscrates/stdlib/src/mmap.rscrates/vm/src/builtins/bytearray.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/getset.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/mappingproxy.rscrates/vm/src/builtins/property.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/str.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/weakproxy.rscrates/vm/src/exception_group.rscrates/vm/src/stdlib/collections.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rs
💤 Files with no reviewable changes (14)
- crates/stdlib/src/contextvars.rs
- crates/stdlib/src/mmap.rs
- crates/vm/src/builtins/weakproxy.rs
- crates/vm/src/builtins/mappingproxy.rs
- crates/vm/src/builtins/bytearray.rs
- crates/vm/src/builtins/range.rs
- crates/vm/src/builtins/bytes.rs
- crates/stdlib/src/array.rs
- crates/vm/src/builtins/list.rs
- crates/vm/src/builtins/dict.rs
- crates/vm/src/exception_group.rs
- crates/vm/src/builtins/str.rs
- crates/vm/src/stdlib/collections.rs
- crates/vm/src/builtins/tuple.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/builtins/getset.rscrates/vm/src/builtins/property.rscrates/vm/src/types/slot.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/types/slot_defs.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/stdlib/ctypes/array.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/stdlib/ctypes/array.rs
🧬 Code graph analysis (2)
crates/vm/src/stdlib/ctypes/pointer.rs (7)
crates/vm/src/stdlib/ctypes/array.rs (12)
as_mapping(472-492)zelf(307-308)__getitem__(944-959)value(317-317)value(759-759)value(772-772)value(784-784)value(804-804)value(848-848)value(1188-1189)value(1244-1245)__setitem__(1051-1074)crates/vm/src/builtins/bytes.rs (2)
as_mapping(620-629)__getitem__(245-247)crates/vm/src/builtins/mappingproxy.rs (4)
as_mapping(220-231)mapping(66-66)mapping(67-67)__getitem__(115-118)crates/vm/src/builtins/tuple.rs (3)
as_mapping(372-381)zelf(277-281)__getitem__(319-321)crates/vm/src/stdlib/ctypes/base.rs (12)
new(149-165)new(1249-1257)zelf(1332-1333)zelf(1613-1614)value(862-863)value(871-871)value(871-871)value(893-893)value(907-907)value(922-922)value(932-932)value(1364-1364)crates/vm/src/function/protocol.rs (1)
mapping(142-144)crates/vm/src/stdlib/ctypes/structure.rs (1)
zelf(136-137)
crates/vm/src/stdlib/ctypes/array.rs (2)
crates/vm/src/types/slot.rs (4)
as_mapping(1965-1965)new(33-38)new(190-196)mapping_downcast(1968-1970)crates/vm/src/stdlib/ctypes/pointer.rs (13)
as_mapping(790-807)zelf(74-75)zelf(168-168)__getitem__(350-365)value(84-84)value(108-108)value(109-109)value(315-315)value(318-319)value(583-583)value(597-597)value(605-605)__setitem__(533-549)
🔇 Additional comments (5)
crates/vm/src/builtins/property.rs (2)
99-127: Excellent deadlock prevention with explicit lock management.The implementation consistently clones values and releases locks before calling Python code (lines 109, 118), which is critical for preventing deadlocks. The use of
PySetterValueenum to distinguish between set and delete operations in a singledescr_setslot is a clean design pattern.
128-139: Both__set__and__delete__appear to lack#[pymethod]attributes, contrary to the AI summary.The shell results show neither method has
#[pymethod]in property.rs. However, incrates/vm/src/stdlib/ctypes/base.rs, both methods have#[pymethod]attributes. Additionally,crates/vm/src/types/slot.rsshows that__set__and__delete__are dynamically invoked viavm.call_special_method()from the slot implementation, which typically requires#[pymethod]for proper Python method lookup. Verify whether both methods should have#[pymethod]to match the ctypes pattern and enable proper dynamic method resolution.crates/vm/src/builtins/descriptor.rs (1)
432-440: Split Seq/Map ass-item SlotFunc variants correctly encode set vs del semanticsThe refactor of
SlotFuncintoSeqSetItem/SeqDelItemandMapSetSubscript/MapDelSubscript, plus the corresponding branches incall, looks right:
- Sequence wrappers now distinguish
__setitem__(self, index, value)from__delitem__(self, index)viaSome(value)vsNoneon a sharedSeqAssItemFunc.- Mapping wrappers do the same for
keyvia a sharedMapAssSubscriptFunc.- Debug formatting and imports are updated consistently, so introspection and logging remain clear.
This aligns nicely with the shared-slot handling added in
slot.rsandslot_defs.rs, and should resolve the previous ambiguity between set and delete for these slots.Also applies to: 473-481, 607-640
crates/vm/src/types/slot_defs.rs (1)
224-235: Shared-slot and mapping/sequence precedence changes are correctThis file's updates work together well:
is_shared_slot()andextract_from_slot_func()now explicitly treatSqAssItem/MpAssSubscriptas the shared__setitem__/__delitem__slots, matching the newSlotFunc::{SeqSetItem, SeqDelItem, MapSetSubscript, MapDelSubscript}variants.get_slot_func()returns the "set" variants by default, whileget_slot_func_with_op(..., Some(SlotOp::Delete))remaps to the "del" variants. This gives a clean separation between call paths while sharing the underlying function pointer.SLOT_DEFSis correctly reordered so mapping protocol (MpLength,MpSubscript,MpAssSubscript) appears before sequence protocol, with explicit comment referencing CPython typeobject.c:10995-11006. Parallel entries for__len__,__getitem__,__setitem__, and__delitem__ensure mapping subscripts win over sequence items, and the code includes tests validating dual entries viatest_find_by_name().The changes correctly implement CPython's precedence rules for types implementing both sequence and mapping views.
crates/vm/src/stdlib/ctypes/pointer.rs (1)
3-5: Pointer mapping support is correctly implemented; add explicit tests for mapping access and deletionThe AsMapping integration for
PyCPointeris properly implemented:
- Imports and
with(Constructor, Initializer, AsNumber, AsBuffer, AsMapping)correctly enable mapping slots alongside number and buffer behavior.AsMapping'ssubscriptandass_subscriptclosures properly delegate to existing__getitem__and__setitem__, while the deletion path correctly raisesTypeError("Pointer does not support item deletion").- Using
..PyMappingMethods::NOT_IMPLEMENTEDkeeps the mapping surface minimal and consistent with similar implementations likePyCArray.Consider adding tests that:
- Exercise mapping-style access on
_ctypes._Pointerinstances viaoperator.getitem.- Verify that
del p[key]raises the expected TypeError.
7165ad1 to
ab1cb9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/types/slot.rs (2)
1321-1349: Shared sequence/mapping set/del slot handling is correct; consider refining the removal path
- The new
SqAssItemandMpAssSubscriptbranches correctly treat these as shared slots for__setitem__and__delitem__:
- If the class defines either
__setitem__or__delitem__, you installsequence_setitem_wrapper/mapping_setitem_wrapper, so both operations go through the Python methods viasetitem_wrapper.- Otherwise, you try to reuse
SeqSetItem/SeqDelItemorMapSetSubscript/MapDelSubscriptfrom the MRO and fall back to the same wrappers if no slot wrapper is found.- The underlying wrappers map
Some(value)to__setitem__andNoneto__delitem__, which matches how the shared slots are intended to work.One subtle corner case worth considering: in the
ADD == falsepath forSqAssItem/MpAssSubscriptyou alwaysinherit_from_mroand don’t re-evaluate whether the other method (__delitem__when removing__setitem__, or vice versa) is still present on the class. In exotic scenarios where users dynamically delete only one of the pair at runtime, that could cause the class to ignore the remaining local override and fully fall back to the base slot. If you care about that edge case, you could mirror thehas_owncheck from the ADD-true path and only inherit when both__setitem__and__delitem__are gone.Also applies to: 1367-1394, 375-396, 422-430
688-727:update_sub_slot!’sass_item/ass_subscriptspecial-case is now redundantThe
update_sub_slot!macro still has a branch that checksstringify!($slot)for"ass_item"/"ass_subscript"to look for paired__setitem__/__delitem__, butSqAssItemandMpAssSubscriptnow have dedicated match arms with custom logic and no longer call this macro with those slots. That branch is effectively dead code and could be removed or simplified to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/getset.rscrates/vm/src/builtins/property.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rs
💤 Files with no reviewable changes (3)
- crates/vm/src/stdlib/ctypes/base.rs
- crates/vm/src/builtins/property.rs
- crates/vm/src/builtins/getset.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/types/slot.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/types/slot_defs.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/ctypes/pointer.rs (3)
crates/vm/src/builtins/dict.rs (6)
as_mapping(435-451)mapping(1137-1139)mapping(1200-1202)mapping(1263-1265)__getitem__(377-379)__setitem__(239-246)crates/vm/src/builtins/tuple.rs (3)
as_mapping(372-381)zelf(277-281)__getitem__(319-321)crates/vm/src/function/protocol.rs (1)
mapping(142-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (4)
crates/vm/src/stdlib/ctypes/array.rs (1)
471-493: AsMapping implementation forPyCArraylooks consistent and safeThe mapping table reuses the existing
__getitem__/__setitem__/__delitem__implementations and computes length fromStgInfo, so mapping operations (len, indexing, assignment, and deletion error) stay consistent with the sequence behavior and current__delitem__semantics. No issues spotted here.Also applies to: 495-498
crates/vm/src/types/slot_defs.rs (1)
392-405: Slot accessor wiring for setitem/delitem and mapping vs sequence precedence is coherent
SqAssItem/MpAssSubscriptnow correctly treat their slots as shared between set and delete by mapping toSeqSetItem/SeqDelItemandMapSetSubscript/MapDelSubscript, andget_slot_func_with_opreturns the delete variants only whenSlotOp::Delete.- The SLOT_DEFS reordering (mapping entries for
__len__/__getitem__/__setitem__/__delitem__before the sequence ones, plus__contains__→SqContains) matches CPython’s rule that mapping slots win over sequence slots for__getitem__and clarifies ownership of__contains__.This all lines up cleanly with the new
SlotFuncvariants indescriptor.rs; I don’t see functional issues here.Also applies to: 761-786, 799-812, 1080-1137
crates/vm/src/stdlib/ctypes/pointer.rs (1)
262-265: Pointer AsMapping integration correctly reuses existing item semantics
PyCPointer’s mapping table delegates subscript and ass_subscript to__getitem__/__setitem__, and explicitly raises on delete attempts, which matches the existing “no deletion” semantics. AddingAsMappingto the pyclass mixins cleanly exposes the mapping protocol without changing pointer behavior.Also applies to: 789-808
crates/vm/src/builtins/descriptor.rs (1)
431-440: Seq/Map set vs delete SlotFunc variants are well factoredThe new
SeqSetItem/SeqDelItemandMapSetSubscript/MapDelSubscriptvariants correctly:
- Reuse the existing
SeqAssItemFunc/MapAssSubscriptFuncsignatures.- Bind arguments to
(index, Some(value))vs(index, None)for sequences and(key, Some(value))vs(key, None)for mappings.- Mirror the pattern used by
SetAttro/DelAttroandDescrSet/DescrDel.This makes the distinguishing of set vs delete explicit at the SlotFunc level without changing the underlying slot function contracts.
Also applies to: 473-480, 607-640
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.