Introduce slot wrapper to __init__ #4884
Conversation
205273b to
8952836
Compare
8952836 to
0360767
Compare
0360767 to
bef6014
Compare
bef6014 to
3cc6d97
Compare
3cc6d97 to
bf67ff4
Compare
bf67ff4 to
ff7c16a
Compare
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds wrapper descriptor types (PySlotWrapper and PyMethodWrapper), exposes them in the type zoo, wires an init slot wrapper into class extension, hides the Initializer::slot_init Python binding, refines init return errors, and adds a no-op Initializer for PyWeak; also adjusts BufferedMixin init exposure. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant Class as Class Object
participant SW as PySlotWrapper
participant MW as PyMethodWrapper
participant Impl as Underlying Slot Impl
User->>Class: access __init__
Class->>SW: class-level descriptor lookup (PySlotWrapper)
SW->>MW: __get__(instance) -> return PyMethodWrapper bound to instance
User->>MW: call bound method ()
MW->>SW: delegate call (enforce type, unwrap, forward)
SW->>Impl: invoke underlying slot function (Init/New)
Impl-->>User: result / return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (7)
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 |
d7eb525 to
3a803e3
Compare
3a803e3 to
6ffb2d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/derive-impl/src/pyclass.rs (1)
911-915: Remove commented-out code or add a tracking TODO.Commented-out error-handling code is dead code that reduces readability. If the
__init__restriction is permanently removed (as the PR intent suggests), delete these lines. If this is temporary or under consideration for reinstatement, add a// TODO:or// FIXME:comment explaining the context.🔎 Suggested fix: remove dead code
if py_name == "__init__" { - // return Err(syn::Error::new( - // ident.span(), - // "#[pymethod] cannot define '__init__'. Use #[pyclass(with(Initializer))] instead.", - // )); + // __init__ is now exposed via PySlotWrapper descriptor in class.rs }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_weakref.pyis excluded by!Lib/**
📒 Files selected for processing (8)
crates/derive-impl/src/pyclass.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/weakref.rscrates/vm/src/class.rscrates/vm/src/stdlib/io.rscrates/vm/src/types/slot.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/derive-impl/src/pyclass.rscrates/vm/src/types/zoo.rscrates/vm/src/class.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/weakref.rscrates/vm/src/stdlib/io.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/descriptor.rs
🧠 Learnings (2)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/derive-impl/src/pyclass.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/class.rscrates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (3)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(40-49)
crates/vm/src/builtins/object.rs (1)
crates/vm/src/types/slot.rs (1)
slot_init(949-974)
crates/vm/src/builtins/weakref.rs (3)
crates/vm/src/types/slot.rs (1)
init(976-976)crates/vm/src/stdlib/ctypes/pointer.rs (2)
init(22-65)init(251-259)crates/vm/src/stdlib/ctypes/structure.rs (1)
init(69-128)
🔇 Additional comments (16)
crates/vm/src/types/zoo.rs (2)
97-98: LGTM!The new
wrapper_descriptor_typeandmethod_wrapper_typefields are correctly declared with the standard&'static Py<PyType>pattern, consistent with other descriptor types in the struct.
192-193: LGTM!Initialization follows the established pattern using
init_builtin_type()and is correctly placed alongside other descriptor type initializations. Thedescriptor::init(context)call at line 244 will handle extending these types.crates/vm/src/class.rs (2)
4-5: LGTM!The imports for
PyPayloadanddescriptor::PySlotWrapperare correctly added to support the new__init__wrapper functionality.
139-151: PySlotWrapper struct definition matches code usage perfectly.The
__init__slot wrapper logic correctly mirrors the__new__pattern:
- Checks for slot existence before creating wrapper
- Avoids overwriting existing
__init__definitions in the class dict- Uses appropriate CPython-style docstring
- Properly creates and attaches the
PySlotWrapperdescriptor with all required fields (typ,name,wrapped,doc)crates/vm/src/types/slot.rs (1)
468-471: LGTM!The improved error message now includes the actual returned type name with a
.200width limiter, which matches CPython's behavior and provides more actionable diagnostic information to users.crates/vm/src/builtins/object.rs (2)
498-501: LGTM!The
__init__pymethod correctly delegates to<Self as Initializer>::slot_init, providing the public Python-facing entry point for initialization that routes through the proper excess-args validation logic.
121-162: Verify the excess-args validation logic matches CPython'sobject_init.The implementation correctly handles CPython's nuanced behavior:
- Compares init/new slots via function pointer addresses for built-in types
- For heap types, traverses MRO to detect
__new__definitions beforeobject- Allows excess args only when both
__init__and__new__are overriddenThe MRO traversal at lines 140-143 uses
take_whileto stop beforeobject_type, which is safe given Python's type system guarantees. The slot-level comparison approach inslot_initcorrectly complements the Python attribute-level comparison inslot_new(called during construction). The error message format matches CPython's conventions.crates/vm/src/stdlib/io.rs (1)
1464-1473: LGTM!The change to directly call
zelf.init(raw, BufferSize { buffer_size }, vm)instead of routing through a public__init__is appropriate. This simplifies the initialization path while maintaining the same behavior, aligning with the PR's goal of restructuring initialization to use slot-based wrappers.crates/vm/src/builtins/weakref.rs (2)
54-61: LGTM!The no-op
Initializerimplementation correctly mirrors CPython'sweakref_tp_initbehavior where__init__accepts arguments but performs no action since all initialization is handled inslot_new. This is necessary for the wrapper-based__init__exposure mechanism while maintaining compatibility with Python's weakref semantics.
63-73: LGTM!The
#[pyclass]attribute correctly includesInitializerin the trait list, enabling the new slot wrapper mechanism to expose__init__on theweakreftype.crates/vm/src/builtins/descriptor.rs (6)
388-390: LGTM!The
initfunction is correctly extended to register the newPySlotWrapperandPyMethodWrappertypes, ensuring they are properly initialized in the type system.
411-448: LGTM!The
GetDescriptorandCallableimplementations forPySlotWrapperare well-structured:
- The descriptor correctly returns itself when accessed from a class or on
None, and creates a boundPyMethodWrapperwhen accessed from an instance.- The callable implementation properly validates the object type before delegating to the wrapped slot function.
- Returning
Noneafter calling the wrapped function is correct for__init__semantics.
564-570: LGTM!The
Hashableimplementation correctly combines the object's hash with the wrapper's identity hash using XOR. This follows the standard pattern for hashing bound method objects where both the method and the bound instance contribute to the hash.
572-584: LGTM!The
Comparableimplementation correctly useseq_onlyto restrict comparisons to equality only, and properly checks both wrapper identity (is) and object value equality (bool_eq). This matches CPython's method-wrapper comparison semantics.
534-549: LGTM!The
__reduce__implementation correctly enables pickling by returninggetattrwith the bound object and method name, allowing the method wrapper to be reconstructed on unpickling.
491-497: Clarify the GC traversal design forobjfield.The
#[pytraverse(skip)]onobjappears intentional (thewrapperfield is traversed), but the design rationale should be confirmed. IfPyMethodWrappercan be stored long-term in user code whereobjforms cyclic references, this skip could prevent proper garbage collection. Verify this matches the intended lifecycle of method-wrapper objects—whether they're truly ephemeral or can persist as stored references.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_weakref.pyis excluded by!Lib/**
📒 Files selected for processing (7)
crates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/weakref.rscrates/vm/src/class.rscrates/vm/src/stdlib/io.rscrates/vm/src/types/slot.rscrates/vm/src/types/zoo.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/class.rs
- 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 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/builtins/weakref.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/stdlib/io.rs
🧠 Learnings (1)
📚 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/builtins/object.rs (2)
crates/vm/src/types/slot.rs (1)
slot_init(949-974)crates/vm/src/builtins/type.rs (5)
zelf(1569-1569)zelf(1571-1571)typ(1322-1330)__new__(548-558)name(579-584)
crates/vm/src/builtins/descriptor.rs (2)
crates/vm/src/types/slot.rs (5)
hash(1107-1107)descr_get(1029-1034)obj(1345-1346)repr_str(1133-1133)cmp(1155-1160)crates/vm/src/class.rs (1)
extend_class(80-158)
crates/vm/src/stdlib/io.rs (4)
crates/vm/src/builtins/descriptor.rs (1)
zelf(422-422)crates/vm/src/stdlib/ctypes/base.rs (2)
zelf(1332-1333)zelf(1613-1614)crates/vm/src/exception_group.rs (1)
zelf(212-213)crates/vm/src/builtins/type.rs (2)
zelf(1569-1569)zelf(1571-15 8000 71)
🪛 GitHub Actions: CI
crates/vm/src/builtins/object.rs
[error] 499-499: #[pymethod] cannot define 'init'. Use #[pyclass(with(Initializer))] instead.
🔇 Additional comments (9)
crates/vm/src/stdlib/io.rs (1)
1465-1473: BufferedMixin::slot_init delegation looks correctDirectly delegating to
Self::initafter bindingBufferSizekeeps initialization logic centralized and consistent with the new slot/initializer model; no behavioral issues spotted.crates/vm/src/types/slot.rs (1)
465-471: Improved init return error message is fineIncluding the actual return type in
"__init__ should return None, not '<type>'"matches CPython’s style and doesn’t alter control flow.crates/vm/src/builtins/object.rs (1)
121-162: LGTM: Comprehensive excess argument validation.The slot_init implementation correctly handles the complex logic for validating excess arguments to
__init__. The differentiation between heap types (checking MRO) and built-in types (slot comparison) aligns with CPython's behavior, and the error messages are appropriately descriptive.crates/vm/src/builtins/weakref.rs (1)
7-7: LGTM: Clean Initializer integration.The addition of the
Initializertrait toPyWeakis correctly implemented. The no-opinitmethod is appropriate since all initialization is handled inslot_new(as documented in the comment on line 57), and the import additions and pyclass extension are properly structured.Also applies to: 10-12, 54-61, 64-71
crates/vm/src/builtins/descriptor.rs (5)
6-10: LGTM: Import additions support new wrapper types.The added imports (
PyHash,InitFunc,Hashable,Comparable,PyComparisonOp,Representable) are all properly utilized in the newPySlotWrapperandPyMethodWrapperimplementations.
392-427: LGTM: PySlotWrapper follows descriptor protocol.The
PySlotWrapperstruct and itsGetDescriptorimplementation correctly implement the descriptor protocol: returning the descriptor itself when accessed from the class, and creating a boundPyMethodWrapperwhen accessed from an instance.
429-485: LGTM: PySlotWrapper Callable implementation is sound.The
Callableimplementation correctly:
- Binds the first argument as the object and validates its type
- Provides a clear error message for type mismatches
- Delegates to the wrapped function and returns
None(appropriate for__init__semantics)The accessor methods and
Representableimplementation follow standard descriptor patterns.
487-585: LGTM: PyMethodWrapper implementation is comprehensive.The
PyMethodWrapperimplementation correctly:
- Binds the wrapper to an instance via the
objfield- Delegates calls to the wrapped function with the bound object
- Implements
__reduce__for pickle support using the standard(getattr, (obj, name))pattern- Provides sound
Hashable(XOR of object hash and wrapper identity) andComparable(wrapper identity + object equality) implementationsAll implementations align with Python's descriptor and method-wrapper semantics.
385-390: LGTM: Descriptor type registration follows established patterns.The addition of
PySlotWrapperandPyMethodWrapperto theinitfunction correctly follows the existing pattern for registering descriptor types alongsidePyMemberDescriptorandPyMethodDescriptor.
6ffb2d8 to
3cb6b52
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/vm/src/builtins/weakref.rs (1)
54-61: Consider expanding the comment to explain the CPython pattern.The no-op
initimplementation correctly mirrors CPython's behavior wheretp_initexists but does nothing for types like weakref that complete initialization intp_new. The current comment is clear, but you might optionally expand it to explain this is matching CPython's design pattern for future maintainers.crates/vm/src/builtins/object.rs (1)
121-162: Re‑check override detection for object.init excess‑args logicThe new
slot_initlogic is a good idea, but the way overrides are detected may not match how slots are actually wired:
typ_init/object_initandtyp_new/object_neware read fromtyp.slots.init/new.- However,
PyType::update_slotsets these slots to generic wrappers (init_wrapper/new_wrapper) whenever__init__/__new__are installed as attributes (including via the newPySlotWrapperand the existingslot_new_wrapper).- For builtins that use the new wrappers, and for many heap types, this likely makes
typ_init == object_initandtyp_new == object_neweven when the logical__init__or__new__is overridden, sotyp_init != object_init && new_overriddenmay never be true.- In that case,
object.__init__would always reject extra args for such instances, even when both__init__and__new__are conceptually overridden, which may diverge from the intended CPython‑like semantics forsuper().__init__(...)and directobject.__init__(...)calls on subclasses.Consider basing “override” detection on attribute/MRO inspection for both heap and non‑heap types (e.g., scanning the MRO for
__new__/__init__definitions beforeobject) instead of, or in addition to, raw slot function pointer comparisons. It would be good to verify this behavior against a few representative cases (builtin subclass, pure Python subclass, subclass overriding only__init__, only__new__, and both) to ensure excess‑arg handling matches expectations.crates/vm/src/builtins/descriptor.rs (1)
492-503: Consider letting GC traverse PyMethodWrapper.obj instead of skipping it
PyMethodWrapperstores aPyObjectRefinobjbut marks it with#[pytraverse(skip)]:#[pyclass(name = "method-wrapper", module = false, traverse)] #[derive(Debug)] pub struct PyMethodWrapper { pub wrapper: PyRef<PySlotWrapper>, #[pytraverse(skip)] pub obj: PyObjectRef, }Because
objis a real Python object reference, skipping it in traversal means cycles likeobj.attr = obj.__init__won’t be visible to the GC, and those graphs may never be collected if reference counting alone can’t break them.Unless there’s a specific reason to hide this edge, it would be safer to let the derive macro traverse
objnormally (matching how other bound‑method‐like types typically behave), or to implement a custom traverse that still visitsobj.Possible adjustment
#[pyclass(name = "method-wrapper", module = false, traverse)] #[derive(Debug)] pub struct PyMethodWrapper { pub wrapper: PyRef<PySlotWrapper>, - #[pytraverse(skip)] - pub obj: PyObjectRef, + pub obj: PyObjectRef, }If a custom traversal is actually needed for performance or correctness, consider documenting it here and adding an explicit
Traverseimpl that still visitsobj.Also applies to: 552-585
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_descr.pyis excluded by!Lib/**Lib/test/test_weakref.pyis excluded by!Lib/**
📒 Files selected for processing (7)
crates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/weakref.rscrates/vm/src/class.rscrates/vm/src/stdlib/io.rscrates/vm/src/types/slot.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/zoo.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/weakref.rscrates/vm/src/stdlib/io.rscrates/vm/src/class.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/object.rs
🧠 Learnings (1)
📚 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/class.rscrates/vm/src/builtins/descriptor.rs
🧬 Code graph analysis (3)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(40-49)
crates/vm/src/builtins/descriptor.rs (3)
crates/vm/src/types/slot.rs (3)
hash(1107-1107)descr_get(1029-1034)obj(1345-1346)crates/vm/src/class.rs (1)
extend_class(80-158)crates/vm/src/builtins/getset.rs (2)
descr_get(50-69)zelf(110-110)
crates/vm/src/builtins/object.rs (1)
crates/vm/src/builtins/type.rs (5)
zelf(1569-1569)zelf(1571-1571)typ(1322-1330)__new__(548-558)name(579-584)
⏰ 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). (3)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (7)
crates/vm/src/builtins/weakref.rs (2)
7-7: LGTM! Imports are correctly updated.The addition of
PyRefandInitializerimports are necessary for the new Initializer implementation below.Also applies to: 10-12
63-73: LGTM! Correctly adds Initializer to the pyclass traits.The
pyclassattribute is properly updated to includeInitializerin thewith()clause, grouping it logically withConstructor.crates/vm/src/types/zoo.rs (1)
97-98: Wrapper / method-wrapper types are correctly integrated into TypeZooThe added
wrapper_descriptor_typeandmethod_wrapper_typefields, initialized viaPySlotWrapper::init_builtin_type()andPyMethodWrapper::init_builtin_type(), follow the existing pattern for builtin descriptor types and cleanly expose these new types throughContext::types.Also applies to: 192-193
crates/vm/src/stdlib/io.rs (1)
1465-1473: slot_init now delegates directly to the trait initializerRouting
slot_initthroughzelf.init(raw, BufferSize { buffer_size }, vm)keeps all initialization logic in theinitmethod while preserving the existing error-wrapping onargs.bind(vm). This avoids any extra public__init__indirection and looks consistent with the new slot-wrapper design.crates/vm/src/class.rs (1)
4-5: init slot wrapper wiring is sound and non-intrusiveImporting
PySlotWrapperand creating a wrapper only whenclass.slots.initis set and__init__is absent ensures:
- Builtin types with an init slot get a proper
wrapper_descriptor-style__init__.- Explicit
__init__definitions (Python-level or Rust-level) are not overwritten.- The captured
init_funcgives the wrapper a stableInitFunceven thoughupdate_slotlater repointsslots.initto the genericinit_wrapper.This matches the existing
__new__handling pattern and fits the new descriptor types cleanly.Also applies to: 139-151
crates/vm/src/types/slot.rs (1)
465-474: More informative init return-value errorUpdating
init_wrapperto report"__init__ should return None, not '<type>'"keeps the same guard while giving callers the concrete return type, which makes debugging bad initializers much easier without changing control flow.crates/vm/src/builtins/descriptor.rs (1)
6-10: Slot / method wrapper descriptors are well‑integrated into the descriptor systemThe additions of
PySlotWrapperandPyMethodWrapperplus:
- importing
InitFunc,Callable,Representable,Hashable,Comparable, andPyComparisonOp;- registering them in
init(ctx)alongside existing descriptor types; and- implementing
GetDescriptor+Callableso that unbound calls go through the slot and bound access returns amethod-wrapperform a coherent wrapper_descriptor/method-wrapper pair. The type / name / doc plumbing and reprs all look consistent with the existing
method_descriptor/member_descriptorpatterns and should make slot‑backed methods like__init__behave naturally from Python.Also applies to: 385-390, 392-485
3cb6b52 to
1f9df15
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.