Init copyslot #6515
Init copyslot #6515
Conversation
📝 WalkthroughWalkthroughThis PR refactors Python class initialization by separating object construction from initialization using the Initializer trait pattern. It modifies the derive macro to generate direct slot overrides for Initializer and Constructor traits, improves MRO-based slot inheritance logic, and adds Initializer implementations to core types (PyType, StringIO, BytesIO) and exception groups, along with specialized init/del slot resolution in the type system. Changes
Sequence DiagramsequenceDiagram
participant User
participant Derive as Derive Macro
participant Type as Type System
participant Slot as Slot Resolution
participant Init as Initialization
User->>Derive: #[pyclass(..., with(Constructor, Initializer))]
Derive->>Derive: Detect Initializer trait
Derive->>Type: Emit direct slot_init override
Derive->>Type: Emit direct slot_new override
Type->>Type: Object instantiation (via __new__)
Type->>Slot: Check for __init__ in MRO
alt __init__ explicitly defined
Slot->>Init: Install wrapper init function
else __init__ found in MRO (real method)
Slot->>Init: Install wrapper init function
else No real __init__ in MRO
Slot->>Init: Inherit slot from base
end
Init->>Init: Call slot_init via Initializer trait
Init-->>User: Object ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
f7cbca1 to
5575cc0
Compare
5575cc0 to
b9602cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/types/slot.rs (1)
929-957:has_real_method_in_mrologic is sound; comment could be slightly clarifiedThe helper correctly treats:
- A
PySlotWrapperwhosetypmatches the class being inspected as “pure slot wrapper” → returnfalseto favor the inherited C-level slot.- Inherited
PySlotWrappers (wherewrapper.typ != cls) as ignorable while continuing to scan for a real method.- Any non-wrapper descriptor found in the MRO as a “real” method → return
true.Combined with the separate
has_own_*checks inupdate_slot, this yields the intended “use wrappers only when there is a genuine Python-level implementation” behavior.Minor doc nit: the comment says “including self”, but the loop only walks
self.mro(which explicitly excludesself), andselfis handled viahas_own_init/has_own_del. You might tweak the comment to mention that separation to avoid future confusion.crates/derive-impl/src/pyclass.rs (1)
1648-1693: Directly wiringConstructor/Initializerslots avoids trait-impl blind spotsSpecial-casing
with(Constructor)andwith(Initializer)to storeslots.newandslots.initdirectly ensures types use their concreteslot_new/slot_initimplementations, rather than depending on trait-level__extend_slotscode that doesn't see per-type overrides. TheDefaultConstructorerror message also nudges users towardwith(Constructor, ...), which is clearer sinceDefaultConstructoralready impliesConstructor.A minor robustness consideration: this currently matches only bare idents (
Constructor/Initializer); fully-qualified paths (e.g.with(rustpython_vm::types::Constructor)) would still go through the__extend_slotsroute. To future-proof against such usages, consider matching on the last path segment instead ofpath.is_ident().
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Lib/test/test_codecs.pyis excluded by!Lib/**Lib/test/test_memoryio.pyis excluded by!Lib/**Lib/test/test_pickle.pyis excluded by!Lib/**Lib/test/test_pickletools.pyis excluded by!Lib/**Lib/test/test_urllib2.pyis excluded by!Lib/**Lib/test/test_zipfile.pyis excluded by!Lib/**
📒 Files selected for processing (7)
crates/derive-impl/src/pyclass.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/type.rscrates/vm/src/exception_group.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/builtins/type.rscrates/vm/src/exception_group.rscrates/vm/src/types/slot.rscrates/vm/src/stdlib/io.rscrates/vm/src/builtins/object.rs
🧠 Learnings (2)
📚 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/derive-impl/src/pyclass.rs
📚 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/vm/src/exception_group.rs
🧬 Code graph analysis (7)
crates/derive-impl/src/pyclass.rs (1)
crates/vm/src/class.rs (1)
extend_slots(206-206)
crates/vm/src/types/zoo.rs (4)
crates/vm/src/builtins/object.rs (2)
init(156-158)init(585-594)crates/vm/src/builtins/type.rs (2)
init(1582-1584)init(1914-1916)crates/vm/src/types/slot.rs (1)
init(1060-1060)crates/vm/src/builtins/weakref.rs (2)
init(58-60)init(143-145)
crates/vm/src/builtins/type.rs (2)
crates/vm/src/builtins/object.rs (3)
init(156-158)init(585-594)slot_init(122-154)crates/vm/src/types/slot.rs (2)
init(1060-1060)slot_init(1033-1058)
crates/vm/src/exception_group.rs (5)
crates/vm/src/builtins/object.rs (1)
slot_init(122-154)crates/vm/src/builtins/type.rs (1)
slot_init(1571-1580)crates/vm/src/types/slot.rs (1)
slot_init(1033-1058)crates/vm/src/exceptions.rs (20)
slot_init(1406-1409)slot_init(1456-1468)slot_init(1511-1527)slot_init(1699-1751)slot_init(2105-2137)slot_init(2159-2162)slot_init(2239-2249)slot_init(2297-2306)slot_init(2352-2360)init(730-733)init(746-910)init(1411-1413)init(1470-1472)init(1529-1531)init(1753-1755)init(2139-2141)init(2164-2166)init(2251-2253)init(2308-2310)init(2362-2364)crates/vm/src/stdlib/ast/python.rs (1)
slot_init(56-84)
crates/vm/src/types/slot.rs (2)
crates/vm/src/builtins/type.rs (2)
name(758-763)__new__(727-737)crates/vm/src/builtins/descriptor.rs (1)
new(39-49)
crates/vm/src/stdlib/io.rs (2)
crates/vm/src/builtins/object.rs (3)
py_new(113-115)init(156-158)init(585-594)crates/vm/src/types/slot.rs (6)
py_new(998-998)py_new(1022-1024)default(262-264)new(33-38)new(190-196)init(1060-1060)
crates/vm/src/builtins/object.rs (1)
crates/vm/src/types/slot.rs (1)
slot_init(1033-1058)
⏰ 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). (6)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (15)
crates/vm/src/stdlib/io.rs (5)
3681-3691: LGTM: Two-phase initialization for StringIO Constructor.The Constructor correctly creates an empty object with default state, deferring content initialization to the Initializer. The underscore prefix on
_argsappropriately indicates the parameter is intentionally unused at this stage.
3693-3708: LGTM: StringIO Initializer correctly implements content initialization.The Initializer properly handles the
StringIONewArgsto populate the buffer, completing the two-phase initialization pattern. The implementation correctly converts the optional string input to bytes for the internal buffer.
3824-3835: LGTM: BytesIO Constructor follows the two-phase initialization pattern.The Constructor correctly initializes an empty BytesIO with
exports: AtomicCell::new(0), which is essential for the buffer resize guard used in the Initializer.
3837-3852: LGTM: BytesIO Initializer with proper buffer export guard.The Initializer correctly guards against resizing the buffer when there are active exports (e.g., from
getbuffer()). This prevents data corruption that could occur if the underlying buffer were reallocated while memoryviews still reference it. The check at lines 3841-3845 is essential for memory safety.
3720-3720: LGTM: pyclass attributes correctly include Initializer trait.Both StringIO and BytesIO pyclass declarations are properly updated to include the
Initializertrait alongsideConstructor, enabling the two-phase initialization pattern. BytesIO retainsPyReffor thegetbuffermethod implementation.Also applies to: 3864-3864
crates/vm/src/types/zoo.rs (1)
201-206: Init order change forobject/typeis correct and necessaryCalling
object::init(context)beforetype_::init(context)ensuresobject_type.slots.initis already wired toPyBaseObject’sInitializer::slot_init, soPyType’sinherit_slotscan correctly see and copy the baseinitslot. The explanatory comment is accurate and helpful.crates/derive-impl/src/pyclass.rs (1)
225-229:extend_slotscomposition remains sound with newwith_slotsbehavior
extend_slotsstill runs#with_slotsbefore#impl_ty::__extend_slots(slots), so the new directslots.init/slots.newstores fromwith(Initializer)/with(Constructor)execute first and can be further augmented (but not accidentally overridden) by any impl-local#[pyslot]s. This keeps overall slot wiring order predictable.crates/vm/src/builtins/object.rs (2)
121-153:object.__init__excess-argument validation matches CPython’s behaviorThe new
slot_init:
- Fast-paths
args.is_empty()as a no-op, allowingobject.__init__(self)calls.- Raises
TypeError("object.__init__() takes exactly one argument ...")when the instance’s type has a differenttp_initthanobjectbutobject.__init__is being called with extra args.- Raises
TypeError("<type>.__init__() takes exactly one argument ...")when bothtp_newandtp_initare still the baseobjectimplementations but extra args are passed.Using function-pointer equality via the
slots.init/slots.newcells is a reasonable way to mirror CPython’stp_init/tp_newchecks.
585-593: Manual wiring ofobject_type.slots.initis appropriate afterpyclasschangesExplicitly setting:
ctx.types.object_type.slots.init .store(Some(<PyBaseObject as Initializer>::slot_init));before
PyBaseObject::extend_classguarantees that theobjecttype’stp_inituses the customslot_initimplementation, independent of how#[pyclass(with(Initializer))]expands. This is the right place to do it given the newpyclassbehavior.crates/vm/src/exception_group.rs (1)
353-363: ExceptionGroupInitializerimpl is now explicit and consistentMaking
slot_inita documented no-op and markinginitasunreachable!("slot_init is overridden")accurately reflects thatBaseExceptionGroup’s initialization is handled entirely inslot_new. This is consistent with the new Initializer-based initialization model and doesn’t change behavior.crates/vm/src/types/slot.rs (2)
634-657:__init__slot handling correctly distinguishes real methods from slot wrappersThe new
__init__branch inupdate_slot:
For
ADD = true, installsinit_wrapperonly when:
- The current type defines
__init__in its own dict (has_own_init), or- Some base in the MRO has a non–
PySlotWrapper__init__(has_real_method_in_mro).Otherwise, it leaves
slots.initas inherited (which, for builtins usingInitializer, will be the C-levelslot_initcopied viainherit_slots/copyslot_defined).For
ADD = false, it re-inherits theinitslot from the MRO (skipping self), which matches the “delete dunder, recompute from bases” expectation.This avoids clobbering
Initializer-basedtp_initfor builtins while still giving user-defined classes the usual Python-level__init__semantics.
662-677:__del__slot logic mirrors the new__init__semanticsThe
__del__arm now:
Installs
del_wrapperwhen the type either:
- Defines its own
__del__, or- Has a real (non–
PySlotWrapper)__del__somewhere in its MRO.Re-inherits
slots.delfrom the MRO on deletion.This keeps C-level
Destructor::slot_delslots for builtins from being overridden just because a slot wrapper descriptor exists, while preserving the expected behavior for Python-side__del__definitions.crates/vm/src/builtins/type.rs (3)
414-443: MRO-basedinit_slotsbetter matches CPython’s slot inheritance rulesHaving
init_slots:
- Iterate over the full MRO (
self.mro) and callinherit_slotsfor each base, and then- Collect all dunder names from both
selfand its MRO to driveupdate_slot::<true>,aligns slot inheritance (including
__hash__,__getattr__,__len__, etc.) with the method-resolution order instead of just the immediate base. That’s important for heap types with multiple inheritance and brings behavior closer to CPython’sPyType_UpdateSlots.
465-521:copyslot_defined!(init)correctly encodes CPython’s SLOTDEFINED behaviorThe new
copyslot_defined!macro for theinitslot:
- Copies
base.slots.initintoselfonly when:
self.slots.initis currentlyNone, and- The base’s
initis set, and- Either the base has no base (
basebase == None) or itsinitdiffers frombasebase.slots.init.This matches CPython’s
SLOTDEFINEDmacro fortp_init, ensuring that in multiple-inheritance hierarchies, the first base that actually defines aninitslot (not just inherits one) is the one that contributes to the subclass. That’s exactly what you want for coexistingInitializer-driven C-level inits and Python-level__init__wrappers in complex MROs.
28-30: HookingPyTypethroughInitializerenforces correcttype.__init__arityAdding
Initializerto:
- The import list, and
- The
#[pyclass(with(Py, Constructor, Initializer, ...))]attribute,and implementing:
impl Initializer for PyType { type Args = FuncArgs; fn slot_init(_zelf: PyObjectRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult<(< 9DE9 span class="pl-kos">)> { if args.args.len() == 1 && !args.kwargs.is_empty() { return Err(vm.new_type_error("type.__init__() takes no keyword arguments".to_owned())); } if args.args.len() != 1 && args.args.len() != 3 { return Err(vm.new_type_error("type.__init__() takes 1 or 3 arguments".to_owned())); } Ok(()) } fn init(..) -> PyResult<()> { unreachable!("slot_init is defined") } }means
type.__call__’s post-__new__initialization stage now obeys Python’stype.__init__arity rules without requiring a Python-level__init__method. This is consistent with CPython and integrates cleanly with the newpyclassbehavior that wiresInitializer::slot_initdirectly intoslots.init.To be safe, you may want to exercise a few metaclass scenarios (e.g., custom
typesubclasses overriding__init__) to confirm that:
type()still accepts1or3positional args as before.- Custom
__init__on a metaclass behaves as expected under the new slot inheritance rules.Also applies to: 857-867, 1567-1585
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.