-
Notifications
You must be signed in to change notification settings - Fork 1.4k
PyWrapperDescrObject and rewrite toggle_slot #6536
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.
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
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors VM slot/descriptor system to be data-driven: introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as ClassInit
participant Class as add_operators
participant SlotDefs as SLOT_DEFS
participant SlotLogic as slot.rs (lookup/update)
participant Desc as builtins::descriptor::PyWrapper
participant Dict as TypeDict
Init->>Class: extend_class(class, ctx)
Class->>SlotDefs: iterate SLOT_DEFS
loop per SlotDef
Class->>SlotDefs: resolve accessor/op/metadata
Class->>Dict: check existing attribute (dunder)
alt attribute exists
Class-->>Dict: skip insertion
else
Class->>SlotLogic: lookup_slot_in_mro(def.accessor, ctx)
SlotLogic-->>Class: Option<SlotFunc> / binding
alt descriptor needed
Class->>Desc: PyWrapper::new(slot_func, doc)
Desc-->>Class: descriptor instance
Class->>Dict: insert(dunder_name, descriptor)
else
Class->>Dict: set resolved binding
end
end
end
Class-->>Init: extension complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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 |
9b141d4 to
5388680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/types/slotdefs.rs (1)
82-98: Minor: Comment says "Bitwise operations (47-64)" but the range seems different.The comment at line 83 says "(47-64)" but
NumLshiftstarts afterNumInplaceMatrixMultiply. This is a documentation nit - the code functionality is correct.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (9)
.cspell.dict/cpython.txtcrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slotdefs.rscrates/vm/src/types/zoo.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 Rust 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/mod.rscrates/vm/src/protocol/mod.rscrates/vm/src/types/zoo.rscrates/vm/src/builtins/type.rscrates/vm/src/types/slotdefs.rscrates/vm/src/class.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/types/slot.rs
🧠 Learnings (3)
📚 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/vm/src/protocol/mod.rscrates/vm/src/types/slot.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 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/vm/src/protocol/mod.rscrates/vm/src/types/slot.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (3)
crates/vm/src/protocol/mod.rs (1)
crates/vm/src/protocol/number.rs (1)
handle_bytes_to_int_err(558-578)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(86-95)
crates/vm/src/types/slot.rs (1)
crates/vm/src/types/slotdefs.rs (2)
find_slotdefs_by_name(1190-1192)inherit_from_mro(277-409)
⏰ 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). (2)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (25)
.cspell.dict/cpython.txt (1)
4-4: LGTM!The "attro" token is a valid CPython term (used in
tp_getattro,tp_setattro) and is correctly placed in alphabetical order.crates/vm/src/protocol/mod.rs (1)
13-16: LGTM!The addition of
PyNumberTernaryFuncto the public exports is necessary for the new ternary slot wrappers (power operations) and is properly ordered.crates/vm/src/types/zoo.rs (1)
192-192: LGTM!The update from
PySlotWrappertoPyWrapperis consistent with the type rename across the codebase.crates/vm/src/types/mod.rs (1)
2-2: LGTM!The new
slotdefsmodule is properly declared and the key public items (SLOT_DEFS,SlotAccessor,SlotDef) are correctly re-exported.Also applies to: 7-7
crates/vm/src/builtins/type.rs (1)
466-472: LGTM!The refactor to iterate
SLOT_DEFSfor slot inheritance is clean and maintainable. The comment correctly notes thatas_bufferis handled separately ininherit_readonly_slots(lines 459-462) since it's not anAtomicCell.crates/vm/src/class.rs (2)
13-57: Well-structured implementation mirroring CPython'sadd_operators().The function correctly:
- Skips
__new__which has special handling viaslot_new_wrapper- Handles
__hash__ = Nonefor unhashable types- Only creates wrappers for slots that have functions and aren't already in the dict
185-186: LGTM!Clean replacement of the previous macro-based slot wrapper logic with the new
add_operatorscall.crates/vm/src/types/slot.rs (6)
292-305: LGTM!The new type aliases for sequence and mapping sub-slot function types improve code clarity and match the patterns used elsewhere in the codebase.
350-373: LGTM!The ternary operation wrapper macros correctly handle the optional modulus argument for
__pow__,__rpow__, and__ipow__. When the third argument isNone, only two arguments are passed to the special method, matching Python's semantics.
550-561: LGTM!The refactored
update_slotmethod cleanly delegates tofind_slotdefs_by_nameandupdate_one_slot, aligning with the data-driven approach usingSLOT_DEFS.
618-643: Correct handling of__hash__ = Nonefor unhashable types.The special case properly detects when
__hash__is explicitly set toNonein the class dict or MRO, and storeshash_not_implementedaccordingly. This matches CPython's behavior.
1174-1214: LGTM!The
lookup_slot_in_mrohelper correctly distinguishes between slot wrappers (from which slot functions can be extracted) and real Python methods (which should use the generic wrapper). The early return on finding any attribute ensures the first definition in MRO wins.
1107-1120: TheSeqConcat/SeqRepeatslot handling is intentional and correct.When
ADD=true(during type creation), the sequence concat/repeat slots are skipped to allow the dual__add__definition to be processed via theNumAddslot instead. This is the correct design: list and other sequence types define__add__as a method (using#[pymethod]), which gets stored in theNumAddslot. The "number protocol fallback" comment refers to the runtime behavior in_add(): the operation first triesNumAdd(number protocol), and only if that returnsNotImplementeddoes it fall back to the sequence protocol'sconcat()method. This approach works correctly for types likelistthat define__add__via the sequence protocol.crates/vm/src/types/slotdefs.rs (6)
1-5: LGTM!Clear module documentation explaining the purpose of this file as the CPython
slotdefs[]equivalent.
27-124: Well-organizedSlotAccessorenum with#[repr(u8)]for compact storage.The enum comprehensively covers all CPython slot types with clear grouping via comments. The explicit discriminant values help ensure stable ordering.
463-475: Good implementation of SLOTDEFINED check for__init__inheritance.This correctly handles the multiple inheritance edge case where
__init__should only be inherited if it was explicitly defined in the base class (not inherited from a grandparent). This matches CPython's behavior.
762-938: Comprehensive SLOT_DEFS array covering main slots, comparison, descriptors, and sequence/mapping protocols.The duplicate entries for
__len__,__getitem__,__setitem__,__delitem__,__add__,__mul__,__iadd__,__imul__correctly map to both sequence and number/mapping protocols, matching CPython's slotdefs behavior.
1189-1195: LGTM!The
find_slotdefs_by_nameutility provides efficient lookup for slot updates, andSLOT_DEFS_COUNTis useful for testing/validation.
1197-1244: Good test coverage for the slot definitions.Tests verify count bounds, name lookups (including duplicate entries),
#[repr(u8)]size guarantee, andrich_compare_opmappings.crates/vm/src/builtins/descriptor.rs (6)
7-16: LGTM! Import additions support the expanded slot system.The new imports properly support the SlotFunc enum expansion, bringing in function types for attribute access, descriptors, sequences, mappings, and numeric operations.
404-445: Well-structured slot function expansion.The SlotFunc enum expansion is cleanly organized by protocol category with clear comments. The design choice to use the same function type for Set/Del variants (e.g., SetAttro/DelAttro both use SetattroFunc) with different PySetterValue handling is elegant and reduces code duplication.
447-483: LGTM! Complete Debug implementation.All SlotFunc variants are properly covered with consistent formatting. The additional detail for RichCompare showing the comparison operator is helpful for debugging.
530-646: Comprehensive and correct slot call implementations.All new SlotFunc::call implementations are well-structured and handle their respective protocols correctly:
- Attribute access slots (GetAttro, SetAttro, DelAttro): Properly use PySetterValue to distinguish set from delete operations
- Descriptor slots (DescrGet, DescrSet, DescrDel): Correctly implement Python's descriptor protocol with proper None handling for instance access
- Sequence/Mapping slots: Consistently use
sequence_unchecked()/mapping_unchecked()for protocol access- Number slots: Proper handling of unary, binary, and ternary operations with correct default argument handling for
__pow__The use of
OptionalArgandinto_option()for assignment slots (SeqAssItem, MapAssSubscript) elegantly handles both__setitem__and__delitem__semantics.
394-394: Clean and consistent rename from PySlotWrapper to PyWrapper.The rename is thoroughly applied across all trait implementations (PyPayload, GetDescriptor, Callable, Representable), the struct definition, and class extension call. All references are updated consistently.
Also applies to: 655-655, 662-666, 668-683, 685-703, 709-729, 731-740
749-749: LGTM! PyMethodWrapper correctly updated to reference PyWrapper.The wrapper field type is properly updated from
PyRef<PySlotWrapper>toPyRef<PyWrapper>, maintaining consistency with the rename.
5388680 to
ade6c6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/types/slot_defs.rs (1)
1223-1227: Test assumesSlotAccessorsize is exactly 1 byte.The assertion
std::mem::size_of::<SlotAccessor>() == 1enforces#[repr(u8)]sizing, but the enum doesn't have the#[repr(u8)]attribute. With 80+ variants, the compiler should still use 1 byte, but adding explicit#[repr(u8)]would make this guarantee explicit and prevent future breakage if variants exceed 256.Consider adding explicit repr(u8)
/// Slot accessor /// /// Flat enum with all slot types inlined for `#[repr(u8)]` support. /// Each variant directly corresponds to a slot field in PyTypeSlots. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[repr(u8)] pub enum SlotAccessor {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (8)
.cspell.dict/cpython.txtcrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .cspell.dict/cpython.txt
- crates/vm/src/protocol/mod.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 Rust 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/mod.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/class.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/type.rscrates/vm/src/types/slot_defs.rs
🧠 Learnings (3)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.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 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/vm/src/types/slot.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/vm/src/types/slot.rs
🧬 Code graph analysis (2)
crates/vm/src/builtins/descriptor.rs (1)
crates/vm/src/class.rs (1)
extend_class(126-194)
crates/vm/src/types/slot.rs (1)
crates/vm/src/types/slot_defs.rs (2)
find_slot_defs_by_name(1187-1189)inherit_from_mro(276-408)
⏰ 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 on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (15)
crates/vm/src/types/mod.rs (1)
1-9: LGTM!The module organization and public re-exports are clean. The new
slot_defsmodule is properly exposed alongside existing exports.crates/vm/src/builtins/type.rs (1)
466-472: LGTM!The refactored
inherit_slotsis much cleaner than per-slot macros. The data-driven iteration viaSLOT_DEFSimproves maintainability and reduces boilerplate. The comment correctly notes thatas_bufferhas separate handling.crates/vm/src/class.rs (2)
26-35: Function pointer comparison forhash_not_implementedis correct.The pattern
h as usize == hash_not_implemented as usizeis a valid way to compare function pointers in Rust when the types match. This correctly identifies types that have been explicitly marked unhashable to set__hash__ = Nonein the type dict.
13-57: Well-structuredadd_operatorsfunction.The implementation correctly:
- Skips
__new__which has special handling- Handles
__hash__ = Nonefor unhashable types- Only creates wrappers for slots that have functions and don't already exist in the dict
This aligns well with CPython's
add_operatorsbehavior.crates/vm/src/builtins/descriptor.rs (3)
398-444: SlotFunc enum expansion looks comprehensive.The new variants cover all necessary slot categories:
- Basic slots (Call, Del)
- Attribute access (GetAttro, SetAttro, DelAttro)
- Rich comparison with op parameter
- Descriptor protocol (DescrGet, DescrSet, DescrDel)
- Sequence/Mapping sub-slots
- Number sub-slots (boolean, unary, binary, ternary)
The design properly separates slots by their function signatures.
650-665: LGTM! Clean rename fromPySlotWrappertoPyWrapper.The struct definition and payload implementation are straightforward. The rename aligns with the PR's goal of matching CPython's
PyWrapperDescrObjectnaming conventions.
599-604: TheSeqAssItemargument binding correctly handles both__setitem__and__delitem__semantics.When
__delitem__is called with a single argument, theOptionalArg<PyObjectRef>binding correctly interprets the missing second parameter asNone. TheOptionalArgenum declaresarity()as0..=1, and itsfrom_args()implementation converts an absent argument toOptionalArg::Missing, which is then converted toNoneviainto_option(). This design properly routes__delitem__calls throughSeqAssItemwith the value parameter set toNone.crates/vm/src/types/slot.rs (4)
350-373: Ternary wrapper macros handle optional modulus correctly.The logic properly checks
vm.is_none(c)to decide whether to passmodargument. This matches Python'spow(x, y[, mod])semantics where mod is optional.
549-561: Clean refactoring ofupdate_slotto useSLOT_DEFS.The method now iterates over all matching slot definitions for a given name and delegates to
update_one_slot. This is more maintainable than the previous approach and correctly handles cases where one dunder name maps to multiple slots (e.g.,__add__for both sequence and number protocols).
1174-1214:lookup_slot_in_mroimplementation is sound.The function correctly:
- Checks self's dict first
- Falls through to MRO lookup
- Returns
Noneif attribute exists but isn't a wrapper descriptor (meaning a real method override)- Extracts the slot function only from
PyWrapperinstancesThis ensures that user-defined Python methods properly override C-level slot wrappers.
1107-1120: The code is correct and matches CPython's behavior. In CPython,PySequence_ConcatandPySequence_Repeatfall back to the numeric slotsnb_addandnb_multiplywhen sequence slots are unavailable—exactly as the comment suggests. WhenADDis true (number protocol present), the sequence slots don't require explicit update because CPython automatically handles the fallback to numeric operations. The!ADDbranch correctly inherits sequence slots from the MRO when the number protocol is not defined.crates/vm/src/types/slot_defs.rs (4)
1-21: Well-designedSlotDefstructure.The design with
name,accessor, anddocfields provides a clean, extensible way to define slots. The static lifetime strings are appropriate for compile-time constants.
462-474: Init slot inheritance has special SLOTDEFINED logic.The implementation correctly checks if the base type's
initslot differs from its base's base - this matches CPython's behavior forSLOTDEFINEDwhich prevents inheriting__init__through multiple levels when intermediate classes don't define it. Good attention to detail.
869-934: Duplicate dunder names intentionally map to multiple slot accessors.The SLOT_DEFS array correctly has entries like:
__len__→SeqLengthandMapLength__getitem__→SeqItemandMapSubscript__add__→SeqConcatandNumAddThis is by design to match CPython's slot system where the same dunder can trigger different C-level slots depending on the protocol. The
find_slot_defs_by_namefunction returns all matching entries.
759-1184: Comprehensive SLOT_DEFS array covering all major protocols.The array includes:
- Main slots (init, new, del, repr, str, hash, call, iter, next)
- Attribute access (getattribute, setattr, delattr)
- Rich comparison (eq, ne, lt, le, gt, ge)
- Descriptor protocol (get, set, delete)
- Sequence protocol (len, getitem, setitem, delitem, contains, add, mul, iadd, imul)
- Mapping protocol (len, getitem, setitem, delitem)
- Full numeric protocol (all binary, unary, and ternary operations)
This provides excellent coverage for the data-driven slot system.
ade6c6e to
33e8547
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/vm/src/types/slot.rs (1)
292-305: Sequence/mapping sub-slot aliases and ternary number wrappers look consistentThe new
Seq*Func/Map*Funcaliases and their use in sequence/mappingupdate_sub_slot!arms line up with the protocol types and helpers likelen_wrapper,getitem_wrapper,setitem_wrapper, andcontains_wrapper. The ternary number wrapper macros correctly model__pow__(self, other, mod=None)and the right-hand variants by only passing the third argument when it’s notNone, which matches Python’s pow semantics. The smallVec<PyObjectRef>allocation per call is acceptable for this path.Also applies to: 350-373
crates/vm/src/class.rs (1)
3-10: SLOT_DEFS‑driven operator insertion viaadd_operatorsis well‑structured
add_operators’ logic:
- Skips
__new__(which is handled by the dedicatedslot_new_wrapperbinding) and mapshash_not_implementedto__hash__ = Nonefor unhashable types, matching Python’s__hash__/unhashable semantics.- Uses
def.accessor.get_slot_func(&class.slots)to only create wrappers when the underlying slot is actually populated, and avoids clashes by skipping attributes that already exist inclass.attributes.- Resolves overlapping names like
__len__and__add__correctly by relying on which protocol (sequence vs mapping vs number) has the slot set; the first non-None accessor wins and later duplicates see an existing attribute and are skipped.Hooking this in from
extend_classbeforeinherit_slotsand method extension is a clean replacement for the old per-slot macros.If you find yourself frequently needing to reason about which protocol “owns” a name (e.g.
__add__), consider adding a brief comment near theSLOT_DEFSentries that share names to document the intended precedence (sequence vs number vs mapping).Also applies to: 13-57, 166-187
crates/vm/src/types/slot_defs.rs (2)
127-273: Inheritance and copy logic correctly walks the full MRO and all slot groupsThe helper methods on
SlotAccessor:
inherit_from_mrowalks the entiretyp.mro(which excludestypitself) without skipping the immediate base, and uses focused macros to handle main, number, sequence, and mapping slots; this fixes the earlierskip(1)issue called out in past reviews.copyslot_if_nonemirrors CPython’s slot inheritance semantics, including the specialInithandling (only copying when the base actually defines, rather than inherits,tp_init) and leavingNewto be managed byset_new.- Handle shared slots (
SetAttro/DelAttro,DescrSet/DescrDel) correctly by copying/inheriting the underlying storage slot only once.This makes both initial inheritance (
PyType::inherit_slots) and per-attribute deletion updates (update_slot::<false>) coherent across all slot families.Given how many match arms are kept in sync across
inherit_from_mro,copyslot_if_none, andget_slot_func, consider a small macro or table-driven helper to reduce the risk of divergence when adding future slots.Also applies to: 275-402, 404-552
554-747:get_slot_funcandSLOT_DEFSentries align for all slot kinds, including ternary numbers
get_slot_func:
- Correctly wraps each populated slot field in the appropriate
SlotFuncvariant, including:
- Binary number slots via
get_number_slot→SlotFunc::NumBinary.- Ternary power slots as
SlotFunc::NumTernary.- Unary, sequence, and mapping slots into their dedicated
SlotFuncvariants, with inplace sequence ops mapping back to the same underlying concat/repeat slot (matching CPython’s sharing).- Respects the decision that
__new__has special handling and thus never produces a wrapper for it.SLOT_DEFS itself:
- Covers the expected main, descriptor, sequence, mapping, and number dunders, with deliberate duplicates (
__len__,__getitem__,__setitem__,__delitem__,__add__, etc.) to support multiple protocols, which is validated by the tests (test_find_by_name).- Provides clear, CPython-like doc strings for wrapper descriptors, which
add_operatorscan attach directly.The unit tests around counts, name overlaps, size-of, and
rich_compare_opgive good basic coverage for this table.
SlotAccessor::extract_from_slot_funccurrently treats allRichCompareSlotFuncs as matching any RichCompare accessor, regardless of the embedded op. If you end up using it for finer-grained checks, you may want to also compare thePyComparisonOpto avoid accidental matches.Also applies to: 753-1178
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (9)
.cspell.dict/cpython.txtcrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/zoo.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .cspell.dict/cpython.txt
🧰 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 Rust 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/mod.rscrates/vm/src/types/zoo.rscrates/vm/src/types/slot.rscrates/vm/src/protocol/mod.rscrates/vm/src/class.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/types/slot_defs.rscrates/vm/src/builtins/type.rs
🧠 Learnings (3)
📚 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/vm/src/types/slot.rscrates/vm/src/protocol/mod.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/vm/src/types/slot.rscrates/vm/src/protocol/mod.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (4)
crates/vm/src/types/mod.rs (2)
crates/vm/src/builtins/type.rs (1)
slot(643-643)crates/vm/src/types/slot.rs (2)
slot(69-69)slot(93-93)
crates/vm/src/types/slot.rs (1)
crates/vm/src/types/slot_defs.rs (1)
find_slot_defs_by_name(1181-1183)
crates/vm/src/protocol/mod.rs (1)
crates/vm/src/protocol/number.rs (1)
handle_bytes_to_int_err(558-578)
crates/vm/src/class.rs (1)
crates/vm/src/types/slot.rs (1)
hash_not_implemented(424-426)
⏰ 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: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-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: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (9)
crates/vm/src/protocol/mod.rs (1)
13-16: ExposePyNumberTernaryFuncto match new numeric ternary slotsAdding
PyNumberTernaryFuncto the publicnumberre-exports is consistent with the newSlotFunc::NumTernaryand ternary number wrappers; no issues here.crates/vm/src/types/slot.rs (2)
549-561: MRO-based slot updating logic is sound and matches wrapper semanticsThe new
update_slot/update_one_slotflow, driven bySlotAccessorandfind_slot_defs_by_name, correctly:
- Handles multiple slot defs per dunder (e.g.
__len__for sequence/mapping,__add__for sequence/number) by updating each accessor separately.- Prefers explicit wrapper descriptors over generic wrappers via
lookup_slot_in_mro, and stops on the first non-wrapper attribute (treating it as a “real” override), which matches CPython’s “first hit in MRO” behavior.- Gives
__hash__special treatment: respects__hash__ = None(mapping it tohash_not_implemented), otherwise prefers a realSlotFunc::Hashwrapper from the MRO, and only falls back tohash_wrapperwhen none is present.- Shares slots correctly for
SetAttro/DelAttro,DescrSet/DescrDel, and the various rich-compare accessors, while usingaccessor.inherit_from_mro(self)on deletion to restore the inherited slot.Overall this centralizes slot wiring cleanly and should behave correctly for both builtin and heap types.
Also applies to: 572-611, 614-1171
1174-1214:lookup_slot_in_mro’s wrapper detection and early-return semantics are appropriateThe helper correctly:
- Recognizes wrapper descriptors via
ctx.types.wrapper_descriptor_typeand downcasts toPyWrapperto extract the innerSlotFunc.- Returns
Noneas soon as it encounters a non-wrapper attribute inselfor the first MRO class, ensuring that a “real” Python method override prevents inheriting a base wrapper.- Iterates the full MRO (which does not include
self), avoiding the earlierskip(1)pitfall highlighted in past reviews.This matches the documented contract (“Some(slot_func) if a wrapper, None otherwise”) and plays well with the new SLOT_DEFS-driven slot updates.
crates/vm/src/types/mod.rs (1)
1-8: Public exposure ofslot_defsis appropriate for the new slot systemRe-exporting
SLOT_DEFS,SlotAccessor, andSlotDeffromtypes::slot_defscleanly centralizes slot metadata for consumers likebuiltins::typeandclass, without overexposing unrelated internals. Looks good.crates/vm/src/types/zoo.rs (1)
185-193: Wrapper descriptor type wiring matches the newPyWrapperUpdating
wrapper_descriptor_typetodescriptor::PyWrapper::init_builtin_type()aligns the type zoo with the renamed/extended wrapper descriptor; it keepsctx.types.wrapper_descriptor_typeconsistent withPyWrapperusages elsewhere (e.g., inlookup_slot_in_mro). No issues.crates/vm/src/builtins/type.rs (1)
27-30: SLOT_DEFS‑driven slot initialization and inheritance look correctUsing
SLOT_DEFSto:
- In
init_slots: discover all dunder names inselfand its MRO, then callupdate_slot::<true>once per unique name, ensures heap types wire all relevant slots based on the final attribute set.- In
inherit_slots: iterateSLOT_DEFSand delegate todef.accessor.copyslot_if_none(self, base)gives a uniform, order-preserving way to inherit slots across all groups (main, number, sequence, mapping), while leavingas_buffertoinherit_readonly_slotsandtp_newtoset_new.The interaction with
set_new(skippingSlotAccessor::Newincopyslot_if_none) avoids clobberingtp_new, and the copy‑if‑none semantics remain compatible with multiple inheritance. This all aligns with the new slot_defs machinery.Also applies to: 414-445, 465-472
crates/vm/src/builtins/descriptor.rs (2)
7-17: ExtendedSlotFuncvariants and dispatcher correctly cover new slot shapesThe expanded
SlotFuncenum plus itscallimplementation:
- Enforces zero-argument contracts for slots like
__hash__,__repr__,__str__,__iter__,__next__, and__del__, returning sensibleTypeErrors when misused.- Correctly binds arguments for attribute access (
__getattribute__,__setattr__,__delattr__), rich comparison (returning either the raw object orPyComparisonValueconverted viato_pyobject), and descriptor operations (__get__,__set__,__delete__).- Uses the sequence/mapping views (
sequence_unchecked(),mapping_unchecked()) and number view (number()) with the appropriate function types for Seq*/Map*/Num* variants, including ternary number slots whereNumTernaryreceives(self, y, z)withzdefaulted toNone.Overall the dispatcher matches the type signatures wired in
types::slotandtypes::slot_defs, and should letPyWrapper/PyMethodWrapperuniformly expose all these slots as Python-callable descriptors.Please run
cargo test -p rustpython-vm --features full(or your usual subset) to exercise the new SlotFunc branches, especially for ternary numeric methods (__pow__,__rpow__,__ipow__) and seq/map__setitem__/__delitem__.Also applies to: 400-444, 446-647
650-739:PyWrapper/PyMethodWrapperrefactor preserves wrapper semanticsThe rename to
PyWrapperand the updated wiring:
- Keep the wrapper descriptor type consistent (
ctx.types.wrapper_descriptor_type) and ensurelookup_slot_in_mrocan downcast toPyWrapper.- Maintain the descriptor protocol:
PyWrapper::descr_getreturns the descriptor when accessed on the class and aPyMethodWrapperbound to the instance when accessed on an object.- Route calls for both unbound (
Type.__foo__(obj, ...)) and bound (obj.__foo__(...)) cases through the sameSlotFunc::callmachinery, with thefast_isinstance(zelf.typ)guard enforcing correct receiver types.This matches the previous
PySlotWrapperbehavior with the extended SlotFunc coverage.Also applies to: 743-803, 817-838
crates/vm/src/types/slot_defs.rs (1)
10-23: Core slot metadata (SlotDef,SlotAccessor) design is consistentThe
SlotDef+SlotAccessorsetup:
- Gives a single authoritative place mapping dunder names to concrete slot accessors and docs, including shared-name cases like
__len__and__add__.- Keeps
SlotAccessoras a flat enum suitable forrepr(u8)size constraints (validated bytest_repr_u8), which is useful if you ever want to store it compactly.This provides a solid foundation for SLOT_DEFS‑driven behavior in
class.rsandslot.rs.Also applies to: 27-123
33e8547 to
025c248
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
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/protocol/number.rs (1)
239-295: Right‑hand slots are added but not yet used by wrappers
PyNumberSlotsnow tracksright_*funcs and populates them fromPyNumberMethods, andright_binary_op/right_ternary_opexpose them correctly. But the wrapper‑descriptor path (SlotAccessor::get_slot_func+add_operators) still only reads left slots (add,floor_divide, etc.), so builtin__r*__methods end up bound to left‑hand behavior. WiringSlotAccessor::get_slot_functo selectright_*based onSlotOp::Right(see slot_defs.rs) would complete this refactor.Also applies to: 297-352, 386-421
🧹 Nitpick comments (1)
crates/vm/src/builtins/type.rs (1)
465-472: SLOT_DEFS‑basedinherit_slotslooks correct; fix stale commentThe new loop over
SLOT_DEFSwithcopyslot_if_none(self, base)is a clean replacement for per‑slot macros and matches the intended inheritance behavior. The only nit is the comment mentioninginherit_static_slotswhile the actual helper isinherit_readonly_slots, which can confuse future readers.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (10)
.cspell.dict/cpython.txtcrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/protocol/number.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/zoo.rs
✅ Files skipped from review due to trivial changes (1)
- crates/vm/src/protocol/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .cspell.dict/cpython.txt
🧰 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 Rust 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/zoo.rscrates/vm/src/types/mod.rscrates/vm/src/class.rscrates/vm/src/builtins/type.rscrates/vm/src/types/slot.rscrates/vm/src/protocol/number.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/types/slot_defs.rs
🧠 Learnings (3)
📚 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/vm/src/types/slot.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/vm/src/types/slot.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (4)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(86-95)
crates/vm/src/types/mod.rs (2)
crates/vm/src/types/slot.rs (2)
slot(69-69)slot(93-93)crates/vm/src/builtins/type.rs (1)
slot(643-643)
crates/vm/src/class.rs (2)
crates/vm/src/types/slot.rs (1)
hash_not_implemented(424-426)crates/vm/src/builtins/descriptor.rs (4)
class(57-59)class(240-242)class(662-664)class(754-756)
crates/vm/src/builtins/type.rs (3)
crates/vm/src/stdlib/ctypes/structure.rs (1)
base(111-111)crates/vm/src/stdlib/ctypes/union.rs (1)
base(114-114)crates/vm/src/stdlib/ctypes.rs (1)
base(61-61)
⏰ 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: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (4)
crates/vm/src/types/zoo.rs (1)
191-196: Rename toPyWrapper::init_builtin_typeis consistent and safeThe field type and initialization pattern match other descriptor types; switching from
PySlotWrappertoPyWrapperhere is behavior-preserving.crates/vm/src/types/mod.rs (1)
2-8: Publicly re‑exportingslot_defsis reasonableMaking
slot_defsa public module and re‑exportingSLOT_DEFS,SlotAccessor, andSlotDefprovides a clear, centralized API for slot metadata with no local downsides.crates/vm/src/class.rs (1)
13-57:add_operatorswiring viaSLOT_DEFSis solidCentralizing wrapper_descriptor creation in
add_operators(with__new__skipping andhash_not_implemented⇒__hash__ = None) is a good replacement for the old macro boilerplate. Combined with the attribute‑existence guard and post‑processing ininherit_slots, this keeps behavior consistent while making slot wiring data‑driven; any remaining left/right or delete‑semantics tweaks belong inSlotAccessor::get_slot_func/SLOT_DEFS, not here.Also applies to: 135-187
crates/vm/src/types/slot.rs (1)
292-306: SLOT_DEFS‑drivenupdate_slotimplementation looks correctThe new
update_slot/update_one_slotlogic cleanly routes each dunder name throughSlotAccessor, uses appropriate wrapper functions for number/sequence/mapping sub‑slots, and defers toinherit_from_mroon deletion. The__hash__special‑case and use oflookup_slot_in_mrofor existingPyWrappers match CPython’s behavior, andinherit_from_mronow walks the full MRO (no skipped immediate base), addressing the earlier inheritance gap.Also applies to: 335-373, 549-585, 614-708, 710-1183
025c248 to
a6dbe23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/types/slot.rs (1)
350-373: Consider avoiding Vec allocation in ternary wrappers.The ternary wrappers allocate a
Vecon every call. Sincecall_special_methodacceptsimpl IntoPyArgs, you could use a tuple or array instead for zero-allocation.🔎 Proposed optimization
macro_rules! number_ternary_op_wrapper { ($name:ident) => { |a, b, c, vm: &VirtualMachine| { - let args: Vec<PyObjectRef> = if vm.is_none(c) { - vec![b.to_owned()] + if vm.is_none(c) { + vm.call_special_method(a, identifier!(vm, $name), (b.to_owned(),)) } else { - vec![b.to_owned(), c.to_owned()] - }; - vm.call_special_method(a, identifier!(vm, $name), args) + vm.call_special_method(a, identifier!(vm, $name), (b.to_owned(), c.to_owned())) + } } }; }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (10)
.cspell.dict/cpython.txtcrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/protocol/number.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/zoo.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- .cspell.dict/cpython.txt
- crates/vm/src/types/mod.rs
- crates/vm/src/protocol/number.rs
- crates/vm/src/builtins/type.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/protocol/mod.rscrates/vm/src/class.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/types/slot.rscrates/vm/src/types/zoo.rscrates/vm/src/types/slot_defs.rs
🧠 Learnings (3)
📚 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/vm/src/protocol/mod.rscrates/vm/src/types/slot.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 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/vm/src/protocol/mod.rscrates/vm/src/types/slot.rs
📚 Learning: 2025-12-27T14:03:49.011Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (3)
crates/vm/src/protocol/mod.rs (1)
crates/vm/src/protocol/number.rs (1)
handle_bytes_to_int_err(558-578)
crates/vm/src/types/slot.rs (1)
crates/vm/src/types/slot_defs.rs (2)
find_slot_defs_by_name(869-871)inherit_from_mro(408-515)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(86-95)
⏰ 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 rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (25)
crates/vm/src/protocol/mod.rs (1)
13-16: LGTM!Trivial reordering of exports within the
PyNumbergroup. No functional changes.crates/vm/src/types/zoo.rs (1)
192-192: LGTM!Correctly updates to use the renamed
PyWrappertype, aligning with the broader refactor fromPySlotWrappertoPyWrapper.crates/vm/src/class.rs (3)
3-10: LGTM!Imports correctly updated to bring in
PyWrapperfrom the descriptor module andSLOT_DEFS,hash_not_implementedfrom the types module.
13-57: Well-structured SLOT_DEFS-driven operator addition.The
add_operatorsfunction cleanly replaces the previous macro-based approach with a data-driven loop. Key observations:
- Correctly skips
__new__which has special handling elsewhere- Properly handles
__hash__ = Nonefor unhashable types by detectinghash_not_implemented- Uses
get_slot_func_with_opto correctly wire right-hand and delete variants- Respects existing attributes to avoid overwriting user-defined methods
185-186: LGTM!Correctly integrates the new
add_operatorscall into the class extension flow.crates/vm/src/builtins/descriptor.rs (6)
7-16: LGTM!Imports correctly updated to include new slot function types (
SeqLenFunc,MapSubscriptFunc, etc.) andToPyResultfor the expandedSlotFunc::callimplementation.
394-396: LGTM!Correctly updates to use the renamed
PyWrappertype in the init function.
400-444: Comprehensive SlotFunc enum expansion.The enum now covers all slot categories (basic, attribute access, rich compare, descriptor, sequence, mapping, number) with appropriate function pointer types. The separation of
SetAttro/DelAttroandDescrSet/DescrDelcorrectly enables distinct delete semantics.
484-647: SlotFunc::call correctly dispatches to all slot variants.The implementation:
- Properly validates argument counts for no-arg methods
- Uses
PySetterValue::AssignvsPySetterValue::Deleteappropriately for setter/deleter variants- Correctly binds arguments via
args.bind(vm)?- Uses
sequence_unchecked()andmapping_unchecked()which is safe becausePyWrapper::callvalidates the type before reaching hereOne observation: the
SeqAssItemandMapAssSubscriptbranches useOptionalArgfor the value parameter, which correctly handles both__setitem__(key, value)and__delitem__(key)calling conventions.
650-739: PyWrapper (formerly PySlotWrapper) correctly implements wrapper_descriptor.The rename is consistent throughout, and the implementations for
GetDescriptor,Callable, andRepresentableare properly adapted. The type check inCallable::call(line 691) ensures the object is an instance of the expected type before delegating toSlotFunc::call.
743-785: LGTM!
PyMethodWrappercorrectly updated to holdPyRef<PyWrapper>instead of the old type. TheCallableimplementation properly delegates to the wrapped slot function.crates/vm/src/types/slot.rs (7)
17-17: LGTM!Import correctly added for the new SLOT_DEFS infrastructure.
292-305: LGTM!New type aliases for sequence and mapping sub-slot functions match the protocol method signatures and enable type-safe slot storage.
549-612: Clean refactor to SLOT_DEFS-driven slot updates.The new
update_slotandupdate_one_slotmethods provide a centralized, data-driven approach to slot management. The macrosupdate_main_slotandupdate_sub_slotcorrectly:
- Look up real implementations from the MRO via
lookup_slot_in_mro- Fall back to wrapper functions when no native implementation exists
- Delegate to
inherit_from_mrowhen deleting (ADD = false)
618-644: Correct special handling for__hash__ = None.The logic properly detects when a class explicitly sets
__hash__ = None(making it unhashable) by checking if the method object is the context's None singleton, and correctly storeshash_not_implementedin that case.
659-689: Shared slot handling is correct.
TpSetattrocorrectly handles both__setattr__and__delattr__by accepting eitherSetAttroorDelAttrovariants from the MRO lookup. Same pattern correctly applied forTpDescrSethandling__set__and__delete__.
711-727: Binary op left/right dispatch is correct.The code correctly distinguishes between
__add__and__radd__by checking the method name string, then updates the appropriate slot field (addvsright_add). This pattern is consistently applied across all binary number operations.
1186-1226: LGTM!
lookup_slot_in_mrocorrectly searches for wrapper descriptors in the type's dict and MRO chain, returningNonewhen a non-wrapper (real method) is found to trigger the wrapper fallback path.crates/vm/src/types/slot_defs.rs (7)
1-48: LGTM!
SlotOpenum cleanly distinguishes comparison operations, binary op direction (left/right), and delete operations. The helper methodsas_compare_op()andis_right()are correct.
50-167: LGTM!
SlotDefandSlotAccessorprovide a clean, CPython-compatible slot definition system. The#[repr(u8)]enum values correctly match CPython's typeslots.h IDs.
407-515: LGTM!The
inherit_from_mromethod correctly iterates the MRO (without skip, addressing the previous review comment) to find and inherit slot implementations.
785-865: Right-hand and delete variants are correctly wired.
get_slot_func_with_opproperly handles:
SlotOp::Delete→ returnsDelAttro/DescrDelvariantsSlotOp::Right→ usesright_*slot fields for binary number operationsThis addresses the previous review comment about mis-wired delete and right-hand variants.
868-874: LGTM!
find_slot_defs_by_nameprovides efficient lookup by name, correctly returning an iterator for cases like__len__that appear in multiple protocols.
876-1385: Comprehensive SLOT_DEFS array covering all Python dunder methods.The array correctly:
- Distinguishes left/right variants for binary ops via
SlotOp::Left/Right- Marks delete operations with
SlotOp::Deletefor__delattr__and__delete__- Includes both sequence (
SqLength,SqItem) and mapping (MpLength,MpSubscript) variants for shared methods like__len__and__getitem__- Sets appropriate comparison ops for rich comparison methods
1387-1443: LGTM!Tests validate CPython slot ID compatibility and
find_slot_defs_by_namebehavior. The test at line 1429 correctly expects 2 entries for__add__(NbAdd with Left and SqConcat).
520c965 to
e1c4161
Compare
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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/protocol/number.rs (1)
238-295: Populate all right_ fields incopy_fromto matchFrom<&PyNumberMethods>behavior*
PyNumberSlots::copy_fromdoes not initialize theright_*fields, butSlotAccessor::get_slot_func_with_op(..., Some(SlotOp::Right))expects them to be set for creating__radd__,__rsub__,__rmul__,__ror__,__rfloordiv__, and similar right-hand operator wrappers. SinceAsNumber::extend_slots(line 1714 intypes/slot.rs) callscopy_fromfor all builtin types using theAsNumbertrait (int, float, complex, bool, str, etc.), these types end up with allright_*slots asNone. Consequently,get_slot_func_with_opreturnsNoneand fails to create the wrapper descriptors for these methods.The
From<&PyNumberMethods>implementation correctly initializes all right_* fields by copying from their left-hand counterparts.copy_fromshould mirror this approach for consistency and correctness.Proposed fix: populate right_* fields in
copy_from@@ impl PyNumberSlots { - /// Copy from static PyNumberMethods - pub fn copy_from(&self, methods: &PyNumberMethods) { + /// Copy from static PyNumberMethods + /// + /// For native types, right_* slots default to the same implementation as + /// the corresponding left‑hand op. + pub fn copy_from(&self, methods: &PyNumberMethods) { @@ - if let Some(f) = methods.floor_divide { - self.floor_divide.store(Some(f)); - } - if let Some(f) = methods.true_divide { - self.true_divide.store(Some(f)); - } + if let Some(f) = methods.floor_divide { + self.floor_divide.store(Some(f)); + self.right_floor_divide.store(Some(f)); + } + if let Some(f) = methods.true_divide { + self.true_divide.store(Some(f)); + self.right_true_divide.store(Some(f)); + } @@ - if let Some(f) = methods.matrix_multiply { - self.matrix_multiply.store(Some(f)); - } + if let Some(f) = methods.matrix_multiply { + self.matrix_multiply.store(Some(f)); + self.right_matrix_multiply.store(Some(f)); + } if let Some(f) = methods.inplace_matrix_multiply { self.inplace_matrix_multiply.store(Some(f)); } + + // Copy all other right_* binary slots from their left‑hand peers + if let Some(f) = methods.add { + self.right_add.store(Some(f)); + } + if let Some(f) = methods.subtract { + self.right_subtract.store(Some(f)); + } + if let Some(f) = methods.multiply { + self.right_multiply.store(Some(f)); + } + if let Some(f) = methods.remainder { + self.right_remainder.store(Some(f)); + } + if let Some(f) = methods.divmod { + self.right_divmod.store(Some(f)); + } + if let Some(f) = methods.power { + self.right_power.store(Some(f)); + } + if let Some(f) = methods.lshift { + self.right_lshift.store(Some(f)); + } + if let Some(f) = methods.rshift { + self.right_rshift.store(Some(f)); + } + if let Some(f) = methods.and { + self.right_and.store(Some(f)); + } + if let Some(f) = methods.xor { + self.right_xor.store(Some(f)); + } + if let Some(f) = methods.or { + self.right_or.store(Some(f)); + } }
🧹 Nitpick comments (1)
crates/vm/src/types/slot.rs (1)
292-306: Slot update machinery is solid but doesn’t yet propagate right‑hand number slotsThe new
update_slot/update_one_slotimplementation (withupdate_main_slot/update_sub_slot) correctly wires:
- main slots (repr/str/hash/call/iter/init/new/del/getattro/descr*/richcompare) via wrappers and MRO lookup, including
__hash__ = Noneand shared slots for__setattr__/__delattr__and__set__/__delete__;- number, sequence, and mapping sub‑slots through the appropriate
SlotFuncvariants and wrapper closures;- AsNumber traits into
PyTypeSlotsviaslots.as_number.copy_from(Self::as_number()).The only notable gap is consistency with the new right‑hand number fields:
inherit_from_mroandcopyslot_if_noneinSlotAccessor(seecrates/vm/src/types/slot_defs.rs) currently copy only left‑hand number slots;right_*fields are never inherited/copied from bases.- Combined with
copy_fromnot populating right_* (see the comment inprotocol/number.rs),__r*__wrappers created via SLOT_DEFS will have no underlying slot to bind for many types, and right‑side slots won’t be inherited.Once
copy_fromis fixed to populate right_* slots, consider extendinginherit_from_mro/copyslot_if_noneto propagate the right_* counterparts as well so derived numeric types fully inherit both sides of the protocol.Also applies to: 335-371, 547-782, 784-910, 1710-1716
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.cspell.dict/cpython.txtcrates/vm/src/builtins/bool.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/number.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rs
💤 Files with no reviewable changes (2)
- crates/vm/src/builtins/complex.rs
- crates/vm/src/builtins/float.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .cspell.dict/cpython.txt
- crates/vm/src/builtins/type.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/bool.rscrates/vm/src/class.rscrates/vm/src/protocol/number.rscrates/vm/src/builtins/int.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/descriptor.rs
🧠 Learnings (3)
📚 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/vm/src/types/slot.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/vm/src/types/slot.rs
📚 Learning: 2025-12-27T14:03:49.011Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (5)
crates/vm/src/builtins/bool.rs (5)
crates/vm/src/builtins/int.rs (3)
__or__(332-334)__and__(336-338)__xor__(328-330)crates/vm/src/builtins/type.rs (1)
__or__(960-962)crates/vm/src/builtins/dict.rs (4)
__or__(298-306)__or__(1069-1073)__and__(1061-1065)__xor__(1053-1057)crates/vm/src/builtins/genericalias.rs (1)
__or__(245-247)crates/vm/src/builtins/set.rs (6)
__or__(598-608)__or__(1043-1053)__and__(612-626)__and__(1057-1071)__xor__(664-678)__xor__(1110-1124)
crates/vm/src/class.rs (1)
crates/vm/src/types/slot.rs (1)
hash_not_implemented(422-424)
crates/vm/src/builtins/int.rs (1)
crates/vm/src/builtins/bool.rs (3)
__xor__(161-173)__or__(133-145)__and__(147-159)
crates/vm/src/types/slot_defs.rs (3)
crates/vm/src/builtins/descriptor.rs (1)
init(391-396)crates/vm/src/protocol/number.rs (3)
int(568-598)float(634-664)index(601-631)crates/vm/src/protocol/sequence.rs (1)
inplace_repeat(216-232)
crates/vm/src/types/slot.rs (2)
crates/vm/src/types/slot_defs.rs (1)
find_slot_defs_by_name(914-916)crates/vm/src/class.rs (1)
extend_slots(215-215)
⏰ 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). (7)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (4)
crates/vm/src/class.rs (1)
13-57: add_operators logic and integration into extend_class look soundThe SLOT_DEFS‑driven
add_operatorshelper correctly:
- skips
__new__,- maps
hash_not_implemented→__hash__ = None,- respects existing attributes, and
- wraps only slots with an installed function.
Calling it once in
extend_classbeforeinherit_slotskeeps builtin slot wiring coherent while avoiding double‑wrapping inherited Python methods. No issues from a correctness perspective.Also applies to: 185-187
crates/vm/src/builtins/bool.rs (1)
131-174: Bitwise helpers for bool moved to slot-backed functions are OKThe crate-private
__or__,__and__, and__xor__maintain the existing semantics (bool‑vs‑bool short‑circuit, otherwise delegate toPyIntor returnNotImplemented) and are wired viaAsNumberinto slots. The refactor away from#[pymethod]looks behaviour‑preserving.Also applies to: 176-186
crates/vm/src/builtins/int.rs (1)
322-339: Int arithmetic helpers and relocated__int__are consistent with slot usageThe new crate-private
__xor__,__or__, and__and__simply reuseint_op, and moving__int__ontoPyRef<PyInt>aligns with its use in__trunc__,__floor__,real,numerator, etc. Together withPyInt::AS_NUMBER, this keeps number slots coherent without changing observable behaviour.Also applies to: 382-387, 424-437, 530-543, 566-576
crates/vm/src/builtins/descriptor.rs (1)
400-445: SlotFunc expansion and PyWrapper plumbing are coherentThe extended
SlotFuncenum plus itscallimplementation cover all new slot kinds (attribute access, rich compare, descriptor get/set/del, sequence/mapping, number left/right/ternary) with correct argument binding and return handling. The PySlotWrapper→PyWrapper rename, along with updated GetDescriptor/Callable and PyMethodWrapper wiring, is internally consistent and matches how SLOT_DEFS and update_slot consumeSlotFunc.Also applies to: 486-653, 656-745, 747-829
| /// Extract the raw function pointer from a SlotFunc if it matches this accessor's type | ||
| pub fn extract_from_slot_func(&self, slot_func: &SlotFunc) -> bool { | ||
| match self { | ||
| // Type slots | ||
| Self::TpHash => matches!(slot_func, SlotFunc::Hash(_)), | ||
| Self::TpRepr => matches!(slot_func, SlotFunc::Repr(_)), | ||
| Self::TpStr => matches!(slot_func, SlotFunc::Str(_)), | ||
| Self::TpCall => matches!(slot_func, SlotFunc::Call(_)), | ||
| Self::TpIter => matches!(slot_func, SlotFunc::Iter(_)), | ||
| Self::TpIternext => matches!(slot_func, SlotFunc::IterNext(_)), | ||
| Self::TpInit => matches!(slot_func, SlotFunc::Init(_)), | ||
| Self::TpDel => matches!(slot_func, SlotFunc::Del(_)), | ||
| Self::TpGetattro => matches!(slot_func, SlotFunc::GetAttro(_)), | ||
| Self::TpSetattro => { | ||
| matches!(slot_func, SlotFunc::SetAttro(_) | SlotFunc::DelAttro(_)) | ||
| } | ||
| Self::TpDescrGet => matches!(slot_func, SlotFunc::DescrGet(_)), | ||
| Self::TpDescrSet => { | ||
| matches!(slot_func, SlotFunc::DescrSet(_) | SlotFunc::DescrDel(_)) | ||
| } | ||
| Self::TpRichcompare => matches!(slot_func, SlotFunc::RichCompare(_, _)), | ||
|
|
||
| // Number - Power (ternary) | ||
| Self::NbPower | Self::NbInplacePower => { | ||
| matches!(slot_func, SlotFunc::NumTernary(_)) | ||
| } | ||
| // Number - Boolean | ||
| Self::NbBool => matches!(slot_func, SlotFunc::NumBoolean(_)), | ||
| // Number - Unary | ||
| Self::NbNegative | ||
| | Self::NbPositive | ||
| | Self::NbAbsolute | ||
| | Self::NbInvert | ||
| | Self::NbInt | ||
| | Self::NbFloat | ||
| | Self::NbIndex => matches!(slot_func, SlotFunc::NumUnary(_)), | ||
| // Number - Binary | ||
| Self::NbAdd | ||
| | Self::NbSubtract | ||
| | Self::NbMultiply | ||
| | Self::NbRemainder | ||
| | Self::NbDivmod | ||
| | Self::NbLshift | ||
| | Self::NbRshift | ||
| | Self::NbAnd | ||
| | Self::NbXor | ||
| | Self::NbOr | ||
| | Self::NbFloorDivide | ||
| | Self::NbTrueDivide | ||
| | Self::NbMatrixMultiply | ||
| | Self::NbInplaceAdd | ||
| | Self::NbInplaceSubtract | ||
| | Self::NbInplaceMultiply | ||
| | Self::NbInplaceRemainder | ||
| | Self::NbInplaceLshift | ||
| | Self::NbInplaceRshift | ||
| | Self::NbInplaceAnd | ||
| | Self::NbInplaceXor | ||
| | Self::NbInplaceOr | ||
| | Self::NbInplaceFloorDivide | ||
| | Self::NbInplaceTrueDivide | ||
| | Self::NbInplaceMatrixMultiply => matches!(slot_func, SlotFunc::NumBinary(_)), | ||
|
|
||
| // Sequence | ||
| Self::SqLength => matches!(slot_func, SlotFunc::SeqLength(_)), | ||
| Self::SqConcat | Self::SqInplaceConcat => matches!(slot_func, SlotFunc::SeqConcat(_)), | ||
| Self::SqRepeat | Self::SqInplaceRepeat => matches!(slot_func, SlotFunc::SeqRepeat(_)), | ||
| Self::SqItem => matches!(slot_func, SlotFunc::SeqItem(_)), | ||
| Self::SqAssItem => matches!(slot_func, SlotFunc::SeqAssItem(_)), | ||
| Self::SqContains => matches!(slot_func, SlotFunc::SeqContains(_)), | ||
|
|
||
| // Mapping | ||
| Self::MpLength => matches!(slot_func, SlotFunc::MapLength(_)), | ||
| Self::MpSubscript => matches!(slot_func, SlotFunc::MapSubscript(_)), | ||
| Self::MpAssSubscript => matches!(slot_func, SlotFunc::MapAssSubscript(_)), | ||
|
|
||
| // New and reserved slots | ||
| Self::TpNew => false, | ||
| _ => false, // Reserved slots | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual file and locate extract_from_slot_func with its documentation
cd crates/vm/src/types && cat -n slot_defs.rs | sed -n '310,410p'Repository: RustPython/RustPython
Length of output: 5180
🏁 Script executed:
# Run the suggested verification commands
rg -n "get_slot_func_with_op" crates/vm -S -C2Repository: RustPython/RustPython
Length of output: 724
🏁 Script executed:
# Check for get_slot_func calls (without _with_op)
rg -n "get_slot_func\(" crates/vm -S -C3Repository: RustPython/RustPython
Length of output: 1026
🏁 Script executed:
# Check for all direct get_slot_func calls outside of slot_defs.rs itself
rg -n "\.get_slot_func\(" --type rs -C2 | grep -v slot_defs.rsRepository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Also check the exact implementation context of get_slot_func_with_op to understand the fallback
cat -n crates/vm/src/types/slot_defs.rs | sed -n '786,910p'Repository: RustPython/RustPython
Length of output: 5478
🏁 Script executed:
# Search for all get_slot_func calls across the codebase
rg "\.get_slot_func\(" crates/vm -C2Repository: RustPython/RustPython
Length of output: 354
🏁 Script executed:
# Also check if there are any usages in other crate directories beyond crates/vm
rg "\.get_slot_func\(" --include="*.rs" -C2Repository: RustPython/RustPython
Length of output: 506
🏁 Script executed:
# Check if extract_from_slot_func is actually used anywhere in the codebase
rg "extract_from_slot_func" --include-zeroRepository: RustPython/RustPython
Length of output: 170
🏁 Script executed:
# Check the full context around where extract_from_slot_func might have been used or planned
rg -B5 -A10 "extract_from_slot_func" crates/vm/src/types/slot_defs.rsRepository: RustPython/RustPython
Length of output: 880
🏁 Script executed:
# Search for extract_from_slot_func in test files as well
rg "extract_from_slot_func" . -C3Repository: RustPython/RustPython
Length of output: 596
🏁 Script executed:
# Also verify that the only get_slot_func usage is the internal fallback
rg "get_slot_func" crates/vm/src/types/slot_defs.rs -B2 -A2 | head -40Repository: RustPython/RustPython
Length of output: 598
extract_from_slot_func docstring is misleading: the method checks type compatibility and returns bool, it does not extract a function pointer
The docstring at line 325 claims the method "Extract[s] the raw function pointer from a SlotFunc", but the implementation only checks type compatibility via matches!() and returns a bool. This naming mismatch could confuse future maintainers. Either rename the method to reflect its actual purpose (e.g., is_compatible_with_slot_func, matches_slot_func) or update the docstring to clarify it performs type compatibility checking only.
🤖 Prompt for AI Agents
In crates/vm/src/types/slot_defs.rs around lines 325 to 405, the method
extract_from_slot_func is misnamed and its docstring is misleading: it does not
extract a raw function pointer but only checks type compatibility and returns a
bool. Rename the method to something like is_compatible_with_slot_func or
matches_slot_func (and update all call sites), or alternatively update the
docstring to state clearly that the function only checks whether the SlotFunc
variant matches this accessor and returns a boolean; keep the implementation
using matches!() as-is.
edd2713 to
112b79a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (14)
.cspell.dict/cpython.txtcrates/vm/src/builtins/bool.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/protocol/number.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/zoo.rs
✅ Files skipped from review due to trivial changes (1)
- .cspell.dict/cpython.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/types/zoo.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/mod.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/builtins/bool.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/protocol/mod.rscrates/vm/src/builtins/int.rscrates/vm/src/protocol/number.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/float.rs
🧠 Learnings (3)
📚 Learning: 2025-12-27T14:03:49.011Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/class.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/complex.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/vm/src/protocol/mod.rscrates/vm/src/types/slot.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 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/vm/src/protocol/mod.rscrates/vm/src/types/slot.rs
🧬 Code graph analysis (8)
crates/vm/src/types/mod.rs (2)
crates/vm/src/types/slot.rs (2)
slot(69-69)slot(93-93)crates/vm/src/builtins/type.rs (1)
slot(643-643)
crates/vm/src/class.rs (1)
crates/vm/src/types/slot.rs (1)
hash_not_implemented(422-424)
crates/vm/src/builtins/bool.rs (6)
crates/vm/src/builtins/int.rs (3)
__or__(331-333)__and__(335-337)__xor__(327-329)crates/vm/src/builtins/type.rs (1)
__or__(960-962)crates/vm/src/builtins/mappingproxy.rs (1)
__or__(201-203)crates/vm/src/builtins/dict.rs (4)
__or__(298-306)__or__(1069-1073)__and__(1061-1065)__xor__(1053-1057)crates/vm/src/builtins/genericalias.rs (1)
__or__(245-247)crates/vm/src/builtins/set.rs (6)
__or__(598-608)__or__(1043-1053)__and__(612-626)__and__(1057-1071)__xor__(664-678)__xor__(1110-1124)
crates/vm/src/builtins/descriptor.rs (1)
crates/vm/src/types/slot.rs (1)
obj(1634-1635)
crates/vm/src/protocol/mod.rs (1)
crates/vm/src/protocol/number.rs (1)
handle_bytes_to_int_err(667-687)
crates/vm/src/builtins/int.rs (1)
crates/vm/src/builtins/bool.rs (3)
__xor__(161-173)__or__(133-145)__and__(147-159)
crates/vm/src/protocol/number.rs (2)
crates/vm/src/protocol/mapping.rs (1)
copy_from(37-47)crates/vm/src/protocol/sequence.rs (1)
copy_from(44-69)
crates/vm/src/types/slot.rs (2)
crates/vm/src/types/slot_defs.rs (1)
find_slot_defs_by_name(918-920)crates/vm/src/class.rs (1)
extend_slots(215-215)
⏰ 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). (10)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (20)
crates/vm/src/builtins/complex.rs (2)
8-8: LGTM - Clean import updateThe import has been focused to only include
PyComparisonValue, aligning with the removal of arithmetic methods that are now handled through theAsNumbertrait.
344-378: LGTM - Arithmetic operations properly centralizedThe
AsNumberimplementation correctly provides all arithmetic operations (add, subtract, multiply, power, negative, positive, absolute, boolean, true_divide) through the slot system. This is consistent with the PR's goal of moving to a SLOT_DEFS-driven architecture.crates/vm/src/builtins/float.rs (1)
389-432: LGTM - Consistent refactor patternThe
AsNumberimplementation forPyFloatcorrectly provides all arithmetic operations through the slot system, consistent with the broader refactor to centralize numeric operations.crates/vm/src/protocol/mod.rs (1)
13-16: LGTM - Export ordering updateThe reordering of
PyNumberTernaryFuncbeforePyNumberTernaryOpmaintains consistency with the updated slot/wrapper infrastructure. No functional changes.crates/vm/src/types/mod.rs (1)
2-7: LGTM - New slot_defs module properly exposedThe new
slot_defsmodule and its re-exported items (SLOT_DEFS,SlotAccessor,SlotDef) establish the public API for the data-driven slot system, which is central to this refactor.crates/vm/src/builtins/type.rs (1)
467-471: LGTM - Clean migration to SLOT_DEFS-driven slot inheritanceThe replacement of per-slot macros with a generic loop over
SLOT_DEFSis a significant simplification. The comment correctly documents thatas_bufferis handled separately ininherit_readonly_slots.crates/vm/src/class.rs (2)
18-57: LGTM - Well-structured SLOT_DEFS iteration with appropriate special casesThe
add_operatorsfunction correctly:
- Iterates through
SLOT_DEFSto create wrapper descriptors- Skips
__new__which has special handling inextend_class(lines 169-183)- Sets
__hash__toNonewhenhash_not_implementedis set, matching CPython behavior- Avoids overwriting existing attributes in the type dict
- Creates
PyWrapperdescriptors with proper documentationThe accessor-based slot function retrieval (
def.accessor.get_slot_func_with_op) is the right approach for the new architecture.
186-186: LGTM - Proper integration of SLOT_DEFS-driven operator wrappingThe call to
add_operatorscorrectly replaces the previous macro-driven wrapper initialization, centralizing the logic for creating slot wrapper descriptors.crates/vm/src/builtins/bool.rs (1)
131-174: LGTM - Correct refactor to crate-internal methods with slot exposureThe bitwise operations (
__or__,__and__,__xor__) are correctly changed topub(crate)functions that are exposed via theAsNumberslots (lines 179-181) and wrapped as descriptors by the SLOT_DEFS system. The comment on line 132 helpfully clarifies this pattern.crates/vm/src/builtins/int.rs (3)
326-337: LGTM - Bitwise operations correctly refactored for slot exposureThe bitwise operations (
__xor__,__or__,__and__) are appropriately changed topub(crate)and exposed via theAS_NUMBERslot table (lines 642-644). The comment clarifies this is called from bothbool.rsand exposed as wrapper descriptors.
567-574: LGTM -__int__properly exposed via slot systemThe
__int__method is correctly moved to apub(crate)function in thePyRef<PyInt>impl and wired through theAS_NUMBER.intslot (line 645). The comments on lines 567-568 helpfully document the multiple call sites and exposure mechanism.
618-654: LGTM - Comprehensive AS_NUMBER slot implementationThe
AS_NUMBERconstant correctly defines all numeric operations forPyInt, including:
- Basic arithmetic (add, subtract, multiply, remainder, divmod, power)
- Unary operations (negative, positive, absolute, boolean, invert)
- Bit operations (lshift, rshift, and, xor, or)
- Conversions (int, float, index)
- Division operations (floor_divide, true_divide)
This centralizes all numeric behavior in the slot system, consistent with the PR's architecture.
crates/vm/src/builtins/descriptor.rs (2)
400-662: LGTM! SlotFunc expansion and dispatch logic are well-implemented.The extensive SlotFunc enum now covers all major Python protocols (attributes, descriptors, rich comparison, sequences, mappings, and numbers) with proper variants for right-hand operations, delete operations, and sub-slots. The
callmethod correctly handles each variant's calling convention:
- Right-hand binary/ternary operations properly swap arguments (lines 646, 658)
- Delete operations use
PySetterValue::Delete(lines 554, 582)- All argument binding, type conversions, and return value handling are appropriate for each protocol
This provides a solid foundation for the data-driven slot system.
394-394: Rename from PySlotWrapper to PyWrapper is clean and consistent.The initialization, type references, and all related code have been updated to use
PyWrapperthroughout, maintaining consistency with the new naming.crates/vm/src/protocol/number.rs (2)
355-462: LGTM!copy_frommethod correctly implements slot copying pattern.The implementation follows the established pattern from
PyMappingMethodsandPySequenceMethods(as shown in relevant snippets), usingif let Some(f)guards to conditionally copy each slot. This enables dynamic slot propagation fromPyNumberMethodsintoPyNumberSlots, supporting the broader shift to SLOT_DEFS-driven slot management.
260-270: Right-hand slot additions are well-structured and documented.The new
right_*fields provide dedicated storage for reverse binary operations (__radd__,__rsub__, etc.), and the comment correctly notes these are internal. The initialization inFrom<&PyNumberMethods>(lines 319-349) appropriately reuses the same function pointers for native types, aligning with Python's semantics where builtin types typically implement commutative operations with a single underlying function.crates/vm/src/types/slot.rs (2)
350-371: Ternary operation wrapper macros correctly handle optional third argument.The
number_ternary_op_wrapper!andnumber_ternary_right_op_wrapper!macros properly checkvm.is_none(c)to determine whether to pass the third argument, matching Python'spow(x, y)vspow(x, y, z)semantics. The right variant correctly swaps the first two arguments for reverse operations.
1187-1224:lookup_slot_in_mrocorrectly searches for wrapper descriptors in the MRO.The helper first checks the class's own attributes, then walks the MRO, extracting the slot function if a
PyWrapperis found with a matchingSlotFuncvariant. ReturningNonewhen a non-wrapper method is found ensures user-defined methods take precedence over inherited slot functions, which is the correct Python behavior.crates/vm/src/types/slot_defs.rs (2)
1-920: LGTM! Comprehensive slot system implementation with CPython compatibility.This new module provides a data-driven slot system that:
- Defines
SlotOp,SlotDef, andSlotAccessorwith values matching CPython's typeslots.h (lines 15-167)- Implements inheritance (
inherit_from_mro,copyslot_if_none) correctly, with the comment at line 409 accurately noting thattyp.mrodoes NOT includetypitself- Provides
get_slot_func_with_op(lines 785-914) that properly handlesSlotOp::Deletefor delete operations (lines 792-798),SlotOp::Rightfor reverse binary operations (lines 800-902), and comparison ops (lines 904-911)- Exposes
SLOT_DEFSandfind_slot_defs_by_namefor consumption by other modulesThe implementation addresses all issues raised in past reviews and provides a solid foundation for the centralized slot dispatch system.
922-1434: SLOT_DEFS array is comprehensive and well-documented.The static array covers all Python special methods across:
- Type slots (
__init__,__new__,__del__,__repr__,__str__,__hash__,__call__,__iter__,__next__)- Attribute access (
__getattribute__,__setattr__,__delattr__)- Rich comparisons (all six comparison operators)
- Descriptor protocol (
__get__,__set__,__delete__)- Sequence/mapping protocols (
__len__,__getitem__,__setitem__,__delitem__,__contains__)- Number protocol (binary ops with left/right variants, unary ops, in-place ops)
Each entry correctly specifies the accessor, operation type (where applicable), and documentation string. The use of
SlotOp::Left/Rightfor binary operations andSlotOp::Deletefor delete operations enables proper dispatch viaget_slot_func_with_op.
112b79a to
e7de2be
Compare
330139a to
2804655
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/vm/src/types/slot_defs.rs (1)
324-404: Docstring mismatch:extract_from_slot_funcreturnsbool, not a function pointer.The docstring at line 324 says "Extract the raw function pointer from a SlotFunc" but the method only checks type compatibility via
matches!()and returns abool. Consider renaming tomatches_slot_funcor updating the docstring to clarify it performs type compatibility checking.🔎 Suggested fix
- /// Extract the raw function pointer from a SlotFunc if it matches this accessor's type + /// Check if a SlotFunc variant is compatible with this accessor's expected type pub fn extract_from_slot_func(&self, slot_func: &SlotFunc) -> bool {Or rename the method:
- pub fn extract_from_slot_func(&self, slot_func: &SlotFunc) -> bool { + pub fn matches_slot_func(&self, slot_func: &SlotFunc) -> bool {
🧹 Nitpick comments (2)
crates/vm/src/builtins/complex.rs (1)
366-371: Overflow detection logic is correct.The check properly detects when
norm()overflows for finite inputs. Minor style nit:.to_owned()on line 369 may be unnecessary for consistency with line 127 which passes a string literal directly tonew_overflow_error.Optional: remove unnecessary `.to_owned()`
if result.is_infinite() && value.re.is_finite() && value.im.is_finite() { - return Err(vm.new_overflow_error("absolute value too large".to_owned())); + return Err(vm.new_overflow_error("absolute value too large")); }crates/vm/src/types/slot.rs (1)
744-760: Verify right-hand operation SlotFunc matching in update_sub_slot.Looking at lines 744-760 for
__radd__, theupdate_sub_slot!macro usesNumBinaryas the match pattern. However, per past review comments,add_operatorscreates wrappers withSlotFunc::NumBinaryRightfor right-hand operations.The macro at line 595-601 matches against
SlotFunc::$variant(f), so when looking up__radd__in the MRO, it will try to matchSlotFunc::NumBinarybut the inherited wrapper containsSlotFunc::NumBinaryRight. This mismatch meanslookup_slot_in_mrowon't find inherited right-hand wrappers.While the fallback wrapper will work correctly, this prevents optimization from reusing parent class slot functions.
🔎 Suggested fix direction
For right-hand operations, use the correct variant in the match:
SlotAccessor::NbAdd => { if name.as_str() == "__radd__" { update_sub_slot!( as_number, right_add, number_binary_right_op_wrapper!(__radd__), - NumBinary + NumBinaryRight )Apply similar changes to all
__r*__operations (__rsub__,__rmul__, etc.) and__rpow__(useNumTernaryRight).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**Lib/test/test_sqlite3/test_factory.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testhelpers.pyis excluded by!Lib/**
📒 Files selected for processing (14)
.cspell.dict/cpython.txtcrates/vm/src/builtins/bool.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/protocol/mod.rscrates/vm/src/protocol/number.rscrates/vm/src/types/mod.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/types/zoo.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/vm/src/protocol/mod.rs
- crates/vm/src/builtins/type.rs
- crates/vm/src/types/zoo.rs
- crates/vm/src/types/mod.rs
- crates/vm/src/protocol/number.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/bool.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/class.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/float.rscrates/vm/src/types/slot_defs.rs
🧠 Learnings (3)
📚 Learning: 2025-12-27T14:03:49.011Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/descriptor.rscrates/vm/src/class.rscrates/vm/src/builtins/complex.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 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/vm/src/types/slot.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/vm/src/types/slot.rs
🧬 Code graph analysis (4)
crates/vm/src/builtins/bool.rs (6)
crates/vm/src/builtins/int.rs (3)
__or__(332-334)__and__(336-338)__xor__(328-330)crates/vm/src/builtins/type.rs (1)
__or__(965-967)crates/vm/src/builtins/dict.rs (4)
__or__(298-306)__or__(1069-1073)__and__(1061-1065)__xor__(1053-1057)crates/vm/src/builtins/mappingproxy.rs (1)
__or__(201-203)crates/vm/src/builtins/genericalias.rs (1)
__or__(245-247)crates/vm/src/builtins/set.rs (6)
__or__(598-608)__or__(1043-1053)__and__(612-626)__and__(1057-1071)__xor__(664-678)__xor__(1110-1124)
crates/vm/src/types/slot.rs (2)
crates/vm/src/types/slot_defs.rs (1)
find_slot_defs_by_name(917-919)crates/vm/src/class.rs (1)
extend_slots(216-216)
crates/vm/src/builtins/int.rs (1)
crates/vm/src/builtins/bool.rs (3)
__xor__(161-173)__or__(133-145)__and__(147-159)
crates/vm/src/types/slot_defs.rs (1)
crates/vm/src/builtins/descriptor.rs (6)
hash(832-836)call(105-107)call(490-661)call(701-715)call(776-778)new(43-53)
⏰ 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 rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
🔇 Additional comments (16)
crates/vm/src/builtins/complex.rs (1)
8-8: LGTM!Import cleanup aligns with the refactoring that moves arithmetic operations to the
AsNumbertrait implementation.crates/vm/src/builtins/float.rs (3)
11-11: LGTM: Import inlining looks correct.The inlined
PyArithmeticValue::*import is properly used in theComparableimplementation (lines 376, 378).
389-432: No issues found. Thenumber_downcastandnumber_downcast_exactmethods are defined as part of theAsNumbertrait incrates/vm/src/types/slot.rs(lines 1758 and 1763) and are available to all types implementingAsNumber, includingPyFloat.
215-339: No breaking changes from removed operator methods.Verification confirms that the removal of operator methods (
__add__,__sub__,__mul__,__truediv__,__floordiv__,__mod__,__pow__,__divmod__,__abs__,__bool__,__int__,__float__, and reverse variants) fromPyFloatdoes not break any Rust code. While similar method names exist on other types (PyDeque,PyInt,PySet,PyHkey), no direct calls to removedPyFloatmethods were found in the codebase..cspell.dict/cpython.txt (1)
4-4: LGTM!The dictionary additions (
attro,releasebuffer) align with the new slot accessor terminology introduced in this PR (e.g.,TpGetattro,TpSetattro,BfReleaseBuffer).Also applies to: 54-54
crates/vm/src/builtins/bool.rs (1)
131-174: LGTM!The operator methods are correctly refactored to
pub(crate)visibility for internal slot dispatch. The logic properly handles:
- Fast-path for bool-bool operands returning Python bools
- Delegation to
PyIntoperations for integer operandsNotImplementedfallback for unsupported typesThis aligns well with the SLOT_DEFS-driven wrapper approach.
crates/vm/src/builtins/int.rs (2)
327-338: LGTM!The bitwise operators are appropriately changed to
pub(crate)visibility, aligning with the wrapper_descriptor exposure pattern. These are correctly called fromPyBool's implementations (as shown in the relevant code snippets).
568-576: LGTM!Moving
__int__toPyRef<PyInt>withpub(crate)visibility is appropriate. The method is correctly used by__trunc__,__floor__,__ceil__,conjugate, and thereal/numeratorproperties, and exposed viaAS_NUMBER.intslot.crates/vm/src/class.rs (1)
18-57: LGTM!The
add_operatorsfunction is well-structured:
- Correctly skips
__new__which has special handling viaslot_new_wrapper- Properly handles
__hash__ = Noneby checking forhash_not_implementedand setting the attribute toNone- Uses
get_slot_func_with_opto correctly resolve SlotFunc variants (including right-hand and delete operations)- Avoids overwriting existing attributes in the type dict
The function pointer comparison at line 31 (
h as usize == hash_not_implemented as usize) is an acceptable pattern for comparing function pointers in Rust.crates/vm/src/builtins/descriptor.rs (2)
668-716: LGTM!
PyWrapperis correctly structured with:
- Static type and name references
SlotFuncwrapped callable- Optional documentation
The
GetDescriptorandCallableimplementations properly:
- Return the descriptor itself when accessed from class (
obj: None)- Create bound
PyMethodWrapperwhen accessed from instance- Validate type compatibility in
call()before delegating toSlotFunc::call
575-584: Argument binding for descriptor__set__and__delete__is correct.The
SlotFunc::DescrSetvariant properly binds two required arguments(instance, value)and passesPySetterValue::Assign(value)to the underlying function. This correctly implements Python's__set__(self, instance, value)protocol. The relatedSlotFunc::DescrDelbinds only(instance)and passesPySetterValue::Delete, correctly implementing__delete__(self, instance). The design of using the sameDescrSetFunctype with differentPySetterValuevariants to distinguish between set and delete operations is sound.crates/vm/src/types/slot.rs (2)
690-741: Rich comparison slot update logic is well-designed.The implementation correctly:
- Checks if any Python-defined comparison method exists in self or MRO
- Filters out
wrapper_descriptortypes to detect actual Python overrides- Falls back to inherited
slot_richcompareif available- Uses
richcompare_wrapperas the defaultThis ensures that Python-defined comparison methods take precedence over inherited C-level slot functions.
1219-1259: LGTM!The
lookup_slot_in_mroimplementation correctly:
- Checks self's dict first, then iterates the MRO
- Only extracts slot functions from
wrapper_descriptortypes- Returns
Nonewhen a Python method is found (allowing wrapper fallback)- Returns the extracted function when a compatible wrapper is found
This enables slot function reuse from parent classes while correctly falling back to Python method wrappers when needed.
crates/vm/src/types/slot_defs.rs (3)
784-913: LGTM!
get_slot_func_with_opcorrectly handles:
- Delete operations (lines 791-797): Returns
DelAttro/DescrDelvariants forTpSetattro/TpDescrSetwhenop == Some(SlotOp::Delete)- Right-hand operations (lines 799-901): Returns
NumBinaryRight/NumTernaryRightusingright_*slot fields- Comparison operations (lines 903-910): Uses
op.as_compare_op()to get the correctPyComparisonOp- Fallback (line 912): Delegates to
get_slot_funcfor left/other operationsThis addresses the issues raised in past reviews about right-hand and delete wrappers.
924-1433: LGTM!The
SLOT_DEFSarray is comprehensive and well-structured:
- Covers all type slots (
__init__,__repr__,__hash__, etc.)- Correctly duplicates entries for shared slots (
__len__for bothSqLengthandMpLength)- Uses appropriate
SlotOpfor comparison operators and binary left/right variants- Includes descriptor protocol (
__get__,__set__,__delete__)- Covers sequence and mapping protocols with proper accessor assignments
The documentation strings match CPython's conventions.
1435-1490: LGTM!Tests provide good coverage:
- Validates
SlotAccessorenum values match CPython's slot IDs- Verifies
SLOT_DEFScount is within expected bounds- Tests
find_slot_defs_by_namefor duplicate entries (__len__,__add__)- Validates
SlotOpcomparison and right-hand operations
2804655 to
112aff6
Compare
Summary by CodeRabbit
Chores
Refactor
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.