Actionable comments posted: 0

crates/vm/src/types/slot.rs (1)

929-957: has_real_method_in_mro logic is sound; comment could be slightly clarified

The helper correctly treats:

• A PySlotWrapper whose typ matches the class being inspected as “pure slot wrapper” → return false to favor the inherited C-level slot.
• Inherited PySlotWrappers (where wrapper.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 in update_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 excludes self), and self is handled via has_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 wiring Constructor/Initializer slots avoids trait-impl blind spots

Special-casing with(Constructor) and with(Initializer) to store slots.new and slots.init directly ensures types use their concrete slot_new / slot_init implementations, rather than depending on trait-level __extend_slots code that doesn't see per-type overrides. The DefaultConstructor error message also nudges users toward with(Constructor, ...), which is clearer since DefaultConstructor already implies Constructor.

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_slots route. To future-proof against such usages, consider matching on the last path segment instead of path.is_ident().

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 2063c1e and b9602cf.

• Lib/test/test_codecs.py is excluded by !Lib/**
• Lib/test/test_memoryio.py is excluded by !Lib/**
• Lib/test/test_pickle.py is excluded by !Lib/**
• Lib/test/test_pickletools.py is excluded by !Lib/**
• Lib/test/test_urllib2.py is excluded by !Lib/**
• Lib/test/test_zipfile.py is excluded by !Lib/**

• crates/derive-impl/src/pyclass.rs
• crates/vm/src/builtins/object.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/exception_group.rs
• crates/vm/src/stdlib/io.rs
• crates/vm/src/types/slot.rs
• crates/vm/src/types/zoo.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint 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.rs
• crates/vm/src/types/zoo.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/exception_group.rs
• crates/vm/src/types/slot.rs
• crates/vm/src/stdlib/io.rs
• crates/vm/src/builtins/object.rs

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

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

crates/vm/src/class.rs (1)

• extend_slots (206-206)

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/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/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/builtins/type.rs (2)

• name (758-763)
• __new__ (727-737)

crates/vm/src/builtins/descriptor.rs (1)

• new (39-49)

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/types/slot.rs (1)

• slot_init (1033-1058)

• 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)

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 _args appropriately indicates the parameter is intentionally unused at this stage.

---

3693-3708: LGTM: StringIO Initializer correctly implements content initialization.

The Initializer properly handles the StringIONewArgs to 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 Initializer trait alongside Constructor, enabling the two-phase initialization pattern. BytesIO retains PyRef for the getbuffer method implementation.

Also applies to: 3864-3864

crates/vm/src/types/zoo.rs (1)

201-206: Init order change for object/type is correct and necessary

Calling object::init(context) before type_::init(context) ensures object_type.slots.init is already wired to PyBaseObject’s Initializer::slot_init, so PyType’s inherit_slots can correctly see and copy the base init slot. The explanatory comment is accurate and helpful.

crates/derive-impl/src/pyclass.rs (1)

225-229: extend_slots composition remains sound with new with_slots behavior

extend_slots still runs #with_slots before #impl_ty::__extend_slots(slots), so the new direct slots.init/slots.new stores from with(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 behavior

The new slot_init:

• Fast-paths args.is_empty() as a no-op, allowing object.__init__(self) calls.
• Raises TypeError("object.__init__() takes exactly one argument ...") when the instance’s type has a different tp_init than object but object.__init__ is being called with extra args.
• Raises TypeError("<type>.__init__() takes exactly one argument ...") when both tp_new and tp_init are still the base object implementations but extra args are passed.

Using function-pointer equality via the slots.init / slots.new cells is a reasonable way to mirror CPython’s tp_init/tp_new checks.

---

585-593: Manual wiring of object_type.slots.init is appropriate after pyclass changes

Explicitly setting:

ctx.types.object_type.slots.init
    .store(Some(<PyBaseObject as Initializer>::slot_init));

before PyBaseObject::extend_class guarantees that the object type’s tp_init uses the custom slot_init implementation, independent of how #[pyclass(with(Initializer))] expands. This is the right place to do it given the new pyclass behavior.

crates/vm/src/exception_group.rs (1)

353-363: ExceptionGroup Initializer impl is now explicit and consistent

Making slot_init a documented no-op and marking init as unreachable!("slot_init is overridden") accurately reflects that BaseExceptionGroup’s initialization is handled entirely in slot_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 wrappers

The new __init__ branch in update_slot:

• For ADD = true, installs init_wrapper only 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.init as inherited (which, for builtins using Initializer, will be the C-level slot_init copied via inherit_slots / copyslot_defined).

For ADD = false, it re-inherits the init slot from the MRO (skipping self), which matches the “delete dunder, recompute from bases” expectation.

This avoids clobbering Initializer-based tp_init for builtins while still giving user-defined classes the usual Python-level __init__ semantics.

---

662-677: __del__ slot logic mirrors the new __init__ semantics

The __del__ arm now:

• Installs del_wrapper when the type either:

  • Defines its own __del__, or
  • Has a real (non–PySlotWrapper) __del__ somewhere in its MRO.

• Re-inherits slots.del from the MRO on deletion.

This keeps C-level Destructor::slot_del slots 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-based init_slots better matches CPython’s slot inheritance rules

Having init_slots:

• Iterate over the full MRO (self.mro) and call inherit_slots for each base, and then
• Collect all dunder names from both self and its MRO to drive update_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’s PyType_UpdateSlots.

---

465-521: copyslot_defined!(init) correctly encodes CPython’s SLOTDEFINED behavior

The new copyslot_defined! macro for the init slot:

• Copies base.slots.init into self only when:
  • self.slots.init is currently None, and
  • The base’s init is set, and
  • Either the base has no base (basebase == None) or its init differs from basebase.slots.init.

This matches CPython’s SLOTDEFINED macro for tp_init, ensuring that in multiple-inheritance hierarchies, the first base that actually defines an init slot (not just inherits one) is the one that contributes to the subclass. That’s exactly what you want for coexisting Initializer-driven C-level inits and Python-level __init__ wrappers in complex MROs.

---

28-30: Hooking PyType through Initializer enforces correct type.__init__ arity

Adding Initializer to:

• 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<()> {
        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’s type.__init__ arity rules without requiring a Python-level __init__ method. This is consistent with CPython and integrates cleanly with the new pyclass behavior that wires Initializer::slot_init directly into slots.init.

To be safe, you may want to exercise a few metaclass scenarios (e.g., custom type subclasses overriding __init__) to confirm that:

• type() still accepts 1 or 3 positional args as before.
• Custom __init__ on a metaclass behaves as expected under the new slot inheritance rules.

Also applies to: 857-867, 1567-1585